Fix millis() rollover in deadline, interval, and timestamp handling

[NomDeTom]

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 every last_heard, rx_time, message and position stamp inherited it. The queued-packet rx_time reconciliation 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 a last_heard at 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 of isWithinTimespanMs()) where it stores the last event — with disarmed-sentinel values (0, UINT32_MAX) tested before the arithmetic, and a millis-deadline-check CI job plus allowlist keeping naive compares out of src/. Timestamps ride on Time::getMillisMonotonic(), a 64-bit wrap-counting read whose carry is advanced by exactly one writer — Time::serviceMonotonic(), called at the top of the main loop() — 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 in HostMetrics) and is documented as not ISR-safe — getMillis() remains the ISR-safe read. An earlier commit here removes getMillis64(), 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-clock rx_time placeholder 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 into last_heard as a real epoch once the clock becomes trusted — last_heard never 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). Throttle and both clocks are injectable through the UptimeClock test seam; the native suites drive the wrap boundary, the wall clock across it, and the last_heard backfill directly. The conventions are documented in .github/copilot-instructions.md and mirrored to AGENTS.md and CLAUDE.md.

Summary by CodeRabbit

• Bug Fixes

  • Improved timer and deadline handling across millisecond counter rollover, reducing premature or delayed actions.
  • Improved power scheduling, screen timeouts, notifications, pairing windows, LED behavior, and network retries.
  • Improved airtime and utilization reporting after sleep or delayed processing.
  • Improved timestamp handling before and after the device clock becomes trusted.
  • Corrected uptime tracking and GPS fix-hold behavior across clock rollover.

• Tests

  • Added coverage for timer rollover, uptime tracking, airtime windows, deadlines, GPS fix holds, and timestamp reconciliation.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• ✅ Review completed - (🔄 Check again to review again)

📝 Walkthrough

Walkthrough

This PR adds wrap-safe time helpers, monotonic uptime publication, updated airtime and timestamp handling, migrated deadline checks, native tests, CI enforcement, and timing guidance.

Changes

Rollover-safe time handling

Layer / File(s)	Summary
Time and Throttle contracts src/UptimeClock.*, src/mesh/Throttle.*	Adds monotonic snapshot publication and wrap-safe elapsed/deadline helpers.
Monotonic airtime synchronization src/airtime.*, test/test_airtime/*	Synchronizes rolling airtime windows from monotonic uptime and tests rotation, decay, sleep gaps, and wrap behavior.
Deadline and elapsed-time migration src/Power*, src/gps/GPS.cpp, src/graphics/*, src/mesh/Throttle.*, src/modules/*, src/platform/*	Replaces direct timing arithmetic with Throttle helpers and explicit sentinel guards.
RTC and untrusted-clock timestamps src/gps/RTC.cpp, src/mesh/MeshService.*, src/mesh/NodeDB.*, src/mesh/PhoneAPI.cpp, src/mesh/Router.*	Uses monotonic anchors and uptime placeholders, then backfills epoch timestamps after clock trust.
Tests and CI guardrails test/*, .github/workflows/test_native.yml, .github/millis-deadline-allowlist.txt, AGENTS.md, .github/copilot-instructions.md	Adds coverage for wraps, sentinels, concurrency, fix holds, timestamp handling, and unsafe millis() comparisons.

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

Loading

sequenceDiagram
  participant Radio
  participant NodeDB
  participant RTC
  Radio->>NodeDB: record uptime placeholder
  RTC->>NodeDB: signal trusted clock
  NodeDB->>NodeDB: backfill last_heard epoch

Loading

Possibly related issues

• [Tracking]: Time handling: scattered millis() #11281: Covers the same rollover-safe millis() deadline remediation and sentinel handling.

Possibly related PRs

• meshtastic/firmware#10227: Directly overlaps retransmission deadline handling.
• meshtastic/firmware#10582: Directly overlaps airtime synchronization.
• meshtastic/firmware#11274: Directly overlaps RTC and packet timestamp handling.

Suggested labels: requires-protos, enhancement

Suggested reviewers: rcgv1

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 41.44% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the primary fix for rollover-safe deadline, interval, and timestamp handling.
Description check	✅ Passed	The description is detailed and on-topic, but it omits the template's Attestations section and explicit hardware-testing status.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[NomDeTom]

@RCGV1 here's my time PR

[github-actions]

⚡ Try this PR in the Web Flasher

Warning

This is an automated, unreviewed CI test build. Back up your device configuration
before flashing, and only flash devices you are able to recover.

Supported boards built by this PR (14)

	Device	Board	Platform
	Heltec HT62	heltec-ht62-esp32c3-sx1262	esp32-c3
	Heltec Mesh Node T114	heltec-mesh-node-t114	nrf52840
	Heltec V3	heltec-v3	esp32-s3
	Raspberry Pi Pico W	picow	rp2040
	RAK WisMesh Pocket V3	rak_wismesh_pocket	nrf52840
	RAK WisMesh Repeater Mini V2	rak_wismesh_repeater_mini	nrf52840
	RAK WisBlock 4631	rak4631	nrf52840
	Seeed SenseCAP Mesh-Tracker-X1	seeed_mesh_tracker_X1	nrf52840
	Seeed Wio Tracker L1	seeed_wio_tracker_L1	nrf52840
	Seeed SenseCAP Indicator	seeed-sensecap-indicator-tft	esp32-s3
	LILYGO T-Deck	t-deck-tft	esp32-s3
	LILYGO T-Echo Plus	t-echo-plus	nrf52840
	LilyGo T3-C6	tlora-c6	esp32-c6
	Seeed SenseCAP T1000-E	tracker-t1000-e	nrf52840

Build artifacts expire on 2026-08-30. Updated for 44a53ed.

[coderabbitai]

Actionable comments posted: 7

🧹 Nitpick comments (2)

src/airtime.cpp (1)

78-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Drop the unused lastUtilPeriod markers.

lastUtilPeriod and lastUtilPeriodTX are only assigned in AirTime::syncNow() and never read elsewhere. Remove the declarations from src/airtime.h and the assignments at src/airtime.cpp:78-79 and 134, 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 win

Use the injected clock for lastStateSaveMs.

millis() bypasses the injectable timing path, making this checkpoint uncontrolled by Time::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

📥 Commits

Reviewing files that changed from the base of the PR and between 9c260ad and d84b960.

📒 Files selected for processing (35)

• .github/copilot-instructions.md
• .github/millis-deadline-allowlist.txt
• .github/workflows/test_native.yml
• AGENTS.md
• CLAUDE.md
• src/Power.cpp
• src/PowerFSMThread.h
• src/UptimeClock.cpp
• src/UptimeClock.h
• src/airtime.cpp
• src/airtime.h
• src/gps/GPS.cpp
• src/graphics/EInkDynamicDisplay.cpp
• src/graphics/Screen.cpp
• src/graphics/draw/NotificationRenderer.cpp
• src/input/RotaryEncoderImpl.cpp
• src/mesh/NextHopRouter.cpp
• src/mesh/Throttle.cpp
• src/mesh/Throttle.h
• src/mesh/eth/ethClient.cpp
• src/modules/DropzoneModule.cpp
• src/modules/ExternalNotificationModule.cpp
• src/modules/NodeInfoModule.cpp
• src/modules/StatusLEDModule.cpp
• src/modules/Telemetry/Sensor/BME680Sensor.cpp
• src/modules/Telemetry/Sensor/BME680Sensor.h
• src/motion/MotionSensor.cpp
• src/platform/extra_variants/t5s3_epaper/variant.cpp
• src/platform/nrf52/NRF52Bluetooth.cpp
• src/platform/nrf52/main-nrf52.cpp
• test/native-suite-count
• test/test_airtime/test_main.cpp
• test/test_packet_signing/test_main.cpp
• test/test_throttle/test_main.cpp
• test/test_uptime_clock/test_main.cpp

💤 Files with no reviewable changes (1)

• src/UptimeClock.cpp

[Copilot]

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) and Throttle::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-deadline millis() 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]

@coderabbitai can you check if I've resolved your comments?

[coderabbitai]

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.cpp

Length of output: 51409

---

@NomDeTom: Yes — I verified the current PR head, and all five remaining findings are resolved:

• CI deadline guard now uses set -euo pipefail.
• Retransmission due checks use the snapshot-aware Throttle::deadlinePassedAt().
• BME680 save checkpoints are recorded only after a successful write path.
• The packet-signing test uses Time::getMillis().
• The rollover test comment correctly states 0x00000090.

The two other findings were already withdrawn as inapplicable. Resolving my review comments now.

[resolve]

_{You are interacting with an AI system.}

[coderabbitai]

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 win

Restore test-global duty-cycle state in teardown

test_C14 leaves config.lora.region at EU_868 and airTime->utilizationTX[0] at MS_IN_HOUR when a body assertion fails, because tearDown() 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 win

Rotation copies air_period_* from the airtimes.* arrays.

Lines 107-108 source air_period_tx[i+1]/air_period_rx[i+1] from airtimes.periodTX[i]/periodRX[i] rather than from air_period_tx[i]/air_period_rx[i]. Today the two arrays hold identical values (both incremented in logAirtime), 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

📥 Commits

Reviewing files that changed from the base of the PR and between d84b960 and 105aea6.

📒 Files selected for processing (49)

• .github/copilot-instructions.md
• .github/millis-deadline-allowlist.txt
• .github/workflows/test_native.yml
• AGENTS.md
• CLAUDE.md
• src/Power.cpp
• src/PowerFSMThread.h
• src/UptimeClock.cpp
• src/UptimeClock.h
• src/airtime.cpp
• src/airtime.h
• src/gps/GPS.cpp
• src/gps/RTC.cpp
• src/graphics/EInkDynamicDisplay.cpp
• src/graphics/Screen.cpp
• src/graphics/draw/NotificationRenderer.cpp
• src/input/RotaryEncoderImpl.cpp
• src/mesh/MeshService.cpp
• src/mesh/MeshService.h
• src/mesh/NextHopRouter.cpp
• src/mesh/NodeDB.cpp
• src/mesh/NodeDB.h
• src/mesh/PhoneAPI.cpp
• src/mesh/Router.cpp
• src/mesh/Router.h
• src/mesh/Throttle.cpp
• src/mesh/Throttle.h
• src/mesh/eth/ethClient.cpp
• src/modules/DropzoneModule.cpp
• src/modules/ExternalNotificationModule.cpp
• src/modules/NodeInfoModule.cpp
• src/modules/StatusLEDModule.cpp
• src/modules/Telemetry/DeviceTelemetry.cpp
• src/modules/Telemetry/DeviceTelemetry.h
• src/modules/Telemetry/HostMetrics.h
• src/modules/Telemetry/Sensor/BME680Sensor.cpp
• src/modules/Telemetry/Sensor/BME680Sensor.h
• src/motion/MotionSensor.cpp
• src/platform/extra_variants/t5s3_epaper/variant.cpp
• src/platform/nrf52/NRF52Bluetooth.cpp
• src/platform/nrf52/main-nrf52.cpp
• test/native-suite-count
• test/test_airtime/test_main.cpp
• test/test_meshpacket_serializer/ports/test_timestamp.cpp
• test/test_meshpacket_serializer/test_helpers.h
• test/test_packet_signing/test_main.cpp
• test/test_stream_api/test_main.cpp
• test/test_throttle/test_main.cpp
• test/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

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (1)

src/UptimeClock.cpp (1)

40-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the shared extended-timestamp formula to avoid divergence.

getMillisMonotonic() (Line 46) and serviceMonotonic() (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

📥 Commits

Reviewing files that changed from the base of the PR and between 105aea6 and 274e598.

📒 Files selected for processing (14)

• .github/copilot-instructions.md
• AGENTS.md
• src/UptimeClock.cpp
• src/UptimeClock.h
• src/airtime.cpp
• src/gps/GPS.cpp
• src/main.cpp
• src/modules/NodeInfoModule.cpp
• src/modules/NodeInfoModule.h
• test/native-suite-count
• test/test_airtime/test_main.cpp
• test/test_gps_fix_hold/test_main.cpp
• test/test_packet_signing/test_main.cpp
• test/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

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 274e598 and 4be26bc.

📒 Files selected for processing (4)

• src/UptimeClock.cpp
• src/gps/GPS.cpp
• src/modules/NodeInfoModule.cpp
• test/test_gps_fix_hold/test_main.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

• src/modules/NodeInfoModule.cpp

[Copilot]

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.

[NomDeTom]

@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.

[mcenderdragon]

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

[NomDeTom]

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

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.

[NomDeTom]

@thebentern putting this one back a bit - it was tripping CI. Now ignored.

[NomDeTom]

I have a better fix for this, but it won't be possible until Wednesday night