arduino / arduino/ArduinoCore-mbed
RP2040: us_ticker implementation causes system uptime to wrap sporadically after 2^32µs
- Dominant language
- C
- Stars
- 411
- Forks
- 225
- PR merge metrics
- No merged PRs in 30d
Description
There are several issues with [the current us_ticker implementation](https://github.com/arduino/ArduinoCore-mbed/blob/b20c4e0bcfb2b2a6ef72d515ddabcbc459cd3a8f/patches/0064-RP2040-us_ticker-fix-missing-interrupts-after-32bit-.patch#L37-L61).
The most visible behaviour is that the system uptime can suddenly jump backwards by a multiple of 2^32µs when `us_ticker_set_interrupt()` is called, but it may also result in a 71 minute or indefinite hang of timer operations that use the us_ticker interface.
### Times in the past are not handled correctly
~The [API documentation for `us_ticker_set_interrupt()`](https://github.com/arduino/ArduinoCore-mbed/blob/b20c4e0bcfb2b2a6ef72d515ddabcbc459cd3a8f/cores/arduino/mbed/hal/include/hal/us_ticker_api.h#L232-L253) states that the function should handle being called with 32-bit times in the past, although it's not clear what that means - presumably the recent past.~
"Automatically fixing a timestamp in the past" is impossible to implement with a short counter that wraps, and unlike the mbed timer queue implementation the maximum delay isn't specified anywhere.
It is possible to do it with the 64-bit time value, which `hardware_alarm_set_target()` does and returns `true` to indicate that the alarm time has already passed. With the current implementation, any time even slightly in the past is going to be handled as 71 minutes in the future.
The return value of `hardware_alarm_set_target()` must not be ignored.
### System time is modified
This must be removed, it's modifying the 64-bit system time that should only ever be counting up from boot monotonically and never wrap:
```
timer_hw->timehw = 0;
```
This is completely unnecessary because it already demonstrates the ability to calculate the next 64-bit timestamp: `_timestamp = (((time_us_64() >> 32) + 1) << 32) + timestamp`.
This will also get rid of the related flawed calculation for wrapping time that's off by 1µs:
```
uint64_t wrapped_time = current_time - 0xFFFFFFFF;
```
This type of calculation is unnecessary because the CPU can already discard the top 32-bits of the 64-bit value when assigning it to a 32-bit value.
### Multiple calls to get the current time
The current time can change and wrap between the calls to `time_us_32()` and `time_us_64()`. The current time should only be read once with `time_us_64()`.
There's also a risk of the 32-bit time wrapping after the checks but before the call to `hardware_alarm_set_target()`. Because it only sets a 32-bit time value without the correct high value the alarm will never happen because it's in the past.
### Firing timer interrupt immediately doesn't happen in interrupt context
The [API documentation for `us_ticker_fire_interrupt()`](https://github.com/arduino/ArduinoCore-mbed/blob/b20c4e0bcfb2b2a6ef72d515ddabcbc459cd3a8f/cores/arduino/mbed/hal/include/hal/us_ticker_api.h#L89#L232-L253) states that the IRQ handler function must be called from an interrupt context.
It's called from whatever the current context is which may be wrong and so the IRQ handler could make incorrect assumptions, or a real interrupt could happen resulting in the IRQ handler function being called while it's already running.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with the RP2040 us_ticker implementation in patches/0064-RP2040-us_ticker-fix-missing-interrupts-after-32bit-.patch and compare its calls to hardware_alarm_set_target(), time_us_64(), and us_ticker_fire_interrupt() with the referenced API documentation. Done means timestamps remain monotonic across 32-bit wrapping, past targets are handled using the alarm return value, and immediate firing occurs in interrupt context without timer hangs.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c
- Domain
- embedded-iot
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100