fix(lora): validate 2.4 GHz bandwidth options - #2225
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe change adds region- and hardware-aware LoRa bandwidth options. The bandwidth enum defines four new values and validates selections by region and hardware type. The configuration UI filters options, preserves unsupported stored values, displays warnings, and blocks invalid saves. Tests cover selection rules, picker mapping, and ChangesRegion-aware bandwidth selection
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: 🟡 Moderate · up to The bandwidth validation change is not merge-ready until the failing capability assertion is corrected; otherwise the test suite does not reliably verify the supported 2.4 GHz target behavior. The duplicate documentation heading should also be cleaned up before merging. Sequence Diagram(s)sequenceDiagram
participant User
participant LoRaConfig
participant Bandwidths
participant CustomBandwidthPicker
User->>LoRaConfig: select custom bandwidth
LoRaConfig->>Bandwidths: selectable(region, pioEnv)
Bandwidths-->>LoRaConfig: supported options
LoRaConfig->>CustomBandwidthPicker: display options and validation state
User->>LoRaConfig: save configuration
LoRaConfig->>Bandwidths: validate stored value
Bandwidths-->>LoRaConfig: validation result
LoRaConfig-->>User: save or display warning
Suggested reviewers: 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 review |
✅ Action performedReview finished.
|
![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
🧹 Nitpick comments (2)
MeshtasticTests/LoraDeviceEnumTests.swift (1)
179-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the nil-region fallback.
Bandwidths.selectablefalls back tosubGHzCaseswhenregionisnil. This branch is reachable in production whenRegionCodes(rawValue:)fails to resolve an unrecognized region code (for example, a newer firmware region the app doesn't yet know). Add a test assertingBandwidths.selectable(region: nil, pioEnv: nil)returns the sub-GHz set.🤖 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 `@MeshtasticTests/LoraDeviceEnumTests.swift` around lines 179 - 183, Add a test alongside subGHzOptions_preserveExistingWireValues that calls Bandwidths.selectable with region: nil and pioEnv: nil, then asserts its pickerValue list matches the expected sub-GHz set. Keep the assertion consistent with the existing selectable-options test.Meshtastic/Views/Settings/Config/LoRaConfig.swift (1)
251-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate docs/user/settings.md for this bandwidth picker change.
This changes general LoRa settings behavior: the bandwidth picker now filters options by region/hardware and can block saves on an unsupported value. Update
docs/user/settings.mdto describe the new behavior.As per path instructions, "Update
docs/user/telemetry.mdfor telemetry/sensor settings changes anddocs/user/settings.mdfor general settings changes."🤖 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/Views/Settings/Config/LoRaConfig.swift` around lines 251 - 264, Update docs/user/settings.md to document the CustomBandwidthPicker behavior in the general LoRa settings: available bandwidth options are filtered by region and hardware, and saving is blocked when the selected value is unsupported. Keep the documentation aligned with the existing settings terminology and describe the user-visible behavior without changing unrelated sections.Source: Path instructions
🤖 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/Views/Settings/Config/LoRaConfig.swift`:
- Around line 522-551: Update the warning Text inside CustomBandwidthPicker to
apply the existing .localized pattern to the static unsupported-bandwidth
message, matching the neighboring “Licensed band” restriction text while
preserving the current wording and display behavior.
---
Nitpick comments:
In `@Meshtastic/Views/Settings/Config/LoRaConfig.swift`:
- Around line 251-264: Update docs/user/settings.md to document the
CustomBandwidthPicker behavior in the general LoRa settings: available bandwidth
options are filtered by region and hardware, and saving is blocked when the
selected value is unsupported. Keep the documentation aligned with the existing
settings terminology and describe the user-visible behavior without changing
unrelated sections.
In `@MeshtasticTests/LoraDeviceEnumTests.swift`:
- Around line 179-183: Add a test alongside
subGHzOptions_preserveExistingWireValues that calls Bandwidths.selectable with
region: nil and pioEnv: nil, then asserts its pickerValue list matches the
expected sub-GHz set. Keep the assertion consistent with the existing
selectable-options test.
🪄 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: bd65aa9d-92af-4586-8f1e-fd338e305a11
📒 Files selected for processing (3)
Meshtastic/Enums/LoraConfigEnums.swiftMeshtastic/Views/Settings/Config/LoRaConfig.swiftMeshtasticTests/LoraDeviceEnumTests.swift
RCGV1
removed
the
skip-docs-check
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Hardware follow-up on the firmware failure mode this UI prevents:
This supports filtering unsupported values in the client: it prevents both the fault-screen experience and the persisted radio/config mismatch. The Apple app itself was not used to write the invalid value during this reproduction. The companion firmware-side validation is in meshtastic/firmware#11317. |
|
Reviewed this together with meshtastic/Meshtastic-Android#6529 and meshtastic/firmware#11317.
|
|
Addressed the remaining bandwidth review findings in bb96dd0 (after merging current
Validation: the changed Swift files parse cleanly, the string catalog passes |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/user/settings.md (1)
181-181: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename the duplicate
Toolsheading.Line 181 triggers markdownlint MD024 because another heading has the same content. Use a unique heading so generated documentation has unambiguous navigation.
🤖 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 `@docs/user/settings.md` at line 181, Rename the duplicate “Tools” heading in the documentation to a unique, descriptive heading while preserving the section’s content and navigation meaning.Source: Linters/SAST tools
🤖 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/settings.md`:
- Line 37: Update the Bandwidth row to distinguish stored value 0 behavior by
region: treat it as 250 kHz in sub-GHz custom mode, while lora24 treats it as
selection required and disables Save until a supported bandwidth is chosen.
Regenerate the bundled Markdown and HTML documentation using the documented
build command.
In `@MeshtasticTests/MeshPacketsAndTelemetryTests.swift`:
- Around line 92-93: Update the positive assertion around mesh.myInfoPacket to
use a pioEnv included in the sx128xTargets allowlist so .sixteenHundred is
selectable; keep my-esp32s3-diy-oled only if it is explicitly added to that
allowlist, and add a separate negative assertion for omitted targets if
fail-closed behavior is intended.
---
Outside diff comments:
In `@docs/user/settings.md`:
- Line 181: Rename the duplicate “Tools” heading in the documentation to a
unique, descriptive heading while preserving the section’s content and
navigation meaning.
🪄 Autofix
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: f0f58679-c0b3-4d51-a52d-81c221f6267a
📒 Files selected for processing (6)
Localizable.xcstringsMeshtastic/Enums/LoraConfigEnums.swiftMeshtastic/Views/Settings/Config/LoRaConfig.swiftMeshtasticTests/LoraDeviceEnumTests.swiftMeshtasticTests/MeshPacketsAndTelemetryTests.swiftdocs/user/settings.md
💤 Files with no reviewable changes (1)
- Localizable.xcstrings
🚧 Files skipped from review as they are similar to previous changes (3)
- Meshtastic/Views/Settings/Config/LoRaConfig.swift
- MeshtasticTests/LoraDeviceEnumTests.swift
- Meshtastic/Enums/LoraConfigEnums.swift
|
Updated the protobuf-default bandwidth handling in 313c176. Following existing client patterns for proto scalar defaults, wire value Validation:
|
What changed?
Why did it change?
The Apple custom LoRa bandwidth picker only offered sub-GHz values (31/62/125/250/500 kHz), even for the LORA_24 region. Pre-fix firmware accepted and persisted an unsupported value such as 125 kHz before RadioLib rejected it and recorded
INVALID_RADIO_SETTING(critical error 7). On display-equipped nodes, firmware focuses the critical-fault screen, which looks like a frozen node even though the scheduler and serial API remain responsive. The saved configuration can also disagree with the bandwidth retained by the radio.Firmware accepts the canonical high-band codes 200/400/800 on both supported radio families, while 1600 is SX128x-only.
tlora-t3s3-v1is deliberately limited to 200/400/800 because that target can contain either SX128x or LR1121 and the protocol does not currently report the runtime radio chip.How is this tested?
BandwidthsTests: 10 tests passed, including SX128x, LR1121, mixed-target, unknown-region fallback, invalid stored value, and legacy 250 kHz coverage.git diff --checkpassed.Critical fault #7display frame, not a stopped CPU or API.TLORA_T3_S3(tlora-t3s3-v1) passed 200/400/800 configuration and RF delivery; that specific unit also accepted 1600.Screenshots/Videos (when applicable)
No visual evidence is needed for the model-layer change. Firmware bench evidence is summarized above; the final exact-commit endurance matrix is still pending.
Checklist
docs/user/ordocs/developer/, and updateddocs/user/settings.mdwith the filtered bandwidth picker and blocked-save behavior.Summary by CodeRabbit
New Features
Bug Fixes
Documentation