chore(deps): bump protobufs to 2.7.26.140 (optional rx_time) - #6510
Conversation
MeshPacket.rx_time became a proto3 optional field upstream, so Wire now generates it as Int? instead of Int. Old firmware still sends 0 when it has no clock at reception, so both absent and 0 mean "unknown" — route every call site through a single rxTimeOrNull() helper rather than scattering ?: 0 and losing the old-firmware case. Two sites needed more than null-safety: applySenderPacketUpdate would have written the epoch into lastHeard for an unstamped packet, and oldestTimestampSeconds let an unknown timestamp win the min() and collapse every chart's time axis to 1970. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesRx-time normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshMessageProcessorImplTest.kt (1)
566-585: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a regression assertion for
lastHeardpreservation.This test only verifies that the emitted packet receives a positive timestamp; it would still pass while the sender-node
lastHeardbug remains. Capture the transform passed toupdateNodeAndPersist, apply it to a remote node with an existinglastHeard, and assert that null/zerorx_timeleaves it unchanged.🤖 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 `@core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshMessageProcessorImplTest.kt` around lines 566 - 585, Extend the test `packets with absent rx_time get current time` to capture the transform supplied to `updateNodeAndPersist`, apply it to a remote node whose `lastHeard` is already set, and assert that a null or zero packet `rx_time` preserves the existing `lastHeard`. Keep the current positive emitted-timestamp assertion, and use the captured transform rather than testing only the emitted packet.
🤖 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
`@core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshMessageProcessorImpl.kt`:
- Around line 186-187: Preserve the original nullable receive timestamp before
creating stampedPacket in the packet-processing flow. Update
applySenderPacketUpdate to use that raw timestamp when calculating the sender
node’s lastHeard, so null or zero values retain node.lastHeard, while downstream
consumers continue receiving the normalized preparedPacket.
In
`@feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/SignalMetrics.kt`:
- Line 140: Preserve unknown receive times as nullable throughout PacketEntry
and its consumers: in
feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/SignalMetrics.kt:140,
keep timeSeconds nullable; at 156 filter via nullable extraction, at 357-363
exclude null timestamps from plotting, and at 559 avoid rendering them as epoch
time. In
feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/MetricsViewModel.kt:456,
omit unstamped rows or emit blank date/time CSV fields; do not use zero as a
sentinel.
---
Nitpick comments:
In
`@core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshMessageProcessorImplTest.kt`:
- Around line 566-585: Extend the test `packets with absent rx_time get current
time` to capture the transform supplied to `updateNodeAndPersist`, apply it to a
remote node whose `lastHeard` is already set, and assert that a null or zero
packet `rx_time` preserves the existing `lastHeard`. Keep the current positive
emitted-timestamp assertion, and use the captured transform rather than testing
only the emitted packet.
🪄 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: ee20e89f-fca6-4500-a8c5-01cf9cf27892
📒 Files selected for processing (10)
core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshMessageProcessorImpl.ktcore/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshMessageProcessorImplTest.ktcore/model/src/commonMain/kotlin/org/meshtastic/core/model/Message.ktcore/model/src/commonMain/kotlin/org/meshtastic/core/model/util/Extensions.ktcore/model/src/commonMain/kotlin/org/meshtastic/core/model/util/MeshDataMapper.ktcore/model/src/commonTest/kotlin/org/meshtastic/core/model/util/RxTimeExtensionsTest.ktfeature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/MetricsViewModel.ktfeature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/SignalMetrics.ktfeature/node/src/commonMain/kotlin/org/meshtastic/feature/node/model/MetricsState.ktgradle/libs.versions.toml
Review follow-up: rather than relying on the threshold comparison to incidentally exclude them, filter unstamped packets explicitly so the downstream `?: 0` fallbacks are provably unreachable rather than a 1970 sentinel waiting to render. Also clarifies why the lastHeard fallback is unreachable by design: a packet that just arrived SHOULD refresh lastHeard from the phone's clock, so the fallback guards only against a caller that skips normalization. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Why
Upstream made
MeshPacket.rx_timea proto3optional fixed32(protobufs #1025), so firmware can now say "I had no trustworthy clock when this packet arrived" instead of emitting a0that reads as 1970-01-01. Wire generates that asInt?, so the pin can't move without touching every call site.The interesting part isn't the null-safety, it's the back-compat: firmware in the field still sends
rx_time = 0for exactly the same state. Absent and 0 both mean unknown, and a 1970 arrival time is never a genuine reading — so the two cases have to collapse into one rule, in one place, rather than each site sprouting a?: 0that silently drops the old-firmware half.This is the same defect class as the RSSI zeros in #6507, but inverted: there,
0was a legitimate reading being discarded; here,0is a sentinel that must not be trusted.What
🛠️ Bump
meshtastic-protobufs2.7.26.138-g26db1b5-SNAPSHOT→2.7.26.140-g6ceceae-SNAPSHOT.🛠️ Add one helper,
MeshPacket.rxTimeOrNull(), that folds "field absent" and "old-firmware 0" into a singlenull. Every one of the ninerx_timereads now goes through it.🐛
applySenderPacketUpdatewould have written the epoch intonode.lastHeardfor an unstamped packet (clampTimestampToNow(0)→0), aging a live node to 1970. Now it keeps the node's existinglastHeard. Unreachable today — packets are normalized upstream athandleReceivedMeshPacket— but it was a live footgun one refactor away from firing.🐛
MetricsState.oldestTimestampSeconds()let an unknown timestamp win themin(), which would collapse every metrics chart's time axis to 1970. NowmapNotNulls them out.🧹 Normalization in
handleReceivedMeshPacketcollapses to a single expression, and theMessage.meshTimeKDoc now names both ways the timestamp can be missing.Everything else —
MeshDataMapper,isDirectSignal(), the fourSignalMetricssites, the signal-metrics CSV export — is behaviour-preserving: absent maps to the same0those sites already handled.Notes for reviewers
This pins an untagged snapshot, deliberately. Upstream holds protobufs tags in lockstep with firmware stable releases, so the latest tag is still
v2.7.26(2026-06-20) while master is well ahead. Waiting for a tag here would be an open-ended stall, and the app catalog has been on a snapshot pin for a while already. Renovate may briefly try to "upgrade" us down to thev2.7.26release, since a bare release numerically out-ranks a-g<sha>-SNAPSHOTqualifier; it self-resolves.Deliberately out of scope: the new presence bit does not survive to the UI.
handleReceivedMeshPacketstill overwrites an absentrx_timewithnowbefore storing, so nothing downstream can tell "radio had no clock" from "radio was stamped at that second". Preserving it end-to-end means touchingDataPacket.time, the Room column, andMessage.displayTime— a separate change, not something to smuggle into a version bump.Testing Performed
New
RxTimeExtensionsTest(:core:model) covers stamped / absent / old-firmware-zero for bothrxTimeOrNull()andisDirectSignal(). NewMeshMessageProcessorImplTest > packets with absent rx_time get current timecovers the normalization path alongside the existing zero and non-zero cases.Both new tests were confirmed to have actually executed rather than replaying from the build cache (checked the test-results XML — a green tick on a
FROM-CACHEtask proves nothing).Full baseline green:
Not device-verified — the behavioural guards fire only against firmware that omits
rx_time, which needs a clockless node (no GPS, no phone yet connected) to reproduce for real.🤖 Generated with Claude Code
Summary by CodeRabbit
rx_time(and keeps it consistent throughout processing).