[RFC] Timer & Alarm

[diondokter]

I propose we add a timer & alarm trait.

Currently the traits and RFC text are hosted here:

• github
• rfc
• crates.io

For general design, let's keep the discussion here.
For specific things like typos, small fixes, let's do that in the linked repo.

[bugadani]

I have a concern that this API is for libraries to use, but imposes no requirements for the range of the timer. If an operation overflows, there is no good way to know by how much it overflows, so a library can't really wait on the timer, short of binary searching for a delay amount that doen't fail and so it can be used to wait for in a loop. Trying to use the "current remaining time" functions is a TOC-TOU issue.

Waiting for any period of time shouldn't overflow in my opinion.

[jannic]

The "elapsed time is rounded down" requirements are tricky to get right for tick-based timers. The first tick may happen immediately after calling .start(), so all calculations must subtract one from the tick difference - making sure to not underflow below zero.
Eg., for a timer with one tick per microsecond:

    fn elapsed_micros(&self) -> Result<u64, embedded_hal_timer::OverflowError> {
        let ticks = self.elapsed_ticks().unwrap().saturating_sub(1);
        Ok(ticks)
    }

And effectively, this may do more than rounding down. The actual elapsed time may be 1.9µs, but only one tick happened in the mean time, so elapsed_micros must return 0.

To be clear, I think it is a good thing to clearly specify such behavior, so just removing the requirement wouldn't be better. Not sure what's best. 🤷

[diondokter]

Trying to use the "current remaining time" functions is a TOC-TOU issue.

True for virtual timers yes. Though that can be fixed by changing the requirement to give the max time the timer support from the perspective of the last start call. Then you could call start, query the max and see if there's enough space.

I have a concern that this API is for libraries to use, but imposes no requirements for the range of the timer.

The trait itself doesn't impose requirements no, just like the SpiDevice trait doesn't impose the frequency of the bus.
This is something the user should document: The timer must be able to run for at least a minute without overflow with a precision of at least 5ms or something like that

[bugadani]

The trait itself doesn't impose requirements no, just like the SpiDevice trait doesn't impose the frequency of the bus.

I find the analogy and the reasoning itself to be lacking tbh, but I can't really put into words why. But for SpiDevice the requirement usually comes from the hardware, not from the driver.

Next concern: there is no way to tell if a Timer is running. If start can restart the Timer, and users generally won't know if the timer is running, they may defensively call start without regard of other users of the same Timer.

[diondokter]

they may defensively call start without regard of other users of the same Timer.

There are no other users. start requires &mut Self

[jannic]

Instead of the max_ methods, I suggest to add a single guaranteed_range_ticks() -> u64 that returns the amount of ticks that are guaranteed to elapse without an overflow. That should be a constant as in "doesn't change between two calls", so a user only needs to call it once to decide if the timer fulfills the requirements.

In addition, we could add a has_overflow_detection() -> bool. If this is true, the driver reliably returns an overflow error if the timer did overflow.

[bugadani]

There are no other users. start requires &mut Self

Consider the following:

fn caller(mut timer: &mut impl Timer) {
    timer.start();
    third_party_driver::use_timer(timer);
    let elapsed = timer.elapsed();
}

mod third_party_driver {
    fn use_timer(mut timer: &mut impl Timer) {
        // not under our control, calls timer.start();
    }
}

Sure, it may be contrived, but nothing prevents this.

Thirdly, what's the expected behaviour for Alarm methods when the Timer hasn't been started?

[diondokter]

How strict should the elapsed time be? tweedegolf/embedded-hal-timer#9

[diondokter]

Thirdly, what's the expected behaviour for Alarm methods when the Timer hasn't been started?

Docs on the start function says that the time of the timer is undefined before start is called. So it's pretty much up to the implementation. In the PR for the embassy-time impl I made it return the overflow error since it's easy to make a bang there and because I think a bang is better than silence.

[diondokter]

Sure, it may be contrived, but nothing prevents this.

I see. I personally don't think this is of great concern. I think it's obvious that when you give out a timer to another part of the code, that they can use the timer to the fullest extent.
Same when you give out a reference to a SPI. The other code might perform an illegal transaction that messes up your assumptions.

[BartMassey]

After discussion in this week's meeting, I'm re-proposing that we require the timer to be 64-bit for all implementations:

• I've seen (and written) so much buggy timer-wrap code; it's really nice for the users to not have to make this mistake.

• The existing impls (RTIC and Embassy) already do this.

• It's not that hard to get right once for an arbitrary small timer.

One counter-argument is that now we're requiring implementing RTOS-like functionality that users may not need: if all I'm doing is debouncing a button 64 bits is kind of way too much.

[diondokter]

After discussion in this week's meeting, I'm re-proposing that we require the timer to be 64-bit for all implementations:

* I've seen (and written) _so much_ buggy timer-wrap code; it's really nice for the users to not have to make this mistake.

* The existing impls (RTIC and Embassy) already do this.

* It's not _that_ hard to get right once for an arbitrary small timer.

One counter-argument is that now we're requiring implementing RTOS-like functionality that users may not need: if all I'm doing is debouncing a button 64 bits is kind of way too much.

I don't think we should do that. We'd end up with pretty much 2 implementors in the ecosystem (embassy & rtic).

I also don't think we should recommend building timer wrapping on top of the trait. The trait just isn't very good for that.
If someone needs a timer that runs for at least X seconds, then the driver should document that and optionally assert that.

If we only want to support long running timers, then we wouldn't need a start function and then we'd try to get an embassy-time style trait in EH.