fix(sx126x_hal): add t_woff settle delay to close wake-up BUSY-poll race - #4
Mikefly123 wants to merge 1 commit into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe SX126x driver now supports optional external RF-switch GPIOs, controls them during mode changes, and adds a wake-up delay. Kconfig removes an incompatible selection. The development environment uses the flattened LR-FHSS source path. ChangesSX126x RF-switch control
Kconfig compatibility
LR-FHSS source path
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant sx126x_hal_write
participant sx126x_hal_update_rf_switch
participant RF_Switch_GPIOs
participant SX126x_SPI
sx126x_hal_write->>sx126x_hal_update_rf_switch: SX126x mode-change command
sx126x_hal_update_rf_switch->>RF_Switch_GPIOs: deassert previous path
sx126x_hal_update_rf_switch->>RF_Switch_GPIOs: assert TX, RX, or inactive path
sx126x_hal_write->>SX126x_SPI: transmit command
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
…close BUSY-poll race sx126x_hal_check_device_ready() wakes a sleeping SX126x with a glitch on NSS and then immediately polls BUSY via sx126x_hal_wait_on_busy(). Per the datasheet, the chip needs up to ~340us (t_woff, warm start) after the wake-up NSS edge before it reliably asserts BUSY. Polling right away can sample BUSY before the chip has driven it, so the caller believes the radio is ready when it is still starting up; the very next SPI command (frequently SET_FREQ) is then clocked into a device that isn't listening yet and is silently dropped. This reproduces ~40% of the time in release builds where the post-wake instruction path is fast enough to win the race. Fix: insert a k_busy_wait(500) between the wake-up NSS toggle and the BUSY poll to wait out the chip's startup window before trusting BUSY.
1b9ad75 to
31eeaec
Compare
|
Stack position 4/6 of stack Lora-net#8: base #3. Next: #5. |
![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 `@drivers/usp/sx126x/sx126x_board.c`:
- Around line 242-261: In sx126x_init, add validation before configuring the
external RF-switch GPIOs to reject any configured tx_enable or rx_enable
together with dio2_as_rf_switch, returning -EINVAL. Preserve the existing
dio2-gpios/dio2_as_rf_switch validation and normal GPIO configuration for valid
combinations.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 82e5f199-16ca-411e-ab02-d6ec1e75bc5f
📒 Files selected for processing (6)
drivers/usp/Kconfigdrivers/usp/sx126x/sx126x_board.cdrivers/usp/sx126x/sx126x_hal.cdrivers/usp/sx126x/sx126x_hal_context.hdts/bindings/usp/semtech,sx126x-new-common.yamlmodules/usp_drivers/dev_env.cmake
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@drivers/usp/sx126x/sx126x_board.c`:
- Around line 242-261: In sx126x_init, add validation before configuring the
external RF-switch GPIOs to reject any configured tx_enable or rx_enable
together with dio2_as_rf_switch, returning -EINVAL. Preserve the existing
dio2-gpios/dio2_as_rf_switch validation and normal GPIO configuration for valid
combinations.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 82e5f199-16ca-411e-ab02-d6ec1e75bc5f
📒 Files selected for processing (6)
drivers/usp/Kconfigdrivers/usp/sx126x/sx126x_board.cdrivers/usp/sx126x/sx126x_hal.cdrivers/usp/sx126x/sx126x_hal_context.hdts/bindings/usp/semtech,sx126x-new-common.yamlmodules/usp_drivers/dev_env.cmake
🛑 Comments failed to post (1)
drivers/usp/sx126x/sx126x_board.c (1)
242-261: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Search for any existing mutual-exclusion check between dio2_as_rf_switch and tx_enable/rx_enable. rg -n -C5 'dio2_as_rf_switch' drivers/usp/sx126x/sx126x_board.c rg -n -C5 'tx_enable\.port|rx_enable\.port' drivers/usp/sx126x/sx126x_board.cRepository: Open-Source-Space-Foundation/usp_zephyr
Length of output: 2737
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== sx126x_board.c outline ==" ast-grep outline drivers/usp/sx126x/sx126x_board.c --view compact || true echo echo "== sx126x_board.c relevant init/macros ==" sed -n '220,275p;330,395p;1,80p' drivers/usp/sx126x/sx126x_board.c echo echo "== search for dio2_as_rf_switch and rf-switch properties in repo ==" rg -n -C4 'dio2_as_rf_switch|tx_enable_gpios|rx_enable_gpios|dio2-as-rf-switch|tx-enable-gpios|rx-enable-gpios' drivers/usp/sx126x .devicetree.dtsi include drivers 2>/dev/null | head -n 240 echo echo "== read-only behavioral probe for current static guard logic ==" python3 - <<'PY' from pathlib import Path text = Path('drivers/usp/sx126x/sx126x_board.c').read_text() for needle in [ 'dio2_as_rf_switch && config->tx_enable.port', 'dio2_as_rf_switch && config->rx_enable.port', 'dio2_as_rf_switch && ( config->tx_enable.port || config->rx_enable.port )', 'tx-enable-gpios cannot be combined', 'rx-enable-gpios cannot be combined', ]: print(f'{needle}:', needle in text) print('dio2-gpios only conflict macro:', 'DIO2_CONFLICT' in text and 'dio2-gpios and dio2-as-rf-switch are conflicting' in text) PYRepository: Open-Source-Space-Foundation/usp_zephyr
Length of output: 19546
Enforce dio2-as-rf-switch / tx-enable-rx-enable exclusion at init.
sx126x_board.calready rejectsdio2-gpiosplusdio2-as-rf-switch, but it still acceptstx-enable-gpios/rx-enable-gpiostogether withdio2-as-rf-switch. This configures external switch GPIOs whiledio2_as_rf_switchremains enabled, contrary to the documented hardware mutual exclusion. Reject this invalid DT combination insx126x_initwith-EINVAL.🛡️ Proposed validation
/* External RF-switch GPIOs — configure as outputs, inactive (both paths off) */ + if( config->dio2_as_rf_switch && ( config->tx_enable.port || config->rx_enable.port ) ) + { + LOG_ERR( "dio2-as-rf-switch cannot be combined with tx-enable-gpios/rx-enable-gpios" ); + return -EINVAL; + } + if( config->tx_enable.port ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./* External RF-switch GPIOs — configure as outputs, inactive (both paths off) */ if( config->dio2_as_rf_switch && ( config->tx_enable.port || config->rx_enable.port ) ) { LOG_ERR( "dio2-as-rf-switch cannot be combined with tx-enable-gpios/rx-enable-gpios" ); return -EINVAL; } if( config->tx_enable.port ) { ret = gpio_pin_configure_dt( &config->tx_enable, GPIO_OUTPUT_INACTIVE ); if( ret < 0 ) { LOG_ERR( "Could not configure tx-enable gpio" ); return ret; } } if( config->rx_enable.port ) { ret = gpio_pin_configure_dt( &config->rx_enable, GPIO_OUTPUT_INACTIVE ); if( ret < 0 ) { LOG_ERR( "Could not configure rx-enable gpio" ); return ret; } }🤖 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 `@drivers/usp/sx126x/sx126x_board.c` around lines 242 - 261, In sx126x_init, add validation before configuring the external RF-switch GPIOs to reject any configured tx_enable or rx_enable together with dio2_as_rf_switch, returning -EINVAL. Preserve the existing dio2-gpios/dio2_as_rf_switch validation and normal GPIO configuration for valid combinations.
1 participant
What
Inserts a
k_busy_wait(500)between the wake-up NSS toggle and the BUSY poll insx126x_hal_check_device_ready()(drivers/usp/sx126x/sx126x_hal.c).Why
sx126x_hal_check_device_ready()wakes a sleeping SX126x with a glitch on NSS and immediately polls BUSY. Per the datasheet, the chip needs up to ~340us (t_woff, warm start) after the wake-up NSS edge before it reliably asserts BUSY. Polling immediately can sample BUSY before the chip has driven it, so the caller believes the radio is ready when it is still starting up — the very next SPI command (frequently SET_FREQ) is then clocked into a device that isn't listening yet and is silently dropped. This reproduced ~40% of the time in release builds where the post-wake instruction path is fast enough to win the race.Validation