fix(radio): validate 2.4 GHz bandwidths - #11317
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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthrough
ChangesLoRa bandwidth validation
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
|
should this also cover LR2021? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mesh/RadioInterface.cpp (1)
1154-1171: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not pass the non-wide default to an SX128x.
When
wideBandis false,supportsSx128xLoRaBandwidthrejects every bandwidth. The clamp path then derives the regional default as 250 kHz and leaves that value forapplyModemConfig. The rejected bandwidth can still reach the 2.4 GHz radio.Handle the incompatible region before clamping, or revalidate the replacement and stop before applying it. Add a regression test for an SX128x with an
UNSETor sub-GHz region.🤖 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/RadioInterface.cpp` around lines 1154 - 1171, Ensure the clamp path in the bandwidth validation around supportsLoRaBandwidth does not pass a non-wide regional default to an SX128x radio. Detect the incompatible SX128x/region combination before applying the replacement, or revalidate the clamped bandwidth and abort before applyModemConfig; preserve valid clamping for compatible radios. Add a regression test covering an SX128x with an UNSET or sub-GHz region.
🧹 Nitpick comments (1)
src/mesh/RadioInterface.cpp (1)
1256-1264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass the active radio explicitly.
applyModemConfigshould passthistovalidateConfigLoraandclampConfigLora. The current calls use the default argument and depend onRadioLibInterface::instance. That skips radio-specific validation when the active implementation is not aRadioLibInterface.Suggested call-site update
- if (!validateConfigLora(loraConfig)) { + if (!validateConfigLora(loraConfig, this)) { loraConfig.modem_preset = newRegion->getDefaultPreset(); } ... - if (validateConfigLora(loraConfig)) { + if (validateConfigLora(loraConfig, this)) { } else { LOG_WARN("Invalid LoRa config settings, cannot apply requested modem config - falling back to %s defaults", newRegion->name); - clampConfigLora(loraConfig); + clampConfigLora(loraConfig, this); }🤖 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/RadioInterface.cpp` around lines 1256 - 1264, Update the applyModemConfig call sites for validateConfigLora and clampConfigLora to pass the current RadioInterface instance explicitly as this. Do not rely on their default radio argument or RadioLibInterface::instance, so radio-specific validation uses the active implementation.
🤖 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.
Outside diff comments:
In `@src/mesh/RadioInterface.cpp`:
- Around line 1154-1171: Ensure the clamp path in the bandwidth validation
around supportsLoRaBandwidth does not pass a non-wide regional default to an
SX128x radio. Detect the incompatible SX128x/region combination before applying
the replacement, or revalidate the clamped bandwidth and abort before
applyModemConfig; preserve valid clamping for compatible radios. Add a
regression test covering an SX128x with an UNSET or sub-GHz region.
---
Nitpick comments:
In `@src/mesh/RadioInterface.cpp`:
- Around line 1256-1264: Update the applyModemConfig call sites for
validateConfigLora and clampConfigLora to pass the current RadioInterface
instance explicitly as this. Do not rely on their default radio argument or
RadioLibInterface::instance, so radio-specific validation uses the active
implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f93c2837-6b03-4150-9cd0-1d59dd9a1efa
📒 Files selected for processing (6)
src/mesh/LR11x0Interface.hsrc/mesh/LR20x0Interface.hsrc/mesh/MeshRadio.hsrc/mesh/RadioInterface.cppsrc/mesh/SX128xInterface.htest/test_radio/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/mesh/SX128xInterface.h
- src/mesh/LR11x0Interface.h
- test/test_radio/test_main.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mesh/RadioInterface.cpp (1)
376-376: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftApply probe rollback to the
ARCH_PORTDUINOinitialization path.Line 376 snapshots the configuration, but the
ARCH_PORTDUINOpath callsrIf->init()directly at line 430. A selected LR1120, LR1121, or LR2021 can therefore clampconfig.loraduring probing without suppressed errors, restoration, or reconfiguration.Use one shared probe-initialization path for both Portduino-selected radios and compile-time radio probes.
🤖 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/RadioInterface.cpp` at line 376, Unify the radio probing flow around the shared initialization logic so the ARCH_PORTDUINO path no longer calls rIf->init() directly. Ensure Portduino-selected LR1120, LR1121, and LR2021 radios use the existing loraConfigBeforeProbe snapshot, suppress probe errors, restore config.lora, and reconfigure after probing just like compile-time radio probes.
🤖 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/RadioInterface.cpp`:
- Around line 585-594: Make post-probe reconfiguration failure reject
initialization for all three radio paths: update the LR1120 handling around
`rIf->reconfigure()` at `src/mesh/RadioInterface.cpp` lines 585-594, the LR1121
handling at lines 605-614, and the LR2021 handling at lines 625-634. When
reconfiguration fails, clear or invalidate the radio interface using the
existing initialization-failure flow instead of accepting it; preserve
restoration of `config.lora` in each path.
---
Outside diff comments:
In `@src/mesh/RadioInterface.cpp`:
- Line 376: Unify the radio probing flow around the shared initialization logic
so the ARCH_PORTDUINO path no longer calls rIf->init() directly. Ensure
Portduino-selected LR1120, LR1121, and LR2021 radios use the existing
loraConfigBeforeProbe snapshot, suppress probe errors, restore config.lora, and
reconfigure after probing just like compile-time radio probes.
🪄 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: 74885946-c261-41cb-a417-714643c27f82
📒 Files selected for processing (4)
src/mesh/MeshRadio.hsrc/mesh/RadioInterface.cppsrc/mesh/RadioInterface.htest/test_radio/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- test/test_radio/test_main.cpp
- src/mesh/MeshRadio.h
|
@NomDeTom Yes. LR2021 now uses the full exact bandwidth set accepted by the bundled RadioLib driver, through 1000 kHz, while rejecting 1600 kHz and lossy near-value aliases. The The Portduino initialization path remains intentionally unchanged: it creates one explicitly configured radio and exits if that radio fails, so its validation result is not speculative fallback state. Suppressing/restoring that path would hide genuine invalid configuration on the selected Portduino radio. |
|
@coderabbitai review Final implementation and hardware validation are complete at |
|
The ✅ Action performedReview finished.
|
|
The
I'm not hearing hardware confirmation. |
|
You are right: I do not have LR2021 hardware confirmation. The attached bench covers a T3S3 with SX128x and Muzi/T-Beam devices with LR1121; LR2021 is build- and unit-tested only. The author of #11279 does have dual-band LR2021 hardware, so I opened a focused companion contribution against that branch for the relevant band-switch guards and tests: Ethan-chen1234-zy#1. I will keep #11317’s hardware claim limited to SX128x/LR1121 unless the LR2021 owner reports a successful live matrix. |
2 participants
Summary
Why
A 2.4 GHz node could receive a sub-GHz bandwidth such as 125 kHz. Generic validation accepted it, then the radio driver attempted an unsupported mode and the node could become unresponsive. Mixed T3S3 environments also need validation to follow the radio actually populated on the board rather than the shared PlatformIO target.
Validation
pio run -e tlora-t3s3-v1 -e muzi-base -e meshnology_w12: passed for the mixed SX128x/LR1121 target, Muzi LR1121, and LR2021 production targettrunk fmt,trunk check, andgit diff --check: passed88e643e3b: custom LORA_24 1600 kHz applied, survived reboot, and read back as 1600NotificationRenderer.cpp; the added radio tests are included for Linux CIAttestations
Summary by CodeRabbit
New Features
Bug Fixes
Tests