Fix millis() rollover in deadline, interval, and timestamp handling - #11291
Open
NomDeTom wants to merge 44 commits into
Open
Fix millis() rollover in deadline, interval, and timestamp handling#11291NomDeTom wants to merge 44 commits into
millis() rollover in deadline, interval, and timestamp handling#11291NomDeTom wants to merge 44 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds wrap-safe time helpers, monotonic uptime publication, updated airtime and timestamp handling, migrated deadline checks, native tests, CI enforcement, and timing guidance. ChangesRollover-safe time handling
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Feature
participant Throttle
participant Time
Feature->>Throttle: request elapsed or deadline status
Throttle->>Time: read current time
Time-->>Throttle: wrap-safe clock value
Throttle-->>Feature: timing result
sequenceDiagram
participant Radio
participant NodeDB
participant RTC
Radio->>NodeDB: record uptime placeholder
RTC->>NodeDB: signal trusted clock
NodeDB->>NodeDB: backfill last_heard epoch
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
@RCGV1 here's my time PR |
Contributor
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (14)
Build artifacts expire on 2026-08-30. Updated for |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
src/airtime.cpp (1)
78-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused
lastUtilPeriodmarkers.
lastUtilPeriodandlastUtilPeriodTXare only assigned inAirTime::syncNow()and never read elsewhere. Remove the declarations fromsrc/airtime.hand the assignments atsrc/airtime.cpp:78-79and134, 145.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/airtime.cpp` around lines 78 - 79, Remove the unused lastUtilPeriod and lastUtilPeriodTX member declarations from AirTime, and delete every assignment to them in AirTime::syncNow() and the other referenced code paths, including the assignments near lines 78–79, 134, and 145. Leave the surrounding synchronization logic unchanged.src/modules/Telemetry/Sensor/BME680Sensor.cpp (1)
172-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the injected clock for
lastStateSaveMs.
millis()bypasses the injectable timing path, making this checkpoint uncontrolled byTime::setTestMillis().As per coding guidelines, use the repository timing abstraction so timing can be injected and tested with
Time::setTestMillis().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/Telemetry/Sensor/BME680Sensor.cpp` at line 172, Replace the direct millis() assignment in the lastStateSaveMs update with the repository’s injected timing abstraction, using the same Time-based API supported by Time::setTestMillis(). Preserve the checkpoint assignment behavior while ensuring tests can control the recorded timestamp.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test_native.yml:
- Around line 74-96: Update the shell options at the start of the workflow run
block to enable errexit alongside nounset and pipefail, matching the sibling
suite-count-check job. Ensure failures in the find/xargs/awk scan terminate the
job instead of allowing partial output to be reported as a successful scan;
preserve the existing intentional non-zero handling later in the block.
In `@src/airtime.cpp`:
- Around line 60-146: Guard all shared AirTime state with a concurrency::Lock:
include concurrency/Lock.h, add a lock member to AirTime, and acquire it across
syncNow(), logAirtime(), all bucket/utilization getters, and airtimeReport()
paths. Ensure airtimeReport() keeps the lock while constructing or copying any
returned report data so pointers cannot outlive protected storage, and preserve
existing accounting behavior.
In `@src/mesh/NextHopRouter.cpp`:
- Around line 408-417: Add a snapshot-aware Throttle helper in
src/mesh/Throttle.h that accepts now and deadline and performs the unsigned
half-range due-time comparison. Update the retransmission check in NextHopRouter
to call this helper instead of the inline expression, and remove the
now-redundant explanatory comment while preserving wraparound-safe behavior.
In `@src/modules/Telemetry/Sensor/BME680Sensor.cpp`:
- Around line 167-172: Update the state-save flow around Throttle::hasElapsed so
lastStateSaveMs is assigned only after FSCom.open() and the state write both
succeed. Ensure the initial save uses this same successful-write path to
initialize the checkpoint, while failed persistence leaves the checkpoint
unchanged so the save is retried promptly.
In `@src/motion/MotionSensor.cpp`:
- Around line 262-266: Guard the remaining-time calculation in the calibration
countdown before casting endCalibrationAt to int32_t: when
screen->getEndCalibration() returns the inactive sentinel 0, keep timeRemaining
at its inactive value and skip the countdown calculation. Preserve the existing
signed-delta and rounding behavior for nonzero deadlines.
In `@test/test_packet_signing/test_main.cpp`:
- Line 1220: Update the notDueTxMsec construction in the test to use the
injectable Time::getMillis() clock instead of raw millis(), while preserving the
one-hour deadline offset and the existing router timing path.
In `@test/test_throttle/test_main.cpp`:
- Around line 104-116: Correct the rollover-math comment in
test_deadlinePassed_survives_millis_wrap to state that advancing 400
milliseconds reaches 0x00000090. Leave the deadlinePassed assertions and test
behavior unchanged.
---
Nitpick comments:
In `@src/airtime.cpp`:
- Around line 78-79: Remove the unused lastUtilPeriod and lastUtilPeriodTX
member declarations from AirTime, and delete every assignment to them in
AirTime::syncNow() and the other referenced code paths, including the
assignments near lines 78–79, 134, and 145. Leave the surrounding
synchronization logic unchanged.
In `@src/modules/Telemetry/Sensor/BME680Sensor.cpp`:
- Line 172: Replace the direct millis() assignment in the lastStateSaveMs update
with the repository’s injected timing abstraction, using the same Time-based API
supported by Time::setTestMillis(). Preserve the checkpoint assignment behavior
while ensuring tests can control the recorded timestamp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8733a089-2251-4ea4-a3f6-8a514733c3ad
📒 Files selected for processing (35)
.github/copilot-instructions.md.github/millis-deadline-allowlist.txt.github/workflows/test_native.ymlAGENTS.mdCLAUDE.mdsrc/Power.cppsrc/PowerFSMThread.hsrc/UptimeClock.cppsrc/UptimeClock.hsrc/airtime.cppsrc/airtime.hsrc/gps/GPS.cppsrc/graphics/EInkDynamicDisplay.cppsrc/graphics/Screen.cppsrc/graphics/draw/NotificationRenderer.cppsrc/input/RotaryEncoderImpl.cppsrc/mesh/NextHopRouter.cppsrc/mesh/Throttle.cppsrc/mesh/Throttle.hsrc/mesh/eth/ethClient.cppsrc/modules/DropzoneModule.cppsrc/modules/ExternalNotificationModule.cppsrc/modules/NodeInfoModule.cppsrc/modules/StatusLEDModule.cppsrc/modules/Telemetry/Sensor/BME680Sensor.cppsrc/modules/Telemetry/Sensor/BME680Sensor.hsrc/motion/MotionSensor.cppsrc/platform/extra_variants/t5s3_epaper/variant.cppsrc/platform/nrf52/NRF52Bluetooth.cppsrc/platform/nrf52/main-nrf52.cpptest/native-suite-counttest/test_airtime/test_main.cpptest/test_packet_signing/test_main.cpptest/test_throttle/test_main.cpptest/test_uptime_clock/test_main.cpp
💤 Files with no reviewable changes (1)
- src/UptimeClock.cpp
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens firmware timing logic against the 32-bit millis() rollover by replacing direct millis() deadline comparisons with rollover-safe helpers in Throttle, removing the stateful 64-bit uptime helper, and adding CI + native tests to prevent regressions.
Changes:
- Added
Throttle::deadlinePassed()(absolute deadlines) andThrottle::hasElapsed()(elapsed-since) and migrated multiple rollover-sensitive call sites to these helpers. - Introduced
Time::getMillis()as an injectable uptime seam (UptimeClock) and added new native unit test suites to exercise wrap behavior directly. - Added a CI guard (
millis-deadline-check) plus an allowlist for the remaining non-deadlinemillis()comparisons, and documented the rule in agent guidance.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_uptime_clock/test_main.cpp | Adds unit tests for UptimeClock injection, stepping, and real-clock fallback. |
| test/test_throttle/test_main.cpp | Adds wrap-boundary unit tests for Throttle elapsed/deadline helpers. |
| test/test_packet_signing/test_main.cpp | Updates a retransmission sentinel in tests to be compatible with wrap-safe comparisons. |
| test/test_airtime/test_main.cpp | Adds unit tests for airtime window rotation/decay, including across wrap. |
| test/native-suite-count | Bumps canonical native suite count to include the new suites. |
| src/UptimeClock.h | Removes 64-bit uptime API and documents using Throttle instead. |
| src/UptimeClock.cpp | Deletes Time::getMillis64() implementation. |
| src/PowerFSMThread.h | Converts battery shutdown timing to Throttle::hasElapsed(). |
| src/Power.cpp | Converts reboot/shutdown scheduling to Throttle::deadlinePassed() and removes UINT32_MAX sentinel reboot behavior. |
| src/platform/nrf52/NRF52Bluetooth.cpp | Makes passkey wait loop wrap-safe by using Throttle::deadlinePassed(). |
| src/platform/nrf52/main-nrf52.cpp | Fixes flash-corruption backoff logic to be wrap-safe and sentinel-guarded. |
| src/platform/extra_variants/t5s3_epaper/variant.cpp | Fixes touch resume/suppress windows to be wrap-safe and sentinel-guarded. |
| src/motion/MotionSensor.cpp | Updates calibration countdown computation to be wrap-resilient (see review comment). |
| src/modules/Telemetry/Sensor/BME680Sensor.h | Adds lastStateSaveMs tracking for periodic state saves. |
| src/modules/Telemetry/Sensor/BME680Sensor.cpp | Replaces counter×period millis() compare with Throttle::hasElapsed() (see review comment). |
| src/modules/StatusLEDModule.cpp | Converts multiple LED timing comparisons to Throttle helpers. |
| src/modules/NodeInfoModule.cpp | Switches dedup window to monotonic ms-based stamps and wrap-safe eviction by elapsed time. |
| src/modules/ExternalNotificationModule.cpp | Fixes nag window + output toggle timing to use Throttle helpers with proper armed-flag guarding. |
| src/modules/DropzoneModule.cpp | Converts send delay to Throttle::hasElapsed(). |
| src/mesh/Throttle.h | Adds hasElapsed() and declares deadlinePassed(). |
| src/mesh/Throttle.cpp | Implements deadlinePassed() and routes all time reads through Time::getMillis(). |
| src/mesh/NextHopRouter.cpp | Fixes retransmission due checks using a well-defined unsigned half-range compare. |
| src/mesh/eth/ethClient.cpp | Fixes Ethernet NTP renew deadline handling, including the 0 sentinel. |
| src/input/RotaryEncoderImpl.cpp | Fixes button debounce timing to be wrap-safe via Throttle::hasElapsed(). |
| src/graphics/Screen.cpp | Fixes boot-screen timeout logic to be wrap-safe via Throttle::hasElapsed(). |
| src/graphics/EInkDynamicDisplay.cpp | Removes an unnecessary millis-overflow special-case now that Throttle checks are wrap-correct. |
| src/graphics/draw/NotificationRenderer.cpp | Fixes banner timeout checks and “show indefinitely” logic using Throttle::deadlinePassed(). |
| src/gps/GPS.cpp | Fixes ACK wait loop + GPS hold timing using Throttle::deadlinePassed() with sentinel guarding. |
| src/airtime.h | Adds lastSyncMsec and declares syncNow() for monotonic bucket rotation. |
| src/airtime.cpp | Reworks airtime window maintenance to advance by elapsed uptime, not scheduler ticks; adds wrap-safe rotation/clearing. |
| CLAUDE.md | Updates quick-reference table to point to Throttle for time/deadlines. |
| AGENTS.md | Documents “never compare against millis() directly” rule and sentinel hazards, pointing to CI enforcement. |
| .github/workflows/test_native.yml | Adds millis-deadline-check CI job to reject new naive millis() comparisons in src/. |
| .github/millis-deadline-allowlist.txt | Introduces allowlist entries for legitimate non-deadline millis() comparisons. |
| .github/copilot-instructions.md | Documents the required Throttle idioms and sentinel handling guidance. |
NomDeTom
added
bugfix
Collaborator
Author
|
@coderabbitai can you check if I've resolved your comments? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="meshtastic/firmware"
pr="11291"
echo "== CodeRabbit review comments =="
gh api --paginate "repos/$repo/pulls/$pr/comments?per_page=100" \
--jq '.[] | select(.user.login == "coderabbitai") | {id, path, line, original_line, body, html_url}'
echo
echo "== CodeRabbit top-level PR comments =="
gh api --paginate "repos/$repo/issues/$pr/comments?per_page=100" \
--jq '.[] | select(.user.login == "coderabbitai") | {id, body, html_url}'
echo
echo "== Review threads with CodeRabbit comments =="
gh api graphql \
-F owner="meshtastic" \
-F name="firmware" \
-F number="$pr" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
comments(first: 50) {
nodes {
author { login }
body
url
}
}
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(any(.comments.nodes[]; .author.login == "coderabbitai"))
| {isResolved, path, line, comments: [.comments.nodes[] | select(.author.login == "coderabbitai") | {body, url}]}'Length of output: 28469 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Structure maps =="
ast-grep outline src/mesh/Throttle.h --items all --match 'Throttle|deadlinePassed' --view expanded
ast-grep outline src/mesh/NextHopRouter.cpp --items all --match 'doRetransmissions' --view expanded
ast-grep outline src/modules/Telemetry/Sensor/BME680Sensor.cpp --items all --match 'updateState' --view expanded
ast-grep outline test/test_throttle/test_main.cpp --items all --match 'test_deadlinePassed_survives_millis_wrap' --view expanded
echo
echo "== CI guard shell options =="
rg -n -C 18 'millis-deadline-check|set -[a-z]*uo pipefail|millis-hits' .github/workflows/test_native.yml
echo
echo "== Throttle and retransmission due check =="
rg -n -C 12 'deadlinePassed|doRetransmissions|nextTxMsec|HALF|0x80000000' src/mesh/Throttle.h src/mesh/Throttle.cpp src/mesh/NextHopRouter.cpp
echo
echo "== BME680 state-save checkpoint =="
rg -n -C 22 'lastStateSaveMs|STATE_SAVE_PERIOD|FSCom\.open|file\.write|file\.flush|updateState' \
src/modules/Telemetry/Sensor/BME680Sensor.cpp src/modules/Telemetry/Sensor/BME680Sensor.h
echo
echo "== Packet-signing test clock source =="
rg -n -C 12 'notDueTxMsec|pendingNextTx|Time::getMillis\(\)|millis\(\)' test/test_packet_signing/test_main.cpp
echo
echo "== Throttle wrap-test comment =="
rg -n -C 10 'test_deadlinePassed_survives_millis_wrap|advanceTestMillis\(400\)|0x0000009' test/test_throttle/test_main.cpp
echo
echo "== Current PR diff for affected files =="
git diff --unified=20 HEAD^ HEAD -- \
.github/workflows/test_native.yml \
src/mesh/Throttle.h \
src/mesh/Throttle.cpp \
src/mesh/NextHopRouter.cpp \
src/modules/Telemetry/Sensor/BME680Sensor.cpp \
test/test_packet_signing/test_main.cpp \
test/test_throttle/test_main.cppLength of output: 51409
The two other findings were already withdrawn as inapplicable. Resolving my review comments now. [resolve] You are interacting with an AI system. |
NomDeTom
changed the title
millis() rollover in deadline and interval comparisonsmillis() rollover in deadline, interval, and timestamp handling
NomDeTom
marked this pull request as ready for review
July 30, 2026 20:57
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/test_packet_signing/test_main.cpp (1)
1492-1514: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore test-global duty-cycle state in teardown
test_C14leavesconfig.lora.regionatEU_868andairTime->utilizationTX[0]atMS_IN_HOURwhen a body assertion fails, becausetearDown()only deletes the mock NodeDB. Reset these globals in teardown or use a RAII guard so later tests do not pick up this unrelated duty-cycle/region state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_packet_signing/test_main.cpp` around lines 1492 - 1514, Update tearDown() to restore the global duty-cycle test state changed by test_C14_duty_cycle_limited_reliable_send_remains_pending, including config.lora.region and airTime->utilizationTX[0], even when an assertion aborts the test body. Use teardown reset logic or an RAII guard, while preserving the existing NodeDB cleanup.
🧹 Nitpick comments (1)
src/airtime.cpp (1)
88-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRotation copies
air_period_*from theairtimes.*arrays.Lines 107-108 source
air_period_tx[i+1]/air_period_rx[i+1]fromairtimes.periodTX[i]/periodRX[i]rather than fromair_period_tx[i]/air_period_rx[i]. Today the two arrays hold identical values (both incremented inlogAirtime), so behavior is unchanged, but the cross-array copy silently couples them and will diverge if either accumulation path changes.♻️ Keep each array rotating on its own values
- air_period_tx[i + 1] = this->airtimes.periodTX[i]; - air_period_rx[i + 1] = this->airtimes.periodRX[i]; + air_period_tx[i + 1] = air_period_tx[i]; + air_period_rx[i + 1] = air_period_rx[i];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/airtime.cpp` around lines 88 - 118, Update the rotation loop in the airtime period handling so air_period_tx and air_period_rx receive values from their own corresponding arrays at index i, rather than from airtimes.periodTX and airtimes.periodRX. Leave the airtimes array rotations unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 93: Update the sentinel hazard guidance in AGENTS.md so the
`nagCycleCutoff` case explicitly checks for and excludes `UINT32_MAX` before
calling `Throttle::deadlinePassed`, while retaining the existing zero-sentinel
guard for the other deadline variables.
In `@src/modules/NodeInfoModule.cpp`:
- Around line 45-50: Make the NodeInfo suppression cache multi-wrap-safe by
replacing the 32-bit timestamp handling around lastNodeInfoSeen, including its
eviction logic, with a 64-bit monotonic timestamp or explicit expiration
representation that cannot misclassify entries after uptime wraps. Preserve the
12-hour suppression behavior for recent senders, and add a regression test
covering timestamp rollover and entries older than the signed half-range.
In `@src/modules/Telemetry/Sensor/BME680Sensor.cpp`:
- Around line 168-172: Update the save flow around stateUpdateCounter and the
FSCom write so the schedule advances only after a verified successful write: do
not increment stateUpdateCounter before opening or writing, require the write to
persist the complete state blob rather than relying only on the file handle
check, and update stateUpdateCounter plus lastStateSaveMs only after that full
write succeeds. Preserve the pending first-save behavior and retry immediately
after any open or partial-write failure.
---
Outside diff comments:
In `@test/test_packet_signing/test_main.cpp`:
- Around line 1492-1514: Update tearDown() to restore the global duty-cycle test
state changed by test_C14_duty_cycle_limited_reliable_send_remains_pending,
including config.lora.region and airTime->utilizationTX[0], even when an
assertion aborts the test body. Use teardown reset logic or an RAII guard, while
preserving the existing NodeDB cleanup.
---
Nitpick comments:
In `@src/airtime.cpp`:
- Around line 88-118: Update the rotation loop in the airtime period handling so
air_period_tx and air_period_rx receive values from their own corresponding
arrays at index i, rather than from airtimes.periodTX and airtimes.periodRX.
Leave the airtimes array rotations unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 48428d2d-08d6-4002-b67b-30f783cd1d01
📒 Files selected for processing (49)
.github/copilot-instructions.md.github/millis-deadline-allowlist.txt.github/workflows/test_native.ymlAGENTS.mdCLAUDE.mdsrc/Power.cppsrc/PowerFSMThread.hsrc/UptimeClock.cppsrc/UptimeClock.hsrc/airtime.cppsrc/airtime.hsrc/gps/GPS.cppsrc/gps/RTC.cppsrc/graphics/EInkDynamicDisplay.cppsrc/graphics/Screen.cppsrc/graphics/draw/NotificationRenderer.cppsrc/input/RotaryEncoderImpl.cppsrc/mesh/MeshService.cppsrc/mesh/MeshService.hsrc/mesh/NextHopRouter.cppsrc/mesh/NodeDB.cppsrc/mesh/NodeDB.hsrc/mesh/PhoneAPI.cppsrc/mesh/Router.cppsrc/mesh/Router.hsrc/mesh/Throttle.cppsrc/mesh/Throttle.hsrc/mesh/eth/ethClient.cppsrc/modules/DropzoneModule.cppsrc/modules/ExternalNotificationModule.cppsrc/modules/NodeInfoModule.cppsrc/modules/StatusLEDModule.cppsrc/modules/Telemetry/DeviceTelemetry.cppsrc/modules/Telemetry/DeviceTelemetry.hsrc/modules/Telemetry/HostMetrics.hsrc/modules/Telemetry/Sensor/BME680Sensor.cppsrc/modules/Telemetry/Sensor/BME680Sensor.hsrc/motion/MotionSensor.cppsrc/platform/extra_variants/t5s3_epaper/variant.cppsrc/platform/nrf52/NRF52Bluetooth.cppsrc/platform/nrf52/main-nrf52.cpptest/native-suite-counttest/test_airtime/test_main.cpptest/test_meshpacket_serializer/ports/test_timestamp.cpptest/test_meshpacket_serializer/test_helpers.htest/test_packet_signing/test_main.cpptest/test_stream_api/test_main.cpptest/test_throttle/test_main.cpptest/test_uptime_clock/test_main.cpp
💤 Files with no reviewable changes (2)
- src/modules/Telemetry/HostMetrics.h
- src/modules/Telemetry/DeviceTelemetry.h
🚧 Files skipped from review as they are similar to previous changes (4)
- src/motion/MotionSensor.cpp
- .github/copilot-instructions.md
- src/modules/Telemetry/Sensor/BME680Sensor.h
- src/airtime.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/UptimeClock.cpp (1)
40-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared extended-timestamp formula to avoid divergence.
getMillisMonotonic()(Line 46) andserviceMonotonic()(Line 58) both inline the identical((((uint64_t)high << 32) | low) + (uint32_t)(getMillis() - low))expression. This is precisely the rollover-safety math this PR is built around; keeping two independent copies risks a future edit updating one and not the other, silently reintroducing a wrap bug.♻️ Proposed refactor to de-duplicate the formula
namespace { // The wrap carry, published by Time::serviceMonotonic() and read by everyone else. Split into two // 32-bit atomics behind a sequence counter: a 64-bit store is not atomic on a 32-bit MCU and the // halves must be read as a matched pair. Odd sequence = publish in progress. std::atomic<uint32_t> publishSeq{0}; std::atomic<uint32_t> publishedHigh{0}; // wraps counted as of the last publish std::atomic<uint32_t> publishedLow{0}; // getMillis() at the last publish +// Shared math: extend a published (high, low) snapshot by the elapsed time to `now`. +uint64_t extendTimestamp(uint32_t high, uint32_t low, uint32_t now) +{ + return (((uint64_t)high << 32) | low) + (uint32_t)(now - low); +} + // Seqlock read. Single writer, so this only ever retries against a publish in flight. void readPublished(uint32_t &high, uint32_t &low) { ... } } // namespace uint64_t Time::getMillisMonotonic() { uint32_t high, low; readPublished(high, low); - return ((((uint64_t)high << 32) | low) + (uint32_t)(getMillis() - low)); + return extendTimestamp(high, low, getMillis()); } ... void Time::serviceMonotonic() { const uint32_t low = publishedLow.load(std::memory_order_relaxed); const uint32_t high = publishedHigh.load(std::memory_order_relaxed); - const uint64_t next = ((((uint64_t)high << 32) | low) + (uint32_t)(getMillis() - low)); + const uint64_t next = extendTimestamp(high, low, getMillis()); ... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/UptimeClock.cpp` around lines 40 - 66, Extract the duplicated extended-timestamp calculation into a shared helper near getMillisMonotonic and reuse it from both getMillisMonotonic() and serviceMonotonic(). Preserve the existing uint32_t wraparound subtraction and uint64_t composition exactly so both paths retain identical rollover behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/gps/GPS.cpp`:
- Around line 1432-1446: Condense the explanatory comments surrounding the
post-lock ephemeris hold helper and its re-arm logic to one or two lines each.
Retain only the non-obvious rationale that fixHoldEnds == 0 means no hold is
active and must be checked explicitly because deadlinePassed() can misinterpret
the sentinel after the unsigned half-range; remove the extended narrative while
preserving the implementation.
- Around line 1447-1450: Update fixHoldInForce and the hold-deadline arming
logic so deadlines use Time::getMillis() and rollover-safe Throttle helpers
consistently. When calculating the deadline from the hold duration, remap only a
wrapped result of zero to a nonzero value, while checking the inactive zero
sentinel separately. Preserve correct active-hold behavior across clock
rollover.
In `@src/modules/NodeInfoModule.cpp`:
- Around line 38-40: Shorten the rollover explanation in the nearby
packet-timestamp comment to one or two lines, retaining only the non-obvious
reason for using seconds rather than milliseconds and avoiding the 32-bit
millisecond rollover issue. Remove secondary detail about entry lifetime and
reply suppression.
In `@test/test_gps_fix_hold/test_main.cpp`:
- Around line 150-152: Update the hold-expiry assertion near fixHoldInForce to
use a variable for the sentinel comparison instead of the tautological 0 != 0
expression, ensuring fixHoldInForce is evaluated while preserving the expected
false result for the non-expired case.
---
Nitpick comments:
In `@src/UptimeClock.cpp`:
- Around line 40-66: Extract the duplicated extended-timestamp calculation into
a shared helper near getMillisMonotonic and reuse it from both
getMillisMonotonic() and serviceMonotonic(). Preserve the existing uint32_t
wraparound subtraction and uint64_t composition exactly so both paths retain
identical rollover behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bd924aa2-5ac9-4da4-8082-c4f223512fbb
📒 Files selected for processing (14)
.github/copilot-instructions.mdAGENTS.mdsrc/UptimeClock.cppsrc/UptimeClock.hsrc/airtime.cppsrc/gps/GPS.cppsrc/main.cppsrc/modules/NodeInfoModule.cppsrc/modules/NodeInfoModule.htest/native-suite-counttest/test_airtime/test_main.cpptest/test_gps_fix_hold/test_main.cpptest/test_packet_signing/test_main.cpptest/test_uptime_clock/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/copilot-instructions.md
- src/airtime.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/test_gps_fix_hold/test_main.cpp`:
- Around line 153-155: Shorten the comment inside holdJustExpired() to no more
than two lines while preserving the essential rationale: the sentinel guard must
run every cycle because negating fixHoldInForce() alone treats an unarmed hold
as expired and may call down() incorrectly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d317a369-f986-42fe-8c1a-6644b1657dbe
📒 Files selected for processing (4)
src/UptimeClock.cppsrc/gps/GPS.cppsrc/modules/NodeInfoModule.cpptest/test_gps_fix_hold/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/NodeInfoModule.cpp
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 52 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/mesh/NodeDB.cpp:4111
- NodeDB::evictionRecency() ranks RAM-only "heard while clock untrusted" stamps by adding a 0x80000000 bias. That only guarantees the stamp is above pre-2038 epochs; but the RTC path explicitly accepts times up to BUILD_EPOCH + 40y, which can exceed this bias. In that case, a newly-heard (RAM-stamped) node can still compare as older than nodes dated with a post-2038 epoch and be evicted first, defeating the purpose of the sidecar.
Consider ranking RAM-stamped nodes as newer than any epoch-based stamp without relying on the epoch range, e.g. map them into the UINT32_MAX range based on elapsed uptime seconds.
uint32_t NodeDB::evictionRecency(const meshtastic_NodeInfoLite *n) const
{
const uint32_t stamp = heardAtUptimeSecs(n->num);
// A RAM stamp means heard this boot but not yet datable: more recent than anything dated
// before this boot. The 2^31 bias keeps stamps above every pre-2038 epoch while preserving
// their order among themselves.
return stamp ? 0x80000000u + stamp : n->last_heard;
src/modules/Telemetry/Sensor/BME680Sensor.cpp:192
- BME680Sensor::updateState() only updates lastStateSaveMs on a successful write. Once STATE_SAVE_PERIOD has elapsed since the last success (or if there has never been a success), a persistent FS failure (open/remove failing) will cause updateState() to attempt the write on every call, potentially spamming logs and holding spiLock frequently.
If the intent is "retry sooner than the full period, but not in a tight loop", add a small retry backoff in the failure path so the next attempt is delayed by (e.g.) 60s.
Collaborator
Author
|
@ianmcorvidae would you mind casting an eye over this if you get time (pun intended)? I've tried to keep it focused on solving a few outstanding issues, and not drift off into refactor for the sake of it. The thread-safety in particular is something I'm not confident I know what it should look like. |
3 tasks
|
out of curiousity, libs that use millis() as a timeout are also effected by this right ? eg: Adafruit used from https://github.com/meshtastic/firmware/blob/develop/src/modules/Telemetry/Sensor/BME280Sensor.cpp#L38 |
Collaborator
Author
Possbily - I'm not intending to delve that deep into the hinterland. Your issue with the 280 might be related - the 680 was definitely affected. |
* fix(time): avoid blocking monotonic readers * test(time): make paused-publisher check deterministic * fix(time): address review portability gaps
EvictionRecency{} is {0, false}, which evictionRecencyOlder() ranks as older than
every candidate: without the oldestIndex/oldestBoringIndex guards nothing would
ever be selected and a full node DB would stop evicting entirely.
Init to the genuine maximum instead, so the sentinel is correct on its own. The
index guards stay: two independent reasons the scan is right beats one.
The guard was widened to cover Time::getMillis() and unqualified getMillis(), and renamed to suit. Upstream branch protection matches required checks by name, so a rename means the old name never reports and merges block on a check that will never arrive. Widen the guard, keep the name; the descriptive text carries the broader scope.
Upstream meshtastic#11293 added test_nmea_wpl and took develop's count to 43; this branch had independently reached 46. Merging develop resolved the counter textually, keeping 46, while the directory set became the union of both sides at 47. The suite-count CI gate fails on the mismatch, and it gates the native test jobs, so the tests themselves were being skipped.
The concurrent-reader case started at 0xFFFFF000, leaving 0x1000 to the wrap, so the 0x800 advance annotated "cross the wrap" fell short and the wrap actually happened during the following 60s advance. Start at 0xFFFFF800 instead, so the first advance lands exactly on the wrap while the readers are running and the second is the ordinary time after it - the shape both comments already described. Total elapsed is unchanged, so the closing assertion still holds.
caveman99
requested changes
Aug 4, 2026
caveman99
left a comment
caveman99
left a comment
Member
There was a problem hiding this comment.
Sensible approach. But for the love of god, keep anecdotal documentation and source code comments to a minimum.
thebentern
approved these changes
Aug 4, 2026
thebentern
reviewed
Aug 4, 2026
thebentern
reviewed
Aug 4, 2026
thebentern
reviewed
Aug 4, 2026
Collaborator
Author
On it, boss |
…nion on my belt, which was the style at the time.
Collaborator
Author
|
@caveman99 now 21 lines lighter and less storytelling. |
caveman99
requested changes
Aug 5, 2026
The HAS_I2S_SPEAKER_NRF52 RTTTL block arrived from develop with a raw nagCycleCutoff >= millis(), which the deadline guard rejects. Use the same Throttle::deadlinePassed() form as the two sibling paths in this function.
preFSBegin() runs in the first millisecond of boot, so millis() can legitimately return 0 there. Both readers of last_format_ms treated 0 as "nothing formatted this boot", which would skip the repeat-corruption escalation and let a dead flash reformat-loop instead of reporting FLASH_CORRUPTION_UNRECOVERABLE.
The two constant getters are not constrained, and getSilentMinutes() reads the buckets without rotating them, so "the accessors mutate" was not accurate.
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Aug 6, 2026
Comment-only: with comments stripped, all five files are byte-identical to the previous commit. Removed the references to the planning notes. Those documents are working material and will go stale; the code should not depend on them. The five CHARACTERISATION tags now describe the defect they pin and stop there, and the accuracy TODO names the four defects and points at the tests instead of a plan file. Also removed, as noise rather than information: - comparisons against pre-meshtastic#11291 behaviour, which nobody reading this needs - a comment describing the lock restructure as future work, written before it landed - speculation ("plausible", "worth pinning so a future...") - an aside arguing with an arithmetic slip made while writing the test Kept the mechanical facts that are slow to re-derive: the two storage orderings and which array uses which, RX_LOG/RX_ALL_LOG disjointness, the locking rule and the addSpanned() constraint that protects it, why the re-entry assert is test-only, and the concrete numbers - (N-1)p + phase, 14 164 ms, the 20 pp contention-window steps. Net 16 comment lines out of src/, 33 out of test/.
thebentern
approved these changes
Aug 6, 2026
thebentern
enabled auto-merge
August 6, 2026 10:24
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 6, 2026
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.
Every 49.7 days the 32-bit
millis()counter wraps, and this firmware trusted it in four load-bearing ways. Comparisons made directly against it (millis() > deadline,deadline < millis()) invert while the deadline sits across the wrap, so one-shot actions fire immediately or stall for days — the nRF52 flash-corruption backoff among them.getTime()measured elapsed-since-time-set as a 32-bit delta, so a node up longer than one wrap reported a wall clock 49.7 days in the past, and everylast_heard,rx_time, message and position stamp inherited it. The queued-packetrx_timereconciliation aliased past one wrap, backdating old placeholders to plausible-but-wrong recent epochs. And a node first heard before the clock was trusted never received alast_heardat all: the phone showed "Last heard: unknown" for a node it had just announced.Deadline and interval checks now go through
Throttle—deadlinePassed()/deadlinePassedAt()where a site stores an absolute deadline,hasElapsed()(the complement ofisWithinTimespanMs()) where it stores the last event — with disarmed-sentinel values (0,UINT32_MAX) tested before the arithmetic, and amillis-deadline-checkCI job plus allowlist keeping naive compares out ofsrc/. Timestamps ride onTime::getMillisMonotonic(), a 64-bit wrap-counting read whose carry is advanced by exactly one writer —Time::serviceMonotonic(), called at the top of the mainloop()— while every reader derives its answer from that published snapshot plus the unsigned elapsed time since it, which is exact across the wrap; readers never inspect the boundary and never write back, so no number of concurrent callers can miscount a wrap. It replaces three duplicate private wrap counters (AirTime,DeviceTelemetry, and a dead pair inHostMetrics) and is documented as not ISR-safe —getMillis()remains the ISR-safe read. An earlier commit here removesgetMillis64(), which was the same shape read lazily by a single caller and so could miss a wrap outright; the restored clock differs precisely in that its publish is guaranteed. On that base,getTime()anchors in 64-bit monotonic milliseconds, so the wall clock is exact at any uptime; the untrusted-clockrx_timeplaceholder is monotonic uptime seconds, so reconciliation is exact at any age and a leaked placeholder needs ~50 years of uptime to pass for an epoch instead of 18.3 days; and nodes heard before time arrives keep their arrival instant in a RAM-only sidecar that is backfilled intolast_heardas a real epoch once the clock becomes trusted —last_heardnever persists anything but a real epoch or 0, and PhoneAPI re-reads it at nodeinfo send time so handshake ordering doesn't decide what the phone sees.Two prior PRs are adopted as the baseline with authorship preserved: #10227 (NextHopRouter retransmission rollover; its half-range compare now lives in
Throttle::deadlinePassedAt(), credited at the call site) and #10582 (AirTime monotonic windows, which now read the shared clock).Throttleand both clocks are injectable through theUptimeClocktest seam; the native suites drive the wrap boundary, the wall clock across it, and thelast_heardbackfill directly. The conventions are documented in.github/copilot-instructions.mdand mirrored toAGENTS.mdandCLAUDE.md.Summary by CodeRabbit
Bug Fixes
Tests