[RFC] Timer abstraction design

[MabezDev]

Rationale

Copying from https://github.com/esp-rs/esp-hal/issues/1318#issuecomment-2059823266:

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. :thinking:

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. https://github.com/esp-rs/esp-hal/pull/1527
2. https://github.com/esp-rs/esp-hal/pull/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.

[Ben-PH]

I've been using embedded-time on a project of mine to introduce a constraint that the implementer must be able to get some sort of time-representation.

basically:

impl<..., C: embedded_time::Clock> MyTimeAwareTrait for SomeMCUStruct<..., C> {
    fn some_time_needing_fn&self) {
        let now = self.clock_source.try_now().unwrap();
        let time_delta = now.duraction_since(...);
        let velocity = .....;
    }
}

Broader context, I'm doing a re-impl of the arduino library Simple FOC. The design goal I have in mind, is to put together a series of traits that each contribute to enabling a system that can do Field Oriented Control for BLDC motors. To the user, they must be unconcerned about the platform they are using, other than bringing together the correct resources. E.g., all they must do to setup for setting the duty cycles of the three phases is like so:

    let mut driver = Esp3PWM::new(
        &clocks,
        peripherals.TIMG0,
        peripherals.MCPWM0,
        (pins.gpio1, pins.gpio2, pins.gpio3),
        // snip
    );

the way new is setup, is that the returned Esp3PWM struct satisfies the constraints needed to imlpement my MotorPins trait:

pub trait MotorHiPins {
    fn set_pwms(&mut self, dc_a: DutyCycle, dc_b: DutyCycle, dc_c: DutyCycle);
    fn set_zero(&mut self);
}

and it meets that, because I use the embedded-hal::pwm::SetDutyCycle trait:

impl<A, B, C> MotorHiPins for Triplet<A, B, C> 
where 
    A: SetDutyCycle,
    B: SetDutyCycle,
    C: SetDutyCycle
{
    fn set_pwms(&mut self, dc_a: DutyCycle, dc_b: DutyCycle, dc_c: DutyCycle) {
        let _ = SetDutyCycle::set_duty_cycle_percent(&mut self.member_a, dc_a.0.as_percent() as u8);
        let _ = SetDutyCycle::set_duty_cycle_percent(&mut self.member_b, dc_b.0.as_percent() as u8);
        let _ = SetDutyCycle::set_duty_cycle_percent(&mut self.member_c, dc_c.0.as_percent() as u8);
    }

This means that I can setup something like so:

fn just_move_slow<FocDriver>(&mut driver: FocDriver, speed: f32)
where
    FocDriver: FOC + MotorHiPins +  PosSensor + TimeSource + MotionControl<MODE = BangBang>
{
    let mut timestamp = TimeSource::now(&self);
    let mut posnstamp = PosSensor::get_pos(&self);
    loop {
        // microsecond delay goes here...
        let pos = PosSensor::get_pos(&self);
        let pos_delta = pos - posnstamp;
        let now = TimeSource::now();
        let time_delta = now.duration_since(timestamp);
        /* this would essentiall be the BangBang specific mode of motion control. a, b and c would be
            the return of one of `MotionControl`s methods
        let (a, b, c) = if speed(pos_delta, time_delta) > speed {
            (0, 0, 0);
         } else {
             // speed up a touch, with 10% power
             FOC::get_accel_dcs(PosSensor::elec_angle(&self), 10);
         };
         */
         MotorHiPins::set_pwms(&mut self, a, b, c);
     }
 }

The main point I want to raise: The embedded ecosystem is missing a canonical Clock trait the way we get such traits with, for example InputPin and OutputPin. I believe this will change in the future, and thinking about, and maybe participating in, this addition, would be useful when working on this.

[Ben-PH]

I've put together an RFC in the embedded rust working group that proposes setting up a standard interface for reading counter peripherals. I explicitly had clocks in mind when putting it together: https://github.com/rust-embedded/wg/pull/762

[Ben-PH]

Much the way esp-hal GpioPin<....>s implement embedde-hal traits InputPins, I hope for a future where things like clock, pulse count, and other counting peripherals can be used as implementors of a common embedded-hal trait.

[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.