Phantom node fix perhaps - #11271
Merged
thebentern merged 3 commits intoJul 28, 2026
Merged
Conversation
NomDeTom
added
requires-protos
Contributor
|
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 (1)
📝 WalkthroughWalkthroughChangesMesh metadata semantics
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RadioInterface
participant NodeDB
participant MeshPacketSerializer
RadioInterface->>NodeDB: provide rx_rssi and rx_snr with presence flags
NodeDB->>NodeDB: store measured SNR and set its bitfield
MeshPacketSerializer->>MeshPacketSerializer: serialize rssi when has_rx_rssi is true
Possibly related PRs
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-27. 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: 1
🧹 Nitpick comments (1)
src/mesh/NodeDB.cpp (1)
219-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the NodeDB bitfield accessor here.
Line 222 reads
node.bitfielddirectly, but the repository convention requires NodeDB bitfield access through helpers. Replace the condition withnodeInfoLiteHasSnr(&node).🤖 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/NodeDB.cpp` around lines 219 - 226, In the SNR restoration logic, replace the direct node.bitfield check with the repository accessor nodeInfoLiteHasSnr(&node), while preserving the existing legacy node.snr_q4 fallback and SNR assignment behavior.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/mesh/NodeDB.cpp`:
- Around line 3635-3644: Gate the SNR update in updateFrom() on mp.has_rx_rssi
in addition to the existing TRANSPORT_LORA check. Only assign info->snr and set
NODEINFO_BITFIELD_HAS_SNR_MASK when the packet explicitly indicates a local RF
receipt; preserve the current behavior for valid received measurements.
---
Nitpick comments:
In `@src/mesh/NodeDB.cpp`:
- Around line 219-226: In the SNR restoration logic, replace the direct
node.bitfield check with the repository accessor nodeInfoLiteHasSnr(&node),
while preserving the existing legacy node.snr_q4 fallback and SNR assignment
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: 92754fd0-88d6-4571-b2ed-1a39c290648f
⛔ Files ignored due to path filters (2)
src/mesh/generated/meshtastic/deviceonly.pb.his excluded by!**/generated/**,!src/mesh/generated/**src/mesh/generated/meshtastic/mesh.pb.his excluded by!**/generated/**,!src/mesh/generated/**
📒 Files selected for processing (17)
protobufssrc/mesh/LR11x0Interface.cppsrc/mesh/LR20x0Interface.cppsrc/mesh/MeshService.cppsrc/mesh/NodeDB.cppsrc/mesh/NodeDB.hsrc/mesh/PhoneAPI.cppsrc/mesh/RF95Interface.cppsrc/mesh/SX126xInterface.cppsrc/mesh/SX128xInterface.cppsrc/mesh/TypeConversions.cppsrc/mesh/udp/UdpMulticastHandler.hsrc/modules/StoreForwardModule.cppsrc/modules/TraceRouteModule.cppsrc/serialization/MeshPacketSerializer.cpptest/test_fuzz_packets/test_main.cpptest/test_meshpacket_serializer/test_helpers.h
NomDeTom
marked this pull request as ready for review
July 28, 2026 16:04
8 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets “phantom” nodes shown as direct (0-hop) neighbors with 0 SNR / 0 RSSI after a phone reconnect by tightening replay metadata correctness and adding explicit wire presence for MeshPacket.rx_rssi, so “unknown” no longer aliases to a plausible 0 dBm reading.
Changes:
- Makes
MeshPacket.rx_rssioptional (addshas_rx_rssi) and updates radio RX sites + serializers/logging to respect explicit presence. - Fixes replay packet construction to avoid fabricating 0-hop/direct-neighbor metadata; preserves
channel,via_mqtt, and useslast_heardforrx_time. - Improves persisted SNR handling (rounding + new NodeInfoLite presence bit) and clamps traceroute SNR encoding to avoid colliding with the INT8_MIN sentinel.
Reviewed changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/test_meshpacket_serializer/test_helpers.h | Updates synthetic test packet to set has_rx_rssi when populating rx_rssi. |
| test/test_fuzz_packets/test_main.cpp | Extends fuzzing to exercise both present/absent rx_rssi paths. |
| src/serialization/MeshPacketSerializer.cpp | Switches JSON RSSI emission from rx_rssi != 0 to has_rx_rssi. |
| src/modules/TraceRouteModule.cpp | Clamps q4 SNR to avoid producing -128 and colliding with “unknown” sentinel. |
| src/modules/StoreForwardModule.cpp | Marks S&F replay packets as having measured RSSI (but currently does so unconditionally). |
| src/mesh/udp/UdpMulticastHandler.h | Clears has_rx_rssi when zeroing RSSI/SNR for UDP-relayed packets. |
| src/mesh/TypeConversions.cpp | Notes client-facing NodeInfo SNR still lacks presence, points to NodeInfoLite presence helper. |
| src/mesh/SX128xInterface.cpp | Sets has_rx_rssi=true when capturing real RSSI from radio. |
| src/mesh/SX126xInterface.cpp | Sets has_rx_rssi=true when capturing real RSSI from radio. |
| src/mesh/RF95Interface.cpp | Sets has_rx_rssi=true when capturing real RSSI from radio. |
| src/mesh/RadioInterface.cpp | Updates debug printing to show RSSI only when has_rx_rssi is set. |
| src/mesh/PhoneAPI.cpp | Fixes replay packet hop encoding, rx_time, and preserves channel/via_mqtt; leaves RSSI absent for replay. |
| src/mesh/NodeDB.h | Adds NODEINFO_BITFIELD_HAS_SNR and helper accessor nodeInfoLiteHasSnr(). |
| src/mesh/NodeDB.cpp | Uses rounding for SNR q4 persistence and gates SNR write on genuine RF reception (transport + RSSI presence). |
| src/mesh/MeshService.cpp | Fabrics RSSI only when has_rx_rssi is not set (presence-aware injection behavior). |
| src/mesh/LR20x0Interface.cpp | Sets has_rx_rssi=true when capturing real RSSI from radio. |
| src/mesh/LR11x0Interface.cpp | Sets has_rx_rssi=true when capturing real RSSI from radio. |
| src/mesh/generated/meshtastic/mesh.pb.h | Nanopb regen reflecting rx_rssi optional + updated field comments. |
| src/mesh/generated/meshtastic/deviceonly.pb.h | Nanopb regen comment updates for q4 SNR encode/decode semantics. |
Comment on lines
261
to
+262
| p->rx_rssi = this->packetHistory[i].rx_rssi; | ||
| p->has_rx_rssi = true; // rx_rssi has explicit presence; the stored value was a genuine measurement |
Collaborator
Author
There was a problem hiding this comment.
had to fix it elsehwere
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 28, 2026
… 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.
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.
After a phone reconnects, the client synthesizes history packets from the stored NodeDB
(
PhoneAPI::makeReplay{Position,Telemetry,Environment,Status}Packet) so the app doesn't haveto wait to re-hear every node live. Users report these replayed nodes sometimes appearing as
phantom direct (0-hop) connections with 0 SNR / 0 RSSI, indistinguishable from a real
zero-signal neighbor — originally reported in #11208.
Captured a full NodeDB dump plus a live BLE/serial replay trace from a real device and traced
this to several independent defects in the replay path and in how
snr/rssiare stored onNodeInfoLite, not one bug. All four replay builders start from a blankMeshPacketand copyonly a handful of fields from the stored node header, so several stored fields were silently
dropped — and two of the numeric fields involved (
rx_snr,rx_rssi) had no wire-level way torepresent "never measured" at all, so a dropped value came back as a plausible 0 instead of
nothing.
Fixes
rx_rssihad no "unset" state on the wire. proto3 implicit presence makes an absentvalue byte-identical to a genuine 0 dBm reading — and 0 dBm is a real reading on
SX126x/LR11x0/LR20x0, sometimes even positive on SX127x. Made
MeshPacket.rx_rssioptional(has_rx_rssi). Every genuine-reception site now sets the bit explicitly:SX126xInterface,SX128xInterface,LR11x0Interface,LR20x0Interface,RF95Interface,StoreForwardModule's history replay, and the fabricated link metadata inMeshService::injectAsReceived.UdpMulticastHandlerclears it when it deliberately zeroesSNR/RSSI for a UDP-relayed packet.
MeshPacketSerializer's JSON/serial dump switched fromrx_rssi != 0tohas_rx_rssi.Replay fabricated a direct-neighbor reading instead of leaving hop count unknown.
setReplayHopFieldsdefaulted an unknown hop count to0, producinghop_start == hop_limit— indistinguishable from a genuine 0-hop neighbor. It now leavesboth at
0(documented as the "unknown" encoding onhop_startinmesh.proto), matchinghow firmware's own
getHopsAway()already treats an un-bitfieldedhop_start == 0.Persisted SNR (
snr_q4) used truncation and a bare-zero sentinel. Truncating instead ofrounding collapsed any reading within ±0.25 dB of zero to exactly 0, indistinguishable from
"never stored." Encoding now uses
lroundf, and an explicitNODEINFO_BITFIELD_HAS_SNRpresence bit (bit 10, following the existing
HAS_USER/HAS_IS_UNMESSAGABLE/HAS_XEDDSA_SIGNEDprecedent) disambiguates a genuine 0 dB reading from "unmeasured."Legacy on-disk records (bit unset) still decode correctly, since the old write gate never
stored a real 0 dB value before this bit existed.
The live write gate for SNR was
if (mp.rx_snr)— truthiness, not presence — so agenuine 0 dB reading was silently discarded before it could even reach fix 3. Replaced with
a check on
mp.transport_mechanism == TRANSPORT_LORA, which is set only on the realover-the-air receive path (excludes locally-injected and MQTT-only packets, but still covers
an MQTT-origin packet a gateway rebroadcasts onto LoRa, since that hop was genuinely
measured).
Position replay used the GPS fix time, not reception time, for
rx_time.makeReplayPositionPacketsetpkt.rx_time = pos.time— the position's own, often-zero,GPS timestamp — so a position last heard 11 days ago replayed as "Last heard: Now" on every
reconnect. Now uses
header->last_heard, matching the other three replay builders; the GPSfix time is unaffected since it already round-trips inside the payload via
ConvertToPosition.Replay silently dropped
channelandvia_mqtt. All four builders hardcodedchannel = 0and never setvia_mqtt, so an MQTT-sourced or non-default-channel nodereplayed as a directly-received, default-channel LoRa packet. Both are now copied from the
stored header.
Incidental:
TraceRouteModule::appendMyIDandSNR's SNR→int8 cast could produce exactly-128for a very low but real reading, colliding with theINT8_MIN"unknown" sentinel thatRouteDiscovery.snr_towards/snr_backalready use elsewhere in this file. Clamped to-127so a real reading can no longer alias the sentinel.
Deliberately out of scope
NodeInfoLitehas no RSSI field, so a replayedpacket now leaves
rx_rssiabsent rather than fabricating 0 — honest, but the client showsno RSSI for any node until it's heard live again. Adding storage is a
NodeInfoLiteschemachange with flash-budget impact across the 3-tier node sizing tiers, and is being kept out of
this PR.
hops_away == 0that may or may not be genuinedirect neighbors isn't addressed — needs a live capture of the actual
hop_start/hop_limitheader from one of them, not a heuristic guess.
hopScalingModule's hop histogram deliberately buckets an unproven-but-real hop count as 0for its own conservative sizing purpose. That's an intentional disagreement with the stricter
hops_awaystorage gate, documented in place, not changed here.Testing
./bin/run-tests.sh -e native— full 41-suite native run GREEN on the branch prior to thefinal RSSI-field-removal + comment cleanup pass.
test_type_conversionsre-verified GREEN (28/28) after removing theNodeInfoLite.rssistorage field and regenerating the nanopb header.
test_fuzz_packetsexercises both the presence and absence ofhas_rx_rssi.nrf52_promicro_diy_tcxo,heltec-mesh-pocket-5000-inkhud.Acknowledgements
Hat tip to @Folex-Fire for #11208 - it wasn't right, but it did spur me to make a run at it.
🤝 Attestations
Summary by CodeRabbit