Timer: timeouts longer than 32 bits of ticks expire at the horizon, not the timeout - #49
Open
TokenGoblin wants to merge 1 commit into
Open
Timer: timeouts longer than 32 bits of ticks expire at the horizon, not the timeout#49TokenGoblin wants to merge 1 commit into
TokenGoblin wants to merge 1 commit into
Conversation
…not the timeout
hasElapsedUs() short-circuits "delta >= UINT32_MAX -> expired" before it ever
looks at how long the requested timeout is. That is only valid when the timeout
itself fits in 32 bits of ticks. For longer timeouts the timer reported elapsed
the moment the delta crossed the 32-bit tick horizon:
US_TO_NT_MULTIPLIER horizon
4 (stm32) ~1073 s
100 (simulator/tests) ~43 s
168 (kinetis/cypress) ~25.6 s
So hasElapsedSec(3600) on stm32 fired at ~18 minutes, and on kinetis at ~26
seconds. The "Timer fortify" commit (293f070) added a double-precision path for
huge timeouts, but placed it after the shortcut, where it cannot help once the
delta has crossed the horizon.
There was a second problem in the same zone: the fast path truncates
USF2NT(microseconds) to uint32_t. For timeouts whose microsecond count fits in
32 bits but whose tick count does not (43 s..71 min at multiplier 100), that
float-to-uint32 conversion is out of range - undefined behavior - so the
comparison was garbage even before the horizon was reached.
Fix: route any timeout that does not fit 32 bits AS TICKS through the 64-bit
double-precision compare, before the delta shortcut. The fast path is unchanged
for every timeout that fits, which is every existing sub-horizon caller. Also
guard the double->int64 cast against absurd (>int64 ticks) timeouts, which would
be UB; such a timeout simply never elapses.
Behavior notes:
- brand-new (never reset) timers still report elapsed for any sane timeout,
InitialState keeps the delta enormous; pinned by a new test.
- getElapsedSeconds() still saturates at the horizon as its header documents;
callers comparing that against long thresholds should use hasElapsedSec(),
which this commit makes exact.
Tests: two new tests fail against the previous implementation (60 s timeout
checked at 50 s reported elapsed; 3600 s at 3599 s reported elapsed) and pass
now. Full suite: 42 tests pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UUaUv4kBBZhtCkMpZnh8So
Contributor
|
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.
Problem
hasElapsedUs()short-circuitsdelta >= UINT32_MAX -> expiredbefore it looks at how long the requested timeout is. That is only valid when the timeout itself fits in 32 bits of ticks. For longer timeouts the timer reports elapsed the moment the delta crosses the 32-bit tick horizon:So
hasElapsedSec(3600)on stm32 fires at ~18 minutes; on kinetis at ~26 seconds. The double-precision long-timeout path added by 293f070 ("Timer fortify") sits after the shortcut, where it cannot help once the delta has crossed the horizon.Second problem in the same zone: the fast path truncates
USF2NT(microseconds)touint32_t. For timeouts whose microsecond count fits in 32 bits but whose tick count does not, that float→uint32 conversion is out of range — undefined behavior — so the comparison is garbage even before the horizon is reached.Why it matters downstream: rusEFI compares timers against user-configurable durations that exceed these horizons —
startUpFuelPumpDurationgoes to 6000 s, and rusefi/rusefi#10088 adds a park timeout up to 3600 s. I found this while writing the latter.Change
Route any timeout that does not fit 32 bits as ticks through the existing 64-bit double-precision compare, before the delta shortcut. The fast path is byte-for-byte unchanged for every timeout that fits — which is every existing sub-horizon caller — so no hot-path cost (the added comparison is against a compile-time constant, exactly like the one it replaces in order). Also guards the double→int64 cast against absurd (>int64-ticks) timeouts, which was UB; such a timeout now simply never elapses.
Brand-new (never reset) timers still report elapsed for any sane timeout — pinned by a new test.
getElapsedSeconds()still saturates at the horizon as its header documents; callers comparing that against long thresholds should usehasElapsedSec(), which this PR makes exact.Validation
Test-driven: the two new tests fail against the previous implementation —
timerTimeoutLongerThan32BitsOfTicks: 60 s timeout reported elapsed at 50 stimerHourLongTimeoutStaysExact: 3600 s timeout reported elapsed at 3599 sand pass with the fix. Full suite:
make && ./build/libfirmware_test.exe— 42 tests pass (Windows/MinGW GCC).