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

Need a unified way to specify frequencies / periods #201

Open
Ben-Lichtman opened this issue Apr 11, 2020 · 4 comments
Open

Need a unified way to specify frequencies / periods #201

Ben-Lichtman opened this issue Apr 11, 2020 · 4 comments

Comments

@Ben-Lichtman
Copy link

Ben-Lichtman commented Apr 11, 2020

A little while back I was trying to create a UART bitbang library.

I took a look at https://crates.io/crates/bitbang-hal however it was incompatible since for nrf51, the Timers usable for a serial port implement From<Duration> but in most other embedded-hal implementors (such as stm32) the timers have their own newtype Hertz struct which is used to set timers (stm32f4xx_hal::time::Hertz).

The overall result is that it is impossible to have a bitbang-hal library that is generic over many types of processors.

If embedded-hal provided a Hertz / Period struct, or possibly required From<Duration> for the Periodic trait, this incompatibility could be eliminated in the downstream crates.

For more info please see sajattack/bitbang-hal#10 - the PR tracking the integration of my port + fixes and the original

@Ben-Lichtman
Copy link
Author

see #211

@AlyoshaVasilieva
Copy link

AlyoshaVasilieva commented Aug 15, 2021

IMO specifying the Time type of Timers as a frequency, like the STM32 HALs do, is incorrect. Requiring From<Duration> or similar seems reasonable since it would actually make the timer trait usable. I also ran into this while writing a driver. In addition to not being able to use timers generically (or it being very hard), the STM32 HALs are effectively limited to 1 second being the longest any timer can run, and cannot handle times like 800ms since it's between 1 and 2 Hz.

(edit: I'll note that the STM32H7 HAL supports using a duration, but not as part of its CountDown impl. Much easier for a user to make a newtype wrapper, though.)

@ryankurte
Copy link
Contributor

requiring From<Duration> would be a neat solve, though, there are some drawbacks to core::time::Duration for embedded use. we could perhaps look to using embedded_time::Duration for this bound?

@newAM
Copy link
Member

newAM commented Aug 18, 2021

requiring From<Duration> would be a neat solve, though, there are some drawbacks to core::time::Duration for embedded use. we could perhaps look to using embedded_time::Duration for this bound?

embedded-time is a big dependency. I think the idea of an light-weight duration type is sound; but ideally that functionality would be separated into its own crate to reduce bloat.

embedded-hal compile time:

$ hyperfine --prepare 'cargo clean' 'cargo build --target thumbv7em-none-eabi'
Benchmark #1: cargo build --target thumbv7em-none-eabi
  Time (mean ± σ):     615.6 ms ±  45.0 ms    [User: 531.1 ms, System: 117.1 ms]
  Range (min … max):   586.7 ms … 737.7 ms    10 runs

embedded-time compile time:

$ hyperfine --prepare 'cargo clean' 'cargo build --target thumbv7em-none-eabi'
Benchmark #1: cargo build --target thumbv7em-none-eabi
  Time (mean ± σ):      3.283 s ±  0.088 s    [User: 5.166 s, System: 0.751 s]
  Range (min … max):    3.224 s …  3.483 s    10 runs

Dirbaio added a commit to Dirbaio/embedded-hal that referenced this issue Nov 3, 2021
As discussed in rust-embedded#201 and
rust-embedded#177 (comment) ,
the traits that have an unconstrained `type Time` associated type end up
not being usable for HAL-independent drivers in practice, since there is too
much variation across HALs of what the actual Time type is.

These should be bound to something, probably a `Duration` trait, so that
drivers can actually create and use them. Alternatively embedded-hal could have
actual `struct Duration` and so.

Either way, it's unclear what the final solution would look like. We shouldn't leave
these traits in the 1.0 release, since fixing them later will be a breaking change.
We should temporarily remove them, and add them back once we have figured this out
which won't be a breaking change.
bors bot added a commit that referenced this issue Feb 9, 2022
324: Remove traits with unconstrained `Time` associated types. r=eldruin a=Dirbaio

As discussed in #201 and
#177 (comment) ,
the traits that have an unconstrained `type Time` associated type end up
not being usable for HAL-independent drivers in practice, since there is too
much variation across HALs of what the actual Time type is.

These should be bound to something, probably a `Duration` trait, so that
drivers can actually create and use them. Alternatively embedded-hal could have
actual `struct Duration` and so.

Either way, it's unclear what the final solution would look like. We shouldn't leave
these traits in the 1.0 release, since fixing them later will be a breaking change.
We should temporarily remove them, and add them back once we have figured this out
which won't be a breaking change.

Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>
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

4 participants