From cacc96d98c2a70dc7e5194331ea70746c39746ec Mon Sep 17 00:00:00 2001 From: Jim Mussared Date: Tue, 9 Aug 2022 16:50:58 +1000 Subject: [PATCH] extmod/modbluetooth: Replace def_handle with end_handle in char IRQ. This is technically a breaking change, but: a) We need the end handle to do descriptor discovery properly. b) We have no possible use for the existing definition handle in the characteristic result IRQ. None of the methods can use it, and therefore no existing code should be using it in a way that changing it to a different integer value should break. Unfortunately NimBLE doesn't make it easy to get the end handle, so also implement a mechanism to use the following characteristic to calculate the previous characteristic's end handle. Signed-off-by: Jim Mussared --- docs/library/bluetooth.rst | 2 +- extmod/btstack/modbluetooth_btstack.c | 2 +- extmod/modbluetooth.c | 12 +++-- extmod/modbluetooth.h | 2 +- extmod/nimble/modbluetooth_nimble.c | 70 +++++++++++++++++++++++---- extmod/nimble/modbluetooth_nimble.h | 15 ++++++ 6 files changed, 86 insertions(+), 17 deletions(-) diff --git a/docs/library/bluetooth.rst b/docs/library/bluetooth.rst index 052f7a5c78..8f7041e8d3 100644 --- a/docs/library/bluetooth.rst +++ b/docs/library/bluetooth.rst @@ -166,7 +166,7 @@ Event Handling conn_handle, status = data elif event == _IRQ_GATTC_CHARACTERISTIC_RESULT: # Called for each characteristic found by gattc_discover_services(). - conn_handle, def_handle, value_handle, properties, uuid = data + conn_handle, end_handle, value_handle, properties, uuid = data elif event == _IRQ_GATTC_CHARACTERISTIC_DONE: # Called once service discovery is complete. # Note: Status will be zero on success, implementation-specific value otherwise. diff --git a/extmod/btstack/modbluetooth_btstack.c b/extmod/btstack/modbluetooth_btstack.c index d41c671d68..b58be78a99 100644 --- a/extmod/btstack/modbluetooth_btstack.c +++ b/extmod/btstack/modbluetooth_btstack.c @@ -450,7 +450,7 @@ STATIC void btstack_packet_handler(uint8_t packet_type, uint8_t *packet, uint8_t gatt_client_characteristic_t characteristic; gatt_event_characteristic_query_result_get_characteristic(packet, &characteristic); mp_obj_bluetooth_uuid_t characteristic_uuid = create_mp_uuid(characteristic.uuid16, characteristic.uuid128); - mp_bluetooth_gattc_on_characteristic_result(conn_handle, characteristic.start_handle, characteristic.value_handle, characteristic.properties, &characteristic_uuid); + mp_bluetooth_gattc_on_characteristic_result(conn_handle, characteristic.value_handle, characteristic.end_handle, characteristic.properties, &characteristic_uuid); } else if (event_type == GATT_EVENT_ALL_CHARACTERISTIC_DESCRIPTORS_QUERY_RESULT) { DEBUG_printf(" --> gatt descriptor query result\n"); uint16_t conn_handle = gatt_event_all_characteristic_descriptors_query_result_get_handle(packet); diff --git a/extmod/modbluetooth.c b/extmod/modbluetooth.c index c2cbf0e70a..bd4d9b7179 100644 --- a/extmod/modbluetooth.c +++ b/extmod/modbluetooth.c @@ -1101,7 +1101,7 @@ STATIC mp_obj_t bluetooth_ble_invoke_irq(mp_obj_t none_in) { // conn_handle, start_handle, end_handle, uuid ringbuf_extract(&o->ringbuf, data_tuple, 3, 0, NULL, 0, &o->irq_data_uuid, NULL); } else if (event == MP_BLUETOOTH_IRQ_GATTC_CHARACTERISTIC_RESULT) { - // conn_handle, def_handle, value_handle, properties, uuid + // conn_handle, end_handle, value_handle, properties, uuid ringbuf_extract(&o->ringbuf, data_tuple, 3, 1, NULL, 0, &o->irq_data_uuid, NULL); } else if (event == MP_BLUETOOTH_IRQ_GATTC_DESCRIPTOR_RESULT) { // conn_handle, handle, uuid @@ -1375,8 +1375,9 @@ void mp_bluetooth_gattc_on_primary_service_result(uint16_t conn_handle, uint16_t invoke_irq_handler(MP_BLUETOOTH_IRQ_GATTC_SERVICE_RESULT, args, 3, 0, NULL_ADDR, service_uuid, NULL_DATA, NULL_DATA_LEN, 0); } -void mp_bluetooth_gattc_on_characteristic_result(uint16_t conn_handle, uint16_t def_handle, uint16_t value_handle, uint8_t properties, mp_obj_bluetooth_uuid_t *characteristic_uuid) { - mp_int_t args[] = {conn_handle, def_handle, value_handle, properties}; +void mp_bluetooth_gattc_on_characteristic_result(uint16_t conn_handle, uint16_t value_handle, uint16_t end_handle, uint8_t properties, mp_obj_bluetooth_uuid_t *characteristic_uuid) { + // Note: "end_handle" replaces "def_handle" from the original version of this event. + mp_int_t args[] = {conn_handle, end_handle, value_handle, properties}; invoke_irq_handler(MP_BLUETOOTH_IRQ_GATTC_CHARACTERISTIC_RESULT, args, 4, 0, NULL_ADDR, characteristic_uuid, NULL_DATA, NULL_DATA_LEN, 0); } @@ -1588,12 +1589,13 @@ void mp_bluetooth_gattc_on_primary_service_result(uint16_t conn_handle, uint16_t schedule_ringbuf(atomic_state); } -void mp_bluetooth_gattc_on_characteristic_result(uint16_t conn_handle, uint16_t def_handle, uint16_t value_handle, uint8_t properties, mp_obj_bluetooth_uuid_t *characteristic_uuid) { +void mp_bluetooth_gattc_on_characteristic_result(uint16_t conn_handle, uint16_t value_handle, uint16_t end_handle, uint8_t properties, mp_obj_bluetooth_uuid_t *characteristic_uuid) { MICROPY_PY_BLUETOOTH_ENTER mp_obj_bluetooth_ble_t *o = MP_OBJ_TO_PTR(MP_STATE_VM(bluetooth)); if (enqueue_irq(o, 2 + 2 + 2 + 1 + characteristic_uuid->type, MP_BLUETOOTH_IRQ_GATTC_CHARACTERISTIC_RESULT)) { ringbuf_put16(&o->ringbuf, conn_handle); - ringbuf_put16(&o->ringbuf, def_handle); + // Note: "end_handle" replaces "def_handle" from the original version of this event. + ringbuf_put16(&o->ringbuf, end_handle); ringbuf_put16(&o->ringbuf, value_handle); ringbuf_put(&o->ringbuf, properties); ringbuf_put_uuid(&o->ringbuf, characteristic_uuid); diff --git a/extmod/modbluetooth.h b/extmod/modbluetooth.h index 52053045f8..d490346278 100644 --- a/extmod/modbluetooth.h +++ b/extmod/modbluetooth.h @@ -459,7 +459,7 @@ void mp_bluetooth_gap_on_scan_result(uint8_t addr_type, const uint8_t *addr, uin void mp_bluetooth_gattc_on_primary_service_result(uint16_t conn_handle, uint16_t start_handle, uint16_t end_handle, mp_obj_bluetooth_uuid_t *service_uuid); // Notify modbluetooth that a characteristic was found (either by discover-all-on-service, or discover-by-uuid-on-service). -void mp_bluetooth_gattc_on_characteristic_result(uint16_t conn_handle, uint16_t def_handle, uint16_t value_handle, uint8_t properties, mp_obj_bluetooth_uuid_t *characteristic_uuid); +void mp_bluetooth_gattc_on_characteristic_result(uint16_t conn_handle, uint16_t value_handle, uint16_t end_handle, uint8_t properties, mp_obj_bluetooth_uuid_t *characteristic_uuid); // Notify modbluetooth that a descriptor was found. void mp_bluetooth_gattc_on_descriptor_result(uint16_t conn_handle, uint16_t handle, mp_obj_bluetooth_uuid_t *descriptor_uuid); diff --git a/extmod/nimble/modbluetooth_nimble.c b/extmod/nimble/modbluetooth_nimble.c index c26c09e61a..b0194446bd 100644 --- a/extmod/nimble/modbluetooth_nimble.c +++ b/extmod/nimble/modbluetooth_nimble.c @@ -1310,15 +1310,51 @@ int mp_bluetooth_gattc_discover_primary_services(uint16_t conn_handle, const mp_ return ble_hs_err_to_errno(err); } +STATIC bool match_char_uuid(const mp_obj_bluetooth_uuid_t *filter_uuid, const ble_uuid_any_t *result_uuid) { + if (!filter_uuid) { + return true; + } + ble_uuid_any_t filter_uuid_nimble; + create_nimble_uuid(filter_uuid, &filter_uuid_nimble); + return ble_uuid_cmp(&result_uuid->u, &filter_uuid_nimble.u) == 0; +} + STATIC int ble_gattc_characteristic_cb(uint16_t conn_handle, const struct ble_gatt_error *error, const struct ble_gatt_chr *characteristic, void *arg) { DEBUG_printf("ble_gattc_characteristic_cb: conn_handle=%d status=%d def_handle=%d val_handle=%d\n", conn_handle, error->status, characteristic ? characteristic->def_handle : -1, characteristic ? characteristic->val_handle : -1); if (!mp_bluetooth_is_active()) { return 0; } + + mp_bluetooth_nimble_pending_characteristic_t *pending = &MP_STATE_PORT(bluetooth_nimble_root_pointers)->pending_char_result; + if (pending->ready) { + // If there's a pending characteristic, we now know what it's end handle is, report it up to modbluetooth. + pending->ready = 0; + + // The end handle will either be the end of the query range (there are + // no more results), or one before the current result's definition + // handle. + uint16_t end_handle = MP_STATE_PORT(bluetooth_nimble_root_pointers)->char_disc_end_handle; + if (error->status == 0) { + end_handle = characteristic->def_handle - 1; + } + + // Assume same conn_handle because we're limiting to a single active discovery. + mp_bluetooth_gattc_on_characteristic_result(conn_handle, pending->value_handle, end_handle, pending->properties, &pending->uuid); + } + if (error->status == 0) { - mp_obj_bluetooth_uuid_t characteristic_uuid = create_mp_uuid(&characteristic->uuid); - mp_bluetooth_gattc_on_characteristic_result(conn_handle, characteristic->def_handle, characteristic->val_handle, characteristic->properties, &characteristic_uuid); + // If there's no filter, or the filter matches, then save this result. + if (match_char_uuid(MP_STATE_PORT(bluetooth_nimble_root_pointers)->char_filter_uuid, &characteristic->uuid)) { + pending->value_handle = characteristic->val_handle; + pending->properties = characteristic->properties; + pending->uuid = create_mp_uuid(&characteristic->uuid); + pending->ready = 1; + } } else { + // Finished (or failed). Allow another characteristic discovery to start. + MP_STATE_PORT(bluetooth_nimble_root_pointers)->char_disc_end_handle = 0; + + // Report completion. mp_bluetooth_gattc_on_discover_complete(MP_BLUETOOTH_IRQ_GATTC_CHARACTERISTIC_DONE, conn_handle, error->status == BLE_HS_EDONE ? 0 : error->status); } return 0; @@ -1328,13 +1364,29 @@ int mp_bluetooth_gattc_discover_characteristics(uint16_t conn_handle, uint16_t s if (!mp_bluetooth_is_active()) { return ERRNO_BLUETOOTH_NOT_ACTIVE; } - int err; - if (uuid) { - ble_uuid_any_t nimble_uuid; - create_nimble_uuid(uuid, &nimble_uuid); - err = ble_gattc_disc_chrs_by_uuid(conn_handle, start_handle, end_handle, &nimble_uuid.u, &ble_gattc_characteristic_cb, NULL); - } else { - err = ble_gattc_disc_all_chrs(conn_handle, start_handle, end_handle, &ble_gattc_characteristic_cb, NULL); + + // The implementation of characteristic discovery queries for all + // characteristics, and then UUID filtering is applied by NimBLE on each + // characteristic. Unfortunately, each characteristic result does not + // include its end handle, so you need to know the next characteristic + // before you can raise the previous one to modbluetooth. But if we let + // NimBLE do the filtering, then we don't necessarily see the next one. + // So we make NimBLE return all results and do the filtering here instead. + + if (MP_STATE_PORT(bluetooth_nimble_root_pointers)->char_disc_end_handle) { + // Only allow a single discovery (otherwise we'd need to track a + // pending characteristic per conn handle). + return MP_EBUSY; + } + + // Set the uuid filter (if any). This needs to be a root pointer, + // otherwise we'd use ble_gattc_disc_all_chrs's arg param. + MP_STATE_PORT(bluetooth_nimble_root_pointers)->char_filter_uuid = uuid; + + int err = ble_gattc_disc_all_chrs(conn_handle, start_handle, end_handle, &ble_gattc_characteristic_cb, NULL); + if (!err) { + // Lock out concurrent characteristic discovery. + MP_STATE_PORT(bluetooth_nimble_root_pointers)->char_disc_end_handle = end_handle; } return ble_hs_err_to_errno(err); } diff --git a/extmod/nimble/modbluetooth_nimble.h b/extmod/nimble/modbluetooth_nimble.h index 15648a9959..d9bef64920 100644 --- a/extmod/nimble/modbluetooth_nimble.h +++ b/extmod/nimble/modbluetooth_nimble.h @@ -31,6 +31,13 @@ #define MP_BLUETOOTH_NIMBLE_MAX_SERVICES (8) +typedef struct _mp_bluetooth_nimble_pending_characteristic_t { + uint16_t value_handle; + uint8_t properties; + mp_obj_bluetooth_uuid_t uuid; + uint8_t ready; +} mp_bluetooth_nimble_pending_characteristic_t; + typedef struct _mp_bluetooth_nimble_root_pointers_t { // Characteristic (and descriptor) value storage. mp_gatts_db_t gatts_db; @@ -44,6 +51,14 @@ typedef struct _mp_bluetooth_nimble_root_pointers_t { struct _mp_bluetooth_nimble_l2cap_channel_t *l2cap_chan; bool l2cap_listening; #endif + + #if MICROPY_PY_BLUETOOTH_ENABLE_GATT_CLIENT + // Workaround to allow us to get the end_handle of each characteristic + // during discovery. See mp_bluetooth_gattc_discover_characteristics(). + uint16_t char_disc_end_handle; + const mp_obj_bluetooth_uuid_t *char_filter_uuid; + mp_bluetooth_nimble_pending_characteristic_t pending_char_result; + #endif } mp_bluetooth_nimble_root_pointers_t; enum {