Fix nRF52 freeze + watchdog reset when saving config over BLE - #11202
Merged
Conversation
Since #10967 phone-originated admin messages are handled synchronously on Bluefruit's BLE event task. NRF52Bluetooth::disconnect() busy-waited for BLE_GAP_EVT_DISCONNECTED, which only that same task can process, so any config save that requires a reboot (e.g. position) deadlocked the device until the 90s watchdog fired. Bound the wait to 1s and sleep instead of spinning so lower-priority tasks (including the watchdog feed) keep running; the SoftDevice completes the link termination on its own.
Contributor
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesNRF52 BLE disconnect handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/platform/nrf52/NRF52Bluetooth.cpp (2)
475-480: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the disconnect rationale comment.
This six-line block exceeds the project’s one- or two-line comment limit. Preserve only the non-obvious reason for the best-effort wait.
Proposed wording
- // Wait for disconnection, but only best-effort with a timeout: phone-originated admin - // messages run on Bluefruit's own BLE event task (sendLocal delivers synchronously), and - // there the DISCONNECTED event that clears the connected flag can't be processed until we - // return - an unbounded wait deadlocks until the watchdog fires. The SoftDevice completes - // the link termination on its own regardless. delay() rather than yield() so lower-priority - // tasks (including the watchdog feed in the main loop) keep running while we wait. + // Best-effort wait: BLE callbacks may be blocked on the calling event task. + // The SoftDevice completes link termination asynchronously.As per coding guidelines, comments must be minimal and limited to non-obvious rationale.
🤖 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/platform/nrf52/NRF52Bluetooth.cpp` around lines 475 - 480, Shorten the comment above the disconnect wait to one or two lines, retaining only the non-obvious rationale that an unbounded wait can deadlock because the DISCONNECTED event cannot be processed until the current BLE event-task handler returns. Remove implementation details about SoftDevice termination, delay versus yield, and watchdog behavior.Source: Coding guidelines
481-483: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Throttle::isWithinTimespanMs()for the BLE disconnect timeout.This one-shot bounded wait follows the repo’s elapsed-time convention; include
mesh/Throttle.hand change the loop towhile (Bluefruit.connected() && Throttle::isWithinTimespanMs(start, 1000)).🤖 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/platform/nrf52/NRF52Bluetooth.cpp` around lines 481 - 483, Include mesh/Throttle.h in NRF52Bluetooth.cpp and update the BLE disconnect wait loop to use Throttle::isWithinTimespanMs(start, 1000) instead of manually comparing millis() against the timeout, while preserving the existing connection check and delay.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 `@src/platform/nrf52/NRF52Bluetooth.cpp`:
- Around line 475-480: Shorten the comment above the disconnect wait to one or
two lines, retaining only the non-obvious rationale that an unbounded wait can
deadlock because the DISCONNECTED event cannot be processed until the current
BLE event-task handler returns. Remove implementation details about SoftDevice
termination, delay versus yield, and watchdog behavior.
- Around line 481-483: Include mesh/Throttle.h in NRF52Bluetooth.cpp and update
the BLE disconnect wait loop to use Throttle::isWithinTimespanMs(start, 1000)
instead of manually comparing millis() against the timeout, while preserving the
existing connection check and delay.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 79f015c7-f0a5-403a-aaf5-6340bb5df257
📒 Files selected for processing (1)
src/platform/nrf52/NRF52Bluetooth.cpp
…k' into fix/nrf52-ble-disconnect-deadlock
Contributor
⚡ 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 (31)
Build artifacts expire on 2026-08-24. Updated for |
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Prevents nRF52 devices from freezing (and eventually watchdog-resetting) when a BLE-originated config save triggers a reboot-required path and attempts to disconnect while running on Bluefruit’s BLE event task.
Changes:
- Adds a bounded (1s) best-effort wait for BLE disconnect to avoid deadlock on the BLE event task.
- Replaces a tight
yield()spin withdelay(1)to allow lower-priority tasks (including watchdog feeding) to run. - Logs whether the disconnect completed or timed out before continuing shutdown.
Comment on lines
+478
to
+480
| // return - an unbounded wait deadlocks until the watchdog fires. The SoftDevice completes | ||
| // the link termination on its own regardless. delay() rather than yield() so lower-priority | ||
| // tasks (including the watchdog feed in the main loop) keep running while we wait. |
8 tasks
madeofstown
pushed a commit
to madeofstown/meshtastic-firmware
that referenced
this pull request
Jul 26, 2026
…stic#11202) * Fix nRF52 freeze + watchdog reset when saving config over BLE Since meshtastic#10967 phone-originated admin messages are handled synchronously on Bluefruit's BLE event task. NRF52Bluetooth::disconnect() busy-waited for BLE_GAP_EVT_DISCONNECTED, which only that same task can process, so any config save that requires a reboot (e.g. position) deadlocked the device until the 90s watchdog fired. Bound the wait to 1s and sleep instead of spinning so lower-priority tasks (including the watchdog feed) keep running; the SoftDevice completes the link termination on its own. * Address review: use Throttle helper, tighten comment * Name the disconnect timeout constant * Log unconfirmed BLE disconnect at WARN with elapsed time
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
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.
Problem
Saving any config section that requires a reboot (position, security, role changes,
commit_edit_settings, …) from a BLE-connected phone freezes nRF52 devices completely — screen, mesh, and serial all dead — until the 90 s hardware watchdog resets the board. Reproduced on a Seeed Wio Tracker L1; the mechanism affects every nRF52 target.Root cause
Since #10967,
Router::sendLocaldelivers self-addressed packets synchronously in the calling task, so phone-originated admin messages run on Bluefruit's BLE event task (thetoRadiowrite callback is registered withuseAdaCallback=false).When the config change keeps
requiresReboot=true,AdminModulecallsdisableBluetooth()while the phone is still connected.NRF52Bluetooth::disconnect()then busy-waits:The
BLE_GAP_EVT_DISCONNECTEDevent that clears that flag can only be processed byadafruit_ble_task— the very task now spinning. The wait can never complete. Because the BLE task runs atTASK_PRIO_HIGHandyield()never lets lower-priority tasks run, the Arduino loop task starves, the watchdog feed stops, and the WDT resets the device ~90 s later.Why it wasn't caught earlier: LoRa config applies live (
requiresReboot=false) and skipsdisableBluetooth()entirely, so region/preset saves — the common test case — never hit this path. Position config always takes the reboot path. #11190 (BLE task stack bump) fixed the stack-overflow crash on this same relocated call chain but not this deadlock.Fix
Make the disconnect wait best-effort with a 1 s bound, and sleep (
delay(1)) instead of spinning so lower-priority tasks — including the watchdog feed in the main loop — keep running during the wait:Ended BLE connection.BLE disconnect still pending, continuing shutdown, and proceeds. The SoftDevice completes the link termination on its own once the callback unwinds, and the normal save + scheduled reboot follow.Testing
On a Wio Tracker L1 (nRF52840):
Disable NRF52 bluetoothis the last log line, device frozen, watchdog reset ~90 s later.seeed_wio_tracker_L1build green; formatting checked against trunk's clang-format config.The durable follow-up remains moving phone-originated admin handling off the BLE event task entirely (#11155).
Summary by CodeRabbit