RP2040: Fix 64-bit uptime being wrapped as a 32-bit value whenever a timer event happens#38
Open
nomis wants to merge 2 commits intoarduino:extrapatches-6.17.0from
Open
RP2040: Fix 64-bit uptime being wrapped as a 32-bit value whenever a timer event happens#38nomis wants to merge 2 commits intoarduino:extrapatches-6.17.0from
nomis wants to merge 2 commits intoarduino:extrapatches-6.17.0from
Conversation
us_ticker_irq_handler() must only be called from interrupt context
There is only one user of this API, the mbed timer queue. It sets targets a maximum of (2**32)//16*7 microseconds in the future. Assuming there are no other timer events to be scheduled, after 3 instances of this at 5637s uptime the 64-bit uptime gets wrapped. Never modify the system uptime because that makes it unusable when the 64-bit uptime that should never wrap, unexpectedly does. With the uptime proceeding as normal into large 64-bit values the 32-bit timestamp needs special handling. It's ambiguous what the 32-bit timestamp means because time advances while the ticker functions are being called, so the 64-bit time could wrap between calculating the next 32-bit timestamp and setting it as the target time. The only way to avoid this is to know for certain what the caller meant by keeping track of the last 32-bit value that was read. This relies on there only being caller but other mbed timer implementations already keep track of the last call to us_ticker_read() to handle ambiguity in timestamp values. Track the last read of the full 64-bit time too, so that we can always prepare the correct 64-bit time value. Avoid reading the current time repeatedly because it could change between calls. If the timestamp is in the near future it's possible that it has been set too late and the time has been missed. Force a timer interrupt when this happens.
|
|
|
Hi! Nice find on this! Left a few comments based on my experience with us ticker APIs on other Mbed targets. Any chance you could submit it also to mbed-ce/mbed-os? I will accept it there once we go through the comments! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
The RP2040 timer implementation is wrapping the 64-bit uptime as if it were a 32-bit value, which is unexpected and makes it impossible to use the 64-bit uptime correctly.
Convert 32-bit timestamps to 64-bit values and stop forcing the uptime to be wrapped.
Check for a timer target being missed because it's now in the past and use the correct API to force an immediate timer event in an interrupt context.
Fixes arduino/ArduinoCore-mbed#1063.
Impact of changes
Uptime as a 64-bit value is no longer wrapped as a 32-bit value when a timer event happens.
Uptime as a 32-bit value is not impacted and unaffected.
Migration actions required
Remove workarounds that may have been added because the 64-bit uptime has been wrapped.
Documentation
None
Pull request type
Test results