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

While {} busy waits #160

Open
kaidokert opened this issue Mar 20, 2021 · 1 comment
Open

While {} busy waits #160

kaidokert opened this issue Mar 20, 2021 · 1 comment

Comments

@kaidokert
Copy link

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
Copy link
Contributor

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.

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

No branches or pull requests

2 participants