Skip to content

comm/ble: implement GATT service-changed indication#1498

Merged
gmarull merged 4 commits into
coredevices:mainfrom
Mearman:feat/gatt-service-changed-indication
Jun 15, 2026
Merged

comm/ble: implement GATT service-changed indication#1498
gmarull merged 4 commits into
coredevices:mainfrom
Mearman:feat/gatt-service-changed-indication

Conversation

@Mearman

@Mearman Mearman commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

The service-changed indication was an empty stub at both the service layer and the NimBLE driver. The GATT spec requires the server to send an indication over the Service Changed characteristic (0x1801/0x2a05) when its attribute database changes, so the client knows to rediscover. Without it, connected clients carry stale caches after a firmware update.

comm/ble: implement GATT service-changed indication fills in both stubs. The service layer resolves the live connection under bt_lock, bails if it was torn down between the timer firing and the callback running, and sends a full-range invalidation (0x0001-0xFFFF) per BT Core Spec 2.5.2. The NimBLE driver looks up the Service Changed value handle via ble_gatts_find_chr, builds an os_mbuf from the ATTHandleRange, and emits it with ble_gatts_indicate_custom.

test/comm/ble: re-enable gatt_service_changed_server with bt_driver fakes ports the test off the deleted SS1 fakes onto fake_bt_driver_gatt, which now records the connection id and handle range from each indication rather than just counting them. Adds a test that the indication carries the subscribed connection and the full invalidation range. Removes the test from BROKEN_TESTS.

Merge order: 1 (independent)

@gmarull

gmarull commented Jun 12, 2026

Copy link
Copy Markdown
Member

Suggestion on the driver-side connection lookup: on the nimble port, gatt_connection_id is never actually populated — nothing in src/bluetooth-fw/nimble/ calls bt_driver_cb_gatt_handle_connect (only the disconnect and MTU callbacks exist), so every GAPLEConnection keeps gatt_connection_id == 0. The service layer therefore always passes 0 into bt_driver_gatt_send_changed_indication(), and gap_le_connection_by_gatt_id(0) matches any live connection. It works today because there's a single LE connection, but the id round-trip is effectively dead code — and it's the only reason the driver needs bt_lock and a reach-back into fw/comm/ble connection state.

A cleaner shape, matching how bt_driver_gap_le_device_name_request() already works: have prv_send_service_changed_indication() copy connection->device (by value) while it already holds bt_lock, and pass that to the driver instead:

void bt_driver_gatt_send_changed_indication(const BTDeviceInternal *device, const ATTHandleRange *data);

The nimble implementation then reduces to pebble_device_to_nimble_conn_handle(device, &conn_handle), which resolves through NimBLE's own connection table (ble_gap_conn_find_by_addr) — no bt_lock and no GAPLEConnection access in the driver at all. Given the known lock-ordering hazards between bt_lock and the NimBLE host task (see the comment in bt_driver_cb_gatt_service_changed_server_subscribe), keeping bt_lock out of driver paths where it isn't needed seems worth it.

The signature change is cheap: the only other implementations are empty stubs (src/bluetooth-fw/stub/gatt.c, src/bluetooth-fw/qemu/gatt.c) plus the test fake/stub.

@Mearman Mearman force-pushed the feat/gatt-service-changed-indication branch 6 times, most recently from 7947008 to 380c3d2 Compare June 12, 2026 14:19
@sjp4

sjp4 commented Jun 12, 2026

Copy link
Copy Markdown
Member

Ahh that could help explain a crash we were seeing in Kable on iOS

@gmarull

gmarull commented Jun 12, 2026

Copy link
Copy Markdown
Member

Ahh that could help explain a crash we were seeing in Kable on iOS

yes, not sure how that went unnoticed for so long. We need to blow up the BLE pseudostack we have on top of NimBLE

Mearman and others added 4 commits June 12, 2026 16:57
The service-changed path was an empty stub at both layers. The service layer
now sends an indication when the GATT database changes, and the NimBLE driver
emits it over the Service Changed characteristic (0x1801/0x2a05) via
ble_gatts_indicate_custom with the affected handle range.

Signed-off-by: Joseph Mearman <joseph@mearman.co.uk>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…akes

Port the test off the deleted SS1 fakes onto fake_bt_driver_gatt, which records
the indications the driver emits, and take it out of BROKEN_TESTS.

Signed-off-by: Joseph Mearman <joseph@mearman.co.uk>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Use PBL_LOG_MODULE_DECLARE / PBL_LOG_ERR matching the pattern in
gatt_client_operations.c and other bluetooth-fw files. LOG_DOMAIN_BT
was removed from logging.h in 5483b6d; the new per-module logging
doesn't need a domain argument.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Joseph Mearman <joseph@mearman.co.uk>
Change the driver signature from (connection_id, range) to (device,
range) so the NimBLE implementation resolves the connection handle
through its own connection table (ble_gap_conn_find_by_addr) instead
of reaching back into fw/comm/ble under bt_lock.

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Joseph Mearman <joseph@mearman.co.uk>
@Mearman Mearman force-pushed the feat/gatt-service-changed-indication branch from a5ff8e3 to d5f5b2f Compare June 12, 2026 15:58
@gmarull gmarull merged commit e4badb9 into coredevices:main Jun 15, 2026
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants