Rssi explicit presence - #1024
Conversation
…Lite.rssi storage field MeshPacket.rx_rssi was proto3 singular, so an unset value and a genuine 0 dBm reading were byte-identical on the wire (0 is a legal SX126x return, and SX127x's offset+register formula can even go positive). Mark it optional so absence survives encoding, mirroring the precedent already set by NodeInfo.hops_away. NodeInfoLite had no RSSI field at all, so nothing survived a device reboot for replayed history packets to restore - every replay showed RSSI 0 regardless of the real measurement. Add NodeInfoLite.rssi (tag 20, sint32, next free tag) with the same "0 is not automatically unknown" caveat; presence is tracked via NODEINFO_BITFIELD_HAS_RSSI on the firmware side rather than a second wire-level bool, to avoid a RAM cost per stored node. Companion firmware change: meshtastic/firmware, phantom-node-fix-perhaps branch, .notes/plans/plan-executed-snr-rssi-replay-fidelity.md.
📝 WalkthroughWalkthroughUpdated protobuf documentation for Q4 SNR encoding and persisted zero values. Changed ChangesProtocol semantics
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@meshtastic/deviceonly.proto`:
- Around line 190-196: Add the persistent RSSI field to the NodeInfoLite message
after snr_q4, using the documented sint32 rssi = 20 declaration. Ensure
persistence and history replay use this field, and interpret a zero RSSI value
through NODEINFO_BITFIELD_HAS_RSSI rather than treating zero as inherently
unknown.
🪄 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: 288a599c-92ef-4fc1-8d94-161f09666f11
📒 Files selected for processing (2)
meshtastic/deviceonly.protomeshtastic/mesh.proto
| * Encode: snr_q4 = (int32_t)lroundf(snr * 4.0f). Decode: snr = snr_q4 / 4.0f. | ||
| * float snr is always zeroed on disk; this field carries all persisted SNR. | ||
| * A stored 0 does not by itself mean "unknown" here - see NODEINFO_BITFIELD_HAS_SNR in | ||
| * src/mesh/NodeDB.h for the presence bit that disambiguates a genuine 0 dB reading from | ||
| * "never measured". | ||
| */ | ||
| sint32 snr_q4 = 19; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Add the required persistent RSSI field to NodeInfoLite.
The supplied message ends at snr_q4 = 19; it has no rssi = 20 field. Making MeshPacket.rx_rssi optional preserves presence in packets, but cannot persist RSSI across reboots or history replay as required by this PR. Add the documented sint32 rssi = 20 field and ensure its zero-value interpretation uses NODEINFO_BITFIELD_HAS_RSSI.
🤖 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 `@meshtastic/deviceonly.proto` around lines 190 - 196, Add the persistent RSSI
field to the NodeInfoLite message after snr_q4, using the documented sint32 rssi
= 20 declaration. Ensure persistence and history replay use this field, and
interpret a zero RSSI value through NODEINFO_BITFIELD_HAS_RSSI rather than
treating zero as inherently unknown.
There was a problem hiding this comment.
semantic disagreement, maybe. Can you explain why I need to add a new protobuf?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- NodeInfoLite RSSI / SNR field declarations ---'
rg -n -C 3 '\brssi\b|\bsnr_q4\b' meshtastic/deviceonly.proto
printf '%s\n' '--- RSSI presence bit references ---'
rg -n -C 4 'NODEINFO_BITFIELD_HAS_RSSI|NODEINFO_BITFIELD_HAS_SNR' .
printf '%s\n' '--- NodeInfoLite RSSI assignment/read references ---'
rg -n -C 4 '(NodeInfoLite|node_info|nodeinfo).*rssi|\.rssi\b|->rssi\b' \
-g '!meshtastic/deviceonly.proto' .Length of output: 2434
@NomDeTom, fair disagreement — and I should have said a new field in the existing NodeInfoLite protobuf, not “a new protobuf.”
There are two separate persistence cases:
-
Packet history / receive queue
DeviceState.receive_queuestoresMeshPackets. MakingMeshPacket.rx_rssioptionalis sufficient to preserve whether that packet had an RSSI reading, including a valid0value, when the packet is serialized and replayed. -
Latest per-node state after reboot
NodeDatabase.nodesstoresNodeInfoLite, not the packet history. If firmware expects the current/latest RSSI for each node to remain available after a reboot even when the original packet is no longer consulted, thenNodeInfoLiteneeds a persisted RSSI value plus a presence indicator. That is analogous to the existingsnr/snr_q4arrangement.
So: you do not need NodeInfoLite.rssi merely for replaying MeshPacket history. It is needed only if the PR’s intended contract includes persisting the per-node “last RSSI” independently of those packets.
The current protobuf repository has no NODEINFO_BITFIELD_HAS_RSSI definition or firmware read/write path to validate that latter contract; those belong to the related firmware work. My original finding incorrectly bundled packet replay and node-state persistence together.
2 participants
make rx_rssi optional
needed for meshtastic/firmware#11271
Summary by CodeRabbit
Bug Fixes
Documentation