Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork217
Description
There are several issues withthe current us_ticker implementation.
The most visible behaviour is that the system uptime can suddenly jump backwards by a multiple of 2^32µs whenus_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
TheAPI documentation forus_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, whichhardware_alarm_set_target() does and returnstrue 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 ofhardware_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 totime_us_32() andtime_us_64(). The current time should only be read once withtime_us_64().
There's also a risk of the 32-bit time wrapping after the checks but before the call tohardware_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
TheAPI documentation forus_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.