[RFC] Timer abstraction design

[MabezDev]

Rationale

Copying from #1318 (comment):

We currently have no shared API between timers, i.e TIMG and SYSTIMER are completely different and cannot be abstracted over, but in reality they all do the same things:

1. Count (clock)
2. One-shot alarm
3. Periodic alarm

If we can abstract over this, I see several benefits:

1. esp-wifi can be decoupled from specific timers, and in general it will be possible to write code that can be abstract over timers. Thanks to runtime interrupt binding, we can even create a InterruptHandler in esp-wifi and assign it to the chosen timer.
2. We might be able remove the embassy time features, though this is still unclear it might need some runtime selection because of how embassy_time_driver::time_driver_impl! works
3. De duplicate code between systimer and timg
4. Discoverability, and consistency in the timer APIs

Initial design

In a nutshell, I believe the way to go about this is to abstract over the building blocks of timer behavior, like set_target, clear_interrupt, enable_interrupt. After we have that we can then create concrete structures for specific timer behaviors.

The timer trait

This is probably where we need the most input, as it needs to support the use cases we need for all the timers we have. As a starting point this is my definition:

pub trait Timer: crate::private::Sealed {
    /// Start the timer.
    fn start(&mut self);
    
    // Stop the timer.
    fn stop(&mut self);
    
    /// Reset the timer value to 0.
    fn reset(&mut self);

    /// Is the timer running.
    fn is_running(&self) -> bool;

    /// The current timer value.
    fn now(&self) -> u64;
    
    /// Load a target value into the timer.
    fn load_value(&mut self, value: u64);
    
    /// Enable auto reload of the `load`ed value.
    fn enable_auto_reload(&mut self, auto_reload: bool);

    /// Enable or disable the timer's interrupt.
    fn enable_interrupt(&mut self, state: bool);

    /// Clear the timer's interrupt.
    fn clear_interrupt(&mut self);

    /// Has the timer triggered?
    fn is_interrupt_set(&self) -> bool;
}

Behaviour encapsulation

To encapsulate one shot timer behaviour we can create a OneShotTimer that could look like this:

pub struct OneShotTimer<T> {
    inner: T
}

impl<T> OneShotTimer<T> 
    where T: Timer
{
    // candidate timers:
    // TIMG
    // SYSTIMER
    // others?
    pub fn new(inner: T) -> Self {
        Self {
            inner
        }
    }
}

// All trait impls in once place
impl<T> embedded_hal::delay::Delay for OneShotTimer<T> 
    where T: Timer {
        /// ... impl omitted
}

What about the generic parameter?

In the OP I stated that we might be able to remove the embassy features, but if we know we need to store a timer in a static and it has a generic paramter we're going to have a bad time. I am hoping that for esp-wifi's usecase this won't matter, but regardless, I have an idea: AnyTimer.

AnyTimer, much like AnyPin allows for runtime selection of a timer. Now that the Timer trait exists, we can impl Timer for AnyTimer and boom, we have a single type than can be used for more than one timer.

[bjoernQ]

    /// The current timer value.
    fn now(&self) -> u64;

I guess we also need a way to know what a tick is (i.e. the timer frequency)

I also guess we will have this in addition to the "lower level" drivers for those timer-peripherals which is good since even if the abstraction won't (and probably can't) support everything a specific timer can do user code can still use the timer directly while most other code can use the abstraction.

Having "AnyTimer" is also a good idea

I like the general idea - most likely we will find additional functions we would like to have but this seems like a good start

[MabezDev]

I just had a thought, maybe these trait methods should be all &self. 🤔

Whilst technically some of these changes mutate the peripheral, register writes are atomic, so maybe we don't care given that this is quite a low-level trait? I can foresee mutability issues with embassy time drivers already as they all take &self. I'm not sure how having &mut self helps a user using this API because you can already start a timer that has no load value :D.

What do you think?

[bjoernQ]

load_value needs to write two registers but I guess it's not a real problem - having it &self makes it definitely easier to use and if we already know it's going to cause headaches with embassy timers (which is probably one of the main use-cases) that's already reason enough

[jessebraham]

This will be split into 3 PRs:

1. [1/3] Timer abstraction: refactor systimer and timer modules into a common timer module #1527
2. [2/3] Timer abstraction: add OneShotTimer/PeriodicTimer drivers, Timer trait #1570
3. Documentation, refactoring, and cleanup

[jessebraham]

Just to update, OneShotTimer and PeriodicTimer are both working as expected when backed by TIMG.

OneShotTimer is also working when backed by SYSTIMER, however I'm still having some strange issues with PeriodicTimer. Once this final problem is sorted the second PR will follow shortly.

[jessebraham]

This issue is for tracking progress on our current timer abstraction work, which has been discussed in detail amongst our team. Please open a new issue or discussion for other topics.