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

Prevent underflow when calling delay(n) with n<2 #328

Merged
merged 5 commits into from Feb 9, 2021

Conversation

ovidiusabou
Copy link
Contributor

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.

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.
@rust-highfive
Copy link

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.

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m labels Feb 8, 2021
jonas-schievink
jonas-schievink previously approved these changes Feb 8, 2021
Copy link
Contributor

@jonas-schievink jonas-schievink left a 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?

@jonas-schievink
Copy link
Contributor

Also, run cargo xtask assemble to update the blobs, then CI should pass

@ovidiusabou
Copy link
Contributor Author

I ran cargo xtask assemble but CI still doesn't pass.

@jonas-schievink
Copy link
Contributor

If I run it locally it still changes the archives. What OS are you on?

@ovidiusabou
Copy link
Contributor Author

Mac OS

@ovidiusabou
Copy link
Contributor Author

~/Projects/cortex-m fix-delay-underflow ❯ git status
On branch fix-delay-underflow
Your branch is up to date with 'origin/fix-delay-underflow'.

nothing to commit, working tree clean
~/Projects/cortex-m fix-delay-underflow ❯ cargo xtask assemble
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/xtask assemble`
info: component 'rust-std' for target 'thumbv6m-none-eabi' is up to date
info: component 'rust-std' for target 'thumbv7m-none-eabi' is up to date
info: component 'rust-std' for target 'thumbv7em-none-eabi' is up to date
info: component 'rust-std' for target 'thumbv7em-none-eabihf' is up to date
info: component 'rust-std' for target 'thumbv8m.base-none-eabi' is up to date
info: component 'rust-std' for target 'thumbv8m.main-none-eabi' is up to date
info: component 'rust-std' for target 'thumbv8m.main-none-eabihf' is up to date
building artifacts for thumbv6m-none-eabi
"rustc" "+nightly-2020-08-26" "--target" "thumbv6m-none-eabi" "-g" "-O" "-Clto=yes" "-Cforce-frame-pointers=no" "--remap-path-prefix" "/Users/ovidiu/Projects/cortex-m=." "--emit=obj" "-o" "bin/thumbv6m-none-eabi.o" "asm/lib.rs"
"rustc" "+nightly-2020-08-26" "--target" "thumbv6m-none-eabi" "-g" "-O" "-Clto=yes" "-Cforce-frame-pointers=no" "--remap-path-prefix" "/Users/ovidiu/Projects/cortex-m=." "--emit=obj" "-Clinker-plugin-lto" "-o" "bin/thumbv6m-none-eabi-lto.o" "asm/lib.rs"
building artifacts for thumbv7m-none-eabi
"rustc" "+nightly-2020-08-26" "--target" "thumbv7m-none-eabi" "--cfg=armv7m" "-g" "-O" "-Clto=yes" "-Cforce-frame-pointers=no" "--remap-path-prefix" "/Users/ovidiu/Projects/cortex-m=." "--emit=obj" "-o" "bin/thumbv7m-none-eabi.o" "asm/lib.rs"
"rustc" "+nightly-2020-08-26" "--target" "thumbv7m-none-eabi" "--cfg=armv7m" "-g" "-O" "-Clto=yes" "-Cforce-frame-pointers=no" "--remap-path-prefix" "/Users/ovidiu/Projects/cortex-m=." "--emit=obj" "-Clinker-plugin-lto" "-o" "bin/thumbv7m-none-eabi-lto.o" "asm/lib.rs"
building artifacts for thumbv7em-none-eabi
"rustc" "+nightly-2020-08-26" "--target" "thumbv7em-none-eabi" "--cfg=armv7m" "--cfg=armv7em" "-g" "-O" "-Clto=yes" "-Cforce-frame-pointers=no" "--remap-path-prefix" "/Users/ovidiu/Projects/cortex-m=." "--emit=obj" "-o" "bin/thumbv7em-none-eabi.o" "asm/lib.rs"
"rustc" "+nightly-2020-08-26" "--target" "thumbv7em-none-eabi" "--cfg=armv7m" "--cfg=armv7em" "-g" "-O" "-Clto=yes" "-Cforce-frame-pointers=no" "--remap-path-prefix" "/Users/ovidiu/Projects/cortex-m=." "--emit=obj" "-Clinker-plugin-lto" "-o" "bin/thumbv7em-none-eabi-lto.o" "asm/lib.rs"
building artifacts for thumbv7em-none-eabihf
"rustc" "+nightly-2020-08-26" "--target" "thumbv7em-none-eabihf" "--cfg=armv7m" "--cfg=armv7em" "--cfg=has_fpu" "-g" "-O" "-Clto=yes" "-Cforce-frame-pointers=no" "--remap-path-prefix" "/Users/ovidiu/Projects/cortex-m=." "--emit=obj" "-o" "bin/thumbv7em-none-eabihf.o" "asm/lib.rs"
"rustc" "+nightly-2020-08-26" "--target" "thumbv7em-none-eabihf" "--cfg=armv7m" "--cfg=armv7em" "--cfg=has_fpu" "-g" "-O" "-Clto=yes" "-Cforce-frame-pointers=no" "--remap-path-prefix" "/Users/ovidiu/Projects/cortex-m=." "--emit=obj" "-Clinker-plugin-lto" "-o" "bin/thumbv7em-none-eabihf-lto.o" "asm/lib.rs"
building artifacts for thumbv8m.base-none-eabi
"rustc" "+nightly-2020-08-26" "--target" "thumbv8m.base-none-eabi" "--cfg=armv8m" "--cfg=armv8m_base" "-g" "-O" "-Clto=yes" "-Cforce-frame-pointers=no" "--remap-path-prefix" "/Users/ovidiu/Projects/cortex-m=." "--emit=obj" "-o" "bin/thumbv8m.base-none-eabi.o" "asm/lib.rs"
"rustc" "+nightly-2020-08-26" "--target" "thumbv8m.base-none-eabi" "--cfg=armv8m" "--cfg=armv8m_base" "-g" "-O" "-Clto=yes" "-Cforce-frame-pointers=no" "--remap-path-prefix" "/Users/ovidiu/Projects/cortex-m=." "--emit=obj" "-Clinker-plugin-lto" "-o" "bin/thumbv8m.base-none-eabi-lto.o" "asm/lib.rs"
building artifacts for thumbv8m.main-none-eabi
"rustc" "+nightly-2020-08-26" "--target" "thumbv8m.main-none-eabi" "--cfg=armv7m" "--cfg=armv8m" "--cfg=armv8m_main" "-g" "-O" "-Clto=yes" "-Cforce-frame-pointers=no" "--remap-path-prefix" "/Users/ovidiu/Projects/cortex-m=." "--emit=obj" "-o" "bin/thumbv8m.main-none-eabi.o" "asm/lib.rs"
"rustc" "+nightly-2020-08-26" "--target" "thumbv8m.main-none-eabi" "--cfg=armv7m" "--cfg=armv8m" "--cfg=armv8m_main" "-g" "-O" "-Clto=yes" "-Cforce-frame-pointers=no" "--remap-path-prefix" "/Users/ovidiu/Projects/cortex-m=." "--emit=obj" "-Clinker-plugin-lto" "-o" "bin/thumbv8m.main-none-eabi-lto.o" "asm/lib.rs"
building artifacts for thumbv8m.main-none-eabihf
"rustc" "+nightly-2020-08-26" "--target" "thumbv8m.main-none-eabihf" "--cfg=armv7m" "--cfg=armv8m" "--cfg=armv8m_main" "--cfg=has_fpu" "-g" "-O" "-Clto=yes" "-Cforce-frame-pointers=no" "--remap-path-prefix" "/Users/ovidiu/Projects/cortex-m=." "--emit=obj" "-o" "bin/thumbv8m.main-none-eabihf.o" "asm/lib.rs"
"rustc" "+nightly-2020-08-26" "--target" "thumbv8m.main-none-eabihf" "--cfg=armv7m" "--cfg=armv8m" "--cfg=armv8m_main" "--cfg=has_fpu" "-g" "-O" "-Clto=yes" "-Cforce-frame-pointers=no" "--remap-path-prefix" "/Users/ovidiu/Projects/cortex-m=." "--emit=obj" "-Clinker-plugin-lto" "-o" "bin/thumbv8m.main-none-eabihf-lto.o" "asm/lib.rs"
~/Projects/cortex-m fix-delay-underflow ❯ git status
On branch fix-delay-underflow
Your branch is up to date with 'origin/fix-delay-underflow'.

nothing to commit, working tree clean
~/Projects/cortex-m fix-delay-underflow ❯ git push
Everything up-to-date
~/Projects/cortex-m fix-delay-underflow ❯

Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 9, 2021

Build succeeded:

@bors bors bot merged commit 2281fd6 into rust-embedded:master Feb 9, 2021
@David-OConnor
Copy link
Contributor

David-OConnor commented Feb 26, 2021

I can confirm this. Just started getting it on the f3 hal's adc setup.

@adamgreig
Copy link
Member

I can confirm this. Just started getting it on the f3 hal's adc setup.

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?

@David-OConnor
Copy link
Contributor

David-OConnor commented Feb 27, 2021

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 adc.rs in stm32f3xx hal was crashing it:

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.

@ovidiusabou
Copy link
Contributor Author

0.7.1 doesn't include the fix from this PR from what I can see.

@adamgreig
Copy link
Member

Oops, you're quite right, I was thinking of #317 instead. Are you able to test with cortex-m master from this repository?

bors bot added a commit that referenced this pull request Mar 7, 2021
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 <adam@adamgreig.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants