Add RNG

[Wratox]

Added RNG as per #34, it compiles but I haven't been able to test it since I don't have access to a processor yet.

[astapleton]

Thanks for the PR! Will you be getting hardware to be able to test this? I'll take more of a look in the next few days to a week, but my first inclination is that it would be nice to use generics instead of macros here. From my cursory look, it seems like it would be possible to create a generic RNG type that takes a numeric type as a parameter. That would probably require a trait bound that informs the compiler that the type implements whatever functionality is needed.

[usbalbin]

We have ordered a Nucleo-H533 which should arrive any day now. The plan is to later make a custom pcb with the H523. So we will most likely shift our focus to #38 before we ca do any real tests with this

[astapleton]

We have ordered a Nucleo-H533 which should arrive any day now. The plan is to later make a custom pcb with the H523. So we will most likely shift our focus to #38 before we ca do any real tests with this

Great. I created #39 to track what we need to do to integrate STM32H523/STM32H533

[astapleton]

Thanks! I think this is a good start, but would like to see some seed error recovery, a non-blocking API and removal of the CryptoRng marker trait as mentioned in the comments. I know you brought this directly over from the H7 where it is already in use, but these feel like important changes to me.

[usbalbin]

We have now added the three configurations described in the reference manuals and the application note. All three configurations seems to work on the Nucleo-H533 (the nist configuration when the #[cfg] is commented out). The led blinks sporadically in all cases and we have seen no errors yet.

We still have the recovery thing left to do and your other comments

[astapleton]

We have now added the three configurations described in the reference manuals and the application note. All three configurations seems to work on the Nucleo-H533 (the nist configuration when the #[cfg] is commented out). The led blinks sporadically in all cases and we have seen no errors yet.

We still have the recovery thing left to do and your other comments

This looks great so far. Really appreciate the effort in implementing the different configurations that ST notes. I realized that I was looking at an older version of RM0492 that did not directly state that configuration B and C were AIS-31 compliant, but Rev 3 does. I'm not really familiar with crypto standards, but according to some cursory internet searching, that does lend some credibility to using those configurations as entropy sources for key generation, and given that the note that they are for both the STM32H503 and STM32H56x/STM32H573, perhaps we can implement the CryptoRng trait for those implementations too? Sorry for the back and forth there. I also think it's worth noting in the documentation that they are AIS-31 compliant according to ST and can be used as entropy sources if that is the standard the user requires.

[usbalbin]

Just a heads up stm32-rs/stm32-rs#1220

[astapleton]

Generally looks good, but I think we need some clarification on the seed error recovery behavior (see inline comment).

[astapleton]

Note that I modified the PAC to include this register on STM32H503 and it should be included in the latest release (stm32h5 0.16.0, released yesterday). Nothing to do on this PR, but we should integrate the new PAC and make this change afterwards.

[astapleton]

So, from my understanding, we're configuring the RNG peripheral to automatically recover from seed error failures (no deactivation via the CR.ARDIS bit). Does that mean the consumer is expected to keep polling this function until it gets a valid value if it returns SeedError? If so, this should be documented somewhere. And should we be doing that in the iterator implementation?

[Wratox]

So, from my understanding, we're configuring the RNG peripheral to automatically recover from seed error failures (no deactivation via the CR.ARDIS bit).

Yes, the ARDIS bit is set to 0 by default and 0 means auto-reset enabled and we don't change it in any configuration.

Does that mean the consumer is expected to keep polling this function until it gets a valid value if it returns SeedError? If so, this should be documented somewhere.

Basically yes. We now clear the seed error flag automatically.

And should we be doing that in the iterator implementation?

See the last commit.

[astapleton]

instead of returning a result here, shouldn't we instead keep polling self.value() in a loop until we get a valid value? This is considered a blocking call and given we're automatically recovering, shouldn't we rather absorb the momentary failure for the sake of preserving the convenience of using the iterator here?

[usbalbin]

What do you think we should do when an Error::ClockError is encountered? Just return None?

[usbalbin]

impl<MODE> core::iter::Iterator for Rng<MODE> {
    type Item = u32;

    fn next(&mut self) -> Option<Self::Item> {
        loop {
            match self.value() {
                Ok(x) => return Some(x),
                // We recover automatically from this, so try again
                Err(Error::SeedError) => (),
                Err(Error::ClockError) => return None,
            }
        }
    }
}

[astapleton]

Is the clock error something we can check on init? It seems that would be a configuration error? Returning a None would end iteration, and the manual states that the clock error does not impact random number generation so it doesn't seem like it should end iteration.

[usbalbin]

32.3.7 of rm0481

In parallel to random number generation a health check block verifies the correct noise
source behavior and the frequency of the RNG source clock as detailed in this section.
Associated error state is also described.
Clock error detection
When the clock error detection is enabled (CED = 0) and if the RNG clock frequency is too
low, the RNG sets to 1 both the CEIS and CECS bits to indicate that a clock error occurred.
In this case, the application must check that the RNG clock is configured correctly (see
Section 32.3.6: RNG clocking) and then it must clear the CEIS bit interrupt flag. The CECS
bit is automatically cleared when the clocking condition is normal.
Note: The clock error has no impact on generated random numbers that is the application can still
read the RNG_DR register.
CEIS is set only when CECS is set to 1 by RNG.

--

Is the clock error something we can check on init?

We do have the assert at line 52

// Otherwise clock checker will always flag an error
// See RM0481 Rev 2 Section 32.3.6
assert!(rng_clk > (hclk / 32), "RNG: Clock too slow");

I suppose we could also await sr().read().drdy() on init and check the clock status, that would probably catch any remaining clock setting errors.

However when it comes to check the flag for every value I am not sure. Are the pll's and clocks configs frozen on init or is there any chance of them changing? Also is the check also for the off chance that something fails in the hardware?

[astapleton]

The clock configuration is frozen after configuration so you'd have to do something unsafe to modify them. I think the assert should be fine then? We could perhaps emit a warning log message if a clock error occurs and keep iterating?

[usbalbin]

impl<MODE> core::iter::Iterator for Rng<MODE> {
    type Item = u32;

    fn next(&mut self) -> Option<Self::Item> {
        loop {
            match self.value() {
                Ok(x) => return Some(x),
                // We recover automatically from a seed error, so try again
                Err(Error::SeedError) => (),
                Err(Error::ClockError) => {
                    #[cfg(feature = "log")]
                    log::warn!("RNG Clock error detected, retrying");

                    #[cfg(feature = "defmt")]
                    defmt::warn!("RNG Clock error detected, retrying");
                },
            }
        }
    }
}

do you want any message on seed error? If so, at what level?

[astapleton]

Considering that we recover automatically it's probably worth emitting a warning for that.

Looking at the value function again, it seems if a clock error occurs we will get stuck in a tight loop in the iterator, so perhaps we should be panicking instead on clock error instead?. The only way to recover from that error is to modify the appropriate source clock, which is definitely beyond the responsibility of this module, and it is probably a bug somewhere, or a result of undefined behavior. Either that or we should just disable the clock error detection and not try to handle it (relying on the assert at the beginning).