Fix a number of BLE workflow and bonding issues - #11178
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>
Chrome registers no org.bluez.Agent1, so on Linux a browser cannot complete
pairing with the BLE workflow. BlueZ has no pairing UI of its own; it delegates
every pairing interaction to an agent that some application must register, and
with nothing else providing one bluetoothd refuses outright:
src/device.c:new_auth() No agent available for request type 2
device_confirm_passkey: Operation not permitted
The link then drops. Since both the file transfer and serial services are
`SECURITY_MODE_ENC_NO_MITM`, nothing works past the connect. macOS and Windows
are unaffected because the operating system owns pairing there and shows its
own dialog. This also matches Chromium's own support matrix, which lists
on-demand device pairing as unimplemented on Linux.
Run `python3 tools/workflow/ble_agent.py` and leave it running while pairing.
It registers a NoInputNoOutput agent, so BlueZ selects Just Works and asks
nothing -- the same security the workflow already assumes, so this does not
weaken anything. `ble_terminal.py` registers exactly this agent for itself;
this splits it out so it can be used with a browser.
Only pairing needs the agent. Once the board is bonded, reconnects need
nothing. It does not help with the separate Linux kernel problem where a
connect to an unbonded peripheral often times out.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Observed on Linux with a D-Bus monitor: Chromium does register an org.bluez.Agent1, but only when bluetoothd starts, and it unregisters it again on page load. So usually nothing is registered by the time you connect. When it does hold one, RequestDefaultAgent puts it first in line and it then answers RequestAuthorization with org.bluez.Error.Rejected without prompting, which can preempt a desktop agent that would have asked. The docstring's "Chromium registers no agent" was wrong, and understated the problem. Also correct two smaller claims. Not every BLE workflow characteristic is encrypted: the version characteristics are SECURITY_MODE_OPEN, which is why service discovery and a version read succeed before the link drops. And a RegisterAgent failure cannot be caused by another application's agent, because RegisterAgent keys on the D-Bus sender; only a second registration on the same connection collides. Comments only, no behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2bdaaea to
9b9f6cd
Compare
espressif had the same defect as nordic, by two routes, and adafruit#10785 is the report of it. bleio_reset() had no equivalent of nordic's early return, so every VM transition ran a full disable/enable cycle of the stack, dropping any BLE workflow session in progress. It now returns early when user code never imported _bleio, since the VM cannot then have added anything to the GATT table or created any connections. bleio_user_reset() runs on the same path and called bleio_adapter_reset(), which drops every connection, the workflow's included. Its own "// TODO: Don't stop BLE workflow connection." sat on that line. It now stops user scanning and advertising only, as nordic already did, leaving connection teardown to bleio_reset(). Tested on a Metro ESP32-S3 with a workflow session live in the web editor's serial terminal, against a build with both changes reverted. Reverted: a keypress at the "Press any key to enter the REPL" prompt disconnects the client and the board falls back to private advertising. With the changes: the same keypress, and repeated Ctrl-D VM restarts, leave the session connected and working. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Commented out, as a convenience when debugging this board. Note this is CircuitPython's own console, on the board's TX/RX pins; esp-idf's ESP_LOG output goes to UART0 on the separate DEBUG_TX / DEBUG_RX pins. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tannewt
left a comment
tannewt
left a comment
There was a problem hiding this comment.
I'd prefer it if this was a number of PRs to fix different issues. LLMs make it easier to do so but also requires more review than human written code. There are a number of individual fixes that are great but the raw address change is uneccessary and holds the others up.
|
|
||
| // A BLE address without the object wrapper, for code that must not allocate or raise -- | ||
| // notably supervisor/shared, which runs outside the VM. A NULL pointer to one of these | ||
| // means "no address", just as a NULL bleio_address_obj_t * does. | ||
| typedef struct { | ||
| uint8_t bytes[NUM_BLEIO_ADDRESS_BYTES]; | ||
| uint8_t type; // one of BLEIO_ADDRESS_TYPE_* | ||
| } bleio_raw_address_t; | ||
|
|
||
| // Copies `address` into `*raw`. Raises if `address->bytes` is not a readable buffer, in | ||
| // which case `*raw` is left partly or wholly untouched, so only call this where raising | ||
| // is acceptable: not from supervisor/shared. | ||
| void bleio_address_to_raw(const bleio_address_obj_t *address, bleio_raw_address_t *raw); |
There was a problem hiding this comment.
Don't add this. It's ok for a struct with mp_obj_base_t to live off the GC heap. We don't need a new struct just to do that.
There was a problem hiding this comment.
Sure, I'll remove this.
| # | ||
| # SPDX-License-Identifier: MIT | ||
|
|
||
| """A Just Works BLE pairing agent, so a browser can pair with the BLE workflow. |
There was a problem hiding this comment.
Why do we need this in the repo? Maybe it'd be better as a pip package?
There was a problem hiding this comment.
This was a useful debugging tool on Linux, and I didn't want to lose it. I can make a separate repo of BLE tools. I thought of this as a "manual test" helper, so I asked for it to be added to the repo (it wasn't Claude's idea).
| # | ||
| # SPDX-License-Identifier: MIT | ||
|
|
||
| """A command-line BLE terminal for the CircuitPython BLE workflow REPL. |
There was a problem hiding this comment.
Should this be part of circup or some other tool like cpremote?
There was a problem hiding this comment.
same comment as for ble_agent.py
| void bleio_address_to_raw(const bleio_address_obj_t *address, bleio_raw_address_t *raw) { | ||
| mp_buffer_info_t buf_info; | ||
| mp_get_buffer_raise(address->bytes, &buf_info, MP_BUFFER_READ); | ||
| memcpy(raw->bytes, buf_info.buf, NUM_BLEIO_ADDRESS_BYTES); | ||
| raw->type = address->type; | ||
| } | ||
|
|
||
| void common_hal_bleio_address_construct(bleio_address_obj_t *self, uint8_t *bytes, uint8_t address_type) { |
There was a problem hiding this comment.
| void bleio_address_to_raw(const bleio_address_obj_t *address, bleio_raw_address_t *raw) { | |
| mp_buffer_info_t buf_info; | |
| mp_get_buffer_raise(address->bytes, &buf_info, MP_BUFFER_READ); | |
| memcpy(raw->bytes, buf_info.buf, NUM_BLEIO_ADDRESS_BYTES); | |
| raw->type = address->type; | |
| } | |
| void common_hal_bleio_address_construct(bleio_address_obj_t *self, uint8_t *bytes, uint8_t address_type) { | |
| void common_hal_bleio_address_construct(bleio_address_obj_t *self, uint8_t *bytes, uint8_t address_type) { |
| nlr_raise(exception); | ||
| } | ||
|
|
||
| // Set when user code imports _bleio, cleared once a full bleio_reset() has run. |
There was a problem hiding this comment.
When user code is run, then bleio_user_reset() should be run. That's what it is for.
There was a problem hiding this comment.
This comment in the first post applies to this:
bleio_reset()in espressif had no equivalent of the early return, so every VM transition ran a full disable/enable cycle of the stack. Andbleio_user_reset(), which runs on the same path, called bleio_adapter_reset() — which drops every connection, the BLE workflow's included. There was a comment// TODO: Don't stop BLE workflow connection. sitting on that line. It now stops user scanning and advertising only, matching what nordic already did, and leaves connection teardown tobleio_reset(), which only runs when user code imported _bleio.
So I think what you're asking is to fix bleio_user_reset() to not tear down the workflow connection.
There was a problem hiding this comment.
My LLM is wanting to do this to but I'm pushing it to be ok with the connection drop. BLE is designed for flaky connections so we should embrace disconnects. I think that's why I was doing the high frequency reconnect advertisement.
| anonymous = false; | ||
| // ADV_DIRECT_IND carries no advertising data, so drop whatever we were given. | ||
| advertising_data_len = 0; | ||
| scan_response_data_len = 0; |
There was a problem hiding this comment.
Don't silently drop, instead raise an exception.
| bool directed_reconnect = anonymous && directed_to == NULL && | ||
| bonding_load_directed_reconnect_address(&reconnect_peer); |
There was a problem hiding this comment.
This seems like too much logic with the impl.
|
@tannewt So I think the plan would be to open a new PR with these changes.
The other IRK and pairing logic changes are complicated and were empirically derived, and some recent testing I've done with Claude on Windows and Linux shows that tweaks in how the client does the pairing can improve stability and make the pairing and reconnection more reliable. These problems don't show up on iOS or macOS clients. The host-side code acts differently on different OS's. File Glider and macOS Chrome are kind of the best cases. Those changes might be deferred to another PR. I'm going to research this more and will have more info later. |
|
Heads up on a port this misses: the zephyr-cp compiles CI here will not show it. This PR runs no zephyr-cp board builds, only |
It's my responsibility to make the appropriate Zephyr PR updates. We don't need to worry about it here. |
|
I am closing this in favor of several smaller PR's, starting with #11225. |
3 participants
🤖 Generated with Claude Code
Some revisions of the text below by @dhalbert.
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.The same bug on espressif
espressif had the same defect and two ways to hit it, both fixed here.
bleio_reset()in espressif had no equivalent of the early return, so every VM transition ran a full disable/enable cycle of the stack. Andbleio_user_reset(), which runs on the same path, calledbleio_adapter_reset()— which drops every connection, the BLE workflow's included. There was a comment// TODO: Don't stop BLE workflow connection.sitting on that line. It now stops user scanning and advertising only, matching what nordic already did, and leaves connection teardown tobleio_reset(), which only runs when user code imported_bleio.Tested on a Metro ESP32-S3 with a workflow session live in the web editor's serial terminal. Without these two changes, pressing a key at the "Press any key to enter the REPL" prompt disconnects the client, and the board falls back to private advertising. With them, the same keypress — and repeated Ctrl-D VM restarts — leave the session connected and working.
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 bugs were in code that had been copied into other ports and are fixed there too.
Testing
Feather nRF52840 Express
versionInfo(), and file write all work. Saving a file is the original bug's path — autoreload runs, the board soft-reboots, and the serial stream continues over the same GATT link with no reconnect needed, which is thebleio_reset()early return doing its job. Two host-side prerequisites, neither of them a firmware matter: pair the board from the desktop's Bluetooth settings first, and use a web editor build that bounds its advertisement wait, since the deployed one does not get as far as connecting on Linux. Both are covered below.tools/workflow/ble_terminal.py, added in this PR: reconnects by directed advertising when the stack restart does have to happen.Metro ESP32-S3
The BLE workflow serial session survives a keypress at the REPL prompt and repeated Ctrl-D VM restarts. Verified against a build with the two espressif changes reverted, where the same keypress disconnects the client.
Not tested: silabs and ble_hci. Those changes are the copied bug fixes plus a mechanical signature change.
Debugging tools created
tools/workflow/ble_terminal.pyandtools/workflow/ble_agent.pyare development and test tooling, not intended for end users.Linux pairing details, for posterity
On Linux, pair the board from the desktop's Bluetooth settings before using the editor. Pairing is not something the browser can do for you: on Linux it is delegated over D-Bus to an
org.bluez.Agent1that some application has to register, and when none is registeredbluetoothdrefuses the pairing outright. Which application provides one varies — KDE Plasma'sbluedevilkeeps an agent registered for the whole session, GNOME's lives inside the Settings Bluetooth panel and so exists only while that panel is open, and desktops runningblueman-applethave one from login. Pairing from the desktop's own UI also marks the board trusted, which keeps later connections from needing an agent at all. Put the board in discovery mode first (double reset to the blue flash) so it appears as not yet set up. Once bonded, the editor's Reconnect works on its own, because the encryption needs only the bond. This is pre-existing behaviour, not a change from this PR.The reason an unpaired board fails the way it does: the File Transfer service's transfer characteristic is
SECURITY_MODE_ENC_NO_MITM, so the first access to it requires an encrypted link and triggers pairing, while the version characteristic isSECURITY_MODE_OPEN. Service discovery and the version read therefore succeed, and only the subscribe to the transfer characteristic fails, which makes the drop look unprovoked.bluetoothdlogsNo agent available for request type 2at that moment. The BLE serial characteristics areENC_NO_MITMas well, so the same cause can also present as a serial terminal that connects and then moves no data.Web editor and ble-file-transfer-js problems
Chrome on Windows spins while reading
boot_out.txtfor board information, and Chrome on Linux stops with the initial BLE workflow popup still visible. The Linux one is a web editor issue — the deployed editor waits for anadvertisementreceivedevent that Chrome's BlueZ backend never delivers, with no timeout, so the connect dialog stays up indefinitely. It was fixed in circuitpython/web-editor#546, and the Linux results above were obtained by testing with that fix before it was merged. There are some other problems discovered also: adafruit/ble-file-transfer-js#13 (merged already), adafruit/ble-file-transfer-js#11, and adafruit/ble-file-transfer-js#12Notes on code changes
_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 issue
ble_drv_remove_heap_handlers()class of bug fixed here, which would be worth a retest. @dhalbert will re-test after merge of this PR.