Fix a number of BLE workflow and bonding issues - #11178
Draft
dhalbert wants to merge 2 commits into
Draft
Conversation
Original motivation was to fix BLE workflow serial dropping the link on the first keystroke. Typing at the "Press any key to enter the REPL" prompt over the BLE workflow serial ended the session after exactly one character. The first byte breaks `main.c`'s wait loop, which then calls `bleio_reset()`, and on nordic restarts the SoftDevice. The BLE bonding survives that restart, but the link does not. The main debugging was done in the nordic port. Behaviour changes: * `bleio_reset()` now returns early when user code never imported `_bleio`. The SoftDevice restart was there only to drop leftover user-created GATT services. The SoftDevice offers no way to remove those individually, so it is unnecessary when the VM cannot have created any in this VM instantiation. Tracked by a flag set in `bleio___init__()`. * A bonded central may distribute no identity address; this is true for BlueZ with its default `Privacy=off`, and on Windows. In that case, store the address it connected from in `peer_id.id_addr_info` and reconnect by aiming `ADV_DIRECT_IND` at it. Such a central does not use privacy, so it cannot resolve a private address and will never recognise our undirected advertisement, but it does connect from a stable address we can target. Centrals that do distribute an IRK (iOS, macOS, Android) keep undirected private advertising, which is what works for them and what Apple's accessory guidelines require. Various bugs found during debugging are now fixed. Similar bugs in other ports (due to code copying) were fixed after the fixes were vetted in nordic. They have not been tested yet. * `bleio_adapter_reset()` mistakenly waited zero milliseconds for disconnects to complete: the loop read `while (any_connected && ...)` with `any_connected` initialised to `false`. The SoftDevice was then disabled before the disconnect PDU went out, so the central saw a link supervision timeout rather than a disconnect reason. Now a `do/while`. Same bug was also fixed in espressif and silabs. * Anonymous advertising set `private_addr_cycle_s` to `timeout + 1`, and the workflow advertises with an unlimited timeout encoded as zero, so the resolvable private address rotated every second -- way too fast for a central to resolve an address and still connect to it. Passing zero selects the SoftDevice default of 15 minutes, which is also the maximum rotation period Microsoft's accessory guidelines allow. * Directed advertising selected the high duty cycle type for an unlimited timeout, for the same "zero means unlimited" reason. The spec caps high duty cycle at 1.28 seconds. Also fixed in espressif. * `ble_drv_remove_heap_handlers()` mistakenly stopped after the first handler it removed, because `ble_drv_remove_event_handler()` clears the removed entry's next pointer. Same bug fixed in espressif's `ble_event_remove_heap_handlers()`. * `common_hal_bleio_packet_buffer_deinit()` never cleared `self->characteristic`, so `common_hal_bleio_packet_buffer_deinited()` always reported false and the guards in `supervisor/shared/bluetooth/serial.c` never took effect. It also removed the client event handler for server-side buffers. Same bug fixed in silabs and ble_hci. * The BLE serial RX ringbuf was mistakenly given a size of `sizeof(_incoming) * sizeof(uint32_t)`, four times the 256 bytes it actually has. * `bonding_load_identities()` returned peers that had distributed no IRK, mistakenly handing an all-zero identity to `sd_ble_gap_device_identities_set()`. * Connection slots mistakenly retained the previous peer's keys, because `bonding_keys` was cleared only on adapter enable and not per connection, so a recycled slot could accidentally store one peer's IRK with another's LTK. Now they are cleared. `_common_hal_bleio_adapter_start_advertising()` now takes `directed_to` as a raw `bleio_raw_address_t` instead of a `bleio_address_obj_t`, which allows moving `mp_get_buffer_raise()` up into `shared-bindings`. Now the code in `supervisor/shared` calls the internal function and cannot raise an exception. Some typos about the workflow UUIDs were fixed in `docs/workflows.md`. Tested on a Feather nRF52840 Express. A simple terminal program using bleak was developed for testing. It is now in `tools/workflow/ble_terminal.py`. Linux with the bleak terminal reconnects in about 140 ms by directed advertising. https://code.circuitpython.org only works properly on macOS Chrome now. It reconnects by undirected private advertising. Still to fix: Chrome on Windows spins on startup while trying to read `boot_out.txt` for board information. Chrome on Linux doesn't even get that far: it stops with the initial BLE workflow popup visible. Commit message edited by @dhalbert. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
🤖 Generated with Claude Code
Draft: I want to test espressif and make further commits before this is ready for review.
The original bug
Typing at the "Press any key to enter the REPL" prompt over the BLE workflow serial ended the session after exactly one character. The first byte breaks
main.c's wait loop, which then callsbleio_reset(), and on nordic that restarts the SoftDevice. The bonding survives that restart; the link does not.bleio_reset()now returns early when user code never imported_bleio. The stack restart is only there to drop leftover user-created GATT services, which the SoftDevice offers no way to remove individually, so it is unnecessary when the VM cannot have created any.Reconnecting when the restart does have to happen
The peripheral now chooses its reconnect advertising strategy from what the bonded central distributed at pairing time:
Privacy=off), WindowsADV_DIRECT_INDaimed at the address it connected fromA central that distributed no IRK is not using privacy. It cannot resolve our private address, so it will never recognise an undirected advertisement as us — but it does connect from a stable address, which we now store with the bond and can target. Centrals that do use privacy keep undirected advertising, which is what works for them and what Apple's accessory guidelines require.
Bugs found along the way
The commit message has the full list with details. In brief:
bleio_adapter_reset()waited zero milliseconds, so the stack was torn down before the disconnect went out and the central saw a supervision timeoutble_drv_remove_heap_handlers()stopped after the first handler it removed, leaving the rest pointing into a heap about to be freedcommon_hal_bleio_packet_buffer_deinited()could never return true, so the guards insupervisor/shared/bluetooth/serial.cnever took effectSeveral of these had been copied into other ports and are fixed there too.
Testing
On a Feather nRF52840 Express:
tools/workflow/ble_terminal.py, added in this PR: reconnects in about 140 ms by directed advertising.Not tested: espressif, silabs, ble_hci. Those changes are the copied bug fixes plus a mechanical signature change.
Still broken, and verified as pre-existing against a stock 10.3.0-alpha.4 build, so not regressions from this PR: Chrome on Windows spins while reading
boot_out.txtfor board information, and Chrome on Linux stops with the initial BLE workflow popup still visible.Where to look hardest
_common_hal_bleio_adapter_start_advertising()now takesdirected_toas a rawbleio_raw_address_tinstead of ableio_address_obj_t, movingmp_get_buffer_raise()up into the user-facing wrapper. That touches all four implementations. The reason is thatsupervisor/sharedcalls the internal function and must not raise.peer_id.id_addr_infonow carries two meanings, distinguished by whetherpeer_id.id_infoholds an IRK: either an identity address the peer distributed, or merely the address the peer connected from.bonding_load_identities()skips entries with no IRK so the two cannot be confused.Related issues
bleio_reset()has the identical structure. Not fixed here. It appears to have a second cause as well:bleio_user_reset()there disconnects the workflow connection on every VM exit, which is what its own// TODO: Don't stop BLE workflow connection.is about.ble_drv_remove_heap_handlers()class of bug fixed here, which would be worth a retest.