SYSTIMER peripheral

[MabezDev]

Opening in draft form early to bikeshed the API.

Limitations of this implementation:

• Period alarms are not supported
• Only unit0 of the systimer is used

This is a key part of embassy integration, so I thought I'd upstream this early :).

TODO

• Implement Delay on Alarm
  • Make delay::Delay take an alarm instead of the whole SYSTIMER
• Periodic (might not get to this in this PR)
  • Implement e-h timer traits on Alarm

[MabezDev]

To add some reasoning behind the current API:

• The SYSTIMER peripheral isn't really the singleton here, it's the three alarms so I chose to make them singletons inside the SystemTimer struct.

• Embassy currently requires that alarms are configurable in an immutable way (see the driver trait). So we can't have &mut in our Alarm signatures - this should be okay though because each alarm struct is a singleton, it should have unique access anyway.

• Why const generic channel? It's an attempt to reduce code repetition, I feel its relatively clean, the only bit I don't really like is the unreachable! macros

[bjoernQ]

Why const generic channel? It's an attempt to reduce code repetition, I feel its relatively clean, the only bit I don't really like is the unreachable! macros

I like it, too. Really looking forward to have const generics for enums. There is a nightly feature but don't think it's really worth to force nighly and also that feature is very experimental (warning: the feature adt_const_params is incomplete and may not be safe to use and/or cause compiler crashes 👻 ). I think the unreachable! is kind of okay since the user can't create arbitrary Alarms

I'm a bit more concerned about the unwrapping needed on alarmX() results - no idea how but it would be nice if the compiler could know if an alarm is already taken or not

[MabezDev]

I'm a bit more concerned about the unwrapping needed on alarmX() results - no idea how but it would be nice if the compiler could know if an alarm is already taken or not

Maybe it might be better for SysTimer to own the alarms (outright, not in an option) and let the compiler move the fields out. Much like the Peripherals struct. Only issue with that, is that SystemTimer cannot have any other self methods, so I suppose now() would have to be available on the alarms, which would be kinda weird. Actually, we can just make now an associated method.

Example:

pub struct SystemTimer {
    inner: SYSTIMER,
    pub alrm0: Alarm<Target, 0>,
    pub alrm1: Alarm<Target, 1>,
    pub alrm2: Alarm<Target, 2>,
}

// in main
let s = SystemTimer::new(p.SYSTIMER);

let alarm0 = s.alarm0; // move out of SystemTimer struct

s.alarm0.enable_interrupt() // compiler error, use of moved value, alarm is now in alarm0

[bjoernQ]

Yes, I thought of something like this

[MabezDev]

Should now be ready for review. I had a branch with esp32c3 delay implemented with an alarm here: https://github.com/esp-rs/esp-hal/tree/feature/delay-alarm, but it introduced a generic parameter on Delay and instead chose just to implement with the new SystemTimer::now() method. I won't get to periodic alarms in this PR, I'll open an issue tracking that instead.

[bjoernQ]

Looks great to me

[jessebraham]

The ESP32-S2 and ESP32-S3 have this peripheral as well.

[MabezDev]

The ESP32-S2 and ESP32-S3 have this peripheral as well.

Ah, good point! I'll add support for those (hopefully its the same 😅).

[MabezDev]

S2 compiles, but it looks like SYSTIMER is missing from the S3 PAC, I filed esp-rs/esp-pacs#20.

[jessebraham]

The SYSTIMER peripheral is now present for the ESP32-S3 in esp-pacs.

[MabezDev]

So the C3 & S3 version is identical and works great (yay!). However, the S2 has some changes, see the CI. I see a few options:

• punt implementing SystemTimer for the S2 down the road
• cfg all the s2 specific stuff (ew)
• 'fix' the SVD to be more consistent with the other chip implementations?

What do we think?

[jessebraham]

Thanks for making those changes!

I would say "fixing" the SVD would probably be my preferred approach, however if you just don't want to deal with the S2 right now that's fine too.

[MabezDev]

So I needed a few cfg's but it's not too bad. Yet one more difference is that the S2 system timer does not run at fixed 16MHZ, but is variable. I've fixed it to 40mhz (can't go any lower) for now.

[jessebraham]

I think this is looking pretty good, thanks for all your work!