Use real noise floor for signal rating; unify with SNR color#2078
Open
bruschill wants to merge 6 commits into
Open
Use real noise floor for signal rating; unify with SNR color#2078bruschill wants to merge 6 commits into
bruschill wants to merge 6 commits into
Conversation
getLoRaSignalStrength() blended guessed fixed RSSI thresholds (-115/-120/-126) with the SNR-vs-preset-floor comparison to pick the signal-quality tier, while getSnrColor() was already SNR-only — so the bar indicator and the SNR text color could disagree on the same reading. Unify both around a single signalQuality(snrMargin:) helper. When the receiving node has a recent noise floor (DeviceMetrics.noise_floor from Local Stats telemetry), compute the real link margin (rssi - noiseFloor) and combine it with the SNR margin, taking the more conservative tier — more accurate than Android's SNR-only model. Fall back to SNR-only when no noise floor is available, dropping the guessed RSSI thresholds. Adds NodeInfoEntity.recentNoiseFloor (2h freshness, matching isOnline), ties the SNR text color to the same rating as the bar in the signal meters, and fixes a latent bug where LoRaSignalStrengthMeter colored the SNR text with a hardcoded .longFast preset. Adds LoRaSignalStrengthRatingTests (10 tests): rating bands, SNR-only fallback, noise-floor combine, bar/text color agreement, and the recentNoiseFloor freshness gate. Fixes meshtastic#2042 Docs: updated signal-meter.md for the new rating algorithm
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe signal meter now rates quality from preset-relative SNR, optionally constrained by RSSI-to-noise-floor margin from recent local stats. UI consumers, tests, project wiring, documentation, and search metadata were updated accordingly. ChangesSignal quality rating
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant NodeDetail
participant NodeInfoEntity
participant SignalRating
NodeDetail->>NodeInfoEntity: obtain recentNoiseFloor
NodeDetail->>SignalRating: provide SNR, RSSI, preset, noise floor
SignalRating-->>NodeDetail: return signal tier and color
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
🧹 Nitpick comments (2)
Meshtastic/Resources/docs/markdown/user/signal-meter.md (1)
67-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWarning callout doesn't follow the two-line format.
The title and body are on the same blockquote line, separated by
:, which the repo's callout guideline explicitly disallows.📝 Suggested reformat
-> **Warning — Watch out for local noise:** If you hook up a massive antenna and see a great RSSI (e.g., `-90 dBm`) but your signal meter is only showing **1 Bar (Bad)**, you have a problem. It means you have local interference—perhaps a cheap power supply, a noisy computer, or a nearby radio tower—creating so much static that it is drowning out your mesh. When your radio knows its own noise floor, the meter accounts for exactly this by measuring your real margin above the noise. +> **Warning — Local noise** +> +> If you hook up a massive antenna and see a great RSSI (e.g., `-90 dBm`) but your signal meter is only showing **1 Bar (Bad)**, you have a problem. It means you have local interference—perhaps a cheap power supply, a noisy computer, or a nearby radio tower—creating so much static that it is drowning out your mesh. When your radio knows its own noise floor, the meter accounts for this by measuring your real margin above the noise.If this line originates from
docs/user/signal-meter.md, apply the same fix there and regenerate the bundled HTML/snapshots per the doc build guidelines.
As per coding guidelines, "Use the two-line callout format for Tip and Warning blocks in markdown, with the title on one blockquote line and the body on following blockquote lines" and "Do not place callout body text on the same line as the title, and do not use a:separator in Tip/Warning callouts."🤖 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/Resources/docs/markdown/user/signal-meter.md` at line 67, Reformat the “Warning — Watch out for local noise” callout in the signal-meter documentation so the title occupies its own blockquote line and the explanatory text follows on subsequent blockquote lines, without a colon separator. If the source exists under docs/user, apply the same change there and regenerate the bundled HTML and snapshots using the documentation build process.Source: Coding guidelines
Meshtastic/Extensions/SwiftData/NodeInfoEntityExtension.swift (1)
206-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared "2-hour freshness" check.
This duplicates the "now minus 120 minutes" comparison already used by
isOnline(same file, unrelated diff). Both encode the same 2-hour freshness window independently; if the online/staleness window ever changes, it's easy to update one and miss the other.♻️ Suggested helper
+extension Date { + func isMoreRecentThan(minutesAgo: Int) -> Bool { + guard let threshold = Calendar.current.date(byAdding: .minute, value: -minutesAgo, to: Date()) else { return false } + return self.compare(threshold) == .orderedDescending + } +} + var recentNoiseFloor: Int32? { guard let stats = latestLocalStats, let time = stats.time, let noiseFloor = stats.noiseFloor, noiseFloor != 0, - let twoHoursAgo = Calendar.current.date(byAdding: .minute, value: -120, to: Date()), - time.compare(twoHoursAgo) == .orderedDescending + time.isMoreRecentThan(minutesAgo: 120) else { return nil } return noiseFloor }🤖 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/Extensions/SwiftData/NodeInfoEntityExtension.swift` around lines 206 - 219, Extract the shared two-hour freshness calculation or predicate used by recentNoiseFloor and isOnline into a common helper in NodeInfoEntityExtension, then have both checks reuse it. Preserve the existing behavior: timestamps must be newer than the current 120-minute threshold.
🤖 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 `@docs/user/signal-meter.md`:
- Line 67: Reformat the Warning callout in the signal-meter documentation so its
bold title occupies its own blockquote line, with the closing ** directly after
“noise” and no colon separator; move the explanatory text to subsequent
blockquote line(s). Apply the same two-line format to the nearby Tip callout for
consistency.
In `@Meshtastic/Resources/docs/index.json`:
- Around line 437-468: Update scripts/build-docs.sh to decode HTML entities in
rendered documentation before extracting search keywords, ensuring entities such
as " become their actual characters rather than standalone terms like quot.
Regenerate Meshtastic/Resources/docs/index.json and verify entity names are no
longer included as keywords.
In `@Meshtastic/Views/Nodes/Helpers/NodeDetail.swift`:
- Around line 142-146: Update the Node Detail documentation in
docs/user/nodes.md with a brief user-facing note describing the shared
signal-strength meter and its use of the connected node’s recent noise floor
when available. Keep the existing high-level documentation intact and limit the
change to this behavior.
---
Nitpick comments:
In `@Meshtastic/Extensions/SwiftData/NodeInfoEntityExtension.swift`:
- Around line 206-219: Extract the shared two-hour freshness calculation or
predicate used by recentNoiseFloor and isOnline into a common helper in
NodeInfoEntityExtension, then have both checks reuse it. Preserve the existing
behavior: timestamps must be newer than the current 120-minute threshold.
In `@Meshtastic/Resources/docs/markdown/user/signal-meter.md`:
- Line 67: Reformat the “Warning — Watch out for local noise” callout in the
signal-meter documentation so the title occupies its own blockquote line and the
explanatory text follows on subsequent blockquote lines, without a colon
separator. If the source exists under docs/user, apply the same change there and
regenerate the bundled HTML and snapshots using the documentation build process.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 22c6ecab-8846-4503-850f-73958f3d3592
📒 Files selected for processing (10)
Meshtastic.xcodeproj/project.pbxprojMeshtastic/Extensions/SwiftData/NodeInfoEntityExtension.swiftMeshtastic/Resources/docs/index.jsonMeshtastic/Resources/docs/markdown/user/signal-meter.mdMeshtastic/Resources/docs/user/signal-meter.htmlMeshtastic/Views/Helpers/LoRaSignalStrength.swiftMeshtastic/Views/Helpers/LoRaSignalStrengthIndicator.swiftMeshtastic/Views/Nodes/Helpers/NodeDetail.swiftMeshtasticTests/LoRaSignalStrengthRatingTests.swiftdocs/user/signal-meter.md
- Reformat the signal-meter Tip and Warning callouts to the required two-line format (title on its own blockquote line, no colon separator, closing ** adjacent to the last word). - build-docs.sh: strip HTML entities before keyword extraction so rendered "/& no longer yield bogus 'quot'/'amp' search keywords in index.json (removed repo-wide).
Contributor
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/Resources/docs/user/signal-meter.html`:
- Around line 74-77: Fix the malformed callout markup generated from
docs/user/signal-meter.md by ensuring each callout uses valid paragraph
structure: the title remains inside the div, the body is wrapped in an opening
and closing p, and the div closes afterward. Regenerate
Meshtastic/Resources/docs with bash scripts/build-docs.sh --output
Meshtastic/Resources/docs so the bundled documentation stays synchronized.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 85333165-9fac-41c6-a568-81b8c2b412a2
📒 Files selected for processing (5)
Meshtastic/Resources/docs/index.jsonMeshtastic/Resources/docs/markdown/user/signal-meter.mdMeshtastic/Resources/docs/user/signal-meter.htmldocs/user/signal-meter.mdscripts/build-docs.sh
Node Detail is where the noise-floor-aware signal meter appears, so point docs/user/nodes.md at the Signal Meter page with a one-line summary of the unified SNR/noise-floor rating. (CodeRabbit)
The two-line callout format must be consecutive blockquote lines with no blank '>' line between title and body — a blank line produces two <p> elements, which the docs build's callout converter mangles into invalid HTML. Match the working format used elsewhere (e.g. messages.md) so the Tip/Warning callouts render as valid <div> callouts. (CodeRabbit)
Contributor
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 `@docs/user/nodes.md`:
- Around line 52-53: Update the signal meter description near the node detail
documentation to specify that the radio’s reported noise floor affects
link-quality ratings only when the data is recent, within two hours; otherwise
it is not used. Then regenerate the bundled Markdown and HTML documentation
outputs so they remain synchronized with the updated source under docs/.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f206d67a-5c61-4545-8381-67167ad37465
📒 Files selected for processing (4)
Meshtastic/Resources/docs/index.jsonMeshtastic/Resources/docs/markdown/user/nodes.mdMeshtastic/Resources/docs/user/nodes.htmldocs/user/nodes.md
🚧 Files skipped from review as they are similar to previous changes (1)
- Meshtastic/Resources/docs/index.json
The rating only uses noise-floor data reported within the last ~2h, so say 'recently reported ... when available' rather than implying any noise-floor reading qualifies. (CodeRabbit)
…r-signal-2042 # Conflicts: # Meshtastic.xcodeproj/project.pbxproj # Meshtastic/Resources/docs/index.json # Meshtastic/Views/Helpers/LoRaSignalStrength.swift
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.
What changed?
Reworked the LoRa signal-quality rating in
LoRaSignalStrengthIndicator.swiftaround a singlesignalQuality(snrMargin:)helper that both the bar indicator and the SNR text color derive from, so they can never disagree on the same reading.getLoRaSignalStrength(...)now takes an optionalnoiseFloor:. When the receiving node has a recent noise-floor reading (DeviceMetrics.noise_floor, delivered via Local Stats telemetry), it computes the real link marginrssi − noiseFloor, rates it against the same preset floor, and returns the more conservative of the SNR-based and RSSI-based tiers. When no noise floor is available it falls back to SNR-only — the old guessed fixed RSSI thresholds (−115/−120/−126) are gone.NodeInfoEntity.recentNoiseFloor(present, non-zero, newer than 2 h — matching the existingisOnlineconvention).NodeDetailpasses the connected node'srecentNoiseFloorinto the rating and colors the SNR text from the samesignalStrength.coloras the bar.LoRaSignalStrengthMetercolored its SNR text with a hardcoded.longFastpreset instead of the actual preset.docs/user/signal-meter.md(and regenerated the bundled HTML) to describe the new algorithm.Why did it change?
Per meshtastic/design#15, the RSSI thresholds in the rating were guessed values that can't distinguish signal from noise without knowing the noise floor. Android resolved this by going SNR-only (Meshtastic-Android#5903). iOS can do better:
DeviceMetrics.noise_floorgives us the receiving radio's actual noise floor, so we can compute a real link margin instead of guessing. Separately,getLoRaSignalStrength()andgetSnrColor()used different algorithms and could disagree on the same reading — now unified.Closes #2042
How is this tested?
New
LoRaSignalStrengthRatingTests(10 tests, all passing on simulator):signalQualitytier boundaries.rssi − noiseFloormargin downgrades an otherwise-good SNR; a strong RSSI margin does not upgrade a poor SNR (conservative combine); a zero noise floor is treated as unavailable.getSnrColormatches the bar tier for the same reading.recentNoiseFloorfreshness gate (fresh / stale >2 h / zero / absent) via SwiftData.SourceKit-LSP reports no diagnostics; SwiftLint clean; full test build succeeds. Reviewed by an internal Swift reviewer (approve-with-nits; the actionable lint nit is fixed).
Screenshots/Videos (when applicable)
N/A — the meter's visual layout is unchanged; only the tier computation changed.
Checklist
docs/user/ordocs/developer/, and updated accordingly (see copilot-instructions.md for the view → doc page mapping). If no doc update is needed, add theskip-docs-checklabel.Summary by CodeRabbit