AP_GPS: do not treat a backwards timestamp as a delayed frame - #34069
Open
peterbarker wants to merge 1 commit into
Open
AP_GPS: do not treat a backwards timestamp as a delayed frame#34069peterbarker wants to merge 1 commit into
peterbarker wants to merge 1 commit into
Conversation
tnow is taken from the JitterCorrection-corrected GPS timestamp when one is available. That timestamp is deliberately not monotonic: the corrector tracks a running minimum of the transport latency and steps its estimate backwards whenever a message arrives having taken less time in transit than any seen before. tnow also swaps between that timebase and millis() across the timeout path above. delta_time_ms is unsigned, so a backwards step became a delta of around 65 seconds once truncated into the uint16_t. That is far over the 245ms threshold, so it incremented delayed_count, and two in a row are enough to make AP_GPS::is_healthy() false and refuse the arm with "GPS 1: not healthy". Take the difference as signed instead. That stays correct across the 49.7-day millis() wrap - where the unsigned subtraction is also correct, but a "tnow < last" test is not - while still recognising a backwards step. When one happens we have no measurement of how long the message took to arrive, so leave the health counters untouched rather than either counting a delayed frame or resetting delayed_count. The delayed-frame accounting moves into a helper called from the two places that set delta_time_ms, so each caller decides at the point it does the arithmetic whether it has something worth counting. Measured across 198000 GPS samples from SITL logs: 18.7% of samples in the first 10 seconds after boot stepped backwards, tailing to none after 30 seconds, consistent with the corrector converging on its minimum. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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
AP_GPS computed the inter-message interval as an unsigned subtraction into a uint16_t. The timestamps it subtracts come from JitterCorrection, which is intentionally non-monotonic — it ratchets its offset backwards on any message that beats the best transport latency seen so far. A backwards step therefore underflowed to ~65 s, which read as a badly delayed frame and incremented delayed_count; two in a row fail AP_GPS::is_healthy() and refuse the arm with "GPS 1: not healthy".
It affects every GPS driver using corrected timestamps. It surfaced in InertialLabsEAHRS because that test arms inside that window.
Classification & Testing (check all that apply and add your own)
Description
tnow is taken from the JitterCorrection-corrected GPS timestamp when one is available. That timestamp is deliberately not monotonic: the corrector tracks a running minimum of the transport latency and steps its estimate backwards whenever a message arrives having taken less time in transit than any seen before. tnow also swaps between that timebase and millis() across the timeout path above.
delta_time_ms is unsigned, so a backwards step became a delta of around 65 seconds once truncated into the uint16_t. That is far over the 245ms threshold, so it incremented delayed_count, and two in a row are enough to make AP_GPS::is_healthy() false and refuse the arm with "GPS 1: not healthy".
Take the difference as signed instead. That stays correct across the 49.7-day millis() wrap - where the unsigned subtraction is also correct, but a "tnow < last" test is not - while still recognising a backwards step. When one happens we have no measurement of how long the message took to arrive, so leave the health counters untouched rather than either counting a delayed frame or resetting delayed_count.
The delayed-frame accounting moves into a helper called from the two places that set delta_time_ms, so each caller decides at the point it does the arithmetic whether it has something worth counting.
Measured across 198000 GPS samples from SITL logs: 18.7% of samples in the first 10 seconds after boot stepped backwards, tailing to none after 30 seconds, consistent with the corrector converging on its minimum.