host: do not deinit ble_hs_timer while its event may be queued (#1184) - #1185
host: do not deinit ble_hs_timer while its event may be queued (#1184)#1185rfordinal wants to merge 1 commit into
Conversation
ble_hs_stop_begin() sets ble_hs_enabled_state to STOPPING and then calls ble_hs_timer_resched() on the next line. ble_hs_is_enabled() only returns true for _ON, so ble_hs_timer_reset() takes the disabled branch and calls ble_npl_callout_deinit(), which on the FreeRTOS port runs ble_npl_event_deinit(&co->ev) and memsets the callout. That clears ev.fn. If the timer had already expired, its ev is already on g_eventq_dflt. The memset does not remove it from the queue, so nimble_port_run() dequeues it and calls a NULL ev->fn. On Xtensa this is a CPU exception with pc 0 and the host task is gone. Removing the call leaks nothing: ble_hs_deinit() already ends with ble_npl_callout_deinit(&ble_hs_timer), which runs after the host has stopped and the queue is drained. Both apache/mynewt-nimble and espressif/esp-nimble call only ble_npl_callout_stop() here. Fixes h2zero#1184
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughChangesHost timer lifecycle
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to The change preserves the timer callback until shutdown cleanup, preventing queued timer events from invoking a cleared callback. No remaining merge-readiness risk is identified. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
…ueue BUG-033 has reset the device on four real exits now. Upstream 2.5.1's ble_hs_timer_reset() calls ble_npl_callout_deinit(&ble_hs_timer) when the host is no longer enabled, which memsets ev.fn to NULL while that same ev may already be queued on g_eventq_dflt. The host task dequeues it and jumps to 0. Pins rfordinal/NimBLE-Arduino @ 63d87323: upstream 2.5.1 with that one line removed and nothing else, so the diff to trust is one line. The pin is the commit rather than the branch, so a moved branch cannot change what a clean build fetches. Removing the call leaks nothing -- ble_hs_deinit() still deinits that callout, after the host has stopped and the queue is drained. The spec is named once as base.nimble_dep because two environments need it and they are not on the same branch: default here on develop, t5s3pro on release/lilygo-t5-s3-pro. Spelled out twice, one of them keeps the unpatched library through a merge with nobody noticing. Keeping the stack up instead was rejected on the measured 58,804 bytes end() returns, with BLE at 86 % of the map screen's heap cost. deinit(false) is not an option either: clearAll does not gate nimble_port_stop() or ble_hs_stop(). Offered upstream the same day as h2zero/NimBLE-Arduino#1185. Builds clean on env:default, zero warnings. NOT run on hardware, and a race is not disproved by one clean exit -- T-233 stays open for a counted map-exit loop.
Fixes #1184. One line removed. This is the PR offered in that issue.
The crash
Reproduced twice on real hardware, ESP32-S3, from two independent coredumps
(2026-09-01 and 2026-09-04), both while the application called
NimBLEDevice::deinit(true):pc = 0withInstFetchProhibitedis a call through a NULL function pointer.The faulting instruction is in
nimble_port_run():a10is the dequeued event, and it resolves toble_hs_timer + 8, theevmember of the host's own timer callout. So
ble_hs_timer.ev.fnwas NULL whenthe host task ran the event.
In the second dump
a10holds the identical value and the code registers areoffset by a constant, so it is the same event and the same call site.
Why it happens
ble_hs_stop_begin()(ble_hs_stop.c) setsble_hs_enabled_state = BLE_HS_ENABLED_STATE_STOPPING, then callsble_hs_timer_resched()on the next line.ble_hs_is_enabled()returns true only for_ON, so it is now false.ble_hs_timer_reset()therefore takes the disabled branch and callsble_npl_callout_stop()andble_npl_callout_deinit().npl_freertos_callout_deinit()doesble_npl_event_deinit(&co->ev)and thenmemset(co, 0, sizeof(struct ble_npl_callout)).ev.fnis now NULL.evis already queued ong_eventq_dflt. The memset does not remove it.nimble_port_run()dequeuesit and calls
ev->fn.ble_npl_eventq_remove()exists in that port and is what step 4 would have tocall. Removing the
deinit()is the smaller fix, and it matches what the othertwo trees already do.
Why removing it leaks nothing
ble_hs_deinit()already ends withble_npl_callout_deinit(&ble_hs_timer).That runs after the host has stopped and the queue is drained, which is the
right time. The call in
ble_hs_timer_reset()is redundant as well as harmful.Comparison with the other trees
ble_hs_timer_reset()when not enabledble_npl_callout_stop()onlyble_npl_callout_stop()onlyf566133,e3cbdc0,70439ddstop()anddeinit()stop()anddeinit()The
deinit()came from esp-nimble, espressif has since dropped it, and thistree still carries the older snapshot.
Testing
The removal itself is not yet run on hardware. What is verified is the fault, the
faulting instruction and the event identity, from two coredumps on ESP32-S3
(NimBLE-Arduino 2.5.1, ESP-IDF 5.5.2), plus the five-step chain read off the
vendored source. We are carrying this exact commit in a pinned fork and will
report back once it has soak-tested on our boards.
Happy to adjust the shape of the fix if you would rather remove the queued event
than skip the deinit.
🤖 Generated with Claude Code
Summary by CodeRabbit