Skip to content

RP2040: us_ticker implementation causes system uptime to wrap sporadically after 2^32µs #1063

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nomis opened this issue May 19, 2025 · 1 comment · May be fixed by arduino/mbed-os#38 or #1064
Open

Comments

@nomis
Copy link

nomis commented May 19, 2025

There are several issues with the current us_ticker implementation.

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() 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() 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.

@nomis
Copy link
Author

nomis commented May 19, 2025

The way several of the other mbed drivers handle these types of timing problems is to store the last value from us_ticker_read(), so I'll make a PR that does this as soon as I'm able to test my changes (which takes a few hours).

Firing a timer interrupt immediately may be harder to do because the hardware has no way to do that.

@nomis nomis changed the title Flaws in RP2040 us_ticker implementation RP2040: us_ticker implementation causes system uptime to wrap sporadically after 2^32µs May 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant