espressif: Don't lose PacketBuffer data when NimBLE runs out of buffers - #11286
Merged
Merged
Conversation
A burst of outgoing packets (e.g., a BLE workflow directory listing or long REPL output) exhausts NimBLE's mbuf pool, which drains only as fast as the radio can send. So the `BLE_HS_ENOMEM` error only means the mbuf pool is temporarily empty. But it can stay that way for seconds as the data is sent. `queue_next_write()` mishandled it and lost data. Problems: - On a failed notify or indicate it restored `pending_size` for a retry, then fell through to an unconditional `pending_size = 0` that discarded the data anyway. - The retry could not have worked regardless: `ble_gatts_notify_custom()` raises `BLE_GAP_EVENT_NOTIFY_TX` synchronously, inside the failing call, and a failed send gets no later completion event to prompt another attempt. - A failed client write set `packet_queued` even though a failed `ble_gattc_write()` frees its proc without ever calling the callback, wedging the buffer; a failed notify flipped `pending_index`, which it never flips on success, corrupting the index and pointing single-buffer `PacketBuffer`s at a NULL second buffer. Fix by reworking the send path: - `queue_next_write()` now claims the pending buffer under the interrupt- disabled critical section (the `nimble_host` task is pinned to the same core, so this excludes it) by setting `send_in_progress`, which also makes the synchronous `NOTIFY_TX` event and competing send attempts no-ops. On `BLE_HS_ENOMEM` the data is kept for retry; on real errors it is dropped as before. - Every NimBLE send call copies the flat buffer into an mbuf before returning, so the buffer is reusable immediately and the second outgoing buffer and index flipping are unnecessary. Writes coalesce into the single pending buffer while a packet awaits its completion event, or while waiting for mbufs, so 4-byte filename chunks still merge into MTU-sized packets. - The wait loops in `write()` and `flush()` retry the send inline. They may run inside a background callback (the BLE workflow does this), where `RUN_BACKGROUND_TASKS` cannot re-enter background callbacks to help. - A background callback retries trailing data, such as a directory listing's final entry, that no later write or flush would push out. It reschedules itself while still out of mbufs and stops on disconnect or deinit. - `packet_queued` is set before submitting a write with response or an indicate and cleared by the completion event, so a completion that arrives while the submitting task is still preempted is not lost. - Disconnect and unsubscribe discard pending data so that stale data is not sent into a later connection or subscription. - Retries are paced: after a failure, skip further attempts for 10 ms, since the pool refills only a few packets per connection interval and attempting sooner just burns CPU. Successful and hard-failed sends reset the pacing. The now-unused second outgoing buffer stays allocated, with TODO comments recording that NimBLE's copy-on-send is what makes it unnecessary here and that dropping the shared static allocation needs a way for a port to declare how many outgoing buffers it wants. The nordic SoftDevice retains the caller's buffer until TX completes and needs both. The visible symptom was a web workflow directory listing of 30+ files losing its final entry and the terminator that marks the end of the listing, which left the web editor's file dialog spinning forever. Tested on a Metro ESP32-S3 with the web editor over BLE: listings complete, including after fifteen repeated reloads that churn the heap; a multi-kilobyte file write, REPL bursts of 2000 lines, and a disconnect mid-burst followed by a reconnect all behave. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Zephyr test failure is the ongoing saved word failure, unrelated to this. |
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
2 participants
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.
🤖 Generated with Claude Code
Note by @dhalbert: this fix was reworked several times to be clean, and well tested. I've edited this note and the code comments for clarity. The functionality and implementation was also compared against those of other ports.
This is the follow-up promised in #11255, whose body noted this as "a known, pre-existing espressif PacketBuffer send bug, to be fixed in a following PR".
The problem
A burst of outgoing packets, such as a BLE workflow directory listing or long REPL output, exhausts NimBLE's mbuf pool. The pool drains only as fast as the radio sends, so
BLE_HS_ENOMEMmeans the pool is temporarily empty rather than that anything went wrong, and it can stay empty for seconds.queue_next_write()incorrectly treatedBLE_HS_ENOMEMas an error.The visible symptom was a directory listing of 30+ files losing its final entry and the end-of-listing terminator, which left the web editor's file dialog spinning forever.
The fix
BLE_HS_ENOMEMoccurs and retry the send, paced at 10 ms so retries don't outpace the radio. Real errors are still dropped.write()andflush(). The BLE workflow calls both from inside a background callback, andbackground_callback_run_all()will not re-enter itself, so a retry scheduled only as a background callback cannot run while a writer is waiting there.deinit(), so a failed packet cannot reach a later connection and a queued retry cannot outlive the object.The commit message has the full reasoning and the per-path detail.
The unused second outgoing buffer
All the ports currently allocate two outgoing buffers. After this PR the second buffer is unused on
espressif, though still allocated. This is noted in three TODO comments. The second buffer is allocated on the heap for user code, and in static RAM for the BLE workflow. To drop the static allocation, a port needs to be able to declare how many outgoing buffers it wants. ThenordicSoftDevice retains the caller's buffer until TX completes and genuinely needs two buffers.This would be a cross-port change, so it is left for a separate PR.
Testing
Tested on a Metro ESP32-S3 with the web editor over BLE on Linux. These scenarios now all work: