stm32-rs / stm32l0xx-hal

A hardware abstraction layer (HAL) for the STM32L0 series microcontrollers written in Rust
BSD Zero Clause License
96 stars 60 forks source link

While {} busy waits #160

Open kaidokert opened 3 years ago

kaidokert commented 3 years ago

Just a suggestion: there are quite a few places where there's a while ..some bit condition.. {} loop.

Sometimes it would be helpful if in debug builds these loops would time out and panic if the condition doesn't get hit in some sane amount of time, i.e. u16::MAX would be usually plenty.

I ran into a thing where RTC::new() was trying to call enable_lse(), which hangs forever if your devel board crystal isn't running properly for some reason. The issue is of course easy enough to find in this case, but in some other situations might save a bit of headache.

Perhaps a busywait function that takes the condition as closure ? I.e. something like

fn busywait<F>(f: F) where
    F: FnOnce() -> bool + Copy {

    let mut countdown = u16::MAX;
    while f() && (countdown != 0) {
        countdown-=1;
    };
    if countdown == 0 {
        panic!("timeout");
    }
}

which can then be called like this

busywait( || self.rb.csr.read().lserdy().bit_is_clear() );

The backtrace when it fails points right at the source of the problem.

One could of course add #[cfg(not(debug_assertions))] path here to eliminate the counter from release builds.

Does this sound like a good change ? If yes, I'll be happy to PR it

hannobraun commented 3 years ago

That sounds very sensible. I think even in cases where you know that such a loop shouldn't deadlock, it makes sense to have such a mechanism to detect wrong assumptions and bugs (hardware and software).

I'm somewhat concerned about different behavior between debug and release builds though (even though there's precedent for that). Maybe we should add a Cargo feature (default-off at first) to make that more explicit? If we do that, should we enable it by default at a later date?

Another concern is the initial value for countdown. You say u16::MAX should be plenty, and that makes sense to me, but it wouldn't hurt to think this through a bit more, on a case-by-case basis.

But all of those are details. I'd definitely merge this, if it's activated via Cargo feature and defaults to off.