-
Notifications
You must be signed in to change notification settings - Fork 168
Prevent underflow when calling delay(n) with n<2 #328
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
Conversation
Calling delay(1) causes a very long wait (freeze) otherwise. 86cd463 introduced this behaviour by changing the cycle count from n / 4 + 1 to n / 2 which forces an underflow when n<2.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @thalesfragoso (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks good to me, thanks! Can you add a short comment explaining why we add 1?
Also, run |
I ran |
If I run it locally it still changes the archives. What OS are you on? |
Mac OS |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bors r+
Build succeeded: |
I can confirm this. Just started getting it on the f3 hal's |
This should have been fixed in cortex-m 0.7.1 which was released a while back, what version of cortex-m are you using? |
I tried versions 0.6.5, 0.7.0, and 0.7.1. The most recent version I had been using where it worked was 0.6.4. This function in fn wait_advregen_startup(&self) {
asm::delay(MAX_ADVREGEN_STARTUP_US * 1_000_000) / self.clocks.sysclk().0);
} I patched it to be this, which stopped the crashes: fn wait_advregen_startup(&self) {
let mut delay = (MAX_ADVREGEN_STARTUP_US * 1_000_000) / self.clocks.sysclk().0;
if delay < 2 {
delay = 2;
}
asm::delay(delay);
} MAX_ADV... is 10, and the clocks are often 72e6, so this works out to less than 2. I tried manually delaying for 0, 1, and 2. 0 and 1 crash, 2 doesn't. |
0.7.1 doesn't include the fix from this PR from what I can see. |
Oops, you're quite right, I was thinking of #317 instead. Are you able to test with cortex-m master from this repository? |
335: Prepare for v0.7.2 release of cortex-m r=thalesfragoso a=adamgreig We've had #328 merged for a while and it fixes a pretty annoying bug which is still in the wild. Can anything think of anything else to get in before a release? If there's nothing coming up (I think #282 needs some more discussion and #331 won't affect the published crate itself) we could get this released. Co-authored-by: Adam Greig <[email protected]>
Calling delay(1) causes a very long wait (freeze) otherwise.
86cd463 introduced this behaviour by
changing the cycle count from n / 4 + 1 to n / 2 which forces an
underflow when n<2.