Arrival time fix perhaps - #11274
Merged
thebentern merged 9 commits intoJul 29, 2026
Merged
Conversation
rx_time is now proto3 optional with a has_rx_time presence bit, matching the rx_rssi treatment. A node with no GPS and no phone connected yet has no time source at all, so a bare 0 was indistinguishable from a genuine 1970-01-01 reading; downstream consumers (replay packets, JSON serialization) now check has_rx_time instead of the value.
Extract the repeated haveTime/rx_time/has_rx_time stamp logic (5 call sites across Router.cpp, MeshBeaconModule.cpp, MeshService.cpp) into Router::computeRxTimeStamp()/stampRxTime(). Also shorten the new RTC.cpp LOG_DEBUG string. Saves 48 bytes of flash on rak4631 (measured), no behavior change.
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change introduces monotonic uptime timing and explicit ChangesReceive timestamp contracts and stamping
RTC reconciliation and time consumers
Replay, storage, and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RTC
participant MeshService
participant toPhoneQueue
RTC->>MeshService: onTimeSourceQualityChanged()
MeshService->>toPhoneQueue: inspect packets with has_rx_time=false
MeshService->>MeshService: convert millis placeholder to epoch rx_time
MeshService->>toPhoneQueue: set has_rx_time=true and re-enqueue
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
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 (31)
Build artifacts expire on 2026-08-28. Updated for |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a monotonic uptime clock abstraction (Time::getMillis() / Time::getMillis64()) and refactors meshtastic_MeshPacket::rx_time to have explicit presence via has_rx_time, ensuring packets only claim a valid wall-clock timestamp when RTC quality is trustworthy. It also adds a reconciliation step to retroactively convert queued “unknown-time” packets (carrying a millis placeholder) into real epochs once net-quality time becomes available.
Changes:
- Added
src/Time.{h,cpp}as a single test-injectable seam for monotonic uptime timekeeping. - Implemented explicit
rx_timepresence (has_rx_time) across packet creation/receive, serialization, NodeDB updates, and replay paths. - Added
MeshService::reconcilePendingRxTimes()and hooked it into RTC quality transitions to backdate queued packets once time becomes trustworthy.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_traffic_management/test_main.cpp | Updates synthetic test packet to set has_rx_time=true. |
| test/test_meshpacket_serializer/test_helpers.h | Updates synthetic test helper packet to set has_rx_time=true. |
| src/Time.h | New monotonic uptime API + unit-test injection seam. |
| src/Time.cpp | Implements Time::getMillis() and Time::getMillis64(). |
| src/serialization/MeshPacketSerializer.cpp | Emits JSON timestamp=0 when has_rx_time=false to avoid leaking placeholder values. |
| src/modules/StoreForwardModule.cpp | Sets has_rx_time for stored history packets (needs adjustment; see PR comments). |
| src/modules/NodeInfoModule.cpp | Switches NodeInfo reply-suppression timing to gate on has_rx_time. |
| src/modules/MeshBeaconModule.cpp | Uses stampRxTime() so beacons don’t incorrectly appear to have epoch time when clock is untrusted. |
| src/mesh/Router.h | Introduces RxTimeStamp, computeRxTimeStamp(), and stampRxTime() helpers. |
| src/mesh/Router.cpp | Centralizes rx-time stamping and applies placeholder semantics in dispatchReceived() and send allocation. |
| src/mesh/RadioInterface.cpp | Prints rx time only when has_rx_time is set. |
| src/mesh/PhoneAPI.cpp | Sets has_rx_time when replaying packets based on whether last_heard exists. |
| src/mesh/NodeDB.cpp | Ensures last_heard and age calculations are gated on has_rx_time. |
| src/mesh/MeshService.h | Declares reconcilePendingRxTimes(). |
| src/mesh/MeshService.cpp | Implements rx-time reconciliation for queued-to-phone packets once RTC quality improves. |
| src/mesh/generated/meshtastic/mesh.pb.h | Updates generated nanopb bindings: makes rx_time optional and adds has_rx_time. |
| src/gps/RTC.cpp | Triggers rx-time reconciliation when RTC quality crosses RTCQualityFromNet. |
| src/configuration.h | Includes Time.h globally for consistent access to the uptime seam. |
![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: 3
🧹 Nitpick comments (3)
src/Time.h (1)
7-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the clock documentation within the repository’s comment-size rule.
src/Time.h#L7-L25: shorten the multi-paragraph API rationale to a compact contract comment.src/Time.cpp#L1-L10: shorten the implementation header comment and move extended rationale to external documentation.As per coding guidelines, C++ comments should be one or two lines maximum and avoid multi-paragraph explanations.
🤖 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/Time.h` around lines 7 - 25, Shorten the clock documentation in src/Time.h lines 7-25 to a compact one- or two-line contract covering monotonic uptime, getMillis64() rollover behavior, and the ISR-safety distinction; remove the extended rationale. Also shorten the implementation header comment in src/Time.cpp lines 1-10 to the same concise style, moving no additional rationale into code comments.Source: Coding guidelines
test/test_meshpacket_serializer/test_helpers.h (1)
40-40: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for absent receive timestamps.
This shared fixture now always exercises the present case. Add a parameter or companion fixture with
has_rx_time = falseand a nonzero placeholder, then assert serializers emit zero rather than the placeholder.🤖 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_meshpacket_serializer/test_helpers.h` at line 40, Add coverage alongside the shared packet fixture for an absent receive timestamp by setting has_rx_time to false while retaining a nonzero placeholder rx_time. Update the relevant serializer tests to use this fixture and assert they emit zero instead of the placeholder, while preserving the existing present-timestamp coverage.src/mesh/Router.h (1)
13-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the new timestamp comments concise. These blocks repeat one cross-layer contract and exceed the repository’s one-to-two-line comment limit.
src/mesh/Router.h#L13-L15: reduce theRxTimeStampcontract to its essential invariant.src/mesh/Router.cpp#L296-L299: shorten the allocation-time placeholder explanation.src/mesh/Router.cpp#L1325-L1332: shorten the dispatch-time timestamp explanation.src/mesh/Router.cpp#L1472-L1476: shorten the trace-path explanation.src/mesh/MeshService.cpp#L183-L188: shorten the reconciliation rationale.src/mesh/MeshService.cpp#L253-L257: shorten the injection-path explanation.src/mesh/MeshService.cpp#L296-L299: shorten the phone-originated packet explanation.src/mesh/MeshService.cpp#L637-L640: shorten the age-calculation explanation.src/modules/MeshBeaconModule.cpp#L345-L349: shorten the beacon stamping explanation.src/modules/StoreForwardModule.cpp#L260-L267: shorten the replay timestamp explanation.src/gps/RTC.cpp#L21-L24: shorten the threshold-crossing explanation.src/mesh/MeshService.h#L140-L143: shorten the method declaration comment.src/mesh/NodeDB.cpp#L3236-L3239: shorten thesinceReceived()explanation.src/mesh/NodeDB.cpp#L3639-L3642: shorten thelast_heardexplanation.🤖 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/mesh/Router.h` around lines 13 - 15, The timestamp comments are overly verbose; reduce each listed comment to one or two concise lines while preserving only the essential RxTimeStamp invariant and the specific behavior relevant to its nearby code. Update the comments at src/mesh/Router.h:13-15, src/mesh/Router.cpp:296-299, 1325-1332, and 1472-1476; src/mesh/MeshService.cpp:183-188, 253-257, 296-299, and 637-640; src/modules/MeshBeaconModule.cpp:345-349; src/modules/StoreForwardModule.cpp:260-267; src/gps/RTC.cpp:21-24; src/mesh/MeshService.h:140-143; and src/mesh/NodeDB.cpp:3236-3239 and 3639-3642.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 `@src/modules/NodeInfoModule.cpp`:
- Around line 36-38: Update the NodeInfo suppression-window logic around
lastNodeInfoSeen to use a single monotonic Time value for both the current
timestamp and stored comparisons, rather than selecting between mp.rx_time and
getTime(). Preserve the existing has_rx_time handling for packet metadata, but
ensure an unset or adjusted RTC cannot cause repeated requests to remain
suppressed or extend the throttle window.
In `@src/modules/StoreForwardModule.cpp`:
- Line 268: Update the packet history record and its write path to persist the
original timestamp validity separately from the history-age field and retain the
captured timestamp value. In the restore logic around has_rx_time in
StoreForwardModule, assign the stored timestamp and presence state instead of
deriving validity from the current getRTCQuality() result, preserving correct
handling by reconcilePendingRxTimes().
In `@src/Time.cpp`:
- Around line 22-31: The 64-bit time accumulator must be rebased when the clock
source changes or resets. In src/Time.cpp lines 22-31, update Time::getMillis64
and its static state so lastLow/highWord are reset or rebased after
injected-clock resets; in src/Time.h lines 45-50, make useRealClock() invoke
that rebase instead of only clearing test-clock variables.
---
Nitpick comments:
In `@src/mesh/Router.h`:
- Around line 13-15: The timestamp comments are overly verbose; reduce each
listed comment to one or two concise lines while preserving only the essential
RxTimeStamp invariant and the specific behavior relevant to its nearby code.
Update the comments at src/mesh/Router.h:13-15, src/mesh/Router.cpp:296-299,
1325-1332, and 1472-1476; src/mesh/MeshService.cpp:183-188, 253-257, 296-299,
and 637-640; src/modules/MeshBeaconModule.cpp:345-349;
src/modules/StoreForwardModule.cpp:260-267; src/gps/RTC.cpp:21-24;
src/mesh/MeshService.h:140-143; and src/mesh/NodeDB.cpp:3236-3239 and 3639-3642.
In `@src/Time.h`:
- Around line 7-25: Shorten the clock documentation in src/Time.h lines 7-25 to
a compact one- or two-line contract covering monotonic uptime, getMillis64()
rollover behavior, and the ISR-safety distinction; remove the extended
rationale. Also shorten the implementation header comment in src/Time.cpp lines
1-10 to the same concise style, moving no additional rationale into code
comments.
In `@test/test_meshpacket_serializer/test_helpers.h`:
- Line 40: Add coverage alongside the shared packet fixture for an absent
receive timestamp by setting has_rx_time to false while retaining a nonzero
placeholder rx_time. Update the relevant serializer tests to use this fixture
and assert they emit zero instead of the placeholder, while preserving the
existing present-timestamp coverage.
🪄 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: e23d641a-1439-4b1e-8e40-23fc5e27eda2
⛔ Files ignored due to path filters (1)
src/mesh/generated/meshtastic/mesh.pb.his excluded by!**/generated/**,!src/mesh/generated/**
📒 Files selected for processing (18)
protobufssrc/Time.cppsrc/Time.hsrc/configuration.hsrc/gps/RTC.cppsrc/mesh/MeshService.cppsrc/mesh/MeshService.hsrc/mesh/NodeDB.cppsrc/mesh/PhoneAPI.cppsrc/mesh/RadioInterface.cppsrc/mesh/Router.cppsrc/mesh/Router.hsrc/modules/MeshBeaconModule.cppsrc/modules/NodeInfoModule.cppsrc/modules/StoreForwardModule.cppsrc/serialization/MeshPacketSerializer.cpptest/test_meshpacket_serializer/test_helpers.htest/test_traffic_management/test_main.cpp
… replay preparePayload() set has_rx_rssi = true unconditionally on replay, regardless of whether the packet's rx_rssi at store time was a genuine measurement (e.g. MQTT-relayed packets carry no real RSSI). Store the presence bit alongside rx_rssi in PacketHistoryStruct and restore it on replay instead. Flagged by Copilot on meshtastic#11271 (same root cause the has_rx_time explicit presence work fixes) but never addressed before that PR merged.
.github/copilot-instructions.md:338 caps code comments at 1-2 lines; several blocks added across the rx_time explicit-presence work ran well past that. Also consolidates Time.cpp's file-level doc comment into Time.h, where the rest of the Time:: API contract already lives. No behavior change.
- test_meshpacket_serializer: has_rx_time=false fixture plus tests asserting JsonSerialize/JsonSerializeEncrypted emit 0 rather than leaking the millis() placeholder, alongside the has_rx_time=true baseline. - test_stream_api: two tests driving a real PhoneAPI handshake (want_config_id through STATE_SEND_PACKETS) that simulate a phone time-giving transaction arriving before vs. after a queued packet is drained - covering both the reconciled and the ships-with-placeholder-absent paths of MeshService::reconcilePendingRxTimes().
- Time.h: drop the reserved-identifier include guard (_MT_TIME_H); pragma once already covers it, matching convention elsewhere (e.g. RTC.h). - Time.cpp: rebase getMillis64()'s wrap accumulator when the test seam swaps clock sources, so a real<->injected clock jump isn't miscounted as a genuine 32-bit wrap. - NodeInfoModule: the 12h reply-suppression window is a local dedup duration, not a wall-clock reading - switch it to Time::getMillis64() so RTC-quality jumps and replayed packets' stale rx_time can't perturb it. - StoreForwardModule: has_rx_time was derived from *current* RTC quality at replay time rather than stored at capture time, so a history entry saved while time-blind could be misreported as a valid epoch once the clock later improved. Persist the presence bit in PacketHistoryStruct instead.
Collaborator
Author
|
Addressed all actionable review feedback: Correctness fixes:
Style: trimmed every flagged comment block to the repo's 1-2 line guideline ( Test coverage:
Verification: native suite 793/793 passing; Commits are split by concern: |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/test_stream_api/test_main.cpp (1)
571-643: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAssertion failure mid-test leaks global
nodeDB/Time/RTC state to later tests.Both new tests restore
nodeDB,Time::useRealClock(), and RTC state only at the very end of the function. Unity'sTEST_ASSERT*aborts vialongjmpon failure, which skips both the remaining restore statements and the destructors oftestNodeDB/ScopedMeshService/PhoneAPITestShim. If any assertion before the restore lines fails (e.g.drainHandshakeForPacketFromreturns false, or thehas_rx_time/rx_timechecks fail),nodeDBis left pointing at a destroyed stack object andTimestays on the injected test clock for every subsequent test in the binary — turning one genuine failure into a cascade of unrelated ones.Consider an RAII guard (mirroring the existing
ScopedMeshServicepattern) that swapsnodeDBin its constructor and restoresnodeDB/Time/RTC state in its destructor, so cleanup runs even when an assertion aborts the test.🤖 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_stream_api/test_main.cpp` around lines 571 - 643, Make both handshake tests exception/longjmp-safe by introducing an RAII cleanup guard, modeled on ScopedMeshService, that restores nodeDB, switches back to the real clock, and resets RTC state in its destructor. Initialize it before replacing nodeDB and remove the manual end-of-test restoration so cleanup also occurs when any TEST_ASSERT aborts execution.
🤖 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.
Nitpick comments:
In `@test/test_stream_api/test_main.cpp`:
- Around line 571-643: Make both handshake tests exception/longjmp-safe by
introducing an RAII cleanup guard, modeled on ScopedMeshService, that restores
nodeDB, switches back to the real clock, and resets RTC state in its destructor.
Initialize it before replacing nodeDB and remove the manual end-of-test
restoration so cleanup also occurs when any TEST_ASSERT aborts execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 407e7451-5235-426a-91a3-316f50cf76af
📒 Files selected for processing (18)
src/Time.cppsrc/Time.hsrc/gps/RTC.cppsrc/mesh/MeshService.cppsrc/mesh/MeshService.hsrc/mesh/NodeDB.cppsrc/mesh/PhoneAPI.cppsrc/mesh/Router.cppsrc/mesh/Router.hsrc/modules/MeshBeaconModule.cppsrc/modules/NodeInfoModule.cppsrc/modules/StoreForwardModule.cppsrc/modules/StoreForwardModule.hsrc/serialization/MeshPacketSerializer.cpptest/test_meshpacket_serializer/ports/test_timestamp.cpptest/test_meshpacket_serializer/test_helpers.htest/test_meshpacket_serializer/test_serializer.cpptest/test_stream_api/test_main.cpp
💤 Files with no reviewable changes (1)
- src/modules/MeshBeaconModule.cpp
🚧 Files skipped from review as they are similar to previous changes (11)
- test/test_meshpacket_serializer/test_helpers.h
- src/Time.cpp
- src/serialization/MeshPacketSerializer.cpp
- src/mesh/MeshService.h
- src/Time.h
- src/mesh/NodeDB.cpp
- src/mesh/Router.h
- src/mesh/MeshService.cpp
- src/gps/RTC.cpp
- src/mesh/PhoneAPI.cpp
- src/mesh/Router.cpp
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
src/mesh/PhoneAPI.cpp:1293
- pkt.has_rx_time is currently derived from
header->last_heard != 0, but NodeDB::addFromContact can stamp last_heard via getTime() (boot-relative seconds) even when RTCQualityNone, making last_heard nonzero but not a real epoch. That will cause telemetry replay packets (and their embedded Telemetry.time field) to claim a valid timestamp and leak a bogus “epoch” to the phone.
Consider only treating last_heard as an epoch when it is outside the boot-relative wrap range (getTime() when RTCQualityNone stays < ~4,294,967).
const meshtastic_NodeInfoLite *header = nodeDB->getMeshNode(num);
pkt.rx_time = header ? header->last_heard : 0;
// last_heard is only ever written from a has_rx_time-gated reception (NodeDB::updateFrom),
// so a legacy 0 unambiguously means "never".
pkt.has_rx_time = (header && header->last_heard != 0);
src/mesh/PhoneAPI.cpp:1470
- pkt.has_rx_time is currently derived from
header->last_heard != 0, but NodeDB::addFromContact can stamp last_heard via getTime() (boot-relative seconds) even when RTCQualityNone, making last_heard nonzero but not a real epoch. That will cause status replay packets to claim a valid timestamp and leak a bogus “epoch” to the phone.
Consider only treating last_heard as an epoch when it is outside the boot-relative wrap range (getTime() when RTCQualityNone stays < ~4,294,967).
const meshtastic_NodeInfoLite *header = nodeDB->getMeshNode(num);
pkt.rx_time = header ? header->last_heard : 0;
// last_heard is only ever written from a has_rx_time-gated reception (NodeDB::updateFrom),
// so a legacy 0 unambiguously means "never".
pkt.has_rx_time = (header && header->last_heard != 0);
src/mesh/PhoneAPI.cpp:1402
- pkt.has_rx_time is currently derived from
header->last_heard != 0, but NodeDB::addFromContact can stamp last_heard via getTime() (boot-relative seconds) even when RTCQualityNone, making last_heard nonzero but not a real epoch. That will cause environment replay packets (and their embedded Telemetry.time field) to claim a valid timestamp and leak a bogus “epoch” to the phone.
Consider only treating last_heard as an epoch when it is outside the boot-relative wrap range (getTime() when RTCQualityNone stays < ~4,294,967).
const meshtastic_NodeInfoLite *header = nodeDB->getMeshNode(num);
pkt.rx_time = header ? header->last_heard : 0;
// last_heard is only ever written from a has_rx_time-gated reception (NodeDB::updateFrom),
// so a legacy 0 unambiguously means "never".
pkt.has_rx_time = (header && header->last_heard != 0);
src/modules/StoreForwardModule.cpp:264
- StoreForwardModule local-to-phone replay sets p->has_rx_time based on when the packet was stored (good), but when has_rx_time is false it still copies
time = getTime()into p->rx_time. When RTCQualityNone, getTime() is seconds since boot, but the new reconciliation logic (MeshService::reconcilePendingRxTimes) assumes rx_time placeholders are milliseconds since boot (Time::getMillis()). If the clock becomes trustworthy while these S&F packets are still queued, they can be reconciled to the wrong epoch.
To keep placeholder units consistent, convert the stored boot-seconds value into a millis()-style placeholder when has_rx_time is false.
p->rx_time = this->packetHistory[i].time;
p->has_rx_time = this->packetHistory[i].has_rx_time; // presence captured at store time, not replay time
src/mesh/PhoneAPI.cpp:1263
- pkt.has_rx_time is currently derived from
header->last_heard != 0, but NodeDB::addFromContact can stamp last_heard via getTime() (boot-relative seconds) even when RTCQualityNone (see NodeDB.cpp around addFromContact), making last_heard nonzero but not a real epoch. That will cause replay packets to claim a valid timestamp and leak a bogus “epoch” to the phone.
Consider only treating last_heard as an epoch when it is outside the boot-relative wrap range (getTime() when RTCQualityNone stays < ~4,294,967).
This issue also appears in the following locations of the same file:
- line 1289
- line 1398
- line 1466
pkt.rx_time = header ? header->last_heard : 0;
// last_heard is only ever written from a has_rx_time-gated reception (NodeDB::updateFrom),
// so a legacy 0 unambiguously means "never".
pkt.has_rx_time = (header && header->last_heard != 0);
src/mesh/MeshService.cpp:194
- reconcilePendingRxTimes() currently rewrites all queued packets with has_rx_time==false, including packets where rx_time==0 is being used as an explicit “unknown/never” sentinel (e.g. replay packets that intentionally have no timestamp). In that case, the unsigned subtraction makes elapsedMs huge and the code will fabricate a bogus epoch timestamp.
Consider skipping reconciliation when rx_time==0 so “unknown” stays unknown; reconciliation should only run on real millis() placeholders captured at receive time.
if (!p->has_rx_time) {
NomDeTom
marked this pull request as ready for review
July 29, 2026 00:48
Collaborator
Author
|
@coderabbitai review, please |
Contributor
|
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
test/test_stream_api/test_main.cpp:643
- This test depends on time(NULL), so its input varies with the system clock. Prefer a fixed epoch value to keep the unit test deterministic and avoid flakes if CI time changes.
struct timeval networkTime;
networkTime.tv_sec = time(NULL) + SEC_PER_DAY;
networkTime.tv_usec = 0;
test/test_stream_api/test_main.cpp:607
- This test depends on the host wall clock via time(NULL), which makes it non-deterministic and can introduce flaky behavior (e.g., if system time changes during CI runs). Use a fixed epoch value instead so the test outcome is stable.
This issue also appears on line 641 of the same file.
struct timeval networkTime;
networkTime.tv_sec = time(NULL) + SEC_PER_DAY;
networkTime.tv_usec = 0;
This was referenced Jul 30, 2026
This was referenced Aug 10, 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.
This pull request introduces a robust monotonic uptime clock abstraction and refactors how packet reception timestamps (
rx_time) are handled throughout the codebase. The main goals are to provide testable, rollover-immune timekeeping and to ensure that packets only claim to have a valid wall-clock timestamp when the device actually has trustworthy time. The changes also ensure that packets received before the wall clock is trustworthy are later reconciled to real epochs once accurate time becomes available.The most important changes are:
Monotonic uptime clock abstraction:
Timenamespace (src/Time.h,src/Time.cpp) providinggetMillis()(32-bit, wraps every ~49.7 days) andgetMillis64()(64-bit, rollover-immune) for monotonic uptime tracking, with support for test injection. This is now included project-wide for consistent timekeeping. [1] [2] [3]rx_time semantics and presence tracking:
has_rx_timeflag: when the wall clock is not trustworthy,rx_timeis set to a monotonic millis() placeholder andhas_rx_timeis false. Only when real time is available doesrx_timerepresent an actual epoch andhas_rx_timeis true. All code readingrx_timeis updated to checkhas_rx_timeinstead of assuming nonzero means valid. [1] [2] [3] [4] [5] [6] [7] [8]Deferred timestamp reconciliation:
MeshService::reconcilePendingRxTimes(), which scans queued packets with placeholderrx_timevalues and, once the wall clock becomes trustworthy, backdates them to a real epoch based on elapsed uptime. This ensures that packets received before accurate time was available are not left with bogus or missing timestamps. [1] [2] [3]Consistent application across the codebase:
rx_timenow consistently use the new semantics, including packet allocation, injection, phone API replay, and NodeDB updates. This prevents accidental use of monotonic placeholders as real epochs and ensures correct presence signaling throughout. [1] [2] [3] [4]Submodule update:
protobufssubmodule to a new commit, likely to reflect protocol changes supporting the newhas_rx_timesemantics.🤝 Attestations
Summary by CodeRabbit