fix(ui): give rx_snr real presence semantics end to end - #6523
Merged
Conversation
Upstream protobufs is converting MeshPacket fields to explicit presence one at a time (rx_rssi in 2.7.26.138, rx_time in .140). rx_snr is a likely next candidate, and each conversion currently breaks every read site at once. Introduce MeshPacket.snrOrNull() and route all nine read sites through it, so the eventual bump is a one-line body change instead of a cross-module diff. Unlike rxTimeOrNull(), the seam must NOT fold zero. Under proto3 implicit presence a field at its zero value is never serialized, so once rx_snr becomes optional a zero written by older firmware already decodes to null for free. The only zero that can reach a nullable accessor was written explicitly by firmware that has explicit presence -- i.e. a genuine 0 dB reading, which is a signal at the noise floor and demodulable on every preset. Folding it would discard precisely the case the upstream change exists to preserve. Presence cannot be inferred from transport instead: TRANSPORT_INTERNAL is 0, so firmware that never sets transport_mechanism would have every reading suppressed. Also fixes two pre-existing wrong-zero folds this seam replaces: - ExportDataUseCase used `rx_snr != 0f` as a row-inclusion predicate, silently dropping any reception at exactly 0 dB from the CSV export. - DiscoveryScanEngine used `if (rx_snr != 0f)` to decide whether to record a reading, one line above an already-correct nullable rx_rssi check. The `!rx_snr.isNaN()` filters in SignalMetrics were dead code: rx_snr is a non-null primitive float today, so hasSnr was unconditionally true. A null SNR now renders as unknown rather than as a bad reading: MetricFormatter.snr(null) yields the em dash used by rssi(null), and Snr() renders nothing, mirroring its Rssi sibling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Node.snr defaults to Float.MAX_VALUE and Node.rssi to Int.MAX_VALUE, but there was no canonical resolver, so five call sites each invented their own check: `< 100f` in two core/ui gates, an exact `!= Float.MAX_VALUE` in NodeDetailsSection, an exact check in feature/car, and nothing at all in the AI paths. determineSignalQuality() has no sentinel handling, so Float.MAX_VALUE falls into `snr > limit -> GOOD`: a node with no reading at all rated as an excellent signal. Add Node.snrOrNull / Node.rssiOrNull plus named SNR_UNSET / RSSI_UNSET constants, route every read through them, and delete the divergent thresholds (MAX_VALID_SNR, MAX_VALID_RSSI, SNR_UNSET_THRESHOLD had no other references). Three latent presence-vs-sentinel-zero bugs fell out of this: - SignalInfo hid the whole signal row for a genuine 0 dBm reading, because the gate was `node.rssi < MAX_VALID_RSSI` where MAX_VALID_RSSI is 0. It now gates on SNR presence and renders an absent RSSI as the unknown marker. - buildNodeDescription suppressed the TalkBack signal announcement for the same reason. Since RSSI is deliberately excluded from the rating (#5446), its rssi parameter existed only to feed that broken gate, so it is removed. - NodeItem and NodeItemCompact required RSSI to be present before showing the quality icon, again contradicting the SNR-only rating. feature/car mapped the sentinel to SignalQuality.NONE, which claims a measured but undemodulable link. Add SignalQuality.UNKNOWN so absence is distinct; the exhaustive `when`s over the enum forced every render site to be updated. NodeDetails and the App Functions response now expose snr/rssi as nullable, so the raw sentinel can no longer be surfaced to a model as a superb signal. Also route six raw SNR interpolations through MetricFormatter.snr (NeighborInfo, NeighborInfoHandlerImpl, DiscoveryReportFormatter, both discovery map marker snippets, DebugViewModel), and delete NodeSignalQuality and SnrAndRssi, which had no call sites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughSNR and RSSI telemetry now use nullable accessors to distinguish unavailable values from valid zero readings. Formatting, packet processing, AI responses, exports, discovery output, standard UI, signal metrics, maps, and car UI were updated accordingly with expanded tests. ChangesTelemetry contracts and propagation
Estimated code review effort: 4 (Complex) | ~45 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 |
![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: 4
🤖 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
`@androidApp/src/google/kotlin/org/meshtastic/app/ai/appfunctions/AppFunctionModels.kt`:
- Around line 129-132: Update the KDoc for nullable fields snr and rssi in the
AppFunction model to state that values may be unavailable/null, and correct
RSSI’s unit from dB to dBm to match the NodeDetails contract.
In
`@core/model/src/commonMain/kotlin/org/meshtastic/core/model/util/MeshDataMapper.kt`:
- Around line 47-49: Update the DataPacket model so snr is nullable (Float?) and
preserve the null result from snrOrNull() in MeshDataMapper instead of
defaulting to 0f. Adjust all affected consumers, signal logic, serialization,
and tests to handle absent SNR explicitly while retaining genuine 0 dB readings.
In
`@core/model/src/commonTest/kotlin/org/meshtastic/core/model/util/SnrExtensionsTest.kt`:
- Around line 60-65: Add an assertion in the test `snrOrNull does not conflate a
zero reading with an unknown one` that verifies `MeshPacket().snrOrNull()`
returns null, while preserving the existing zero-SNR assertion and the separate
rxTimeOrNull null assertion.
In
`@feature/car/src/main/kotlin/org/meshtastic/feature/car/util/CarScreenDataBuilder.kt`:
- Line 56: Add a buildNodeUi() test covering a node with the canonical unset SNR
representation, and assert the resulting car model signal quality is UNKNOWN.
Ensure the test exercises the production call through node.snrOrNull rather than
invoking determineSignalQuality() directly, so it fails if buildNodeUi()
regresses to node.snr.
🪄 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: 9d4fd649-dbec-466b-94e2-0900fb56df7e
📒 Files selected for processing (36)
androidApp/src/fdroid/kotlin/org/meshtastic/app/map/discovery/DiscoveryOsmMap.ktandroidApp/src/google/kotlin/org/meshtastic/app/ai/appfunctions/AppFunctionModels.ktandroidApp/src/google/kotlin/org/meshtastic/app/map/discovery/DiscoveryGoogleMap.ktcore/common/src/commonMain/kotlin/org/meshtastic/core/common/util/MetricFormatter.ktcore/common/src/commonTest/kotlin/org/meshtastic/core/common/util/MetricFormatterTest.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/ai/AiFunctionProviderImpl.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/ai/AiFunctionResult.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerImpl.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshMessageProcessorImpl.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/NeighborInfoHandlerImpl.ktcore/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/settings/ExportDataUseCase.ktcore/model/src/commonMain/kotlin/org/meshtastic/core/model/NeighborInfo.ktcore/model/src/commonMain/kotlin/org/meshtastic/core/model/Node.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/SnrExtensionsTest.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/BuildNodeDescription.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/LoraSignalIndicator.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItem.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItemCompact.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/SignalInfo.ktcore/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/BuildNodeDescriptionTest.ktcore/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/LoraSignalIndicatorTest.ktcore/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/LoraSignalIndicatorUiTest.ktfeature/car/src/main/kotlin/org/meshtastic/feature/car/model/CarUiModels.ktfeature/car/src/main/kotlin/org/meshtastic/feature/car/screens/NodeDetailScreen.ktfeature/car/src/main/kotlin/org/meshtastic/feature/car/util/CarScreenDataBuilder.ktfeature/car/src/main/kotlin/org/meshtastic/feature/car/util/NodeSubtitleFormatter.ktfeature/car/src/main/res/values/strings.xmlfeature/car/src/test/kotlin/org/meshtastic/feature/car/util/CarScreenDataBuilderTest.ktfeature/discovery/src/commonMain/kotlin/org/meshtastic/feature/discovery/DiscoveryScanEngine.ktfeature/discovery/src/commonMain/kotlin/org/meshtastic/feature/discovery/export/DiscoveryReportFormatter.ktfeature/node/src/commonMain/kotlin/org/meshtastic/feature/node/component/NodeDetailsSection.ktfeature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/MetricsViewModel.ktfeature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/SignalMetrics.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/debugging/DebugViewModel.kt
Address review feedback on #6523. The car signal-quality tests called determineSignalQuality() directly, so nothing proved buildNodeUi() routes through Node.snrOrNull rather than reading node.snr. Add two buildNodeUi() cases: an unset node resolves to UNKNOWN (this fails if the call reverts to node.snr, which would feed Float.MAX_VALUE into the bands and rate a node with no reading as EXCELLENT), and a 0 dB node still rates EXCELLENT. Also document the nullable snr/rssi fields in the AppFunction schema and correct RSSI's unit from dB to dBm to match the NodeDetails contract, and record in SnrExtensionsTest why the proto-absent case is not asserted there: rx_snr is still a non-null float upstream, so snrOrNull() cannot return null for any packet the test could construct. Null handling is covered where a null is representable, in MetricFormatterTest and LoraSignalIndicatorUiTest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The snrOrNull() seam stopped at the module boundary: MeshDataMapper narrowed an
absent reading to 0f because DataPacket.snr was not nullable, so MessageItem
rendered "SNR 0.00 dB" for a direct packet that carried no measurement -- a
reading the radio never took. Message.displayTime hides the equivalent problem
for time by falling back to received_time, but snr had no such guard.
Make the chain nullable end to end, following the template schema 51 already
set for rssi:
- DataPacket.snr, Message.snr, Reaction.snr, MeshBeaconOffer.snr -> Float?
- Room packet.snr and reactions.snr -> nullable, via AutoMigration(51 -> 52)
- MeshDataMapper passes snrOrNull() straight through, no fallback
As with the rssi migration, rows written before schema 52 keep their stored 0,
so a legacy 0 dB reading stays indistinguishable from "no reading" for existing
history; only new rows carry true presence. The column comments say so.
MeshBeaconInvitationCard's `offer.snr != 0f` check becomes a null check, which
also fixes its OR-gate showing "0 dB" as a real reading whenever rssi happened
to be present.
Left out deliberately: discovered_node.snr. Its readers aggregate --
DiscoveryRankingEngine takes a median over `nodes.map { it.snr }` and
DiscoveryMapViewModel dedups with `maxByOrNull { it.snr }` -- so nullability
there is a semantics decision (is an unmeasured node excluded from the median,
or sorted last?), not a mechanical change. It stays NOT NULL until that is
decided.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jamesarich
changed the title
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
1 participant
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.
Upstream
meshtastic/protobufsis convertingMeshPacketfields to explicit presence one at a time (rx_rssiin 2.7.26.138,rx_timein .140), and each conversion turns a non-null Wire field into a nullable one that breaks every read site at once.rx_snris a likely next candidate and the dangerous one, because 0 dB is a genuine, common reading — so thetakeIf { it != 0 }trick used forrx_timewould be actively wrong. This adds the seam ahead of the bump and, in the process, fixes several live instances of the same presence-vs-sentinel-zero defect class.🛠️ Refactoring & Architecture
MeshPacket.snrOrNull()incore/modelalongsiderxTimeOrNull(), and route all nine read sites through it. Whenrx_snrgoes optional upstream, the bump becomes a one-line body change instead of a cross-module diff.Node.snrOrNull/Node.rssiOrNullplus namedSNR_UNSET/RSSI_UNSETconstants.Node.snrdefaults toFloat.MAX_VALUEandNode.rssitoInt.MAX_VALUE, but with no canonical resolver five call sites each invented their own check (< 100fin twocore/uigates, an exact comparison inNodeDetailsSection, another infeature/car, and nothing at all in the AI paths). The divergent thresholdsMAX_VALID_SNR,MAX_VALID_RSSIandSNR_UNSET_THRESHOLDare deleted — they had no other references.SignalQuality.UNKNOWNinfeature/carso "no reading" is distinct fromNONE, which claims a measured but undemodulable link. The exhaustivewhens over the enum forced every render site to be updated.rssi:DataPacket.snr,Message.snr,Reaction.snrandMeshBeaconOffer.snrbecomeFloat?, and the Roompacket.snr/reactions.snrcolumns go nullable viaAutoMigration(51 → 52).MeshDataMappernow passessnrOrNull()straight through with no fallback, so the seam reaches the UI instead of stopping at the module boundary.MetricFormatter.snr(NeighborInfo,NeighborInfoHandlerImpl,DiscoveryReportFormatter, both discovery map marker snippets,DebugViewModel).Why the seam must not fold zero
Under proto3 implicit presence, a field at its zero value is never serialized. So once
rx_snrbecomesoptional, a zero written by older firmware already decodes tonullfor free — there is no old-vs-new firmware ambiguity to defend against. The only zero that can ever reach a nullable accessor was written explicitly by firmware that has explicit presence, i.e. a genuine 0 dB reading: a signal at the noise floor, comfortably demodulable on every preset. Folding zero would discard precisely the case the upstream change exists to preserve.The general rule this gives, for the conversions still to come: fold zero only when zero is physically impossible as a reading.
rx_timequalifies (a 1970 arrival time).rx_snrandrx_rssido not.Presence also cannot be inferred from transport instead —
TRANSPORT_INTERNALis0, so firmware that never setstransport_mechanismwould have every reading suppressed.🐛 Bug Fixes
determineSignalQuality(Float.MAX_VALUE, …)returnedGOOD. A node with no reading at all rated as an excellent signal. Most UI gates guarded first, butAiFunctionProviderImpland the App Functions provider leaked the raw sentinel to a model.NodeDetails.snr/rssiand the App Functions response fields are now nullable.SignalInfohid the entire signal row for a genuine 0 dBm reading, because the gate wasnode.rssi < MAX_VALID_RSSIwhereMAX_VALID_RSSI = 0. It now gates on SNR presence and renders an absent RSSI as the unknown marker.buildNodeDescriptionsuppressed the TalkBack signal announcement for the same reason. RSSI is deliberately excluded from the rating (Use modem-preset-relative SNR thresholds for signal quality #5446), so itsrssiparameter existed only to feed that broken gate and is removed.NodeItem/NodeItemCompactrequired RSSI to be present before showing the quality icon, again contradicting the SNR-only rating.ExportDataUseCaseusedrx_snr != 0fas a row-inclusion predicate, silently dropping any reception at exactly 0 dB from the CSV export.DiscoveryScanEngineusedif (rx_snr != 0f)to decide whether to record a reading, one line above an already-correct nullablerx_rssicheck.MessageItemrendered "SNR 0.00 dB" for a direct packet that carried no measurement — a reading the radio never took.Message.displayTimehides the equivalent problem fortimeby falling back toreceived_time, butsnrhad no such guard. Fixed by the nullable chain above.MeshBeaconInvitationCardshowed "0 dB" as a real reading wheneverrssihappened to be present, because its gate wasoffer.rssi != null || offer.snr != 0f. Now a null check.MetricFormatter.snr(null)yields the same em dash asrssi(null),Snr()renders nothing (mirroring itsRssisibling), andLoraSignalIndicatorshows "Signal Unknown" in a neutral tint instead of falling through toQuality.NONE.🧹 Chores
!rx_snr.isNaN()filters inSignalMetricswere dead code —rx_snris a non-null primitivefloattoday, sohasSnrwas unconditionally true.NodeSignalQualityandSnrAndRssi, which had no call sites anywhere in the repo.Testing Performed
./gradlew spotlessApply spotlessCheck detekt assembleDebug test allTests— green. Executed counts confirmed from the JUnit XML rather than a cached task tick.Added —
core/model/src/commonTest/.../util/SnrExtensionsTest.kt(4 tests):snrOrNullreports negative and positive readings, treats a true zero as a measurement rather than as absent, and does not conflate a zero reading with an unknown one (a regression guard against therxTimeOrNullpattern being copied over).Added —
core/databaseMeshtasticDatabaseMigrationTest.snrColumnsGoNullableWithoutLosingRows: a stored 0 dB survives the 51→52 recreate as0rather than becoming NULL, and a NULL is storable afterwards. Mirrors the existingrssiColumnsGoNullableWithoutLosingRows.Added —
feature/messagingMessageItemTest:directMessageWithoutSnrDoesNotFabricateAZeroReading(the regression guard for the "SNR 0.00 dB" bug) anddirectMessageWithZeroSnrShowsTheReading(the other half — 0 dB must still render). 10 tests in the class.Modified
core/common/.../MetricFormatterTest.kt— addedsnrAbsentIsUnknownandsnrZeroIsARealReading; extendedsnrto cover a negative value.core/ui/.../LoraSignalIndicatorTest.kt— addeda zero SNR reading is rated rather than treated as missingandabsent SNR is not a quality band.core/ui/.../LoraSignalIndicatorUiTest.kt— addedsnrRendersAZeroReading,snrRendersNothingWhenAbsent,loraSignalIndicatorShowsUnknownWhenSnrIsAbsent.core/ui/.../BuildNodeDescriptionTest.kt—signal_hidden_when_rssi_not_negativeandsignal_hidden_when_snr_is_max_floatencoded the buggy behaviour and are replaced bysignal_shown_for_a_zero_snr_readingandsignal_hidden_when_snr_is_absent.feature/car/.../CarScreenDataBuilderTest.kt—determineSignalQuality returns none when snr is max valuebecomesreturns unknown when snr is absent; addedrates a zero snr reading, plusbuildNodeUi resolves an unset node snr to unknownandbuildNodeUi rates a zero node snr reading. ThebuildNodeUipair exercises the production path — they fail if the call reverts to readingnode.snr, sinceFloat.MAX_VALUEwould rate asEXCELLENT. 42 tests in the class.Notes for reviewers
NONEmapping that this PR fixes.rssichange at schema 51: NOT NULL → nullable with no data transformation. As there, rows written before the migration keep their stored0, so a legacy 0 dB reading stays indistinguishable from "no reading" in existing history — only new rows carry true presence. The column comments say so, andsnrColumnsGoNullableWithoutLosingRowscovers it.discovered_node.snris deliberately left NOT NULL. Its readers aggregate —DiscoveryRankingEnginetakes a median overnodes.map { it.snr }andDiscoveryMapViewModeldedups withmaxByOrNull { it.snr }— so nullability there is a semantics question (is an unmeasured node excluded from the median, or sorted last?) rather than a mechanical change.DataPacket.time/Message.meshTimestill carry a 0 sentinel, narrowed atMeshDataMapperby(rxTimeOrNull() ?: 0). Unlikesnrit has a working downstream guard (displayTimefalls back to the always-presentreceived_time), so it is a structural wart with no user-visible symptom — not folded in here to keep this migration to one concern.Node.snr/NodeEntity.snrsentinels also remain; the new accessors make them safe to read, and removing them is a separate migration.via_mqtt(absent ⟺ false; the codebase already writesvia_mqtt == true, which is null-safe).hop_start/hop_limitcarry a trap for whoever does that bump:isDirectSignal()compareshop_start == hop_limit, and on nullable typesnull == nullistruein Kotlin, so an unstamped packet would be wrongly classified as a direct signal. Same shape atMeshMessageProcessorImplandMeshDataHandlerImpl. Nothing was built for these speculatively.feature/carstrings are not Crowdin-managed, socar_signal_unknownneeds no translation round-trip.SnrExtensionsTestdoes not assert the proto-absent case, by necessity rather than omission:rx_snris still a non-nullfloatupstream, sosnrOrNull()cannot return null for any packet the test could build. Null handling is covered where a null is representable —MetricFormatterTestandLoraSignalIndicatorUiTest. The file's KDoc records this.🤖 Generated with Claude Code