nrf52: fix loop-task stack overflow causing reset on BLE pairing/first-sync - #10944
Merged
Conversation
…t-sync The Arduino loop task runs Meshtastic's entire cooperative OSThread scheduler on the Adafruit core's stock 4 KB (1024-word) stack. The 2.8 first-sync path - AdminModule::handleSetConfig -> saveChanges -> configChanged -> RadioInterface::reloadConfig -> LR11x0 reconfigure -> SPI Lock::lock, with vsnprintf/USB-CDC logging frames stacked on top - overflows it. SWD fault capture on a T1000-E (pyocd vector_catch=h) proved it: the loop task's SP was driven 40 bytes BELOW its own pxStack base, the stack paint was consumed to the floor, and the crash was a BusFault in xQueueSemaphoreTake dereferencing a semaphore handle that adjacent-heap corruption had overwritten with log text (0x3f3f207c = "| ??"). The core's HardFault handler is a bare NVIC_SystemReset, so in the field this presents as a silent reboot ~seconds after a phone pairs (or a 30 s zombie hang -> supervision timeout 0x8 when the corruption lands on task TCBs instead). Also explains the set_time_only -> immediate-NodeInfo crash variant reported on empty-DB nodes: same task, different deep chain. 2.7.26 is unaffected because the deep frames (XEdDSA, satellite map conversion, replay engine) did not exist. Fix: set -DLOOP_STACK_SZ=2048 (words = 8 KB) for all nrf52840 targets. Requires the #ifndef guard from meshtastic/Adafruit_nRF52_Arduino#7; until that merges the flag is a harmless redefinition warning. Validated on hardware: pairing + full sync completes, stack paint shows healthy margin under load. Also fix the boot-time reset-reason log: the core's init() caches RESETREAS and W1C-clears the register before setup() runs, so the raw read here has always printed 0. Use readResetReason() instead. (preFSBegin's RESETREAS==0 gate is intentionally left as-is - its degenerate GPREGRET-only behavior is what makes lfs recovery work.)
Contributor
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
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)
📝 WalkthroughWalkthroughThis PR changes nRF52 reset-reason logging to use ChangesNRF52 platform adjustments
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/platform/nrf52/main-nrf52.cpp (1)
381-385: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim the comment to comply with the repo's comment-length guideline.
The functional change (using
readResetReason()) is correct and well-justified, but the accompanying comment spans 4 lines, exceeding the repo's comment-length limit.✏️ Suggested trim
- // The Adafruit core's init() (cores/nRF5/wiring.c) caches RESETREAS into a static and then - // W1C-clears the hardware register before setup() ever runs, so a raw NRF_POWER->RESETREAS - // read here is ALWAYS 0. Use the core's cached copy so this log line is actually meaningful - // (0x1 pin reset, 0x2 watchdog, 0x4 soft reset/SREQ, 0x8 CPU lockup, 0x10000 System OFF wake). + // Adafruit core's init() caches+clears RESETREAS before setup() runs, so a raw + // register read here is always 0; use the core's cached copy instead. uint32_t why = readResetReason();As per coding guidelines, "Keep code comments minimal: one or two lines მაქsimum, only when the reason is not obvious, and do not restate the next line."
🤖 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/main-nrf52.cpp` around lines 381 - 385, Trim the explanatory comment above readResetReason() in main-nrf52.cpp so it fits the repo’s one- or two-line comment guideline. Keep only the essential reason for using the cached reset reason instead of raw NRF_POWER->RESETREAS, and remove the extra detail/list of reset codes while preserving the functional context around the why variable assignment.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 `@variants/nrf52840/nrf52.ini`:
- Around line 26-32: Pin the nRF52 Arduino platform source to an immutable
commit or tag instead of relying on the moving default branch, and keep the
LOOP_STACK_SZ override in place. Update the platform_packages entry in the same
config area that defines the nrf52840 variant so it references a fixed revision
rather than the current master-style target, using the existing nrf52.ini
package configuration to locate it.
---
Nitpick comments:
In `@src/platform/nrf52/main-nrf52.cpp`:
- Around line 381-385: Trim the explanatory comment above readResetReason() in
main-nrf52.cpp so it fits the repo’s one- or two-line comment guideline. Keep
only the essential reason for using the cached reset reason instead of raw
NRF_POWER->RESETREAS, and remove the extra detail/list of reset codes while
preserving the functional context around the why variable assignment.
🪄 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: 563a6986-e9de-43d0-a723-e7e9476c4018
📒 Files selected for processing (2)
src/platform/nrf52/main-nrf52.cppvariants/nrf52840/nrf52.ini
thebentern
added a commit
that referenced
this pull request
Jul 24, 2026
…FS (#11190) Since #10967 made Router::sendLocal handle self-addressed packets synchronously, the entire phone-API chain for a BLE client runs inline in the Bluefruit characteristic write callback: toRadioWriteCb -> PhoneAPI::handleToRadio -> admin set-config -> radio reconfigure -> NodeDB::saveToDisk. That callback executes on the Bluefruit BLE FreeRTOS task, whose stock stack is 5 KB (CFG_BLE_TASK_STACKSIZE = 256*5 words) - not the Arduino loop task that #10944 already raised to 8 KB. The loop-task fix therefore protects the wrong task for BLE-originated writes. On a Seeed Wio Tracker L1 the 5 KB stack overflows during pairing first-sync, resetting the device mid-LittleFS-write, every single time. Repeated mid-write resets tear the LittleFS metadata, lfs_assert fires on the next boot, and the corruption handler formats the whole filesystem: region, channels, module config, and the node's keypair are all lost (critical fault #13, new node identity on next region set). Reproduced end-to-end tonight on stock develop 6908d27; with this change the same device pairs, serves config screens, and survives back-to-back config.proto saves over BLE. Raise the BLE task to the same 2048 words (8 KB) as LOOP_STACK_SZ, for the same reason. bluefruit.cpp's #ifndef guard makes the -D take effect with no framework patch. Costs 3 KB of RAM on nrf52840 targets only. Credit where due: Ixitxachitl independently established in #11155 testing that the save-path crash persists after #11185 and that re-queueing sendLocal (moving the pipeline back to the Router thread) makes it go away - which corroborates this diagnosis from the other direction. This commit is the minimal capacity-side fix; #11155's relocation of the pipeline off the BLE task remains the right architectural follow-up, and this guard stays correct even after it lands. Likely also explains #10905 (L1 display-thread crash when a client requests full configuration) and the 2.8 field reports of idle nodes losing region and keys after a BLE session.
Itzdavid01
pushed a commit
to Itzdavid01/firmware
that referenced
this pull request
Sep 5, 2026
…t-sync (meshtastic#10944) The Arduino loop task runs Meshtastic's entire cooperative OSThread scheduler on the Adafruit core's stock 4 KB (1024-word) stack. The 2.8 first-sync path - AdminModule::handleSetConfig -> saveChanges -> configChanged -> RadioInterface::reloadConfig -> LR11x0 reconfigure -> SPI Lock::lock, with vsnprintf/USB-CDC logging frames stacked on top - overflows it. SWD fault capture on a T1000-E (pyocd vector_catch=h) proved it: the loop task's SP was driven 40 bytes BELOW its own pxStack base, the stack paint was consumed to the floor, and the crash was a BusFault in xQueueSemaphoreTake dereferencing a semaphore handle that adjacent-heap corruption had overwritten with log text (0x3f3f207c = "| ??"). The core's HardFault handler is a bare NVIC_SystemReset, so in the field this presents as a silent reboot ~seconds after a phone pairs (or a 30 s zombie hang -> supervision timeout 0x8 when the corruption lands on task TCBs instead). Also explains the set_time_only -> immediate-NodeInfo crash variant reported on empty-DB nodes: same task, different deep chain. 2.7.26 is unaffected because the deep frames (XEdDSA, satellite map conversion, replay engine) did not exist. Fix: set -DLOOP_STACK_SZ=2048 (words = 8 KB) for all nrf52840 targets. Requires the #ifndef guard from meshtastic/Adafruit_nRF52_Arduino#7; until that merges the flag is a harmless redefinition warning. Validated on hardware: pairing + full sync completes, stack paint shows healthy margin under load. Also fix the boot-time reset-reason log: the core's init() caches RESETREAS and W1C-clears the register before setup() runs, so the raw read here has always printed 0. Use readResetReason() instead. (preFSBegin's RESETREAS==0 gate is intentionally left as-is - its degenerate GPREGRET-only behavior is what makes lfs recovery work.)
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
1 participant
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.
Fixes the 2.8 nRF52 crash where devices silently reset seconds after a phone connects/pairs (tracked in field reports as a T1000-E "bootloop on BLE connect"; also reproduced deterministically with
set_time_onlyon an empty-DB node).Root cause — SWD-proven
The Arduino loop task runs Meshtastic's entire cooperative OSThread scheduler on the Adafruit core's stock 4 KB (1024-word) stack (
LOOP_STACK_SZ, previously not overridable). The 2.8 first-sync window stacks the deepest excursion:…with
_vsnprintf_r/ USB-CDC logging frames interleaved on the same stack.A hardware fault capture (RAKDAP +
pyocd vector_catch=h, T1000-E, 100 % repro) showed:pxStackbase; stack paint (0xa5a5a5a5) consumed to the floor0x3f3f207c="| ??")CFSR=0x8200(precise BusFault),BFAR = handle+0x38The core's HardFault handler is a bare
NVIC_SystemReset, so in the field this is a silent instant reboot (log severed mid-line); when the corruption lands on task TCBs instead, it's a 30 s zombie hang ending in a supervision-timeout0x8disconnect. 2.7.26 is unaffected because the deep frames (XEdDSA signing, satellite-map conversion, replay engine) didn't exist yet.Fix
-DLOOP_STACK_SZ=2048(words = 8 KB) for all nrf52840 targets. Validated on hardware: pairing + full sync completes; stack paint shows healthy margin under live BLE load.#ifndefguard; merge that first — until then this flag is a harmless macro-redefinition warning with no effect).Reset reason:log has always printed0x0because the core'sinit()caches-and-clearsRESETREASbeforesetup()runs — switched toreadResetReason(). (preFSBegin()'sRESETREAS==0gate is deliberately untouched; its degenerate GPREGRET-only behavior is what makes lfs recovery work today.)RAM cost: 4 KB from the heap arena per nrf52840 device — more than covered by the +8 KB reclaimed in #10903.
Verification
tracker-t1000-eandrak4631build green;mov.w r2, #2048confirmed in both binariesSummary by CodeRabbit