Skip to content

Timer: timeouts longer than 32 bits of ticks expire at the horizon, not the timeout - #49

Open
TokenGoblin wants to merge 1 commit into
rusefi:masterfrom
TokenGoblin:timer-large-timeouts
Open

Timer: timeouts longer than 32 bits of ticks expire at the horizon, not the timeout#49
TokenGoblin wants to merge 1 commit into
rusefi:masterfrom
TokenGoblin:timer-large-timeouts

Conversation

@TokenGoblin

Copy link
Copy Markdown

Problem

hasElapsedUs() short-circuits delta >= UINT32_MAX -> expired before 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:

US_TO_NT_MULTIPLIER horizon
4 (stm32) ~1073 s
100 (simulator / unit tests) ~43 s
168 (kinetis / cypress) ~25.6 s

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) to uint32_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 — startUpFuelPumpDuration goes 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 use hasElapsedSec(), 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 s
  • timerHourLongTimeoutStaysExact: 3600 s timeout reported elapsed at 3599 s

and pass with the fix. Full suite: make && ./build/libfirmware_test.exe — 42 tests pass (Windows/MinGW GCC).

…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
@rusefillc

Copy link
Copy Markdown
Contributor
  • cypress and kinetis do not exist, could be a separate PR to explicitly articulate that
  • need separate PR for https://github.com/rusefi/rusefi/wiki/TDB-Test-Driven-Bugfixing
  • I am not convinced if we should support timer for such long periods. At this point we can add critical error to make sure the issue is not missed if it happens, but not a change in behavior yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants