Add Timer type

[PTaylor-us]

The Timer type supports one-shot and periodic time functionality.

[eldruin]

Disclaimer: I did not look very thoroughly. Anyway a few thoughts:

I like the type-based configuration/state keeping. However, I noticed one issue: wait() does not change the type.
I could imagine this makes sense for a periodic timer but I would expect a one-shot timer to transit to Armed or Disarmed at the end, so that I could launch a second run.

Something that stroke me on first sight is the unwrapping of self.duration and self.expiration. Then I saw it is by design impossible that one of these values would be unwrapped while being None. However, something like this is a bit unfortunate for a crate poised to become quite central because it renders panic-free reasoning unusable. e.g. panic-never. I am not sure if there would be an alternative, though.

I think it would indeed be pretty straight forward to offer a non-blocking interface as suggested in #26 with this new Timer struct. Something along these lines should do:

pub fn wait(self) -> nb::Result<(), Infallible> {
    // since the timer is running, _is_expired() will return a value
  if !self._is_expired() {
    Err(nb::Error::WouldBlock)
  } else {
    Ok(())
  }
}

Regarding external RTC devices:

1. Clock::now() does not get a mutable reference to an instance of itself. This would require storing the driver instance (containing the I2C peripheral) in a static variable. This makes it impossible to just implement Clock for a driver type.
2. I think adding fallibility would be possible with these changes. However, the strategy on what to do when an error arises needs to be clear. This is specially relevant due to the type-based state-keeping. For example, if an I2C error is raised during Timer::start() the Timer instance will be dropped. I think dropping is fine since because of 1. Timer is a thin object. If the Timer instance keeps valuable resources it would be necessary to return the original object through the error type. Something similar to this. However, this makes error recovery code more complicated.

[PTaylor-us]

Thank you so much for the feedback and pointing out areas that need to be addressed.

I noticed one issue: wait() does not change the type.

There are two wait() methods. One for OneShot and another for Periodic. The OneShot version consumes the timer. This is roughly intentional as a OneShot timer is only for a single use. I understand what you mean about maybe wanting to run it again later. My thought was that a new timer could be created instead of reusing the previous. However, I can also see that may be needlessly cumbersome. I will look at probably just returning a <OneShot, Armed, ...> type from the OneShot wait().

I think it would indeed be pretty straight forward to offer a non-blocking interface as suggested in #26 with this new Timer struct

That interface is already there with the is_expired() and period_complete() methods for OneShot and Periodic respectively. I may rename period_complete() to something like period_completed(). Might be more clear.

Clock::now() does not get a mutable reference to an instance of itself. This would require storing the driver instance (containing the I2C peripheral) in a static variable. This makes it impossible to just implement Clock for a driver type.

I think this new use case does require a change. Certainly if you are using multiple I2C peripheral clocks/timers, all using the same driver, you must be able to create multiple instances of the Clock impl. I will definitely look at changing this as soon as possible.

Something that stroke me on first sight is the unwrapping of self.duration and self.expiration. Then I saw it is by design impossible that one of these values would be unwrapped while being None. However, something like this is a bit unfortunate for a crate poised to become quite central because it renders panic-free reasoning unusable. e.g. panic-never. I am not sure if there would be an alternative, though.

I'm definitely going to look into this. There are a number of places where a panic could happen in the code right now. I feel like I could handle those situations without panicking, it just may get a little hairy with the error handling.

[eldruin]

I noticed one issue: wait() does not change the type.

There are two wait() methods. One for OneShot and another for Periodic. The OneShot version consumes the timer. This is roughly intentional as a OneShot timer is only for a single use. I understand what you mean about maybe wanting to run it again later. My thought was that a new timer could be created instead of reusing the previous. However, I can also see that may be needlessly cumbersome. I will look at probably just returning a <OneShot, Armed, ...> type from the OneShot wait().

Sorry I misread the code there and thought it just returned itself in the OneShot case as well. Consuming would be fine since the Timer instance is just a thin wrapper without valuable resources. I think restarting would be an interesting addition, though.

I think it would indeed be pretty straight forward to offer a non-blocking interface as suggested in #26 with this new Timer struct

That interface is already there with the is_expired() and period_complete() methods for OneShot and Periodic respectively. I may rename period_complete() to something like period_completed(). Might be more clear.

My point is rather the standardization around non-blocking method signatures/mechanics. At the moment in embedded-hal that is returning nb::Result for low-level non-blocking methods that would otherwise busywait. See here for example. This way you do not need the extra is_expired()/period_complete() methods since the return value will already tell you.
I wanted to clarify what my point is to avoid misunderstandings. It would also be understandable to keep the methods as they are.
In any case implementing both e_h::blocking::DelayXs and e_h::timer::CountDown on top of an e_t::Timer remains possible (without TimerBuilder, see below) so this is no showstopper.
Furthermore, changes are expected in embedded non-blocking / async.

About unwrapping/Options, I think the idea of a TimerBuilder is interesting. At the moment I think with that you could completely remove self.duration from the Timer struct and just keep expiration: Instant<Clock>. However, this would make it impossible to implement e_h::timer::CountDown on top of TimerBuilder / Timer since that trait has both methods try_start() and try_wait().

[PTaylor-us]

My point is rather the standardization around non-blocking method signatures/mechanics. At the moment in embedded-hal that is returning nb::Result for low-level non-blocking methods that would otherwise busywait.

Thank you for clarifying, I'm a huge proponent for avoiding misunderstandings 😃 . That is certainly something I could implement either alongside or instead of the current polling functions. Personally, I find the try_wait() sort of semantic, rather unintuitive, but that may simply be because it is new to me. Does the function name match the intention? "I'm going to try to wait on this timer" sounds to me like "I'm trying to blocking-wait on this timer, but if for some reason, I can't, I receive an error." It seems the actual meaning conflicts with try_start() which is, actually, "trying to start" the CountDown.

I think the idea of a TimerBuilder is interesting. At the moment I think with that you could completely remove self.duration from the Timer struct and just keep expiration: Instant.

I had to add the TimerBuilder struct in order to preserve the Clock::new_timer() functionality if a closure task is to be stored in the Timer struct. I'm still not entirely sure if the addition of task scheduling is going to be worth it or what it should look like given the current async discussions you linked to. At the moment the ability to call TimerBuilder::schedule() with a closure is implemented, but I will probably make that a private method (or remove it all together) until the backend of executing that closure upon expiration of the timer is inplace. Anyway, to your point, removing the duration field; because TimerBuilder is consumed, I believe we would then lose any way of restarting a OneShot without resupplying the desired duration, nor would the Periodic timer be able to reload automatically.

I know that CountDown includes the setting of the duration in the try_start(). I think there are some fundamental difference in architecture that make it hard to compare features of e_h and e_t. Namely, the fact that Clock is the hardware abstraction, and Timer is basically a software extension. I think a more helpful comparison is between CountDown and Clock. Given that, adding fallible) start, init, and probably other functions to the Clock trait may be necessary.

However, this would make it impossible to implement e_h::timer::CountDown on top of TimerBuilder / Timer since that trait has both methods try_start() and try_wait().

To clarify, are you saying that the TimerBuilder is what makes it impossible to implement CountDown on top of e_t or it would be impossible if the duration field was removed from Timer?

To make sure I understand, when you say "implement X on top of Y", do you mean the implementation of X using Y?

[PTaylor-us]

At this moment I see the priorities as follows:

1. Make Clock instantiable. Specifically add &mut self to Clock::now()
2. Investigate elimination of panic branches using no-panic
   ...

[PTaylor-us]

@eldruin Please take a look at #27 when you get the chance. I believe this takes care of your stateful Clock impl issue.

[eldruin]

That is certainly something I could implement either alongside or instead of the current polling functions. Personally, I find the try_wait() sort of semantic, rather unintuitive, but that may simply be because it is new to me. Does the function name match the intention? "I'm going to try to wait on this timer" sounds to me like "I'm trying to blocking-wait on this timer, but if for some reason, I can't, I receive an error." It seems the actual meaning conflicts with try_start() which is, actually, "trying to start" the CountDown.

Yes, the non-blocking narrative can be a bit unintuitive in the beginning. As I understand it "I'm trying to blocking-wait on this timer, but if for some reason, I can't, I receive an error." still holds, because WouldBlock is not really an "operation error".
The point would be that the methods represent the high-level operation (e.g. start, wait), and if they are not immediately resolvable (e.g. try_start()), you just get WouldBlock until completion (or real error).

I had to add the TimerBuilder struct in order to preserve the Clock::new_timer() functionality if a closure task is to be stored in the Timer struct. I'm still not entirely sure if the addition of task scheduling is going to be worth it or what it should look like given the current async discussions you linked to. At the moment the ability to call TimerBuilder::schedule() with a closure is implemented, but I will probably make that a private method (or remove it all together) until the backend of executing that closure upon expiration of the timer is inplace. Anyway, to your point, removing the duration field; because TimerBuilder is consumed, I believe we would then lose any way of restarting a OneShot without resupplying the desired duration, nor would the Periodic timer be able to reload automatically.

Ah, yes. I missed the restarting part. The scheduling is an interesting possibility but it might be a bit of a stretch, or rather, independently implementable on top of/using a Timer.

However, this would make it impossible to implement e_h::timer::CountDown on top of TimerBuilder / Timer since that trait has both methods try_start() and try_wait().

To clarify, are you saying that the TimerBuilder is what makes it impossible to implement CountDown on top of e_t or it would be impossible if the duration field was removed from Timer?

Sorry let me clarify that: The fact that the start and wait methods are spread over two structs makes it impossible to do this: impl e_h::CountDown for e_t::Timer.

To make sure I understand, when you say "implement X on top of Y", do you mean the implementation of X using Y?

Yes. Let's clarify: I mean impl X for Y or impl X for impl Y. Sometimes I could also mean this:

struct Z {  y: Y }
impl X for Z {
// use methods from Y
}

[PTaylor-us]

The scheduling is an interesting possibility but it might be a bit of a stretch, or rather, independently implementable on top of/using a Timer.

I suppose I could completely remove the task closure field, and if a user wants to implement scheduling, they would just create a queue of objects bundling a timer with a closure, then independently set, for example, a compare match interrupt on the clock. If that approach makes sense, what do you think about adding a time interrupt trait that can be implemented optionally for a clock providing an interrupt at (Instant) and _interrupt_in (Duration) methods? May also require some enable and disable methods.

[PTaylor-us]

I split all of the scheduling features off into a different branch. Now we just have one type, Timer (no more TimerBuilder).