Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: correct TRNG mechanism #1538

Merged
merged 16 commits into from
Jul 4, 2024

Conversation

playfulFence
Copy link
Contributor

@playfulFence playfulFence commented May 5, 2024

closes #1499

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • My changes were added to the CHANGELOG.md in the proper section.\

Pull Request Details 📖

Description

Based on what's discussed in #1499 and previous related issues, created a TRNG struct, which takes RNG peripheral and ADC instance. Working on using ADC peripheral instead of instance, this will be draft until it's done

Testing

#![no_std]
#![no_main]

use esp_backtrace as _;
use esp_hal::{prelude::*, rng::Trng,
    analog::adc::{AdcConfig, Attenuation, Add},
    clock::ClockControl,
    delay::Delay,
    gpio::Io,
    peripherals::{Peripherals, ADC1}};

use esp_println::println;

#[entry]
fn main() -> ! {
    let peripherals = Peripherals::take();
    let io = Io::new(peripherals.GPIO, peripherals.IO_MUX);

    let analog_pin = io.pins.gpio3.into_analog();
    let mut adc1_config = AdcConfig::new();
    let mut adc1_pin = adc1_config.enable_pin(analog_pin, Attenuation::Attenuation11dB);
    let mut adc1 = Add::<ADC1>::new(peripherals.ADC1, adc1_config);
    let pin_value: u16 = nb::block!(adc1.read_oneshot(&mut adc1_pin)).unwrap();
    
    let mut trng = Trng::new(peripherals.RNG, &mut adc1);
    
    let (mut rng, adc1) = trng.downgrade();
    
    // Fill a buffer with random bytes:
    let (mut rng, adc1) = trng.downgrade();
    
    // Fill a buffer with random bytes:
    let mut buf = [0u8; 16];
    // trng.read(&mut buf); // Compile-time error
    rng.read(&mut buf);
    println!("Random bytes: {:?}", buf);
    println!("Random u32:   {}", rng.random());
    println!("ADC reading = {}", nb::block!(adc1.read_oneshot(&mut adc1_pin)).unwrap());

    loop {}
}

@playfulFence playfulFence marked this pull request as draft May 5, 2024 13:41
@bjoernQ
Copy link
Contributor

bjoernQ commented May 6, 2024

I guess Trng should implement https://docs.rs/rand_core/0.6.4/rand_core/trait.CryptoRngCore.html not just CryptRng

Would deserve some docs (but I see this is draft so you probably already thought about that)

@playfulFence
Copy link
Contributor Author

According to rand_core docs:
CryptoRngCore An extension trait that is automatically implemented for any type implementing RngCore and CryptoRng.

@bjoernQ
Copy link
Contributor

bjoernQ commented May 10, 2024

According to rand_core docs: CryptoRngCore An extension trait that is automatically implemented for any type implementing RngCore and CryptoRng.

But RngCore isn't implemented? Maybe the diff looks weird for me

@playfulFence
Copy link
Contributor Author

Oh, I did it, but maybe it's lost in another branch 😄. Will double check when I'm near my PC again.

@playfulFence playfulFence force-pushed the feature/trng_revert branch from c5eea25 to 9caefa3 Compare May 13, 2024 11:31
esp-hal/src/rng.rs Outdated Show resolved Hide resolved
esp-hal/src/soc/esp32/trng.rs Outdated Show resolved Hide resolved
esp-hal/src/rng.rs Outdated Show resolved Hide resolved
@playfulFence playfulFence force-pushed the feature/trng_revert branch 2 times, most recently from e429145 to b05d194 Compare June 28, 2024 12:17
@playfulFence playfulFence marked this pull request as ready for review June 28, 2024 12:24
esp-hal/src/rng.rs Outdated Show resolved Hide resolved
@bjoernQ
Copy link
Contributor

bjoernQ commented Jul 1, 2024

Looks good! Thanks for your continuous effort here. I think we should address the comment about the example code but otherwise looks good to me

@playfulFence playfulFence force-pushed the feature/trng_revert branch 3 times, most recently from 46901b6 to 878c859 Compare July 1, 2024 13:55
@bjoernQ
Copy link
Contributor

bjoernQ commented Jul 1, 2024

I tested on hardware and with ADC example changed to this:

//! Connect a potentiometer to an IO pin and see the read values change when
//! rotating the shaft.
//!
//! Alternatively, you could also connect the IO pin to GND or 3V3 to see the
//! maximum and minimum raw values read.

//% CHIPS: esp32 esp32c2 esp32c3 esp32c6 esp32h2 esp32s2 esp32s3

#![no_std]
#![no_main]

use esp_backtrace as _;
use esp_hal::{
    analog::adc::{Adc, AdcConfig, Attenuation},
    clock::ClockControl,
    delay::Delay,
    gpio::Io,
    peripherals::Peripherals,
    prelude::*,
    rng::Trng,
    system::SystemControl,
};
use esp_println::println;

#[entry]
fn main() -> ! {
    let peripherals = Peripherals::take();
    let system = SystemControl::new(peripherals.SYSTEM);
    let clocks = ClockControl::boot_defaults(system.clock_control).freeze();

    let io = Io::new(peripherals.GPIO, peripherals.IO_MUX);
    cfg_if::cfg_if! {
        if #[cfg(feature = "esp32")] {
            let analog_pin = io.pins.gpio32;
        } else if #[cfg(any(feature = "esp32s2", feature = "esp32s3"))] {
            let analog_pin = io.pins.gpio3;
        } else {
            let analog_pin = io.pins.gpio2;
        }
    }

    let mut adc = peripherals.ADC1;
    let mut trng = esp_hal::rng::Trng::new(peripherals.RNG, &mut adc);
    let mut buff = [0u8; 32];
    trng.read(&mut buff);
    println!("{:?}", buff);
    core::mem::drop(trng);

    // Create ADC instances
    let mut adc1_config = AdcConfig::new();
    let mut adc1_pin = adc1_config.enable_pin(analog_pin, Attenuation::Attenuation11dB);
    let mut adc1 = Adc::new(&mut adc, adc1_config);

    let delay = Delay::new(&clocks);

    loop {
        let pin_value: u16 = nb::block!(adc1.read_oneshot(&mut adc1_pin)).unwrap();
        println!("ADC reading = {}", pin_value);
        delay.delay_millis(1500);
    }
}

The C6 reads 0 once and then dies. Looks good on all other targets for me

@playfulFence
Copy link
Contributor Author

The C6 reads 0 once and then dies. Looks good on all other targets for me.

After some tests, it turned out that there's a same issue in esp-idf as well. I've created an issue there already: espressif/esp-idf#14124

@playfulFence playfulFence force-pushed the feature/trng_revert branch 2 times, most recently from c401e4c to 5349777 Compare July 2, 2024 13:47
@playfulFence
Copy link
Contributor Author

All the limitations for esp32c6 are temporary since at this moment user cannot use ADC after TRNG is dropped (see espressif/esp-idf#14124)

@playfulFence playfulFence force-pushed the feature/trng_revert branch from c23d46f to 7a91b2d Compare July 4, 2024 08:27
@bjoernQ
Copy link
Contributor

bjoernQ commented Jul 4, 2024

We probably should document that calling core::mem:drop on Trng on ESP32-C6 will result in ADC1 being still unusable for now

@playfulFence playfulFence force-pushed the feature/trng_revert branch from fbf8d52 to c56db37 Compare July 4, 2024 08:47
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Thanks!

@playfulFence playfulFence dismissed MabezDev’s stale review July 4, 2024 08:53

His review was addressed, but Scott's on vacation -> he can't mark it as "completed"

Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks

@playfulFence playfulFence added this pull request to the merge queue Jul 4, 2024
Merged via the queue into esp-rs:main with commit 921ecc4 Jul 4, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TRNG driver
4 participants