Return-Path: From: Arman Uguray To: linux-bluetooth@vger.kernel.org Cc: luiz.dentz@gmail.com, Arman Uguray Subject: [PATCH BlueZ v3 1/5] shared/gatt: Make register_notify cancellable Date: Fri, 30 Jan 2015 16:32:34 -0800 Message-Id: <1422664358-17061-2-git-send-email-armansito@chromium.org> In-Reply-To: <1422664358-17061-1-git-send-email-armansito@chromium.org> References: <1422664358-17061-1-git-send-email-armansito@chromium.org> Sender: linux-bluetooth-owner@vger.kernel.org List-ID: This patch makes CCC writes via bt_gatt_client_register_notify cancellable. The following changes have been introduced: 1. bt_gatt_client_register_notify now returns the id immediately instead of returning it in a callback. The callback is still used to communicate ATT protocol errors. 2. A notify callback is immediately registered, so that if the remote end sends any ATT notifications/indications, the caller will start receiving them right away. --- src/shared/gatt-client.c | 129 ++++++++++++++++++++++++++++------------------- src/shared/gatt-client.h | 7 ++- tools/btgatt-client.c | 17 ++++--- 3 files changed, 91 insertions(+), 62 deletions(-) diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c index 04fb4cb..cbc5382 100644 --- a/src/shared/gatt-client.c +++ b/src/shared/gatt-client.c @@ -168,9 +168,10 @@ struct notify_data { struct bt_gatt_client *client; bool invalid; unsigned int id; + unsigned int att_id; int ref_count; struct notify_chrc *chrc; - bt_gatt_client_notify_id_callback_t callback; + bt_gatt_client_register_callback_t callback; bt_gatt_client_notify_callback_t notify; void *user_data; bt_gatt_client_destroy_func_t destroy; @@ -1011,22 +1012,20 @@ struct service_changed_op { uint16_t end_handle; }; -static void service_changed_reregister_cb(unsigned int id, uint16_t att_ecode, - void *user_data) +static void service_changed_reregister_cb(uint16_t att_ecode, void *user_data) { struct bt_gatt_client *client = user_data; - if (!id || att_ecode) { + if (!att_ecode) { util_debug(client->debug_callback, client->debug_data, - "Failed to register handler for \"Service Changed\""); + "Re-registered handler for \"Service Changed\" after " + "change in GATT service"); return; } - client->svc_chngd_ind_id = id; - util_debug(client->debug_callback, client->debug_data, - "Re-registered handler for \"Service Changed\" after change in " - "GATT service"); + "Failed to register handler for \"Service Changed\""); + client->svc_chngd_ind_id = 0; } static void process_service_changed(struct bt_gatt_client *client, @@ -1090,11 +1089,12 @@ static void service_changed_complete(struct discovery_op *op, bool success, /* The GATT service was modified. Re-register the handler for * indications from the "Service Changed" characteristic. */ - if (bt_gatt_client_register_notify(client, + client->svc_chngd_ind_id = bt_gatt_client_register_notify(client, gatt_db_attribute_get_handle(attr), service_changed_reregister_cb, service_changed_cb, - client, NULL)) + client, NULL); + if (client->svc_chngd_ind_id) return; util_debug(client->debug_callback, client->debug_data, @@ -1184,24 +1184,24 @@ static void service_changed_cb(uint16_t value_handle, const uint8_t *value, queue_push_tail(client->svc_chngd_queue, op); } -static void service_changed_register_cb(unsigned int id, uint16_t att_ecode, - void *user_data) +static void service_changed_register_cb(uint16_t att_ecode, void *user_data) { bool success; struct bt_gatt_client *client = user_data; - if (!id || att_ecode) { + if (att_ecode) { util_debug(client->debug_callback, client->debug_data, "Failed to register handler for \"Service Changed\""); success = false; + client->svc_chngd_ind_id = 0; goto done; } - client->svc_chngd_ind_id = id; client->ready = true; success = true; util_debug(client->debug_callback, client->debug_data, - "Registered handler for \"Service Changed\": %u", id); + "Registered handler for \"Service Changed\": %u", + client->svc_chngd_ind_id); done: notify_client_ready(client, success, att_ecode); @@ -1211,7 +1211,6 @@ static void init_complete(struct discovery_op *op, bool success, uint8_t att_ecode) { struct bt_gatt_client *client = op->client; - bool registered; struct gatt_db_attribute *attr = NULL; bt_uuid_t uuid; @@ -1235,14 +1234,14 @@ static void init_complete(struct discovery_op *op, bool success, * the handler using the existing framework. */ client->ready = true; - registered = bt_gatt_client_register_notify(client, + client->svc_chngd_ind_id = bt_gatt_client_register_notify(client, gatt_db_attribute_get_handle(attr), service_changed_register_cb, service_changed_cb, client, NULL); client->ready = false; - if (registered) + if (client->svc_chngd_ind_id) return; util_debug(client->debug_callback, client->debug_data, @@ -1301,24 +1300,15 @@ static void complete_notify_request(void *data) /* Increment the per-characteristic ref count of notify handlers */ __sync_fetch_and_add(¬ify_data->chrc->notify_count, 1); - /* Add the handler to the bt_gatt_client's general list */ - queue_push_tail(notify_data->client->notify_list, - notify_data_ref(notify_data)); - - /* Assign an ID to the handler and notify the caller that it was - * successfully registered. - */ - if (notify_data->client->next_reg_id < 1) - notify_data->client->next_reg_id = 1; - - notify_data->id = notify_data->client->next_reg_id++; - notify_data->callback(notify_data->id, 0, notify_data->user_data); + notify_data->att_id = 0; + notify_data->callback(0, notify_data->user_data); } static bool notify_data_write_ccc(struct notify_data *notify_data, bool enable, bt_att_response_func_t callback) { uint8_t pdu[4]; + unsigned int att_id; assert(notify_data->chrc->ccc_handle); memset(pdu, 0, sizeof(pdu)); @@ -1338,13 +1328,13 @@ static bool notify_data_write_ccc(struct notify_data *notify_data, bool enable, return false; } - notify_data->chrc->ccc_write_id = bt_att_send(notify_data->client->att, - BT_ATT_OP_WRITE_REQ, - pdu, sizeof(pdu), - callback, - notify_data, notify_data_unref); + att_id = bt_att_send(notify_data->client->att, BT_ATT_OP_WRITE_REQ, + pdu, sizeof(pdu), callback, + notify_data_ref(notify_data), + notify_data_unref); + notify_data->chrc->ccc_write_id = notify_data->att_id = att_id; - return !!notify_data->chrc->ccc_write_id; + return !!att_id; } static uint8_t process_error(const void *pdu, uint16_t length) @@ -1377,7 +1367,8 @@ static void enable_ccc_callback(uint8_t opcode, const void *pdu, * the next one in the queue. If there was an error sending the * write request, then just move on to the next queued entry. */ - notify_data->callback(0, att_ecode, notify_data->user_data); + queue_remove(notify_data->client->notify_list, notify_data); + notify_data->callback(att_ecode, notify_data->user_data); while ((notify_data = queue_pop_head( notify_data->chrc->reg_notify_queue))) { @@ -1426,6 +1417,16 @@ static void complete_unregister_notify(void *data) { struct notify_data *notify_data = data; + /* + * If a procedure to enable the CCC is still pending, then cancel it and + * return. + */ + if (notify_data->att_id) { + bt_att_cancel(notify_data->client->att, notify_data->att_id); + notify_data_unref(notify_data); + return; + } + if (__sync_sub_and_fetch(¬ify_data->chrc->notify_count, 1)) { notify_data_unref(notify_data); return; @@ -1449,6 +1450,10 @@ static void notify_handler(void *data, void *user_data) if (pdu_data->length > 2) value = pdu_data->pdu + 2; + /* + * Even if the notify data has a pending ATT request to write to the + * CCC, there is really no reason not to notify the handlers. + */ if (notify_data->notify) notify_data->notify(value_handle, value, pdu_data->length - 2, notify_data->user_data); @@ -1475,10 +1480,22 @@ static void notify_cb(uint8_t opcode, const void *pdu, uint16_t length, bt_gatt_client_unref(client); } +static void notify_data_cleanup(void *data) +{ + struct notify_data *notify_data = data; + + if (notify_data->att_id) + bt_att_cancel(notify_data->client->att, notify_data->att_id); + + notify_data_unref(notify_data); +} + static void bt_gatt_client_free(struct bt_gatt_client *client) { bt_gatt_client_cancel_all(client); + queue_destroy(client->notify_list, notify_data_cleanup); + if (client->ready_destroy) client->ready_destroy(client->ready_data); @@ -1496,7 +1513,6 @@ static void bt_gatt_client_free(struct bt_gatt_client *client) queue_destroy(client->svc_chngd_queue, free); queue_destroy(client->long_write_queue, request_unref); - queue_destroy(client->notify_list, notify_data_unref); queue_destroy(client->notify_chrcs, notify_chrc_free); queue_destroy(client->pending_requests, request_unref); @@ -2569,9 +2585,9 @@ static bool match_notify_chrc_value_handle(const void *a, const void *b) return chrc->value_handle == value_handle; } -bool bt_gatt_client_register_notify(struct bt_gatt_client *client, +unsigned int bt_gatt_client_register_notify(struct bt_gatt_client *client, uint16_t chrc_value_handle, - bt_gatt_client_notify_id_callback_t callback, + bt_gatt_client_register_callback_t callback, bt_gatt_client_notify_callback_t notify, void *user_data, bt_gatt_client_destroy_func_t destroy) @@ -2580,10 +2596,10 @@ bool bt_gatt_client_register_notify(struct bt_gatt_client *client, struct notify_chrc *chrc = NULL; if (!client || !client->db || !chrc_value_handle || !callback) - return false; + return 0; if (!bt_gatt_client_is_ready(client) || client->in_svc_chngd) - return false; + return 0; /* Check if a characteristic ref count has been started already */ chrc = queue_find(client->notify_chrcs, match_notify_chrc_value_handle, @@ -2596,17 +2612,16 @@ bool bt_gatt_client_register_notify(struct bt_gatt_client *client, */ chrc = notify_chrc_create(client, chrc_value_handle); if (!chrc) - return false; - + return 0; } /* Fail if we've hit the maximum allowed notify sessions */ if (chrc->notify_count == INT_MAX) - return false; + return 0; notify_data = new0(struct notify_data, 1); if (!notify_data) - return false; + return 0; notify_data->client = client; notify_data->ref_count = 1; @@ -2616,13 +2631,24 @@ bool bt_gatt_client_register_notify(struct bt_gatt_client *client, notify_data->user_data = user_data; notify_data->destroy = destroy; + /* Add the handler to the bt_gatt_client's general list */ + queue_push_tail(client->notify_list, notify_data); + + /* Assign an ID to the handler and notify the caller that it was + * successfully registered. + */ + if (client->next_reg_id < 1) + client->next_reg_id = 1; + + notify_data->id = client->next_reg_id++; + /* * If a write to the CCC descriptor is in progress, then queue this * request. */ if (chrc->ccc_write_id) { queue_push_tail(chrc->reg_notify_queue, notify_data); - return true; + return notify_data->id; } /* @@ -2630,16 +2656,17 @@ bool bt_gatt_client_register_notify(struct bt_gatt_client *client, */ if (chrc->notify_count > 0) { complete_notify_request(notify_data); - return true; + return notify_data->id; } /* Write to the CCC descriptor */ if (!notify_data_write_ccc(notify_data, true, enable_ccc_callback)) { + queue_remove(client->notify_list, notify_data); free(notify_data); - return false; + return 0; } - return true; + return notify_data->id; } bool bt_gatt_client_unregister_notify(struct bt_gatt_client *client, diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h index 9a00feb..b84fa13 100644 --- a/src/shared/gatt-client.h +++ b/src/shared/gatt-client.h @@ -49,8 +49,7 @@ typedef void (*bt_gatt_client_write_long_callback_t)(bool success, typedef void (*bt_gatt_client_notify_callback_t)(uint16_t value_handle, const uint8_t *value, uint16_t length, void *user_data); -typedef void (*bt_gatt_client_notify_id_callback_t)(unsigned int id, - uint16_t att_ecode, +typedef void (*bt_gatt_client_register_callback_t)(uint16_t att_ecode, void *user_data); typedef void (*bt_gatt_client_service_changed_callback_t)(uint16_t start_handle, uint16_t end_handle, @@ -110,9 +109,9 @@ unsigned int bt_gatt_client_write_long_value(struct bt_gatt_client *client, void *user_data, bt_gatt_client_destroy_func_t destroy); -bool bt_gatt_client_register_notify(struct bt_gatt_client *client, +unsigned int bt_gatt_client_register_notify(struct bt_gatt_client *client, uint16_t chrc_value_handle, - bt_gatt_client_notify_id_callback_t callback, + bt_gatt_client_register_callback_t callback, bt_gatt_client_notify_callback_t notify, void *user_data, bt_gatt_client_destroy_func_t destroy); diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c index 62c4d3e..8bda89b 100644 --- a/tools/btgatt-client.c +++ b/tools/btgatt-client.c @@ -856,16 +856,15 @@ static void notify_cb(uint16_t value_handle, const uint8_t *value, PRLOG("\n"); } -static void register_notify_cb(unsigned int id, uint16_t att_ecode, - void *user_data) +static void register_notify_cb(uint16_t att_ecode, void *user_data) { - if (!id) { + if (att_ecode) { PRLOG("Failed to register notify handler " "- error code: 0x%02x\n", att_ecode); return; } - PRLOG("Registered notify handler with id: %u\n", id); + PRLOG("Registered notify handler!"); } static void cmd_register_notify(struct client *cli, char *cmd_str) @@ -873,6 +872,7 @@ static void cmd_register_notify(struct client *cli, char *cmd_str) char *argv[2]; int argc = 0; uint16_t value_handle; + unsigned int id; char *endptr = NULL; if (!bt_gatt_client_is_ready(cli->gatt)) { @@ -891,12 +891,15 @@ static void cmd_register_notify(struct client *cli, char *cmd_str) return; } - if (!bt_gatt_client_register_notify(cli->gatt, value_handle, + id = bt_gatt_client_register_notify(cli->gatt, value_handle, register_notify_cb, - notify_cb, NULL, NULL)) + notify_cb, NULL, NULL); + if (!id) { printf("Failed to register notify handler\n"); + return; + } - printf("\n"); + PRLOG("Registering notify handler with id: %u\n", id); } static void unregister_notify_usage(void) -- 2.2.0.rc0.207.ga3a616c