Skip to content

add asm::delay #84

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

Merged
merged 2 commits into from
May 18, 2018
Merged

add asm::delay #84

merged 2 commits into from
May 18, 2018

Conversation

japaric
Copy link
Member

@japaric japaric commented Mar 12, 2018

This PR adds an asm::delay function that can be used to block the program for the specified number
of instruction cycles (NB in absence of interrupts, see API docs). The implementation is done in
assembly so the execution time of that function is the same regardless of the used optimization
level.

Rationale: external devices (sensors) sometimes require delays in their initialization, or to
workaround silicon errata. Having to set up a hardware peripheral like the SysTick can be overkill
when the delay is to be done once.

Why not provide an implementation of embedded_hal::{DelayUs, DelayMs}? Going from instruction
cycles to human time requires knowing the clock configuration and in some cases the Flash access
latency. As all that information is device specific it's best left to device crates to implement.

Thoughts? Perhaps this is too error prone due to Flash access latency and interrupts? The interrupt
issue could be avoided by calling the asm! block within an interrupt::free call.

cc @therealprof @kunerd

@japaric
Copy link
Member Author

japaric commented Mar 12, 2018

bors try

bors bot added a commit that referenced this pull request Mar 12, 2018
src/asm.rs Outdated
/// level.
///
/// NOTE that the delay can take much longer if interrupts are serviced during its execution.
#[inline(never)]
Copy link
Member Author

Choose a reason for hiding this comment

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

Without #[inline(never)] the compiler seems to misoptimize this function: it doesn't reload the $0 register on every invocation but instead it starts decrementing it from its previous value (0).

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not misoptimized, it is intended behavior. You used the "r" constraint and promised to the compiler that you will not modify the value in this register. There is a way to fix it, use "r+" modifier to let compiler know that you will write this input register. Or you can copy the value in some other register, add it to the clobber list and modify it instead of input register.

@bors
Copy link
Contributor

bors bot commented Mar 12, 2018

try

Timed out

@therealprof
Copy link
Contributor

That's a goodie. Far better than the loops I've been using every now and then. I agree with @pftbest that you'll need to change the register clobbers to make it not be inline, also I'd prefer something a bit more obvious than a delay calculated in times 3 cycles. Maybe throw in a NOP for good measure to make it stretch it a cycle to a more obvious 4?

I don't think interrupts are going to be a problem. Someone using this mechanism usually only cares about a minimum delay, not absolute precision.

@kunerd
Copy link

kunerd commented Mar 14, 2018

I see why it doesn't make sense to implement it in embedded-hal, but I also have done some research and the requirement for small delays seems not to be as uncommon as I first thought. Would it make sense to have something like 'BogoMips' and corresponding busy waiting traits in embedded-hal, like they are used in the Linux Kernel? I'm not an OS or experienced embedded guy, so I might be totally wrong with this.

@japaric
Copy link
Member Author

japaric commented May 18, 2018

addressed the review comments

bors r+

bors bot added a commit that referenced this pull request May 18, 2018
84: add asm::delay r=japaric a=japaric

This PR adds an `asm::delay` function that can be used to block the program for the specified number
of instruction cycles (NB in absence of interrupts, see API docs). The implementation is done in
assembly so the execution time of that function is the same regardless of the used optimization
level.

Rationale: external devices (sensors) sometimes require delays in their initialization, or to
workaround silicon errata. Having to set up a hardware peripheral like the SysTick can be overkill
when the delay is to be done once.

Why not provide an implementation of `embedded_hal::{DelayUs, DelayMs}`? Going from instruction
cycles to human time requires knowing the clock configuration and in some cases the Flash access
latency. As all that information is device specific it's best left to device crates to implement.

Thoughts? Perhaps this is too error prone due to Flash access latency and interrupts? The interrupt
issue could be avoided by calling the asm! block within an interrupt::free call.

cc @therealprof @kunerd

Co-authored-by: Jorge Aparicio <[email protected]>
@bors
Copy link
Contributor

bors bot commented May 18, 2018

Build succeeded

@bors bors bot merged commit f04e448 into master May 18, 2018
@jonas-schievink jonas-schievink deleted the delay branch May 26, 2020 21:56
adamgreig pushed a commit that referenced this pull request Jan 12, 2022
84: refactor the linker script r=therealprof a=japaric

to make it more compatible with LLD. This commit contains no functional changes.

fixes #70

Overview of changes:

- Alignment checks are enabled now that rust-lld (LLD 7.0) supports the modulo
operator.

- Removed some private symbols (e.g. __foo) in favor of ADDR and SIZEOF.

- Turned .got into a NOLOAD section now that rust-lld supports it.

- Replaced `ABSOLUTE(.)` with `.` as an old LLD overlap bug seems to be gone and
ABSOLUTE seems to cause problems, like #70, on bigger programs.

- Made the linker assertion messages more uniform.

- Extended test suite to check that linking works with both rust-lld and GNU
LD.

r? therealprof (chosen at random)

Co-authored-by: Jorge Aparicio <[email protected]>
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

Successfully merging this pull request may close these issues.

4 participants