hal: Countdown

[jacobrosenthal]

Closes #25

My implementation in wiegand is working in milliseconds so I create MicroSeconds because I think thats about the resolution we should be at. Im not sure if we could tick slower reliably on the pi?

Also need to make a decision to use bitrate or something else. There doesnt seem to be a lot of movement on standardizing yet but most of the embedded hal stuff wants to live in Hertz land right now.

Im not sure how to put dependencies behind feature flags for void and bitrate

[golemparts]

Thanks for the fast PR submission. You bring up some interesting points that I hadn't fully considered yet.

Microseconds seems like an acceptable resolution, but it makes working with seconds a bit inconvenient. Hertz conversion is nice to have, but I'm hesitant to add another dependency just for this single use, especially since the bitrate crate isn't widely adopted. I think ideally rppal should either have its own set of public newtypes (Second, Millisecond, Microsecond, Hertz, Baud, Bps) that can be converted from/into each other as well as Duration, or embedded-hal needs to standardize on a set of types that we can adopt.

Implementing support for those throughout rppal would be useful, but also causes many breaking changes, which I want to hold off on until the current version in development has been released, so I'll add it to the To Do list. I'll also have to do some more research and see how other implementations (and drivers) currently implement and use CountDown. I did find a few implementations that work with hertz as you mentioned (although most seem to be u32 or u64 internally, which limits the timer to <= 1 s).

Given the above-mentioned considerations, I'll wait with merging this for now until I have a better picture of how best to work with time units in rppal. Any additional thoughts/comments/suggestions are definitely welcome and appreciated.

As for your specific implementation, the one thing I'm curious about is the additional 1 µs sleep after the specified duration has elapsed. Shouldn't that be moved to your own code instead so CountDown remains (more) accurate?

Dependencies can be tied to the hal feature by setting optional to true, and adding the crate name to the dependencies for the hal feature flag, the same way nb and embedded-hal are specified.

[jacobrosenthal]

These are some of the related tickets Ive been watching
rust-embedded/embedded-hal#24
rust-embedded/embedded-hal#59
rust-embedded/embedded-hal#129

[jacobrosenthal]

I added Second and Millisecond for convenience. Also this guy is in those other threads doing so in an external lib if we want to rely on that instead (though his is u32 to support no_std better I imagine)
https://github.com/TheZoq2/embedded-hal-time

[golemparts]

Thanks. I've come across that library as well, and while it's a nice proof of concept, it's not very complete. I wouldn't mind switching to an external crate though once one has matured and is widely used.

Currently I prefer the approach I saw in one of the stm32 device crates, which also implements its own types and adds a number of convenience methods to u32, although I would make a few different choices since we don't have to deal with no_std and the usual microcontroller limitations.

I'm still a bit confused on how driver crates work with different time units used by different implementations, unless start is always called externally, and only wait is used from within the driver, but perhaps I'm missing something obvious. It would've been a lot simpler if they had standardized on using Duration.

It might be a good idea to include your current implementation for now (but without the bitrate dependency, possibly with an additional Hertz newtype and a conversion to microseconds) and potentially rework it in a future update, just so users have something to experiment with, and additional feedback can be gathered.

[jacobrosenthal]

Hrm..Im not sure if I dont rely on a third party hertz implementation in my driver, what a good way to pass it in would be
https://github.com/jacobrosenthal/rust-bitbang-wiegand/blob/timeout/src/lib.rs#L61

[golemparts]

That's what I'm confused about as well. The third-party hertz you're using isn't part of the embedded-hal spec, and there are no guidelines on what the time unit represents. Requiring From<Hertz<u64>> or any other conversion in your driver would make it incompatible with other embedded-hal implementations. But if you left that out, your driver wouldn't know what to pass to start. Even if you settled on just using u32, what the value represents (microseconds? nanoseconds?) is currently up to the implementation.

As far as I can tell there are a few options. The driver could be passed a timer that has already been pre-configured, similar to how a GPIO pin is configured for input before it is passed to a driver that needs an InputPin. But then you would only be able to call wait. Or your driver accepts the required timeout value from the user as an additional parameter of the same generic type so the user can specify the correct value depending on the device crate they're working with. Or the driver requires the user to pass it a function it can call to convert its internal timeout value into the appropriate format. All of those options sound rather cumbersome to me.

I've been searching for drivers that use CountDown, but most seem to only call wait. I did find one that calls start with its own public Hertz newtype. The user could theoretically implement CountDown for their own struct which adheres to the driver's time unit requirements, and then internally use the device crate's CountDown implementation after converting to the correct format, but that doesn't sound like an ideal solution either.

[golemparts]

I'm going to merge this and then make a few tweaks so I'll have something to experiment with, but we can continue the Time type discussion here to keep all of the comments in one place.

[golemparts]

I've changed the Time type to Duration. That eliminates the need for most of the newtypes thanks to the various from_ constructors, except for Hertz. Duration is already used internally, so it removes an additional conversion. And there is a no_std version, so it seems like a good candidate for standardization.

[jacobrosenthal]

I think I read somewhere the reason embedded hal folks dont like duration even though its in core is its u64 and thats costly seeing as how everything embedded is u32
https://doc.rust-lang.org/src/core/time.rs.html#134

in fairness I think I was using u64 in my hertz code? I cant remember and I pushed over it and am using duration for now and its working nicely...