[PATCH BlueZ v1 0/7] shared/gatt-client: Handle notifications.

[PATCH BlueZ v1 0/7] shared/gatt-client: Handle notifications.

[Arman Uguray]

*v1: Handled style comments:
   - Renamed some variables to make them more meaningful.
   - Moved functions around to avoid forward declarations as much as possible.
   - Added an internal bt_gatt_descriptor_t pointer to struct chrc_data while
     keeping the external const pointer in bt_gatt_descriptor_t, both of which
     point to the same array of descriptors.

Arman Uguray (7):
  shared/gatt-client: Introduce struct bt_gatt_characteristic_iter.
  shared/gatt-client: Implement bt_gatt_client_register_notify.
  shared/gatt-client: Implement bt_gatt_client_unregister_notify.
  shared/gatt-client: Handle incoming not/ind PDUs.
  tools/btgatt-client: Add the "register-notify" command.
  tools/btgatt-client: Add "unregister-notify" command.
  TODO: Reference counted notify functions implemented

 TODO                      |  10 -
 src/shared/gatt-client.c  | 564 +++++++++++++++++++++++++++++++++++++++++-----
 src/shared/gatt-client.h  |  57 ++++-
 src/shared/gatt-helpers.c |  67 ------
 src/shared/gatt-helpers.h |   9 -
 tools/btgatt-client.c     | 139 ++++++++++--
 6 files changed, 684 insertions(+), 162 deletions(-)

-- 
2.1.0.rc2.206.gedb03e5

[PATCH BlueZ v1 1/7] shared/gatt-client: Introduce struct bt_gatt_characteristic_iter.

[Arman Uguray]

shared/gatt-client currently defines a service iterator which returns service
data in a copy. The user then accesses the service's characteristics by manually
going through an array of bt_gatt_characteristic_t.

This patch changes this by restricting access to individual characteristic
entries via a new characteristic iterator. This is done so that gatt-client code
can internally store private data on each characteristic (e.g. reference count
for notification sessions) which shouldn't be exposed to external code.

The code also changes the service iterator functions to return a pointer to an
internally stored bt_gatt_service_t structure rather than returning its contents
in a copy.
---
 src/shared/gatt-client.c | 59 ++++++++++++++++++++++++++++++++++++------------
 src/shared/gatt-client.h | 32 ++++++++++++++++----------
 tools/btgatt-client.c    | 31 ++++++++++++++-----------
 3 files changed, 83 insertions(+), 39 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 1a157ec..fd866b2 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -40,6 +40,8 @@
 
 struct service_list {
 	bt_gatt_service_t service;
+	bt_gatt_characteristic_t *chrcs;
+	size_t num_chrcs;
 	struct service_list *next;
 };
 
@@ -93,14 +95,14 @@ static bool gatt_client_add_service(struct bt_gatt_client *client,
 	return true;
 }
 
-static void service_destroy_characteristics(bt_gatt_service_t *service)
+static void service_destroy_characteristics(struct service_list *service)
 {
 	unsigned int i;
 
 	for (i = 0; i < service->num_chrcs; i++)
 		free((bt_gatt_descriptor_t *) service->chrcs[i].descs);
 
-	free((bt_gatt_characteristic_t *) service->chrcs);
+	free(service->chrcs);
 }
 
 static void gatt_client_clear_services(struct bt_gatt_client *client)
@@ -110,7 +112,7 @@ static void gatt_client_clear_services(struct bt_gatt_client *client)
 	l = client->svc_head;
 
 	while (l) {
-		service_destroy_characteristics(&l->service);
+		service_destroy_characteristics(l);
 		tmp = l;
 		l = tmp->next;
 		free(tmp);
@@ -229,8 +231,7 @@ static void discover_descs_cb(bool success, uint8_t att_ecode,
 	op->cur_chrc->num_descs = desc_count;
 	op->cur_chrc->descs = descs;
 
-	for (i = op->cur_chrc_index + 1;
-				i < op->cur_service->service.num_chrcs; i++) {
+	for (i = op->cur_chrc_index + 1; i < op->cur_service->num_chrcs; i++) {
 		op->cur_chrc_index = i;
 		op->cur_chrc++;
 		desc_start = op->cur_chrc->value_handle + 1;
@@ -332,8 +333,8 @@ static void discover_chrcs_cb(bool success, uint8_t att_ecode,
 		i++;
 	}
 
-	op->cur_service->service.chrcs = chrcs;
-	op->cur_service->service.num_chrcs = chrc_count;
+	op->cur_service->chrcs = chrcs;
+	op->cur_service->num_chrcs = chrc_count;
 
 	for (i = 0; i < chrc_count; i++) {
 		op->cur_chrc_index = i;
@@ -627,7 +628,7 @@ bool bt_gatt_service_iter_init(struct bt_gatt_service_iter *iter,
 }
 
 bool bt_gatt_service_iter_next(struct bt_gatt_service_iter *iter,
-						bt_gatt_service_t *service)
+					const bt_gatt_service_t **service)
 {
 	struct service_list *l;
 
@@ -644,18 +645,18 @@ bool bt_gatt_service_iter_next(struct bt_gatt_service_iter *iter,
 	if (!l)
 		return false;
 
-	*service = l->service;
+	*service = &l->service;
 	iter->ptr = l;
 
 	return true;
 }
 
 bool bt_gatt_service_iter_next_by_handle(struct bt_gatt_service_iter *iter,
-						uint16_t start_handle,
-						bt_gatt_service_t *service)
+					uint16_t start_handle,
+					const bt_gatt_service_t **service)
 {
 	while (bt_gatt_service_iter_next(iter, service)) {
-		if (service->start_handle == start_handle)
+		if ((*service)->start_handle == start_handle)
 			return true;
 	}
 
@@ -664,16 +665,46 @@ bool bt_gatt_service_iter_next_by_handle(struct bt_gatt_service_iter *iter,
 
 bool bt_gatt_service_iter_next_by_uuid(struct bt_gatt_service_iter *iter,
 					const uint8_t uuid[BT_GATT_UUID_SIZE],
-					bt_gatt_service_t *service)
+					const bt_gatt_service_t **service)
 {
 	while (bt_gatt_service_iter_next(iter, service)) {
-		if (memcmp(service->uuid, uuid, UUID_BYTES) == 0)
+		if (memcmp((*service)->uuid, uuid, UUID_BYTES) == 0)
 			return true;
 	}
 
 	return false;
 }
 
+bool bt_gatt_characteristic_iter_init(struct bt_gatt_characteristic_iter *iter,
+					const bt_gatt_service_t *service)
+{
+	if (!iter || !service)
+		return false;
+
+	memset(iter, 0, sizeof(*iter));
+	iter->service = (struct service_list *) service;
+
+	return true;
+}
+
+bool bt_gatt_characteristic_iter_next(struct bt_gatt_characteristic_iter *iter,
+					const bt_gatt_characteristic_t **chrc)
+{
+	struct service_list *service;
+
+	if (!iter || !chrc)
+		return false;
+
+	service = iter->service;
+
+	if (iter->pos >= service->num_chrcs)
+		return false;
+
+	*chrc = service->chrcs + iter->pos++;
+
+	return true;
+}
+
 struct read_op {
 	bt_gatt_client_read_callback_t callback;
 	void *user_data;
diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
index 8b0d334..417d964 100644
--- a/src/shared/gatt-client.h
+++ b/src/shared/gatt-client.h
@@ -53,6 +53,12 @@ bool bt_gatt_client_set_debug(struct bt_gatt_client *client,
 					bt_gatt_client_destroy_func_t destroy);
 
 typedef struct {
+	uint16_t start_handle;
+	uint16_t end_handle;
+	uint8_t uuid[BT_GATT_UUID_SIZE];
+} bt_gatt_service_t;
+
+typedef struct {
 	uint16_t handle;
 	uint8_t uuid[BT_GATT_UUID_SIZE];
 } bt_gatt_descriptor_t;
@@ -67,29 +73,31 @@ typedef struct {
 	size_t num_descs;
 } bt_gatt_characteristic_t;
 
-typedef struct {
-	uint16_t start_handle;
-	uint16_t end_handle;
-	uint8_t uuid[BT_GATT_UUID_SIZE];
-	const bt_gatt_characteristic_t *chrcs;
-	size_t num_chrcs;
-} bt_gatt_service_t;
-
 struct bt_gatt_service_iter {
 	struct bt_gatt_client *client;
 	void *ptr;
 };
 
+struct bt_gatt_characteristic_iter {
+	void *service;
+	size_t pos;
+};
+
 bool bt_gatt_service_iter_init(struct bt_gatt_service_iter *iter,
 						struct bt_gatt_client *client);
 bool bt_gatt_service_iter_next(struct bt_gatt_service_iter *iter,
-						bt_gatt_service_t *service);
+					const bt_gatt_service_t **service);
 bool bt_gatt_service_iter_next_by_handle(struct bt_gatt_service_iter *iter,
-						uint16_t start_handle,
-						bt_gatt_service_t *service);
+					uint16_t start_handle,
+					const bt_gatt_service_t **service);
 bool bt_gatt_service_iter_next_by_uuid(struct bt_gatt_service_iter *iter,
 					const uint8_t uuid[BT_GATT_UUID_SIZE],
-					bt_gatt_service_t *service);
+					const bt_gatt_service_t **service);
+
+bool bt_gatt_characteristic_iter_init(struct bt_gatt_characteristic_iter *iter,
+					const bt_gatt_service_t *service);
+bool bt_gatt_characteristic_iter_next(struct bt_gatt_characteristic_iter *iter,
+					const bt_gatt_characteristic_t **chrc);
 
 typedef void (*bt_gatt_client_read_callback_t)(bool success, uint8_t att_ecode,
 					const uint8_t *value, uint16_t length,
diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
index d1395b2..8bde1ee 100644
--- a/tools/btgatt-client.c
+++ b/tools/btgatt-client.c
@@ -169,16 +169,21 @@ static void print_uuid(const uint8_t uuid[16])
 
 static void print_service(const bt_gatt_service_t *service)
 {
+	struct bt_gatt_characteristic_iter iter;
 	const bt_gatt_characteristic_t *chrc;
-	size_t i, j;
+	size_t i;
+
+	if (!bt_gatt_characteristic_iter_init(&iter, service)) {
+		PRLOG("Failed to initialize characteristic iterator\n");
+		return;
+	}
 
 	printf(COLOR_RED "service" COLOR_OFF " - start: 0x%04x, "
 				"end: 0x%04x, uuid: ",
 				service->start_handle, service->end_handle);
 	print_uuid(service->uuid);
 
-	for (i = 0; i < service->num_chrcs; i++) {
-		chrc = service->chrcs + i;
+	while (bt_gatt_characteristic_iter_next(&iter, &chrc)) {
 		printf("\t  " COLOR_YELLOW "charac" COLOR_OFF
 				" - start: 0x%04x, end: 0x%04x, "
 				"value: 0x%04x, props: 0x%02x, uuid: ",
@@ -186,13 +191,13 @@ static void print_service(const bt_gatt_service_t *service)
 				chrc->end_handle,
 				chrc->value_handle,
 				chrc->properties);
-		print_uuid(service->chrcs[i].uuid);
+		print_uuid(chrc->uuid);
 
-		for (j = 0; j < chrc->num_descs; j++) {
+		for (i = 0; i < chrc->num_descs; i++) {
 			printf("\t\t  " COLOR_MAGENTA "descr" COLOR_OFF
 						" - handle: 0x%04x, uuid: ",
-						chrc->descs[j].handle);
-			print_uuid(chrc->descs[j].uuid);
+						chrc->descs[i].handle);
+			print_uuid(chrc->descs[i].uuid);
 		}
 	}
 
@@ -202,7 +207,7 @@ static void print_service(const bt_gatt_service_t *service)
 static void print_services(struct client *cli)
 {
 	struct bt_gatt_service_iter iter;
-	bt_gatt_service_t service;
+	const bt_gatt_service_t *service;
 
 	if (!bt_gatt_service_iter_init(&iter, cli->gatt)) {
 		PRLOG("Failed to initialize service iterator\n");
@@ -212,13 +217,13 @@ static void print_services(struct client *cli)
 	printf("\n");
 
 	while (bt_gatt_service_iter_next(&iter, &service))
-		print_service(&service);
+		print_service(service);
 }
 
 static void print_services_by_uuid(struct client *cli, const bt_uuid_t *uuid)
 {
 	struct bt_gatt_service_iter iter;
-	bt_gatt_service_t service;
+	const bt_gatt_service_t *service;
 
 	if (!bt_gatt_service_iter_init(&iter, cli->gatt)) {
 		PRLOG("Failed to initialize service iterator\n");
@@ -229,13 +234,13 @@ static void print_services_by_uuid(struct client *cli, const bt_uuid_t *uuid)
 
 	while (bt_gatt_service_iter_next_by_uuid(&iter, uuid->value.u128.data,
 								&service))
-		print_service(&service);
+		print_service(service);
 }
 
 static void print_services_by_handle(struct client *cli, uint16_t handle)
 {
 	struct bt_gatt_service_iter iter;
-	bt_gatt_service_t service;
+	const bt_gatt_service_t *service;
 
 	if (!bt_gatt_service_iter_init(&iter, cli->gatt)) {
 		PRLOG("Failed to initialize service iterator\n");
@@ -245,7 +250,7 @@ static void print_services_by_handle(struct client *cli, uint16_t handle)
 	printf("\n");
 
 	while (bt_gatt_service_iter_next_by_handle(&iter, handle, &service))
-		print_service(&service);
+		print_service(service);
 }
 
 static void ready_cb(bool success, uint8_t att_ecode, void *user_data)
-- 
2.1.0.rc2.206.gedb03e5

[PATCH BlueZ v1 2/7] shared/gatt-client: Implement bt_gatt_client_register_notify.

[Arman Uguray]

This patch introduces the bt_gatt_client_register_notify and
bt_gatt_client_unregister_notify functions and implements the latter. The
bt_gatt_client_register_notify function does the following:

  - If the given characteristic has a CCC descriptor, it writes to it based on
    which notify/indicate properties are present in the characteristic
    properties bit field.

  - It maintains a per-characteristic count of how many callbacks have been
    registered, so that the CCC descriptor is written to only if the count is 0.

  - It stores the passed in callback and user data in bt_gatt_client, which will
    be invoked when a notification/indication is received from the corresponding
    characteristic.

  - It queues register requests if the count is 0 and a CCC write is pending,
    and processes them once the write request has completed.
---
 src/shared/gatt-client.c | 361 ++++++++++++++++++++++++++++++++++++++++++-----
 src/shared/gatt-client.h |  25 ++++
 2 files changed, 347 insertions(+), 39 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index fd866b2..974fa1d 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -28,6 +28,9 @@
 #include "src/shared/util.h"
 #include "src/shared/queue.h"
 
+#include <assert.h>
+#include <limits.h>
+
 #ifndef MAX
 #define MAX(a, b) ((a) > (b) ? (a) : (b))
 #endif
@@ -38,9 +41,30 @@
 
 #define UUID_BYTES (BT_GATT_UUID_SIZE * sizeof(uint8_t))
 
+struct chrc_data {
+	/* The public characteristic entry. */
+	bt_gatt_characteristic_t chrc_external;
+
+	/* The private entries. */
+	uint16_t ccc_handle;
+	int not_ref_count;
+
+	/* Internal non-const pointer to the descriptor array. We use this
+	 * internally to modify/free the array, while we expose it externally
+	 * using the const pointer "descs" field in bt_gatt_characteristic_t.
+	 */
+	bt_gatt_descriptor_t *descs;
+
+	/* Pending calls to register_notify are queued here so that they can be
+	 * processed after a write that modifies the CCC descriptor.
+	 */
+	struct queue *reg_notify_queue;
+	unsigned int ccc_write_id;
+};
+
 struct service_list {
 	bt_gatt_service_t service;
-	bt_gatt_characteristic_t *chrcs;
+	struct chrc_data *chrcs;
 	size_t num_chrcs;
 	struct service_list *next;
 };
@@ -69,6 +93,10 @@ struct bt_gatt_client {
 	 */
 	struct queue *long_write_queue;
 	bool in_long_write;
+
+	/* List of registered notification/indication callbacks */
+	struct queue *notify_list;
+	int next_reg_id;
 };
 
 static bool gatt_client_add_service(struct bt_gatt_client *client,
@@ -95,12 +123,17 @@ static bool gatt_client_add_service(struct bt_gatt_client *client,
 	return true;
 }
 
+static void notify_data_unref(void *data);
+
 static void service_destroy_characteristics(struct service_list *service)
 {
 	unsigned int i;
 
-	for (i = 0; i < service->num_chrcs; i++)
-		free((bt_gatt_descriptor_t *) service->chrcs[i].descs);
+	for (i = 0; i < service->num_chrcs; i++) {
+		free(service->chrcs[i].descs);
+		queue_destroy(service->chrcs[i].reg_notify_queue,
+							notify_data_unref);
+	}
 
 	free(service->chrcs);
 }
@@ -124,7 +157,7 @@ static void gatt_client_clear_services(struct bt_gatt_client *client)
 struct discovery_op {
 	struct bt_gatt_client *client;
 	struct service_list *cur_service;
-	bt_gatt_characteristic_t *cur_chrc;
+	struct chrc_data *cur_chrc;
 	int cur_chrc_index;
 	int ref_count;
 };
@@ -176,6 +209,18 @@ static void discover_chrcs_cb(bool success, uint8_t att_ecode,
 						struct bt_gatt_result *result,
 						void *user_data);
 
+static int uuid_cmp(const uint8_t uuid128[16], uint16_t uuid16)
+{
+	uint8_t rhs_uuid[16] = {
+		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00,
+		0x80, 0x00, 0x00, 0x80, 0x5F, 0x9B, 0x34, 0xFB
+	};
+
+	put_be16(uuid16, rhs_uuid + 2);
+
+	return memcmp(uuid128, rhs_uuid, sizeof(rhs_uuid));
+}
+
 static void discover_descs_cb(bool success, uint8_t att_ecode,
 						struct bt_gatt_result *result,
 						void *user_data)
@@ -225,25 +270,28 @@ static void discover_descs_cb(bool success, uint8_t att_ecode,
 		util_debug(client->debug_callback, client->debug_data,
 						"handle: 0x%04x, uuid: %s",
 						descs[i].handle, uuid_str);
+
+		if (uuid_cmp(descs[i].uuid, GATT_CLIENT_CHARAC_CFG_UUID) == 0)
+			op->cur_chrc->ccc_handle = descs[i].handle;
+
 		i++;
 	}
 
-	op->cur_chrc->num_descs = desc_count;
+	op->cur_chrc->chrc_external.num_descs = desc_count;
 	op->cur_chrc->descs = descs;
+	op->cur_chrc->chrc_external.descs = descs;
 
 	for (i = op->cur_chrc_index + 1; i < op->cur_service->num_chrcs; i++) {
 		op->cur_chrc_index = i;
 		op->cur_chrc++;
-		desc_start = op->cur_chrc->value_handle + 1;
-		if (desc_start > op->cur_chrc->end_handle)
+		desc_start = op->cur_chrc->chrc_external.value_handle + 1;
+		if (desc_start > op->cur_chrc->chrc_external.end_handle)
 			continue;
 
-		if (bt_gatt_discover_descriptors(client->att,
-						desc_start,
-						op->cur_chrc->end_handle,
-						discover_descs_cb,
-						discovery_op_ref(op),
-						discovery_op_unref))
+		if (bt_gatt_discover_descriptors(client->att, desc_start,
+					op->cur_chrc->chrc_external.end_handle,
+					discover_descs_cb, discovery_op_ref(op),
+					discovery_op_unref))
 			return;
 
 		util_debug(client->debug_callback, client->debug_data,
@@ -288,7 +336,7 @@ static void discover_chrcs_cb(bool success, uint8_t att_ecode,
 	unsigned int chrc_count;
 	unsigned int i;
 	uint16_t desc_start;
-	bt_gatt_characteristic_t *chrcs;
+	struct chrc_data *chrcs;
 
 	if (!success) {
 		if (att_ecode == BT_ATT_ERROR_ATTRIBUTE_NOT_FOUND) {
@@ -311,25 +359,35 @@ static void discover_chrcs_cb(bool success, uint8_t att_ecode,
 	if (chrc_count == 0)
 		goto next;
 
-	chrcs = new0(bt_gatt_characteristic_t, chrc_count);
+	chrcs = new0(struct chrc_data, chrc_count);
 	if (!chrcs) {
 		success = false;
 		goto done;
 	}
 
 	i = 0;
-	while (bt_gatt_iter_next_characteristic(&iter, &chrcs[i].start_handle,
-							&chrcs[i].end_handle,
-							&chrcs[i].value_handle,
-							&chrcs[i].properties,
-							chrcs[i].uuid)) {
-		uuid_to_string(chrcs[i].uuid, uuid_str);
+	while (bt_gatt_iter_next_characteristic(&iter,
+					&chrcs[i].chrc_external.start_handle,
+					&chrcs[i].chrc_external.end_handle,
+					&chrcs[i].chrc_external.value_handle,
+					&chrcs[i].chrc_external.properties,
+					chrcs[i].chrc_external.uuid)) {
+		uuid_to_string(chrcs[i].chrc_external.uuid, uuid_str);
 		util_debug(client->debug_callback, client->debug_data,
 				"start: 0x%04x, end: 0x%04x, value: 0x%04x, "
 				"props: 0x%02x, uuid: %s",
-				chrcs[i].start_handle, chrcs[i].end_handle,
-				chrcs[i].value_handle, chrcs[i].properties,
+				chrcs[i].chrc_external.start_handle,
+				chrcs[i].chrc_external.end_handle,
+				chrcs[i].chrc_external.value_handle,
+				chrcs[i].chrc_external.properties,
 				uuid_str);
+
+		chrcs[i].reg_notify_queue = queue_new();
+		if (!chrcs[i].reg_notify_queue) {
+			success = false;
+			goto done;
+		}
+
 		i++;
 	}
 
@@ -339,15 +397,14 @@ static void discover_chrcs_cb(bool success, uint8_t att_ecode,
 	for (i = 0; i < chrc_count; i++) {
 		op->cur_chrc_index = i;
 		op->cur_chrc = chrcs + i;
-		desc_start = chrcs[i].value_handle + 1;
-		if (desc_start > chrcs[i].end_handle)
+		desc_start = chrcs[i].chrc_external.value_handle + 1;
+		if (desc_start > chrcs[i].chrc_external.end_handle)
 			continue;
 
-		if (bt_gatt_discover_descriptors(client->att,
-						desc_start, chrcs[i].end_handle,
-						discover_descs_cb,
-						discovery_op_ref(op),
-						discovery_op_unref))
+		if (bt_gatt_discover_descriptors(client->att, desc_start,
+					chrcs[i].chrc_external.end_handle,
+					discover_descs_cb, discovery_op_ref(op),
+					discovery_op_unref))
 			return;
 
 		util_debug(client->debug_callback, client->debug_data,
@@ -516,6 +573,142 @@ static bool gatt_client_init(struct bt_gatt_client *client, uint16_t mtu)
 	return true;
 }
 
+struct notify_data {
+	struct bt_gatt_client *client;
+	unsigned int id;
+	int ref_count;
+	struct chrc_data *chrc;
+	bt_gatt_client_notify_id_callback_t callback;
+	bt_gatt_client_notify_callback_t notify;
+	void *user_data;
+	bt_gatt_client_destroy_func_t destroy;
+};
+
+static struct notify_data *notify_data_ref(struct notify_data *notify_data)
+{
+	__sync_fetch_and_add(&notify_data->ref_count, 1);
+
+	return notify_data;
+}
+
+static void notify_data_unref(void *data)
+{
+	struct notify_data *notify_data = data;
+
+	if (__sync_sub_and_fetch(&notify_data->ref_count, 1))
+		return;
+
+	if (notify_data->destroy)
+		notify_data->destroy(notify_data->user_data);
+
+	free(notify_data);
+}
+
+static void complete_notify_request(void *data)
+{
+	struct notify_data *notify_data = data;
+
+	/* Increment the per-characteristic ref count of notify handlers */
+	__sync_fetch_and_add(&notify_data->chrc->not_ref_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);
+}
+
+static bool notify_data_write_ccc(struct notify_data *notify_data, bool enable,
+						bt_att_response_func_t callback)
+{
+	uint8_t pdu[4];
+
+	memset(pdu, 0, sizeof(pdu));
+	put_le16(notify_data->chrc->ccc_handle, pdu);
+
+	if (enable) {
+		/* Try to enable notifications and/or indications based on
+		 * whatever the characteristic supports.
+		 */
+		if (notify_data->chrc->chrc_external.properties &
+						BT_GATT_CHRC_PROP_NOTIFY)
+			pdu[2] = 0x01;
+
+		if (notify_data->chrc->chrc_external.properties &
+						BT_GATT_CHRC_PROP_INDICATE)
+			pdu[2] |= 0x02;
+
+		if (!pdu[2])
+			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);
+
+	return !!notify_data->chrc->ccc_write_id;
+}
+
+static uint8_t process_error(const void *pdu, uint16_t length)
+{
+	if (!pdu || length != 4)
+		return 0;
+
+	return ((uint8_t *) pdu)[3];
+}
+
+static void enable_ccc_callback(uint8_t opcode, const void *pdu,
+					uint16_t length, void *user_data)
+{
+	struct notify_data *notify_data = user_data;
+	uint16_t att_ecode;
+
+	assert(!notify_data->chrc->not_ref_count);
+	assert(notify_data->chrc->ccc_write_id);
+
+	notify_data->chrc->ccc_write_id = 0;
+
+	if (opcode == BT_ATT_OP_ERROR_RSP) {
+		att_ecode = process_error(pdu, length);
+
+		/* Failed to enable. Complete the current request and move on to
+		 * 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);
+
+		while ((notify_data = queue_pop_head(
+					notify_data->chrc->reg_notify_queue))) {
+
+			if (notify_data_write_ccc(notify_data, true,
+							enable_ccc_callback))
+				return;
+		}
+
+		return;
+	}
+
+	/* Success! Report success for all remaining requests. */
+	complete_notify_request(notify_data);
+	queue_remove_all(notify_data->chrc->reg_notify_queue, NULL, NULL,
+						complete_notify_request);
+}
+
+static void disable_ccc_callback(uint8_t opcode, const void *pdu,
+					uint16_t length, void *user_data)
+{
+	/* TODO */
+}
+
 struct bt_gatt_client *bt_gatt_client_new(struct bt_att *att, uint16_t mtu)
 {
 	struct bt_gatt_client *client;
@@ -533,6 +726,13 @@ struct bt_gatt_client *bt_gatt_client_new(struct bt_att *att, uint16_t mtu)
 		return NULL;
 	}
 
+	client->notify_list = queue_new();
+	if (!client->notify_list) {
+		queue_destroy(client->long_write_queue, NULL);
+		free(client);
+		return NULL;
+	}
+
 	client->att = bt_att_ref(att);
 
 	gatt_client_init(client, mtu);
@@ -567,6 +767,7 @@ void bt_gatt_client_unref(struct bt_gatt_client *client)
 		client->debug_destroy(client->debug_data);
 
 	queue_destroy(client->long_write_queue, long_write_op_unref);
+	queue_destroy(client->notify_list, notify_data_unref);
 	bt_att_unref(client->att);
 	free(client);
 }
@@ -700,7 +901,7 @@ bool bt_gatt_characteristic_iter_next(struct bt_gatt_characteristic_iter *iter,
 	if (iter->pos >= service->num_chrcs)
 		return false;
 
-	*chrc = service->chrcs + iter->pos++;
+	*chrc = &service->chrcs[iter->pos++].chrc_external;
 
 	return true;
 }
@@ -721,14 +922,6 @@ static void destroy_read_op(void *data)
 	free(op);
 }
 
-static uint8_t process_error(const void *pdu, uint16_t length)
-{
-	if (!pdu || length != 4)
-		return 0;
-
-	return ((uint8_t *) pdu)[3];
-}
-
 static void read_cb(uint8_t opcode, const void *pdu, uint16_t length,
 								void *user_data)
 {
@@ -1387,3 +1580,93 @@ bool bt_gatt_client_write_long_value(struct bt_gatt_client *client,
 
 	return true;
 }
+
+bool 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_notify_callback_t notify,
+				void *user_data,
+				bt_gatt_client_destroy_func_t destroy)
+{
+	struct notify_data *notify_data;
+	struct service_list *svc_data = NULL;
+	struct chrc_data *chrc = NULL;
+	struct bt_gatt_service_iter iter;
+	const bt_gatt_service_t *service;
+	size_t i;
+
+	if (!client || !chrc_value_handle || !callback)
+		return false;
+
+	if (!bt_gatt_client_is_ready(client))
+		return false;
+
+	/* Check that chrc_value_handle belongs to a known characteristic */
+	if (!bt_gatt_service_iter_init(&iter, client))
+		return false;
+
+	while (bt_gatt_service_iter_next(&iter, &service)) {
+		if (chrc_value_handle >= service->start_handle &&
+				chrc_value_handle <= service->end_handle) {
+			svc_data = (struct service_list *) service;
+			break;
+		}
+	}
+
+	if (!svc_data)
+		return false;
+
+	for (i = 0; i < svc_data->num_chrcs; i++) {
+		if (svc_data->chrcs[i].chrc_external.value_handle ==
+							chrc_value_handle) {
+			chrc = svc_data->chrcs + i;
+			break;
+		}
+	}
+
+	/* Check that the characteristic supports notifications/indications */
+	if (!chrc || !chrc->ccc_handle || chrc->not_ref_count == INT_MAX)
+		return false;
+
+	notify_data = new0(struct notify_data, 1);
+	if (!notify_data)
+		return false;
+
+	notify_data->client = client;
+	notify_data->ref_count = 1;
+	notify_data->chrc = chrc;
+	notify_data->callback = callback;
+	notify_data->notify = notify;
+	notify_data->user_data = user_data;
+	notify_data->destroy = destroy;
+
+	/* 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;
+	}
+
+	/* If the ref count is not zero, then notifications are already enabled.
+	 */
+	if (chrc->not_ref_count > 0) {
+		complete_notify_request(notify_data);
+		return true;
+	}
+
+	/* Write to the CCC descriptor */
+	if (!notify_data_write_ccc(notify_data, true, enable_ccc_callback)) {
+		free(notify_data);
+		return false;
+	}
+
+	return true;
+}
+
+bool bt_gatt_client_unregister_notify(struct bt_gatt_client *client,
+							unsigned int id)
+{
+	/* TODO */
+	return false;
+}
diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
index 417d964..7612a6e 100644
--- a/src/shared/gatt-client.h
+++ b/src/shared/gatt-client.h
@@ -27,6 +27,16 @@
 
 #define BT_GATT_UUID_SIZE 16
 
+#define BT_GATT_CHRC_PROP_BROADCAST			0x01
+#define BT_GATT_CHRC_PROP_READ				0x02
+#define BT_GATT_CHRC_PROP_WRITE_WITHOUT_RESP		0x04
+#define BT_GATT_CHRC_PROP_WRITE				0x08
+#define BT_GATT_CHRC_PROP_NOTIFY			0x10
+#define BT_GATT_CHRC_PROP_INDICATE			0x20
+#define BT_GATT_CHRC_PROP_AUTH				0x40
+#define BT_GATT_CHRC_PROP_EXT_PROP			0x80
+
+/* Client Characteristic Configuration bit field */
 struct bt_gatt_client;
 
 struct bt_gatt_client *bt_gatt_client_new(struct bt_att *att, uint16_t mtu);
@@ -41,6 +51,12 @@ typedef void (*bt_gatt_client_debug_func_t)(const char *str, void *user_data);
 typedef void (*bt_gatt_client_write_long_callback_t)(bool success,
 					bool reliable_error, uint8_t att_ecode,
 					void *user_data);
+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,
+							void *user_data);
 
 bool bt_gatt_client_is_ready(struct bt_gatt_client *client);
 bool bt_gatt_client_set_ready_handler(struct bt_gatt_client *client,
@@ -131,3 +147,12 @@ bool bt_gatt_client_write_long_value(struct bt_gatt_client *client,
 				bt_gatt_client_write_long_callback_t callback,
 				void *user_data,
 				bt_gatt_client_destroy_func_t destroy);
+
+bool 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_notify_callback_t notify,
+				void *user_data,
+				bt_gatt_client_destroy_func_t destroy);
+bool bt_gatt_client_unregister_notify(struct bt_gatt_client *client,
+							unsigned int id);
-- 
2.1.0.rc2.206.gedb03e5

[PATCH BlueZ v1 3/7] shared/gatt-client: Implement bt_gatt_client_unregister_notify.

[Arman Uguray]

This patch implements bt_gatt_client_unregister_notify, which is used to remove
a previous registered notification/indication handler. This function decreases
the per-characteristic count of handlers, and if the count drops to 0, a write
request is sent to the CCC descriptor to disable notifications/indications.
---
 src/shared/gatt-client.c | 54 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 974fa1d..d68c8aa 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -604,6 +604,14 @@ static void notify_data_unref(void *data)
 	free(notify_data);
 }
 
+static bool match_notify_data_id(const void *a, const void *b)
+{
+	const struct notify_data *notify_data = a;
+	unsigned int id = PTR_TO_UINT(b);
+
+	return notify_data->id == id;
+}
+
 static void complete_notify_request(void *data)
 {
 	struct notify_data *notify_data = data;
@@ -706,7 +714,23 @@ static void enable_ccc_callback(uint8_t opcode, const void *pdu,
 static void disable_ccc_callback(uint8_t opcode, const void *pdu,
 					uint16_t length, void *user_data)
 {
-	/* TODO */
+	struct notify_data *notify_data = user_data;
+	struct notify_data *next_data;
+
+	assert(!notify_data->chrc->not_ref_count);
+	assert(notify_data->chrc->ccc_write_id);
+
+	notify_data->chrc->ccc_write_id = 0;
+
+	/* This is a best effort procedure, so ignore errors and process any
+	 * queued requests.
+	 */
+	while (1) {
+		next_data = queue_pop_head(notify_data->chrc->reg_notify_queue);
+		if (!next_data || notify_data_write_ccc(notify_data, true,
+							enable_ccc_callback))
+			return;
+	}
 }
 
 struct bt_gatt_client *bt_gatt_client_new(struct bt_att *att, uint16_t mtu)
@@ -1667,6 +1691,30 @@ bool bt_gatt_client_register_notify(struct bt_gatt_client *client,
 bool bt_gatt_client_unregister_notify(struct bt_gatt_client *client,
 							unsigned int id)
 {
-	/* TODO */
-	return false;
+	struct notify_data *notify_data;
+
+	if (!client || !id)
+		return false;
+
+	notify_data = queue_find(client->notify_list, match_notify_data_id,
+							UINT_TO_PTR(id));
+	if (!notify_data)
+		return false;
+
+	assert(notify_data->chrc->not_ref_count > 0);
+	assert(!notify_data->chrc->ccc_write_id);
+
+	/* TODO: Delay destruction/removal if we're in the middle of processing
+	 * a notification.
+	 */
+	queue_remove(client->notify_list, notify_data);
+
+	if (__sync_sub_and_fetch(&notify_data->chrc->not_ref_count, 1)) {
+		notify_data_unref(notify_data);
+		return true;
+	}
+
+	notify_data_write_ccc(notify_data, false, disable_ccc_callback);
+
+	return true;
 }
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH BlueZ v1 3/7] shared/gatt-client: Implement bt_gatt_client_unregister_notify.

[Marcel Holtmann]

Hi Arman,

> This patch implements bt_gatt_client_unregister_notify, which is used to remove
> a previous registered notification/indication handler. This function decreases
> the per-characteristic count of handlers, and if the count drops to 0, a write
> request is sent to the CCC descriptor to disable notifications/indications.
> ---
> src/shared/gatt-client.c | 54 +++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> index 974fa1d..d68c8aa 100644
> --- a/src/shared/gatt-client.c
> +++ b/src/shared/gatt-client.c
> @@ -604,6 +604,14 @@ static void notify_data_unref(void *data)
> 	free(notify_data);
> }
> 
> +static bool match_notify_data_id(const void *a, const void *b)
> +{
> +	const struct notify_data *notify_data = a;
> +	unsigned int id = PTR_TO_UINT(b);
> +
> +	return notify_data->id == id;
> +}
> +
> static void complete_notify_request(void *data)
> {
> 	struct notify_data *notify_data = data;
> @@ -706,7 +714,23 @@ static void enable_ccc_callback(uint8_t opcode, const void *pdu,
> static void disable_ccc_callback(uint8_t opcode, const void *pdu,
> 					uint16_t length, void *user_data)
> {
> -	/* TODO */
> +	struct notify_data *notify_data = user_data;
> +	struct notify_data *next_data;
> +
> +	assert(!notify_data->chrc->not_ref_count);
> +	assert(notify_data->chrc->ccc_write_id);
> +
> +	notify_data->chrc->ccc_write_id = 0;
> +
> +	/* This is a best effort procedure, so ignore errors and process any
> +	 * queued requests.
> +	 */
> +	while (1) {
> +		next_data = queue_pop_head(notify_data->chrc->reg_notify_queue);
> +		if (!next_data || notify_data_write_ccc(notify_data, true,
> +							enable_ccc_callback))
> +			return;
> +	}
> }
> 
> struct bt_gatt_client *bt_gatt_client_new(struct bt_att *att, uint16_t mtu)
> @@ -1667,6 +1691,30 @@ bool bt_gatt_client_register_notify(struct bt_gatt_client *client,
> bool bt_gatt_client_unregister_notify(struct bt_gatt_client *client,
> 							unsigned int id)
> {
> -	/* TODO */
> -	return false;
> +	struct notify_data *notify_data;
> +
> +	if (!client || !id)
> +		return false;
> +
> +	notify_data = queue_find(client->notify_list, match_notify_data_id,
> +							UINT_TO_PTR(id));
> +	if (!notify_data)
> +		return false;
> +
> +	assert(notify_data->chrc->not_ref_count > 0);
> +	assert(!notify_data->chrc->ccc_write_id);

do we really need the asserts. I think we should just fall gracefully and cleanup. After all this is an exposed API. I rather have it not assert. Same applies to the case above, really only assert if this is something that should not happen ever and if it did something really horrible went wrong.

> +
> +	/* TODO: Delay destruction/removal if we're in the middle of processing
> +	 * a notification.
> +	 */
> +	queue_remove(client->notify_list, notify_data);
> +
> +	if (__sync_sub_and_fetch(&notify_data->chrc->not_ref_count, 1)) {

I read "this is not a reference count" when I see this variable name. That is not good. We need clear and short variable names. Not some that will confuse the reader of the code.

> +		notify_data_unref(notify_data);
> +		return true;
> +	}
> +
> +	notify_data_write_ccc(notify_data, false, disable_ccc_callback);
> +
> +	return true;
> }

Regards

Marcel

[PATCH BlueZ v1 4/7] shared/gatt-client: Handle incoming not/ind PDUs.

[Arman Uguray]

bt_gatt_client now registers an incoming PDU handler for notification and
indication PDUs. The PDU is parsed and routed to the notify handler registered
with bt_gatt_client_register_notify for the corresponding characteristic value
handle. A confirmation PDU is sent automatically for received indications.

This patch removes the bt_gatt_register function from shared/gatt-helpers.
---
 src/shared/gatt-client.c  | 120 ++++++++++++++++++++++++++++++++++++++++++----
 src/shared/gatt-helpers.c |  67 --------------------------
 src/shared/gatt-helpers.h |   9 ----
 3 files changed, 110 insertions(+), 86 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index d68c8aa..d3c9225 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -97,6 +97,9 @@ struct bt_gatt_client {
 	/* List of registered notification/indication callbacks */
 	struct queue *notify_list;
 	int next_reg_id;
+	unsigned int notify_id, indic_id;
+	bool in_notify;
+	bool need_notify_cleanup;
 };
 
 static bool gatt_client_add_service(struct bt_gatt_client *client,
@@ -575,6 +578,7 @@ static bool gatt_client_init(struct bt_gatt_client *client, uint16_t mtu)
 
 struct notify_data {
 	struct bt_gatt_client *client;
+	bool removed;
 	unsigned int id;
 	int ref_count;
 	struct chrc_data *chrc;
@@ -612,6 +616,18 @@ static bool match_notify_data_id(const void *a, const void *b)
 	return notify_data->id == id;
 }
 
+static bool match_notify_data_removed(const void *a, const void *b)
+{
+	const struct notify_data *notify_data = a;
+
+	return notify_data->removed;
+}
+
+struct pdu_data {
+	const void *pdu;
+	uint16_t length;
+};
+
 static void complete_notify_request(void *data)
 {
 	struct notify_data *notify_data = data;
@@ -638,6 +654,7 @@ static bool notify_data_write_ccc(struct notify_data *notify_data, bool enable,
 {
 	uint8_t pdu[4];
 
+	assert(notify_data->chrc->ccc_handle);
 	memset(pdu, 0, sizeof(pdu));
 	put_le16(notify_data->chrc->ccc_handle, pdu);
 
@@ -733,6 +750,72 @@ static void disable_ccc_callback(uint8_t opcode, const void *pdu,
 	}
 }
 
+static void complete_unregister_notify(void *data)
+{
+	struct notify_data *notify_data = data;
+
+	if (__sync_sub_and_fetch(&notify_data->chrc->not_ref_count, 1)) {
+		notify_data_unref(notify_data);
+		return;
+	}
+
+	notify_data_write_ccc(notify_data, false, disable_ccc_callback);
+}
+
+static void notify_handler(void *data, void *user_data)
+{
+	struct notify_data *notify_data = data;
+	struct pdu_data *pdu_data = user_data;
+	uint16_t value_handle;
+	const uint8_t *value = NULL;
+
+	if (notify_data->removed)
+		return;
+
+	value_handle = get_le16(pdu_data->pdu);
+
+	if (notify_data->chrc->chrc_external.value_handle != value_handle)
+		return;
+
+	if (pdu_data->length > 2)
+		value = pdu_data->pdu + 2;
+
+	if (notify_data->notify)
+		notify_data->notify(value_handle, value, pdu_data->length - 2,
+							notify_data->user_data);
+}
+
+static void notify_cb(uint8_t opcode, const void *pdu, uint16_t length,
+								void *user_data)
+{
+	struct bt_gatt_client *client = user_data;
+	struct pdu_data pdu_data;
+
+	bt_gatt_client_ref(client);
+
+	client->in_notify = true;
+
+	memset(&pdu_data, 0, sizeof(pdu_data));
+	pdu_data.pdu = pdu;
+	pdu_data.length = length;
+
+	queue_foreach(client->notify_list, notify_handler, &pdu_data);
+
+	client->in_notify = false;
+
+	if (client->need_notify_cleanup) {
+		queue_remove_all(client->notify_list, match_notify_data_removed,
+					NULL, complete_unregister_notify);
+		client->need_notify_cleanup = false;
+	}
+
+	if (opcode == BT_ATT_OP_HANDLE_VAL_IND)
+		bt_att_send(client->att, BT_ATT_OP_HANDLE_VAL_CONF, NULL, 0,
+							NULL, NULL, NULL);
+
+	bt_gatt_client_unref(client);
+}
+
 struct bt_gatt_client *bt_gatt_client_new(struct bt_att *att, uint16_t mtu)
 {
 	struct bt_gatt_client *client;
@@ -757,6 +840,23 @@ struct bt_gatt_client *bt_gatt_client_new(struct bt_att *att, uint16_t mtu)
 		return NULL;
 	}
 
+	client->notify_id = bt_att_register(att, BT_ATT_OP_HANDLE_VAL_NOT,
+						notify_cb, client, NULL);
+	if (!client->notify_id) {
+		queue_destroy(client->long_write_queue, NULL);
+		free(client);
+		return NULL;
+	}
+
+	client->indic_id = bt_att_register(att, BT_ATT_OP_HANDLE_VAL_IND,
+						notify_cb, client, NULL);
+	if (!client->indic_id) {
+		bt_att_unregister(att, client->notify_id);
+		queue_destroy(client->long_write_queue, NULL);
+		free(client);
+		return NULL;
+	}
+
 	client->att = bt_att_ref(att);
 
 	gatt_client_init(client, mtu);
@@ -790,8 +890,12 @@ void bt_gatt_client_unref(struct bt_gatt_client *client)
 	if (client->debug_destroy)
 		client->debug_destroy(client->debug_data);
 
+	bt_att_unregister(client->att, client->notify_id);
+	bt_att_unregister(client->att, client->indic_id);
+
 	queue_destroy(client->long_write_queue, long_write_op_unref);
 	queue_destroy(client->notify_list, notify_data_unref);
+
 	bt_att_unref(client->att);
 	free(client);
 }
@@ -1698,23 +1802,19 @@ bool bt_gatt_client_unregister_notify(struct bt_gatt_client *client,
 
 	notify_data = queue_find(client->notify_list, match_notify_data_id,
 							UINT_TO_PTR(id));
-	if (!notify_data)
+	if (!notify_data || notify_data->removed)
 		return false;
 
 	assert(notify_data->chrc->not_ref_count > 0);
 	assert(!notify_data->chrc->ccc_write_id);
 
-	/* TODO: Delay destruction/removal if we're in the middle of processing
-	 * a notification.
-	 */
-	queue_remove(client->notify_list, notify_data);
-
-	if (__sync_sub_and_fetch(&notify_data->chrc->not_ref_count, 1)) {
-		notify_data_unref(notify_data);
+	if (!client->in_notify) {
+		queue_remove(client->notify_list, notify_data);
+		complete_unregister_notify(notify_data);
 		return true;
 	}
 
-	notify_data_write_ccc(notify_data, false, disable_ccc_callback);
-
+	notify_data->removed = true;
+	client->need_notify_cleanup = true;
 	return true;
 }
diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index ede6a67..54876bc 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -911,70 +911,3 @@ bool bt_gatt_discover_descriptors(struct bt_att *att,
 
 	return true;
 }
-
-struct notify_data {
-	struct bt_att *att;
-	bt_gatt_notify_callback_t callback;
-	void *user_data;
-	bt_gatt_destroy_func_t destroy;
-};
-
-static void notify_data_destroy(void *data)
-{
-	struct notify_data *notd = data;
-
-	if (notd->destroy)
-		notd->destroy(notd->user_data);
-
-	free(notd);
-}
-
-static void notify_cb(uint8_t opcode, const void *pdu, uint16_t length,
-								void *user_data)
-{
-	struct notify_data *data = user_data;
-	uint16_t value_handle;
-	const uint8_t *value = NULL;
-
-	value_handle = get_le16(pdu);
-
-	if (length > 2)
-		value = pdu + 2;
-
-	if (data->callback)
-		data->callback(value_handle, value, length - 2, data->user_data);
-
-	if (opcode == BT_ATT_OP_HANDLE_VAL_IND)
-		bt_att_send(data->att, BT_ATT_OP_HANDLE_VAL_CONF, NULL, 0,
-							NULL, NULL, NULL);
-}
-
-unsigned int bt_gatt_register(struct bt_att *att, bool indications,
-					bt_gatt_notify_callback_t callback,
-					void *user_data,
-					bt_gatt_destroy_func_t destroy)
-{
-	struct notify_data *data;
-	uint8_t opcode;
-	unsigned int id;
-
-	if (!att)
-		return 0;
-
-	data = new0(struct notify_data, 1);
-	if (!data)
-		return 0;
-
-	data->att = att;
-	data->callback = callback;
-	data->user_data = user_data;
-	data->destroy = destroy;
-
-	opcode = indications ? BT_ATT_OP_HANDLE_VAL_IND : BT_ATT_OP_HANDLE_VAL_NOT;
-
-	id = bt_att_register(att, opcode, notify_cb, data, notify_data_destroy);
-	if (!id)
-		free(data);
-
-	return id;
-}
diff --git a/src/shared/gatt-helpers.h b/src/shared/gatt-helpers.h
index 75ad4b0..c4a6578 100644
--- a/src/shared/gatt-helpers.h
+++ b/src/shared/gatt-helpers.h
@@ -58,10 +58,6 @@ typedef void (*bt_gatt_discovery_callback_t)(bool success, uint8_t att_ecode,
 						struct bt_gatt_result *result,
 						void *user_data);
 
-typedef void (*bt_gatt_notify_callback_t)(uint16_t value_handle,
-					const uint8_t *value, uint16_t length,
-					void *user_data);
-
 bool bt_gatt_exchange_mtu(struct bt_att *att, uint16_t client_rx_mtu,
 					bt_gatt_result_callback_t callback,
 					void *user_data,
@@ -87,8 +83,3 @@ bool bt_gatt_discover_descriptors(struct bt_att *att,
 					bt_gatt_discovery_callback_t callback,
 					void *user_data,
 					bt_gatt_destroy_func_t destroy);
-
-unsigned int bt_gatt_register(struct bt_att *att, bool indications,
-					bt_gatt_notify_callback_t callback,
-					void *user_data,
-					bt_gatt_destroy_func_t destroy);
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH BlueZ v1 4/7] shared/gatt-client: Handle incoming not/ind PDUs.

[Marcel Holtmann]

Hi Arman,

> bt_gatt_client now registers an incoming PDU handler for notification and
> indication PDUs. The PDU is parsed and routed to the notify handler registered
> with bt_gatt_client_register_notify for the corresponding characteristic value
> handle. A confirmation PDU is sent automatically for received indications.
> 
> This patch removes the bt_gatt_register function from shared/gatt-helpers.
> ---
> src/shared/gatt-client.c  | 120 ++++++++++++++++++++++++++++++++++++++++++----
> src/shared/gatt-helpers.c |  67 --------------------------
> src/shared/gatt-helpers.h |   9 ----
> 3 files changed, 110 insertions(+), 86 deletions(-)
> 
> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> index d68c8aa..d3c9225 100644
> --- a/src/shared/gatt-client.c
> +++ b/src/shared/gatt-client.c
> @@ -97,6 +97,9 @@ struct bt_gatt_client {
> 	/* List of registered notification/indication callbacks */
> 	struct queue *notify_list;
> 	int next_reg_id;
> +	unsigned int notify_id, indic_id;

I have never seen indication shorted into indic. I have to say that I do not like it that much. However I do not have any good suggestion either.

> +	bool in_notify;
> +	bool need_notify_cleanup;
> };
> 
> static bool gatt_client_add_service(struct bt_gatt_client *client,
> @@ -575,6 +578,7 @@ static bool gatt_client_init(struct bt_gatt_client *client, uint16_t mtu)
> 
> struct notify_data {
> 	struct bt_gatt_client *client;
> +	bool removed;
> 	unsigned int id;
> 	int ref_count;
> 	struct chrc_data *chrc;
> @@ -612,6 +616,18 @@ static bool match_notify_data_id(const void *a, const void *b)
> 	return notify_data->id == id;
> }
> 
> +static bool match_notify_data_removed(const void *a, const void *b)
> +{
> +	const struct notify_data *notify_data = a;
> +
> +	return notify_data->removed;
> +}
> +
> +struct pdu_data {
> +	const void *pdu;
> +	uint16_t length;
> +};
> +
> static void complete_notify_request(void *data)
> {
> 	struct notify_data *notify_data = data;
> @@ -638,6 +654,7 @@ static bool notify_data_write_ccc(struct notify_data *notify_data, bool enable,
> {
> 	uint8_t pdu[4];
> 
> +	assert(notify_data->chrc->ccc_handle);

You are going heavy on the asserts. I am not convinced that is always a good idea. If remote devices can exploit such an assert, we have a problem. Gracefully disconnect might be better in many cases.

Regards

Marcel

Re: [PATCH BlueZ v1 4/7] shared/gatt-client: Handle incoming not/ind PDUs.

[Arman Uguray]

Hi Marcel,


>> +     unsigned int notify_id, indic_id;
>
> I have never seen indication shorted into indic. I have to say that I do not like it that much. However I do not have any good suggestion either.
>

Perhaps "indicn_id"? I could just spell out "indication_id" :P.


>> +     assert(notify_data->chrc->ccc_handle);
>
> You are going heavy on the asserts. I am not convinced that is always a good idea. If remote devices can exploit such an assert, we have a problem. Gracefully disconnect might be better in many cases.
>

There are certain invariants that go with how the reference count is
managed and I chose to add asserts in certain places to help catch
potential bugs. I used them only for things that should never ever
fail: i.e. if any of these assertions fails then there is a bug in the
code and we should fix them. I don't mind removing them, but all the
asserts that I added here (and in the previous patch that you
responded to) try to capture the logical state and assumptions that
are made in the code from which there isn't really a good way to
recover if these assumptions do not hold.

In short, these assertions should always succeed in non-buggy code.
Does BlueZ have debug-mode-only assert macros? Maybe such a thing
might be useful to make the code self document these assumptions.
Otherwise, I can remove them and replace them with comments instead?

-Arman

Re: [PATCH BlueZ v1 4/7] shared/gatt-client: Handle incoming not/ind PDUs.

[Johan Hedberg]

Hi Arman,

On Tue, Sep 09, 2014, Arman Uguray wrote:
> >> +     unsigned int notify_id, indic_id;
> >
> > I have never seen indication shorted into indic. I have to say that
> > I do not like it that much. However I do not have any good
> > suggestion either.
> >
> 
> Perhaps "indicn_id"? I could just spell out "indication_id" :P.

What's wrong with "ind"? It's used in many places in the tree as a
shorthand for indication and even shared/att.c uses it. On the other
hand if you want to match the grammatical form of notify_id you should
use indicate_id (which feels much worse to me). Third option that comes
to mind if we want short versions of both is "ind" and "nfy", but not
sure how decipherable the latter is.

Johan

Re: [PATCH BlueZ v1 4/7] shared/gatt-client: Handle incoming not/ind PDUs.

[Arman Uguray]

HI Johan,

>> >> +     unsigned int notify_id, indic_id;
>> >
>> > I have never seen indication shorted into indic. I have to say that
>> > I do not like it that much. However I do not have any good
>> > suggestion either.
>> >
>>
>> Perhaps "indicn_id"? I could just spell out "indication_id" :P.
>
> What's wrong with "ind"? It's used in many places in the tree as a
> shorthand for indication and even shared/att.c uses it. On the other
> hand if you want to match the grammatical form of notify_id you should
> use indicate_id (which feels much worse to me). Third option that comes
> to mind if we want short versions of both is "ind" and "nfy", but not
> sure how decipherable the latter is.
>

I initially had "ind" for indication and "not" for notification (which
seemed to be consistent with other places in the code, as you said),
though Marcel didn't like those :P I guess "ind" isn't super
suggestive of "indication" and, as Marcel pointed out earlier, "not"
is easily confused as "negation" rather than "notification.

I personally prefer not to shorten them at all and spell out
"notification_id" and "indication_id", which I find the most readable.
I only started contracting things after mailing list feedback saying
not to use long names.

In the end it doesn't matter what these are called if their
declaration is accompanied by an explanatory comment. "ind_id" and
"ntfy_id" look fine to me.

-Arman

Re: [PATCH BlueZ v1 4/7] shared/gatt-client: Handle incoming not/ind PDUs.

[Marcel Holtmann]

Hi Arman,

>>>>> +     unsigned int notify_id, indic_id;
>>>> 
>>>> I have never seen indication shorted into indic. I have to say that
>>>> I do not like it that much. However I do not have any good
>>>> suggestion either.
>>>> 
>>> 
>>> Perhaps "indicn_id"? I could just spell out "indication_id" :P.
>> 
>> What's wrong with "ind"? It's used in many places in the tree as a
>> shorthand for indication and even shared/att.c uses it. On the other
>> hand if you want to match the grammatical form of notify_id you should
>> use indicate_id (which feels much worse to me). Third option that comes
>> to mind if we want short versions of both is "ind" and "nfy", but not
>> sure how decipherable the latter is.
>> 
> 
> I initially had "ind" for indication and "not" for notification (which
> seemed to be consistent with other places in the code, as you said),
> though Marcel didn't like those :P I guess "ind" isn't super
> suggestive of "indication" and, as Marcel pointed out earlier, "not"
> is easily confused as "negation" rather than "notification.

lets go with ind_id and notify_id for now. We can always change this later and I do not want to hold up merging patches because of that. It is just that the not_id and not_* is something that makes my brain always go into the negation first and that is not good coding style.

> I personally prefer not to shorten them at all and spell out
> "notification_id" and "indication_id", which I find the most readable.
> I only started contracting things after mailing list feedback saying
> not to use long names.

We need to be careful with super long names since they can be nice and explanatory in some cases, but in other cases they cause large indentation and weird line breaks which then turns that code into unreadable mess. So this is something you have to weight against each other. This is not black and white, and the right answer is normally what makes code the most readable.

Regards

Marcel

Re: [PATCH BlueZ v1 4/7] shared/gatt-client: Handle incoming not/ind PDUs.

[Marcel Holtmann]

Hi Arman,

>>> +     assert(notify_data->chrc->ccc_handle);
>> 
>> You are going heavy on the asserts. I am not convinced that is always a good idea. If remote devices can exploit such an assert, we have a problem. Gracefully disconnect might be better in many cases.
>> 
> 
> There are certain invariants that go with how the reference count is
> managed and I chose to add asserts in certain places to help catch
> potential bugs. I used them only for things that should never ever
> fail: i.e. if any of these assertions fails then there is a bug in the
> code and we should fix them. I don't mind removing them, but all the
> asserts that I added here (and in the previous patch that you
> responded to) try to capture the logical state and assumptions that
> are made in the code from which there isn't really a good way to
> recover if these assumptions do not hold.
> 
> In short, these assertions should always succeed in non-buggy code.
> Does BlueZ have debug-mode-only assert macros? Maybe such a thing
> might be useful to make the code self document these assumptions.
> Otherwise, I can remove them and replace them with comments instead?

actually that is a good idea. We might should look into introducing some nice and handy asserts helpers for exactly the case when you want it on during development and testing.

For now, just keep the asserts in it. I just wanted to point it out since I have seen people going crazy with asserts and then they can be remotely exploited. And that is something I want to avoid at all costs. A remote side should never ever be able to trigger an assert.

Regards

Marcel

Re: [PATCH BlueZ v1 4/7] shared/gatt-client: Handle incoming not/ind PDUs.

[Arman Uguray]

Hi Marcel,


> actually that is a good idea. We might should look into introducing some =
nice and handy asserts helpers for exactly the case when you want it on dur=
ing development and testing.
>
> For now, just keep the asserts in it. I just wanted to point it out since=
 I have seen people going crazy with asserts and then they can be remotely =
exploited. And that is something I want to avoid at all costs. A remote sid=
e should never ever be able to trigger an assert.
>

Sounds good, leaving them in for now. In the future we can explore
adding macros such as DASSERT or DCHECK that are defined as "assert"
if compiled with --debug and nothing otherwise.


-Arman

[PATCH BlueZ v1 5/7] tools/btgatt-client: Add the "register-notify" command.

[Arman Uguray]

This patch adds the "register-notify" command, which registers a
notification/indication handler for a given characteristic value handle.
---
 tools/btgatt-client.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
index 8bde1ee..f95cc9c 100644
--- a/tools/btgatt-client.c
+++ b/tools/btgatt-client.c
@@ -702,6 +702,74 @@ static void cmd_write_long_value(struct client *cli, char *cmd_str)
 	free(value);
 }
 
+static void register_notify_usage(void)
+{
+	printf("Usage: register-notify <chrc value handle>\n");
+}
+
+static void notify_cb(uint16_t value_handle, const uint8_t *value,
+					uint16_t length, void *user_data)
+{
+	int i;
+
+	printf("\n\tHandle Value Not/Ind: 0x%04x - ", value_handle);
+
+	if (length == 0) {
+		PRLOG("(0 bytes)\n");
+		return;
+	}
+
+	printf("(%u bytes): ", length);
+
+	for (i = 0; i < length; i++)
+		printf("%02x ", value[i]);
+
+	PRLOG("\n");
+}
+
+static void register_notify_cb(unsigned int id, uint16_t att_ecode,
+								void *user_data)
+{
+	if (!id) {
+		PRLOG("Failed to register notify handler "
+					"- error code: 9x%02x\n", att_ecode);
+		return;
+	}
+
+	PRLOG("Registered notify handler with id: %u\n", id);
+}
+
+static void cmd_register_notify(struct client *cli, char *cmd_str)
+{
+	char *argv[2];
+	int argc = 0;
+	uint16_t value_handle;
+	char *endptr = NULL;
+
+	if (!bt_gatt_client_is_ready(cli->gatt)) {
+		printf("GATT client not initialized\n");
+		return;
+	}
+
+	if (!parse_args(cmd_str, 1, argv, &argc) || argc != 1) {
+		register_notify_usage();
+		return;
+	}
+
+	value_handle = strtol(argv[0], &endptr, 16);
+	if (!endptr || *endptr != '\0' || !value_handle) {
+		printf("Invalid value handle: %s\n", argv[0]);
+		return;
+	}
+
+	if (!bt_gatt_client_register_notify(cli->gatt, value_handle,
+							register_notify_cb,
+							notify_cb, NULL, NULL))
+		printf("Failed to register notify handler\n");
+
+	printf("\n");
+}
+
 static void cmd_help(struct client *cli, char *cmd_str);
 
 typedef void (*command_func_t)(struct client *cli, char *cmd_str);
@@ -721,6 +789,8 @@ static struct {
 			"\tWrite a characteristic or descriptor value" },
 	{ "write-long-value", cmd_write_long_value,
 			"Write long characteristic or descriptor value" },
+	{ "register-notify", cmd_register_notify,
+			"\tSubscribe to not/ind from a characteristic" },
 	{ }
 };
 
-- 
2.1.0.rc2.206.gedb03e5

[PATCH BlueZ v1 6/7] tools/btgatt-client: Add "unregister-notify" command.

[Arman Uguray]

This patch adds the "unregister-notify" command which can be used to test the
bt_gatt_client_unregister_notify function.
---
 tools/btgatt-client.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
index f95cc9c..d2d997e 100644
--- a/tools/btgatt-client.c
+++ b/tools/btgatt-client.c
@@ -770,6 +770,42 @@ static void cmd_register_notify(struct client *cli, char *cmd_str)
 	printf("\n");
 }
 
+static void unregister_notify_usage(void)
+{
+	printf("Usage: unregister-notify <notify id>\n");
+}
+
+static void cmd_unregister_notify(struct client *cli, char *cmd_str)
+{
+	char *argv[2];
+	int argc = 0;
+	unsigned int id;
+	char *endptr = NULL;
+
+	if (!bt_gatt_client_is_ready(cli->gatt)) {
+		printf("GATT client not initialized\n");
+		return;
+	}
+
+	if (!parse_args(cmd_str, 1, argv, &argc) || argc != 1) {
+		unregister_notify_usage();
+		return;
+	}
+
+	id = strtol(argv[0], &endptr, 10);
+	if (!endptr || *endptr != '\0' || !id) {
+		printf("Invalid notify id: %s\n", argv[0]);
+		return;
+	}
+
+	if (!bt_gatt_client_unregister_notify(cli->gatt, id)) {
+		printf("Failed to unregister notify handler with id: %u\n", id);
+		return;
+	}
+
+	printf("Unregistered notify handler with id: %u\n", id);
+}
+
 static void cmd_help(struct client *cli, char *cmd_str);
 
 typedef void (*command_func_t)(struct client *cli, char *cmd_str);
@@ -791,6 +827,8 @@ static struct {
 			"Write long characteristic or descriptor value" },
 	{ "register-notify", cmd_register_notify,
 			"\tSubscribe to not/ind from a characteristic" },
+	{ "unregister-notify", cmd_unregister_notify,
+						"Unregister a not/ind session"},
 	{ }
 };
 
-- 
2.1.0.rc2.206.gedb03e5

[PATCH BlueZ v1 7/7] TODO: Reference counted notify functions implemented

[Arman Uguray]

---
 TODO | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/TODO b/TODO
index 5277e39..3530de4 100644
--- a/TODO
+++ b/TODO
@@ -152,16 +152,6 @@ ATT/GATT (new shared stack)
   Priority: Medium
   Complexity: C1
 
-- Define API functions in shared/gatt-client for enabling/disabling
-  characteristic handle-value notifications/indications on a reference counted
-  basis. Code that is no longer interested in notifications from a
-  characteristic should be able to unregister any callbacks without writing to
-  the "Client Characteristic Configuration" descriptor if there are others who
-  are still interested.
-
-  Priority: Medium
-  Complexity: C2
-
 - Introduce a handler interface to shared/gatt-client which can be used by the
   upper layer to determine when the link has been disconnected or an ATT
   protocol request times out.
-- 
2.1.0.rc2.206.gedb03e5