esp-rs / esp-hal

no_std Hardware Abstraction Layers for ESP32 microcontrollers
https://docs.esp-rs.org/esp-hal/
Apache License 2.0
741 stars 207 forks source link

TRng should not own the RNG peripheral #2372

Open nishanthkarthik opened 1 week ago

nishanthkarthik commented 1 week ago

Motivations

Wifi on esp32c6 requires an Rng instance. TLS requires a Trng instance. But both Rng and Trng consume the RNG peripheral. Since Rng can be cloned, Trng should have another constructor that takes in an Rng instead of the RNG peripheral directly.

Existing ctor: pub fn new(rng: impl Peripheral<P = RNG>, adc: impl Peripheral<P = ADC1> + 'd) -> Self.

Proposed ctor: pub fn new(rng: Rng, impl Peripheral<P = ADC1> + 'd) -> Self.

MabezDev commented 1 week ago

Hmm, Trng is materially different to Rng so IMO the current constructor is correct. Your proposed API would turn all Rng instances into Trng if one Trng existed (they're the same hardware just with some extra setup for trng). Whilst this might not be wrong, it might be unexpected.

I think a better solution is to:

1) Change esp-wifi's constructor to take some kind of rng using the rand core traits, this way users can use either trng or rng for esp-wifi

bjoernQ commented 1 week ago

Another option might be that esp-wifi could provide a Trng implementation since we know as long as Wif/BLE is enabled we produce true random numbers

nishanthkarthik commented 1 week ago

Your proposed API would turn all Rng instances into Trng if one Trng existed (they're the same hardware just with some extra setup for trng). Whilst this might not be wrong, it might be unexpected.

The number of Trng instances would still be limited by the number of ADCs. So at most ADC_COUNT Rng instances can be turned into Trng. Are you thinking of the Trng::downgrade method too?

  1. Change esp-wifi's constructor to take some kind of rng using the rand core traits, this way users can use either trng or rng for esp-wifi

I am not sure if this fully solves my issue. If esp-wifi consumes an Rng, it would accept it as a mut-ref, blocking the RNG peripheral as well. So, constructing another Trng is not possible for use with TLS without this peripheral.

let rng = Rng::new(peripherals.RNG);
let wifi = Wifi::new(rng.clone(), ...); // alive forever
let trng = Trng::new(peripherals.RNG /*unavailable*/);
let tls = Tls::new(&mut trng, ...);

Making Trng clonable would be great!

nishanthkarthik commented 3 days ago

@MabezDev If my proposal isn't too controversial, I'll be more than happy to make a PR :)

MabezDev commented 3 days ago

The number of Trng instances would still be limited by the number of ADCs. So at most ADC_COUNT Rng instances can be turned into Trng. Are you thinking of the Trng::downgrade method too?

Trng doesn't need an ADC per copy, it just needs the ADC to be globally configured (which we do on Trng::new) to feed entropy into the rng.

We can quite easily make Trng clonable but there are a few considerations:

nishanthkarthik commented 3 days ago

How about a new type ConfiguredADC and Trng instances take a shared ref to ConfiguredADC? When all Trng instances are done, ConfiguredADC's drop handler can put the ADC back in a known state too.

let conf_adc = ConfiguredADC::new(&mut peri.ADC); // blocks ADC
let trng1 = Trng::new(&mut RNG, &conf_adc);
let trng2 = Trng::clone(trng1);
drop(trng1);
// the lifetime of &conf_adc above is encoded in Trng<'confadc>
// trng2 having the same type would prevent conf_adc from being dropped
drop(trng2);
drop(conf_adc);
let adc = peri.ADC; // ADC is now free