[time] Add experimental embedded-hal-timer traits#6168
Conversation
| (Instant::MAX - Instant::now()).as_ticks() | ||
| } | ||
|
|
||
| fn max_nanos(&self) -> u64 { |
There was a problem hiding this comment.
What's the use of these functions? There's a very good chance that the returned value will (after an interruption) actually be more than the value should be at the time of use.
There was a problem hiding this comment.
Indeed, this is an inherent TOCTOU race.
Also, depending on TICK_HZ, the multiplication inside as_nanos() may overflow the u64 long before the tick counter inside Instant overflows.
There was a problem hiding this comment.
Yeah I discovered this too while implementing. (But apparently you guys think this is more of a problem than I do and that's fair!)
It really is there just for utility. It allows a driver to assert there's enough time available for the intended usecase instead of having to find out when it already went wrong.
But for these 'continuously running' virtual timers the value steadily decreases. Maybe that means we simply can't have these max functions...
So, should we just remove them?
There was a problem hiding this comment.
Why does OverflowError even exist in this API? Why is wanting to sleep beyond the end of time an error? The timer can measure the ticks necessary, even if the driver has to manage the overflow - the current Instant will not make sense, sure, but is that an issue when we are beyond the end of time anyway? If someone is lazy and implements that case as a call to pending().await, that's also a valid resolution.
Yes, elapsed_* may be unreliable beyond the end of time, and OverflowError is fair for those functions.
So, yes, I think some part of the API needs a bit more work, and removing the max allowed duration getters is probably a good idea, but it still fells like a bandaid on top of a perhaps underdone concept.
There was a problem hiding this comment.
The goal is that the trait is implementable on really simple hardware timers too (so without virtualization).
And I think I'd rather have an error rather than 'undefined' after the timer can't support operations anymore
There was a problem hiding this comment.
Then this abstraction will become a lot of headache for library users, if they will have to manage range extension themselves.
What's the intended use, and why doesn't DelayNs already cover that use case?
There was a problem hiding this comment.
If the library needs long running timers, then yes. They should document what the requirements are of the timer.
That's the same with SPI or I2c where the driver specifies through docs what the mode/speed/other settings need to be to be capable of communicating with the external device.
There's an RFC text btw:
https://github.com/tweedegolf/embedded-hal-timer/blob/main/rfc.md#why-do-we-need-this
There was a problem hiding this comment.
If the library needs long running timers, then yes.
And how does the current API actually support this? Functions can't enforce a minimum supported delay amount, Alarm has no operation that can wait for "at most" a specified amount of time so you can't loop with smaller timers, and there is no way to measure long time with these timers either because they will just report overflows.
There was a problem hiding this comment.
The behavior has changed according to the discussions of the WG meeting last week.
If the timer is designed to never overflow (in practice), then it should just return u64::MAX.
|
Also this is just a generic wishlist item, but it would be nice if the abstraction could support this use case in some way. |
| } | ||
|
|
||
| fn elapsed_nanos(&self) -> Result<u64, OverflowError> { | ||
| Ok(self.get_instant().map(|i| i.elapsed().as_nanos()).unwrap_or(u64::MAX)) |
There was a problem hiding this comment.
This sometimes returns too large values: The first timer tick may happen immediately after calling start(). So it counts one tick even though only a fraction of a tick actually elapsed.
As the Timer trait documents these functions as rounding down, this additional tick needs to be subtracted.
(The RFC also says "The returned elapsed time is rounded down, so the contract is that at least this amount of time has passed. This is congruent with DelayNs and other timers in the ecosystem.")
There was a problem hiding this comment.
Yes, this is interesting. I wonder if we need to be that strict. After all, a valid implementation could do a really long blocking operation in the start function and it's undefined whether the timer was zeroed at the start or the end.
I'll post this as an issue on the timer repo so this isn't lost.
3 participants
Hey, I'm trying to get some timer and alarm traits into embedded-hal.
This adds those to embassy-time.
I want to add more impls for the HAL hardware timers too, but will do that in different PRs.
To consider:
HalTimera good name?Timeris already taken for something elseu64::MAXand alarm invocations will returnErr(Overflow). Do we think that's good behavior?