Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1415878033-6766-1-git-send-email-marcin.kraglak@tieto.com> <1415878033-6766-2-git-send-email-marcin.kraglak@tieto.com> Date: Fri, 14 Nov 2014 15:51:04 +0200 Message-ID: Subject: Re: [RFC 01/11] shared/gatt-client: Add read-by-uuid function From: Luiz Augusto von Dentz To: Marcin Kraglak Cc: "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Marcin, On Fri, Nov 14, 2014 at 3:13 PM, Marcin Kraglak wrote: > Hi Luiz, > > On 14 November 2014 13:44, Luiz Augusto von Dentz wrote: >> Hi Marcin, >> >> On Thu, Nov 13, 2014 at 1:27 PM, Marcin Kraglak >> wrote: >>> This is exposed to allow client reading characteristic values >>> with known uuid. >>> --- >>> src/shared/gatt-client.c | 271 +++++++++++++++++++++++++++++++++++++++++++++++ >>> src/shared/gatt-client.h | 18 ++++ >>> 2 files changed, 289 insertions(+) >>> >>> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c >>> index 30b271e..0493b05 100644 >>> --- a/src/shared/gatt-client.c >>> +++ b/src/shared/gatt-client.c >>> @@ -44,6 +44,11 @@ >>> #define GATT_SVC_UUID 0x1801 >>> #define SVC_CHNGD_UUID 0x2a05 >>> >>> +static const uint8_t bt_base_uuid[16] = { >>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, >>> + 0x80, 0x00, 0x00, 0x80, 0x5F, 0x9B, 0x34, 0xFB >>> +}; >>> + >>> struct chrc_data { >>> /* The public characteristic entry. */ >>> bt_gatt_characteristic_t chrc_external; >>> @@ -122,6 +127,15 @@ struct bt_gatt_client { >>> bool in_svc_chngd; >>> }; >>> >>> +static bool is_bluetooth_uuid(const uint8_t uuid[BT_GATT_UUID_SIZE]) >>> +{ >>> + if (memcmp(uuid, bt_base_uuid, 2) || >>> + memcmp(&uuid[4], &bt_base_uuid[4], 12)) >>> + return false; >>> + >>> + return true; >>> +} >>> + >> >> Is this really different from uuid_cmp? Perhaps you use set uuid16 >> properly it should just do the same you are doing above. > Ok, I'll use it. >> >>> struct notify_data { >>> struct bt_gatt_client *client; >>> bool removed; >>> @@ -2001,6 +2015,263 @@ bool bt_gatt_client_read_long_value(struct bt_gatt_client *client, >>> return true; >>> } >>> >>> +struct read_by_uuid_res { >>> + uint16_t handle; >>> + uint8_t length; >>> + uint8_t *data; >>> + struct read_by_uuid_res *next; >>> +}; >>> + >>> +bool read_by_uuid_res_get(struct read_by_uuid_res *result, uint16_t *handle, >>> + uint8_t *length, uint8_t **data) >>> +{ >>> + if (!result) >>> + return false; >>> + >>> + *length = result->length; >>> + *handle = result->handle; >>> + *data = result->data; >>> + >>> + return true; >>> +} >>> + >>> +struct read_by_uuid_res *read_by_uuid_res_next(struct read_by_uuid_res *result) >>> +{ >>> + if (!result) >>> + return NULL; >>> + >>> + return result->next; >>> +} >>> + >>> +struct read_by_uuid_op { >>> + struct bt_gatt_client *client; >>> + bt_gatt_client_read_by_uuid_callback_t callback; >>> + int ref_count; >>> + uint16_t start_handle; >>> + uint16_t end_handle; >>> + uint8_t uuid[BT_GATT_UUID_SIZE]; >>> + struct read_by_uuid_res *result_head; >>> + struct read_by_uuid_res *result_tail; >>> + void *user_data; >>> + bt_gatt_client_destroy_func_t destroy; >>> +}; >> >> Im not really sure this lists are really helping us, perhaps this is >> the effect of using iterators in the API but that seems to create >> duplicated code. >> >>> +static bool append_read_by_uuid_result(struct read_by_uuid_op *op, >>> + uint16_t handle, uint16_t length, >>> + const uint8_t *data) >>> +{ >>> + struct read_by_uuid_res *result; >>> + >>> + result = new0(struct read_by_uuid_res, 1); >>> + if (!result) >>> + return false; >>> + >>> + result->data = malloc(length); >>> + if (!result->data) { >>> + free(result); >>> + return false; >>> + } >>> + >>> + result->handle = handle; >>> + result->length = length; >>> + memcpy(result->data, data, length); >>> + >>> + if (op->result_head) { >>> + op->result_tail->next = result; >>> + op->result_tail = result; >>> + } else { >>> + op->result_head = op->result_tail = result; >>> + } >>> + >>> + return true; >>> +} >>> + >>> +static void destroy_read_by_uuid_result(void *data) >>> +{ >>> + struct read_by_uuid_res *result = data; >>> + >>> + free(result->data); >>> + free(result); >>> +} >>> + >>> +static struct read_by_uuid_op *read_by_uuid_op_ref(struct read_by_uuid_op *op) >>> +{ >>> + __sync_fetch_and_add(&op->ref_count, 1); >>> + >>> + return op; >>> +} >>> + >>> +static void read_by_uuid_op_unref(void *data) >>> +{ >>> + struct read_by_uuid_res *next, *temp; >>> + struct read_by_uuid_op *op = data; >>> + >>> + if (__sync_sub_and_fetch(&op->ref_count, 1)) >>> + return; >>> + >>> + if (op->destroy) >>> + op->destroy(op->user_data); >>> + >>> + for (temp = op->result_head; temp; temp = next) { >>> + next = temp->next; >>> + destroy_read_by_uuid_result(temp); >>> + } >>> + >>> + free(op); >>> +} >>> + >>> +static void read_by_uuid_cb(uint8_t opcode, const void *pdu, >>> + uint16_t length, void *user_data) >>> +{ >>> + struct read_by_uuid_op *op = user_data; >>> + size_t data_length; >>> + uint8_t att_ecode; >>> + uint16_t offset, last_handle; >>> + const uint8_t *data; >>> + bool success; >>> + >>> + if (opcode == BT_ATT_OP_ERROR_RSP) { >>> + att_ecode = process_error(pdu, length); >>> + if (att_ecode == BT_ATT_ERROR_ATTRIBUTE_NOT_FOUND) >>> + success = true; >>> + else >>> + success = false; >>> + >>> + goto done; >>> + } >>> + >>> + if (opcode != BT_ATT_OP_READ_BY_TYPE_RSP || (!pdu && length)) { >>> + success = false; >>> + att_ecode = 0; >>> + goto done; >>> + } >>> + >>> + data_length = ((const uint8_t *) pdu)[0]; >>> + if ((length - 1) % data_length) { >>> + success = false; >>> + att_ecode = 0; >>> + goto done; >>> + } >>> + >>> + for (offset = 1; offset < length; offset += data_length) { >>> + data = pdu + offset; >>> + if (!append_read_by_uuid_result(op, get_le16(data), >>> + data_length - 2, data + 2)) { >>> + success = false; >>> + att_ecode = 0; >>> + goto done; >>> + } >>> + } >>> + >>> + last_handle = get_le16(pdu + length - data_length); >>> + if (last_handle < op->end_handle) { >>> + uint8_t pdu[20]; >>> + >>> + put_le16(last_handle + 1, pdu); >>> + put_le16(op->end_handle, pdu + 2); >>> + >>> + if (is_bluetooth_uuid(op->uuid)) { >>> + put_le16((op->uuid[2] << 8) + op->uuid[3], pdu + 4); >>> + /* >>> + * 2 octets - start handle >>> + * 2 octets - end handle >>> + * 2 octets - 16-bit uuid >>> + */ >>> + length = 6; >>> + } else { >>> + bswap_128(&op->uuid, pdu + 4); >>> + /* >>> + * 2 octets - start handle >>> + * 2 octets - end handle >>> + * 16 octets - 128-bit uuid >>> + */ >>> + length = 20; >>> + } >>> + >>> + if (!bt_att_send(op->client->att, BT_ATT_OP_READ_BY_TYPE_REQ, >>> + pdu, length, read_by_uuid_cb, >>> + read_by_uuid_op_ref(op), >>> + read_by_uuid_op_unref)) { >>> + read_by_uuid_op_unref(op); >>> + success = false; >>> + att_ecode = 0; >>> + goto done; >>> + } >>> + >>> + return; >>> + } >>> + >>> + success = true; >>> + att_ecode = 0; >>> + >>> +done: >>> + if (op->callback) >>> + op->callback(success, att_ecode, op->result_head, >>> + op->user_data); >>> +} >>> + >>> +bool bt_gatt_client_read_by_uuid(struct bt_gatt_client *client, >>> + uint16_t start_handle, uint16_t end_handle, >>> + const uint8_t uuid[BT_GATT_UUID_SIZE], >>> + bt_gatt_client_read_by_uuid_callback_t callback, >>> + void *user_data, >>> + bt_gatt_client_destroy_func_t destroy) >>> +{ >>> + struct read_by_uuid_op *op; >>> + uint16_t length; >>> + uint8_t pdu[20]; >>> + >>> + if (!client) >>> + return false; >>> + >>> + if (start_handle > end_handle || start_handle == 0x0000) >>> + return false; >>> + >>> + op = new0(struct read_by_uuid_op, 1); >>> + if (!op) >>> + return false; >>> + >>> + op->client = client; >>> + op->start_handle = start_handle; >>> + op->end_handle = end_handle; >>> + op->destroy = destroy; >>> + op->callback = callback; >>> + op->user_data = user_data; >>> + memcpy(&op->uuid, uuid, 16); >>> + >>> + /* Convert to 16-bit if it is Bluetooth UUID */ >>> + if (is_bluetooth_uuid(uuid)) { >>> + put_le16((uuid[2] << 8) + uuid[3], pdu + 4); >>> + /* >>> + * 2 octets - start handle >>> + * 2 octets - end handle >>> + * 2 octets - 16-bit uuid >>> + */ >>> + length = 6; >>> + } else { >>> + bswap_128(uuid, pdu + 4); >>> + /* >>> + * 2 octets - start handle >>> + * 2 octets - end handle >>> + * 16 octets - 128-bit uuid >>> + */ >>> + length = 20; >>> + } >>> + >>> + put_le16(start_handle, pdu); >>> + put_le16(end_handle, pdu + 2); >>> + >>> + if (!bt_att_send(client->att, BT_ATT_OP_READ_BY_TYPE_REQ, pdu, >>> + length, read_by_uuid_cb, >>> + read_by_uuid_op_ref(op), >>> + read_by_uuid_op_unref)) { >>> + free(op); >>> + return false; >>> + } >>> + >>> + return true; >>> +} >>> + >>> bool bt_gatt_client_write_without_response(struct bt_gatt_client *client, >>> uint16_t value_handle, >>> bool signed_write, >>> diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h >>> index adccfc5..921ed75 100644 >>> --- a/src/shared/gatt-client.h >>> +++ b/src/shared/gatt-client.h >>> @@ -145,6 +145,24 @@ bool bt_gatt_client_read_long_value(struct bt_gatt_client *client, >>> void *user_data, >>> bt_gatt_client_destroy_func_t destroy); >>> >>> +struct read_by_uuid_res; >>> + >>> +struct read_by_uuid_res *read_by_uuid_res_next(struct read_by_uuid_res *result); >>> +bool read_by_uuid_res_get(struct read_by_uuid_res *result, uint16_t *handle, >>> + uint8_t *length, uint8_t **data); >>> + >>> +typedef void (*bt_gatt_client_read_by_uuid_callback_t)(bool success, >>> + uint8_t att_ecode, >>> + struct read_by_uuid_res *result, >>> + void *user_data); >>> + >>> +bool bt_gatt_client_read_by_uuid(struct bt_gatt_client *client, >>> + uint16_t start_handle, uint16_t end_handle, >>> + const uint8_t uuid[BT_GATT_UUID_SIZE], >>> + bt_gatt_client_read_by_uuid_callback_t callback, >>> + void *user_data, >>> + bt_gatt_client_destroy_func_t destroy); >> >> Isn't this essentially duplicating bt_gatt_service_iter_next_by_uuid? >> I mean we should be reading everything already but this doesn't seems >> to using the cache and in case it is just to fulfill a test perhaps it >> is better to do it in gatt-helpers so we keep bt_gatt_client API >> clean. > > It is functionality of GATT client described in chapter 4.8.2 "Read > Using Characteristic UUID". > Difference is that client can read attributes by passing UUID, and > results can contain > different valu lengths. And client interprets results instead of gatt-helpers. > I implemented it here because all read/write operations on > characteristics and descriptors > are exposed in bt_gatt_client, and all discovery stuff is implemented > in gatt_helpers. But if > you don't agree I can change it. I see, it does actually end up reading, perhaps it is a good idea to keep it here then. In the other hand this API seems to be going in the same direction that gatt-db had with access by handles, perhaps we should do an API facelift to access by attribute such as we did with gatt-db, also the iterator could be replaced with queues which I guess is not that different in the end. >> >>> bool bt_gatt_client_write_without_response(struct bt_gatt_client *client, >>> uint16_t value_handle, >>> bool signed_write, >>> -- >>> 1.9.3 >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> >> -- >> Luiz Augusto von Dentz > > BR > Marcin -- Luiz Augusto von Dentz