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

Shared type definitions (e.g. MHz, bps...) #59

Open
nordmoen opened this issue Mar 8, 2018 · 16 comments
Open

Shared type definitions (e.g. MHz, bps...) #59

nordmoen opened this issue Mar 8, 2018 · 16 comments

Comments

@nordmoen
Copy link

nordmoen commented Mar 8, 2018

I want to create this issue to discuss a shared place to store necessary hardware type definitions. These could be stored in the embedded-hal crate, but could also easily be in their own crate. I just think we should discuss a shared place to store these outside the processor specific crates.

There are many type definitions that can be useful when working with low-level sensors. At the moment there are no shared place that such type can or are stored. A couple of very handy types are currently in stm32f30x_hal::time (and also replicated in stm32f103xx_hal) for dealing with time.

An easy first step is to create a type module for embedded-hal and extract the types defined in stm32f30x_hal::time and potentially add more later if necessary.

I'm not sure if there are other definitions apart from time that should be abstracted out. I don't think this issue should be about physical types (e.g. cm or m/s), but should focus on shared types that are necessary for interacting with hardware.

@thejpster
Copy link
Contributor

thejpster commented Mar 8, 2018 via email

@nordmoen
Copy link
Author

nordmoen commented Mar 8, 2018

Not to derail the discussion after the first comment, but would it be possible to transition some of the time handling to use core::time::Duration instead of raw u32s? I understand that the additional u64 is not wanted, but for subsecond durations shouldn't it be optimized away?

The advantage would be more interoperability with the rest of the ecosystem, and also many helpful functions to ensure no overflow etc.

@therealprof
Copy link
Contributor

@thejpster Great stuff! I was actually looking for that, partially also for #32.
@nordmoen I fully agree that the base units required by embedded-hal should be part of it instead of being duplicated in every single implementation. I'm a bit torn about core::time::Duration because I'm not exactly sure how much baggage that would add; guess we'll have to just have to try and see.

@hannobraun
Copy link
Member

Partially related discussion: #24
See also (from the linked issue): https://github.com/TheZoq2/embedded-hal-time

@Nemo157
Copy link

Nemo157 commented Mar 8, 2018

Not to derail the discussion after the first comment, but would it be possible to transition some of the time handling to use core::time::Duration instead of raw u32s? I understand that the additional u64 is not wanted, but for subsecond durations shouldn't it be optimized away?

It would be very likely that it won't be optimized away unless the CountDown implementation (or whatever is using the Duration) gets fully inlined to where the Duration is created. (This does appear to happen in my current prototype that's just blinking an LED based on a CountDown, but I don't know that we could rely on it in larger projects).

I was just thinking about whether a ShortDuration { us: u32 } or something similar should be available in embedded-hal for implementations to use. I would assume most usage of CountDown would be fine with that as the range.


EDIT: Thinking about it a little more. As long as the Duration is created and directly passed into CountDown::start, and CountDown::start is short enough, that could probably be relied upon to be inlined. The difficult cases come if you start passing the Duration around functions, or if CountDown::start is complicated enough that inlining it seems worse to the optimizer than having separate Duration functions.

@therealprof
Copy link
Contributor

True, but then again if you're counting clock cycles and just use a u32 counter, your countdowns aren't going to be very long. I think it'd be better if we just implemented the standard and if it really becomes a problem at some point check out how to address it.

@Kixunil
Copy link

Kixunil commented Mar 13, 2018

There are cases when long times don't make sense. I'd be against calling it ShortDuration, but prefer defining it as Microseconds(pub u32) instead.

@jonas-schievink
Copy link
Contributor

To name a few alternatives to the measurements crate (which forces the use of f64, which might be suboptimal for small µCs), there's dimensioned and uom, both of which support #![no_std] and allow using arbitrary types.

@therealprof
Copy link
Contributor

Yeah, it's a somewhat odd choice to use f64 for a number of reasons, but especially without FPU it does hurt. I was actually going to look into what was required to change that, potentially using https://github.com/japaric/fpa .

@kellerkindt
Copy link

kellerkindt commented Aug 7, 2018

I am trying to implement a new device driver that should ideally only depend on embedded-hal and I found this issue while searching for a solution to the problem below:

For it to work, I need to wait up to a certain amount of microseconds for an input pin to change, check whether it has changed and wehther it is now in the desired state. This can be implemented for example using the MonoTimer of stm32f103xx-hal. Once the current 'time' / Instant is read a simple while loop could check whether the input changed, as long as the timeout has not reached yet (something like while .elapsed() < us * (.frequency() / 1_000_000)).
Since a blocking behaviour might not be desired - as well as the dependency on stm32f103xx-hal - one might think the CountDown could be used. But it requires the Time type to be known - which for the stm32f103 is defined in the stm32f103xx-hal as Hertz.
Both strategies require a dependency on the device-hal crate which I would like to omit. Am I missing and easy / obvious approach?

@fionawhim
Copy link

Hey, I wanted to revive this stalled issue. I’m writing a device driver that would really like a CountDown in milliseconds, so I’m looking for a common Time type that can represent that.

Has anything changed re: core::time::Duration in the past year that would make it more or less attractive to adopt in embedded-hal?

@little-arhat
Copy link
Contributor

There is also https://crates.io/crates/bitrate

@wbuck
Copy link

wbuck commented Jan 6, 2021

@kellerkindt or @fionawhim did either of you ever come up with a solution?

I'm also writing a driver and found that I have to depend on the stm32f3xx_hal device crate for the same reason you both mentioned.

@eldruin
Copy link
Member

eldruin commented Jan 6, 2021

Have you had a look at embedded-time? There is some discussion about it at #211

@wbuck
Copy link

wbuck commented Jan 12, 2021

@eldruin OK, I submitted a PR for integrating embedded-time for the stm32f3xx-hal device crate.

It was a pretty smooth integration for the most part except for the CountDown/Periodic timer due to the generic constraint Into<Self::Time>.

I want to see if there are some other devices crates that would be willing to accept a PR for this.

@MathiasKoch
Copy link

I would love to see the same happen for https://github.com/stm32-rs/stm32l4xx-hal

peckpeck pushed a commit to peckpeck/embedded-hal that referenced this issue Nov 10, 2022
59: Note the change to the MSRV in the changelog r=ryankurte a=eldruin

After merging rust-embedded#55

Co-authored-by: Diego Barrios Romero <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests