Repository navigation
bluetooth improvements - #712
Conversation
crash fix and HID lifecycle fixes Bluetooth peripheral identities
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 70 functions across 13 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
| Title check |  |
The title identifies Bluetooth as the affected area but uses the vague phrase "improvements" and does not identify the primary changes, such as identity support, HID lifecycle fixes, or API changes. | Replace the title with a specific summary of the main change, such as "Add per-app Bluetooth identities and fix HID lifecycle handling". |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
a900d9f2-ca38-4b5c-9d19-ef7663d0bdec
📒 Files selected for processing: 13
Drivers/mock-drivers-module/source/mock_bluetooth.cppModules/bluetooth-module/source/bluetooth_service.cppPlatforms/platform-esp32/private/bluetooth/esp32_ble_internal.hPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_hid.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_hid_host.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_identity.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_midi.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_scan.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_spp.cppTactilityKernel/include/tactility/drivers/bluetooth.hTactilityKernel/source/drivers/bluetooth.cppTactilityKernel/source/symbols.c
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Fail activation when initial bond-slot setup fails. · esp32_ble_identity.cpp:219-231
Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_identity.cpp:219-231
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail activation when initial bond-slot setup fails.
When no owner marker exists,
ble_identity_bond_slot_activate()logs failures fromclear_namespace(BOND_NAMESPACE)andwrite_owner(identity_hash)but still returnstrue. Startup then accepts the requested identity and loads the activenimble_bondrecords. A failed clear can associate stale bonds with the new identity. A failed owner write can allow pairing without a durable owner marker, so a later activation can erase those bonds.Return
falsefrom either failure path so the caller keeps the current identity and can retry activation later.Suggested fix
if (!clear_namespace(BOND_NAMESPACE)) { - LOG_W(TAG, "could not clear stale bonds; continuing"); + LOG_W(TAG, "could not clear stale bonds"); + return false; } if (!write_owner(identity_hash)) { LOG_W(TAG, "could not record bond slot owner; bonds may be shared between identities"); + return false; }
🟡 Minor · Preserve a discoverable name for supported long device names. · esp32_ble.cpp:715-718
Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cpp:715-718
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve a discoverable name for supported long device names.
api_set_device_nameaccepts names up to 64 bytes, but both advertising paths remove names that do not fit in the advertisement. They then pass the complete name to the scan response and continue advertising when that operation fails. Use a shortened name marked incomplete, or reject names that cannot be advertised.
🟡 Minor · Synchronize aborted name-resolution results with scan completion. · esp32_ble_scan.cpp:53
Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_scan.cpp:53
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSynchronize aborted name-resolution results with scan completion.
ble_scan_abort()runs outside the NimBLE host task and publishesBT_EVENT_SCAN_FINISHEDwithout waiting for an activename_read_callback(). The callback can therefore enqueueBT_EVENT_PEER_FOUNDafterward. Guard the callback withname_res_abortand serialize its publication withfinish_scan_events().Suggested fix
static void finish_scan_events(struct Device* device) { - if (!ble_get_scan_active(device)) return; + BleCtx* ctx = ble_get_ctx(device); + xSemaphoreTake(ctx->scan_mutex, portMAX_DELAY); + if (!ble_get_scan_active(device)) { + xSemaphoreGive(ctx->scan_mutex); + return; + } ble_set_scan_active(device, false); struct BtEvent e = {}; e.type = BT_EVENT_SCAN_FINISHED; ble_publish_event(device, e); + xSemaphoreGive(ctx->scan_mutex); }} BtPeerRecord record = (idx < ctx->scan_count) ? ctx->scan_results[idx] : BtPeerRecord{}; - xSemaphoreGive(ctx->scan_mutex); - - struct BtEvent e = {}; - e.type = BT_EVENT_PEER_FOUND; - e.peer = record; - ble_publish_event(ctx->device, e); + if (!ctx->name_res_abort.load()) { + struct BtEvent e = {}; + e.type = BT_EVENT_PEER_FOUND; + e.peer = record; + ble_publish_event(ctx->device, e); + } + xSemaphoreGive(ctx->scan_mutex); }
ℹ️ Review info
⚙️ Run configuration
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
86d02ba5-9d3d-40d5-b4fa-29a4d2c6b501
📒 Files selected for processing: 4
Platforms/platform-esp32/private/bluetooth/esp32_ble_internal.hPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_identity.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_scan.cpp
🚧 Files skipped from review as they are similar to previous changes: 1
- Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_identity.cpp
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Abort HID start when GAP preparation times out. · esp32_ble_hid.cpp:399-404
Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_hid.cpp:399-404
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAbort HID start when GAP preparation times out.
ble_prepare_profile_start(device)can returnfalsewhile the terminated name-resolution link is still closing.hid_device_start()ignores this result and continues toble_hid_switch_profile(), which can fail withERROR_INVALID_STATE. Return the preparation error before changing the identity or HID context.Suggested fix
ℹ️ Review info
-
⚙️ Run configuration
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
5ec53364-41de-41d4-9ab7-db5cd2a6e286
📒 Files selected for processing: 3
-
Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_identity.cppPlatforms/platform-esp32/source/drivers/bluetooth/esp32_ble_scan.cpp
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
-
|
Thanks a lot! |
Crash fix and HID lifecycle fixes
Fix a crash on radio re-enable.
nimble_port_deinit()frees the NPL event pool thatgiveup_event's block came from, andnpl_freertos_event_init()memsets the stale pointerinstead of allocating a new block. When the new context reused that block, the memset
corrupted the new pool's free list and the first HCI event after re-enabling the radio
(typically connection complete) crashed in
os_memblock_get(). The pointer is now cleared onevery enable cycle.
Fix a phantom "connected" device with no app open. The service no longer auto-starts a HID
device profile on radio-on (it advertised whichever identity was current as a keyboard with no
app behind it), profile-less radios stay silent instead of advertising the device name, and
apps stop their HID profile on teardown.
HID device sessions:
BLE_GAP_EVENT_CONNECT, so a stop during a discovery-onlyconnection terminates the link instead of leaving a half-dead HID device,
Nonefrom the disconnect handler on the host task, wherethe GATT registry can safely change,
bluetooth_hid_device_stop()no longer touches the GATT registry, which makes it safe fromapp teardown and non-host tasks.
Profile starts abort the background scan's name resolution and wait for the GAP to go idle. The
resolution chain opens short-lived central connections, and a cancelled connect only clears
asynchronously, so HID profile switches used to fail with a "GAP procedure still active" while
the service was scanning.
The service skips HID host auto-connect while a peripheral profile is serving. Bonds are stored
per identity, so a host connection attempt under the app's identity would authenticate against
the wrong bond set.
Advertising carries the complete device name when it fits, with the scan response covering
names that do not. The shortened 8-character name is gone, it was identical for every
Tactility ...identity.Central and peripheral roles are separated in the API.
bluetooth_host_connect()andbluetooth_host_disconnect()cover the central side (HID host), peripheral profiles keep theirown
bluetooth_<profile>_start()/stop()calls. The old profile-basedbluetooth_connect()andbluetooth_disconnect()were removed and their callers updated.bluetooth_is_peripheral_active()reports whether a HID device, SPP or MIDI profile isserving.
The identity API is new in this firmware. Apps keep setting the device name, which is the
default identity, so the app repo does not need an SDK update to keep pairing separately.
Known limitations:
session. Peripheral profiles that start in that state join the current identity, so they show up
as the same device on the peer.
NimBLE cannot rebuild the GATT table under a connection. Disconnect the host device first (for
example a paired BLE keyboard).
unpairing from Tactility removes the bond from the active identity only.
Bluetooth peripheral identities
Apps that expose a peripheral profile can now look like separate bonded devices on the host.
The identity defaults to the device name (
bluetooth_set_identity()overrides it,NULL/""reverts to the name) and determines two things:
FNV-1a hash of the identity (
ble_identity_derive_addr()),nimble_bondnamespace and onebtid_<hash>archive namespace per identity, because NimBLE keys store records by peer address only.
The swap is crash-safe (archive and transition marker committed before the new set is
installed, so a power loss replays the restore). It runs with the host down in
dispatch_enable(), or on an idle running radio when the next profile starts, so a second appno longer advertises under the first app's identity.
Central roles (scan, name resolution, HID host) keep
BLE_OWN_ADDR_PUBLIC, so bonds withkeyboards and other peripherals are unaffected by app identity changes.
bluetooth_set_identity()may be called while the radio is on: the change is pending until thenext profile start with the radio idle, or the next radio cycle.
bluetooth_get_identity()always reports the identity the running session resolved.
bluetooth_is_peripheral_active()reports whether a HID device, SPP or MIDI profile is serving. Kernel API, symbol exports and
mock driver included.
Summary by CodeRabbit