Lr1121 tcxo optional tries xtal first, and get all my yamls in a row - #11215
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:
📝 WalkthroughWalkthroughLR11x0 initialization now derives TCXO voltage from board and compile-time settings, probes XTAL before optional TCXO fallback, retries SPI command failures with delay, and starts receive only after successful initialization. LR1121 configurations now support E80 and Wio wiring through build-time module selection. ChangesLR11x0 initialization
LR1121 module configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BuildConfig
participant rfswitch_h
participant LR1121Module
BuildConfig->>rfswitch_h: define LR1121_MODULE_E80 or LR1121_MODULE_WIO
rfswitch_h->>LR1121Module: select pin mapping and switch table
LR1121Module-->>BuildConfig: compile selected LR1121 configuration
Possibly related PRs
🚥 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 |
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (14)
Build artifacts expire on 2026-08-30. Updated for |
|
I will test this properly tonight, but it worked in a brief check |
![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 `@src/mesh/LR11x0Interface.cpp`:
- Around line 58-64: Update the tcxoVoltage initialization in the ARCH_PORTDUINO
branch of LR11x0Interface so it tracks whether YAML configured a DIO3 voltage;
use the configured value when present, otherwise apply the existing
TCXO_OPTIONAL 1.6 V default. Preserve the later fallback behavior that depends
on tcxoVoltage > 0.
🪄 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: 2ff25708-0e59-47a9-92cb-4ff123f041ff
📒 Files selected for processing (1)
src/mesh/LR11x0Interface.cpp
e8301a5 to
2f4a846
Compare
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 `@bin/config.d/femtofox/femtofox_E80-900M2213S.yaml`:
- Line 3: Update the hardware model comment at the top of the
femtofox_E80-900M2213S configuration to use the complete identifier
E80-900M2213S, matching the file name and LR1121 module.
🪄 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: 5f0a93e7-272f-4124-9ae1-06e19766ea87
📒 Files selected for processing (3)
bin/config.d/femtofox/femtofox_E80-900M2213S.yamlbin/config.d/femtofox/femtofox_LR1121 generic.yamlbin/config.d/femtofox/femtofox_WIO-LR1121.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
variants/nrf52840/diy/nrf52_promicro_diy_tcxo/rfswitch.h (1)
51-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce duplicated header commentary to the two-line limit. Keep detailed module wiring and truth-table explanations in
readme.md; retain only concise selection/mapping rationale here.
variants/nrf52840/diy/nrf52_promicro_diy_tcxo/rfswitch.h#L51-L62: replace the twelve-line Wio topology explanation with at most two lines.variants/nrf52840/diy/nrf52_promicro_diy_tcxo/rfswitch.h#L9-L14: shorten the module-selection explanation to at most two lines.As per coding guidelines, “Keep code comments minimal—one or two lines maximum.”
🤖 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 `@variants/nrf52840/diy/nrf52_promicro_diy_tcxo/rfswitch.h` around lines 51 - 62, The comments in variants/nrf52840/diy/nrf52_promicro_diy_tcxo/rfswitch.h at lines 51-62 and 9-14 exceed the two-line comment limit. Shorten both module-selection/topology explanations to at most two lines each, retaining only concise selection or mapping rationale; keep the detailed wiring and truth-table information in readme.md.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@variants/nrf52840/diy/nrf52_promicro_diy_tcxo/rfswitch.h`:
- Around line 51-62: The comments in
variants/nrf52840/diy/nrf52_promicro_diy_tcxo/rfswitch.h at lines 51-62 and 9-14
exceed the two-line comment limit. Shorten both module-selection/topology
explanations to at most two lines each, retaining only concise selection or
mapping rationale; keep the detailed wiring and truth-table information in
readme.md.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a4af66d6-60a7-45d3-ba6c-98df6248667a
📒 Files selected for processing (4)
bin/config.d/femtofox/femtofox_WIO-LR1121.yamlvariants/nrf52840/diy/nrf52_promicro_diy_tcxo/readme.mdvariants/nrf52840/diy/nrf52_promicro_diy_tcxo/rfswitch.hvariants/nrf52840/diy/nrf52_promicro_diy_tcxo/variant.h
df10d90 to
1c354c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/mesh/RadioLibInterface.cpp (1)
51-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Throttlefor the elapsed-time check instead of rawmillis()arithmetic.Line 66's
now - busyHighSince < busyWatchTimeoutMsis an elapsed-time check; the repo's ownThrottle::isWithinTimespanMs()helper already exists for exactly this and should be used per guideline forsrc/**/*.cppfiles.♻️ Proposed refactor
+#include "mesh/Throttle.h" + uint32_t LockingArduinoHal::digitalRead(uint32_t pin) { uint32_t value = ArduinoHal::digitalRead(pin); // Not watching, or BUSY is behaving: restart the window if (!busyWatchArmed || pin != busyWatchPin || value == GpioLevelLow) { busyHighSince = 0; return value; } uint32_t now = millis(); if (busyHighSince == 0) { busyHighSince = now; return value; } - if (now - busyHighSince < busyWatchTimeoutMs) + if (Throttle::isWithinTimespanMs(busyHighSince, busyWatchTimeoutMs)) return value;As per coding guidelines, "Use
Throttlefromsrc/mesh/Throttle.hfor time-based rate limiting and elapsed-time checks; do not use rawmillis()arithmetic such asmillis() > lastMs + N, because it is rollover-unsafe."🤖 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/RadioLibInterface.cpp` around lines 51 - 72, Update LockingArduinoHal::digitalRead to use Throttle::isWithinTimespanMs() for the busyHighSince elapsed-time check instead of raw millis() arithmetic. Include the Throttle header and preserve the existing timeout behavior, one-shot reset, logging, and returned GPIO levels.Source: Coding guidelines
src/mesh/RadioLibInterface.h (1)
41-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the BUSY-watchdog comments to comply with the 1–2 line comment guideline.
Both sites explain the same watchdog rationale in multi-paragraph blocks, which the repo's coding guideline explicitly disallows ("Keep code comments minimal—one or two lines maximum ... do not restate straightforward code or add multi-paragraph explanatory blocks"). Condense each to 1–2 lines, keeping only the essential "why" and a link to the upstream issue for anyone who needs the full rationale.
src/mesh/RadioLibInterface.h#L41-L69: shorten the 20-line Doxygen block onarmBusyWatchdog/digitalReadto 1–2 lines plus thejgromes/RadioLib#1844reference.src/mesh/LR11x0Interface.cpp#L47-L52: shorten the 6-lineLR11X0_BUSY_WATCHDOG_MScomment to 1–2 lines.🤖 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/RadioLibInterface.h` around lines 41 - 69, Shorten the BUSY-watchdog comments at src/mesh/RadioLibInterface.h lines 41-69 and src/mesh/LR11x0Interface.cpp lines 47-52 to 1–2 lines each. Retain only the essential reason for the watchdog and the upstream RadioLib issue reference; do not change armBusyWatchdog, disarmBusyWatchdog, or LR11X0_BUSY_WATCHDOG_MS behavior.Source: Coding guidelines
🤖 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/LR11x0Interface.cpp`:
- Around line 148-160: Update the LR11x0 initialization retry and final failure
checks around tryBegin and radioLibResult to also handle
RADIOLIB_ERR_SPI_CMD_TIMEOUT alongside RADIOLIB_ERR_SPI_CMD_FAILED. Preserve the
existing delay/retry behavior and ensure either SPI error causes init() to
return false before subsequent setup calls.
---
Nitpick comments:
In `@src/mesh/RadioLibInterface.cpp`:
- Around line 51-72: Update LockingArduinoHal::digitalRead to use
Throttle::isWithinTimespanMs() for the busyHighSince elapsed-time check instead
of raw millis() arithmetic. Include the Throttle header and preserve the
existing timeout behavior, one-shot reset, logging, and returned GPIO levels.
In `@src/mesh/RadioLibInterface.h`:
- Around line 41-69: Shorten the BUSY-watchdog comments at
src/mesh/RadioLibInterface.h lines 41-69 and src/mesh/LR11x0Interface.cpp lines
47-52 to 1–2 lines each. Retain only the essential reason for the watchdog and
the upstream RadioLib issue reference; do not change armBusyWatchdog,
disarmBusyWatchdog, or LR11X0_BUSY_WATCHDOG_MS behavior.
🪄 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: 7bec1f73-1e2c-4177-b290-5afb89071608
📒 Files selected for processing (10)
bin/config.d/femtofox/femtofox_E80-900M2213S.yamlbin/config.d/femtofox/femtofox_LR1121 generic.yamlbin/config.d/femtofox/femtofox_WIO-LR1121.yamlsrc/mesh/LR11x0Interface.cppsrc/mesh/RadioLibInterface.cppsrc/mesh/RadioLibInterface.hsrc/platform/esp32/MCP23017LockingArduinoHal.cppvariants/nrf52840/diy/nrf52_promicro_diy_tcxo/readme.mdvariants/nrf52840/diy/nrf52_promicro_diy_tcxo/rfswitch.hvariants/nrf52840/diy/nrf52_promicro_diy_tcxo/variant.h
🚧 Files skipped from review as they are similar to previous changes (5)
- bin/config.d/femtofox/femtofox_LR1121 generic.yaml
- variants/nrf52840/diy/nrf52_promicro_diy_tcxo/variant.h
- bin/config.d/femtofox/femtofox_E80-900M2213S.yaml
- variants/nrf52840/diy/nrf52_promicro_diy_tcxo/readme.md
- variants/nrf52840/diy/nrf52_promicro_diy_tcxo/rfswitch.h
e9f1bf6 to
f2cce5b
Compare
NomDeTom
changed the title
NomDeTom
changed the title
|
@caveman99 are we confident that a TCXO LR1121 started with DIO3=0V will definitely fail rather than starting in a degraded mode? |
I was testing with LR2021 a lot and if the previous generation is anything like it (either runs on xtal or tcxo) the startup in the wrong mode will raise a -707 error |
In this case, it does not / is no longer doing that, and instead is going into a perma-busy state. LR2021 with XTAL fails correctly, but LR1121 does not for some reason. |
|
@caveman99 are you happy for this one to go when the checks are finished? |
8e7f565 to
3f017e7
Compare
On boards with TCXO_OPTIONAL, a TCXO-first attempt either hangs RadioLib's calibration wait forever on a bare/non-TCXO module (unpatched upstream), or costs a slow failed attempt before falling back even once that's fixed with a timeout. Measured on hardware: XTAL succeeds immediately on a bare module (~350ms) and fails fast and cleanly on a genuine TCXO module (~300ms, RADIOLIB_ERR_SPI_CMD_FAILED), so trying XTAL first is a strict improvement for hang-avoidance regardless of which oscillator is actually present. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The BUSY watchdog breaks RadioLib's wait, so the next bounded transfer returns SPI_CMD_TIMEOUT rather than SPI_CMD_FAILED. Only the latter was checked, so a watchdog-triggered failure fell through to getVersionInfo(), setRfSwitchTable() and startReceive() against an unresponsive chip. Also use Throttle::isWithinTimespanMs() for the watchdog's elapsed-time check instead of raw millis() arithmetic.
The watchdog bounded RadioLib's unbounded BUSY wait in LR11x0::config() by having LockingArduinoHal::digitalRead() report a stuck pin low exactly once. That let a TCXO-first attempt fail cleanly rather than hang, but it meant lying to RadioLib about a GPIO from a HAL shared by every radio driver. Ordering the attempts XTAL-first avoids the hang outright instead: attempt 1 configures no DIO3 Vref, so there is no calibration wait to get stuck in, and the TCXO fallback is only reached on a module that answered and refused XTAL. Attempts are now XTAL, then TCXO, then a settling retry on whichever oscillator was settled on - after a fallback that is a second TCXO attempt. Only TCXO_OPTIONAL builds probe XTAL; a variant that declares a Vref unconditionally still goes straight to it and never probes XTAL at all. SPI_CMD_TIMEOUT stays a failure alongside SPI_CMD_FAILED: a bounded per-command BUSY wait in Module::SPItransferStream() reports it in its own right, independently of the removed watchdog.
trunk fmt: prettier flags the whitespace-only line inside the <summary> block, which was the only failing check on the PR.
3f017e7 to
b4771a4
Compare
|
@vidplace7 can you take a look at this? Blast radius is low, but I'd prefer another pair of eyes before I merge t. |
|
My weigh-in as I have been somewhat brought in to help with a HAT design for the Luckfox Lyra Zero and an LR1121 radio: Core fix ( Low blast radius is structurally true, not just asserted: when One open question: the femtofox YAML changes correctly move Otherwise this looks mergeable to me. This comment was drafted with the assistance of an LLM, but manually posted by myself |
I'm almost certain noone has had it working with an lr radio |
4 participants
On a board where a TCXO may or may not be fitted (DIY / socketed LR1121 modules), configuring a
DIO3 TCXO reference that isn't physically present makes RadioLib hang forever during
LR11x0::config(). The oscillator never locks, BUSY stays asserted, and the calibration wait isan unbounded
while(digitalRead(busy)). It happens insetup()before any watchdog is running,so the board needs a physical reset. This defeats the existing
TCXO_OPTIONALfallback, whichcan only work if
begin()returns.Upstream bug: jgromes/RadioLib#1844.
This PR bounds tries xtal first, then tcxo, which works without any other fix.
More stuff
Testing
Built for
nrf52_promicro_diy_tcxo. Hang reproduced and traced on a bare (no-TCXO) GNiceRFLR1121 on an nRF52840 Pro Micro; behaviour confirmed independent of the LR1121's own on-die
firmware version (retested after updating
0x0101→0x0104).🤝 Attestations
nRF52840 Pro Micro DIY (
nrf52_promicro_diy_tcxo) with LR1121, both bare/no-TCXO andTCXO-equipped modules.
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Documentation