Improve the SYSTIMER API

[Dominaezzz]

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

• I have updated existing examples or added new ones (if applicable).
• I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
• My changes were added to the CHANGELOG.md in the proper section.
• My changes are in accordance to the esp-rs API guidelines

Extra:

• I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

Description

Fixes #1477 and also expose the second unit.

esp-hal-embassy kinda gets in the way since it insists on owning all the comparators/alarms.
I didn't sign up to touch esp-hal-embassy in this PR so I'm going to mostly leave it as is.

Testing

Ran the systimer example and it works as expected.

SYSTIMER Current value = 2701986
Interrupt lvl1 (alarm0)
Interrupt lvl1 (alarm0)
Interrupt lvl1 (alarm0)
Interrupt lvl2 (alarm1)
Interrupt lvl1 (alarm0)
Interrupt lvl2 (alarm2)
Interrupt lvl1 (alarm0)
Interrupt lvl1 (alarm0)
Interrupt lvl1 (alarm0)
Interrupt lvl1 (alarm0)
Interrupt lvl1 (alarm0)
Interrupt lvl1 (alarm0)
Interrupt lvl1 (alarm0)
Interrupt lvl1 (alarm0)

[Dominaezzz]

I updated the CHANGELOG in esp-hal but didn't do the others since it's internal refactor. (CI is hard)

[bjoernQ]

Code looks really good to me - just some questions/notes

From the orignal issue:

On top of that, I noticed that whilst we can initialize Systimer as async, it makes all the alarms async or blocking, in reality there is no technical reason the alarms cannot be mixed and matched as blocking async. There are also Delay impls on the systimer alarms.

But now we have split which also makes all alarms either blocking or async. Maybe I'm missing something here?

Previously SystemTimer had two constructors to create them in async or blocking mode. Now it's just one constructor and the only thing the user can do with SystemTimer is calling split. Is this intermediate step (split) really necessary?

The 'static life-time on SystemTimer prevents making use of the PeripheralRef pattern where we were able to pass the peripheral as &mut and drop the driver later in order to create a new instance of it. I see why it's 'static and most probably re-creating SystemTimer is not a common use-case but at least we need to make a conscious decision here if we keep it

i.e. something like this was previously possible:

#[entry]
fn main() -> ! {
    let peripherals = Peripherals::take();
    let system = SystemControl::new(peripherals.SYSTEM);
    let clocks = ClockControl::boot_defaults(system.clock_control).freeze();

    let mut syst = peripherals.SYSTIMER;

    let mut systimer = SystemTimer::new(&mut syst);
    println!("SYSTIMER Current value = {}", SystemTimer::now());

    let mut alarm0 = &mut systimer.alarm0;

   

    let delay = Delay::new(&clocks);

    for _ in 0..10 {
        if alarm0.is_running() {
            alarm0.stop();
        }

        alarm0.clear_interrupt();
        alarm0.reset();

        alarm0.enable_auto_reload(false);
        alarm0.load_value(500u64.millis()).unwrap();
        alarm0.start();

        while !alarm0.is_interrupt_set() {
            // Wait
        }

        alarm0.stop();
        alarm0.clear_interrupt();


        println!("hello");
    }

    core::mem::drop(systimer);

    let mut systimer = SystemTimer::new(&mut syst);
    println!("SYSTIMER Current value = {}", SystemTimer::now());

    loop {}
}

[Dominaezzz]

But now we have split which also makes all alarms either blocking or async. Maybe I'm missing something here?

Yes, it's a bit subtle. The call to split is optional, users are free to just create Alarms themselves from a unit and comparator. Alarm::new and Alarm::new_async are public.

I added split for convenience and to ease migration, it should go away in the future once esp-hal-embassy and esp-wifi are more flexible about timers.

[bjoernQ]

Ah ok - maybe a bit more documentation might be useful then 👍

[Dominaezzz]

Thinking about it now yeah it needs some documentation to make it more obvious. I'll update the snippet in the doc and the main example.

[Dominaezzz]

I've updated the docs and the example.

Side note, we should strongly consider making a separate object for enabling/clearing interrupts. It's rather inconvenient to have to place the entire driver in a global static, it'd be nice to just have a tiny interrupt flag object to make global instead, then the main driver can live on the stack.

[Dominaezzz]

I'll reopen this once I work up the courage to resolve the conflicts.