embassy-nrf: Timer HAL broken for extended timers (TIMER3/TIMER4)

[peter9477]

The embassy-nrf timer.rs implementation has an impl_timer! macro that allows defining "extended" timers (which is used for TIMER3 and TIMER4 on chips where those are supported). This creates a constant CCS equal to 4 or 6, which is used in the Timer::new() to initialize the CC registers. This will panic at run time when TIMER3 or TIMER4 are used to create a Timer, because the impl_timer! macro involves only pac::timer0, which has only 4 CC registers.

As noted in the discussion below https://matrix.to/#/!YoLPkieCYHGzdjUhOK:matrix.org/$dC6N7X1MOSACQwlS0GI9wZsCKI1HS-yGsHlTPTAAEt8?via=matrix.org&via=tchncs.de&via=grusbv.com this will need to reference pac::timer3, possibly via a regs_ext() implementation in the trait (as suggested by @Dirbaio).

[peter9477]

We looked at this and couldn't see an obvious and simple way to make the impl_timer macro somehow handle both pac::timer0 and pac::timer3. It looked like by using a few traits and trait bounds it would be feasible, but we've tried something much simpler which appears to work.

aceeedd

This is done only for 52840 for the moment, as a proof of concept. Basically since the timer3 register block is a pure superset of the timer0 register block, and since the Timer type is generic on the CCS count so it knows how many compare registers it can access, by always using timer3 when it's defined for a given chip it appears this can resolve the problem. (We tested and it avoids the panic.)

Is this an acceptable solution? If so we can apply the changes for the other chips and PR this.

(That one changeset isn't complete as we messed up the macro scoping but subsequent commits resolved that.)

[Dirbaio]

yes, it is acceptable! it's not elegant but there's no better solutions I can think of. The STM32 HAL does similar things in some places.

[peter9477]

Just added #3605 with our workaround for this. I'm not sure whether how I made the PR makes sense, since I see there are experimental changes (to use the new PAC) on main which conflict with this. Let me know if I can do this more effectively another way (but don't use big words... I'm a bit clueless about proper use of github and PRs etc.)

[peter9477]

So I pulled the aforementioned PR as it wasn't applied against main/HEAD.

That said, we've now migrated our code base to the latest "main" and it appears TIMER3 is no longer causing the panic.

From an inspection of the nrf-pac pac.rs for the nrf52840, it appears there's no longer two different types, but just one Timer type, and although it has some asserts to check that the CC index is in range, that appears hardcoded to 0..6, not variable. That means technically the PAC would allow one to attempt access to CC 4 or 5 even on one of the non-extended timers, which wouldn't work, but given that asserts aren't applied in release mode anyway I guess that's by design.

The code in embassy-nrf/src/timer.rs still has the old CCS constant set to either 4 or 6, and it will still panic if you try to access an out-of-range CC, but now the initialization loop will work properly because the new (chiptool-based) PAC is more permissive than the previous one.

As near as I can tell this issue can be closed, but perhaps it's best to leave it open until there's an official new embassy-nrf release (0.3?) with the changes.

[Dirbaio]

that appears hardcoded to 0..6, not variable

okay, that explains it, thank you for checking.

given that asserts aren't applied in release mode anyway

assert! is always compiled-in, debug_assert! is compiled-in only in debug.