Skip to content

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

Open
arduino/mbed-os
#38
@nomis

Description

@nomis

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions