[PATCHv2 01/11] android/gatt: Add helper for getting device by conn_id

[PATCHv2 01/11] android/gatt: Add helper for getting device by conn_id

[Marcin Kraglak]

It will look for cgatt_device by conn_id.
---
 android/gatt.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 81bfdc0..7a3db27 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -713,6 +713,17 @@ static struct gatt_device *find_device(bdaddr_t *addr)
 	return NULL;
 }
 
+static struct gatt_device *find_device_by_conn_id(int32_t conn_id)
+{
+	struct gatt_device *dev;
+
+	dev = queue_find(conn_list, match_dev_by_conn_id, INT_TO_PTR(conn_id));
+	if (dev)
+		return dev;
+
+	return NULL;
+}
+
 static void handle_client_connect(const void *buf, uint16_t len)
 {
 	const struct hal_cmd_gatt_client_connect *cmd = buf;
@@ -850,8 +861,7 @@ static void handle_client_disconnect(const void *buf, uint16_t len)
 
 	ba2str((bdaddr_t *)&cmd->bdaddr, addr);
 
-	dev = queue_find(conn_list, match_dev_by_conn_id,
-						INT_TO_PTR(cmd->conn_id));
+	dev = find_device_by_conn_id(cmd->conn_id);
 	if (!dev) {
 		error("gatt: dev %s with conn_id=%d not found",
 							addr, cmd->conn_id);
@@ -916,8 +926,7 @@ static void handle_client_search_service(const void *buf, uint16_t len)
 
 	DBG("");
 
-	dev = queue_find(conn_list, match_dev_by_conn_id,
-						INT_TO_PTR(cmd->conn_id));
+	dev = find_device_by_conn_id(cmd->conn_id);
 	if (!dev) {
 		error("gatt: dev with conn_id=%d not found", cmd->conn_id);
 		status = HAL_STATUS_FAILED;
@@ -1040,8 +1049,7 @@ static bool find_service(int32_t conn_id, struct element_id *service_id,
 	struct gatt_device *device;
 	struct service *service;
 
-	device = queue_find(conn_list, match_dev_by_conn_id,
-							INT_TO_PTR(conn_id));
+	device = find_device_by_conn_id(conn_id);
 	if (!device) {
 		error("gatt: conn_id=%d not found", conn_id);
 		return false;
-- 
1.8.3.1

[PATCHv2 02/11] android/gatt: Add helper to get client by id

[Marcin Kraglak]

It will look for gatt_client by client_id.
---
 android/gatt.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 7a3db27..1af7e33 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -148,6 +148,11 @@ static bool match_client_by_id(const void *data, const void *user_data)
 	return client->id == exp_id;
 }
 
+static struct gatt_client *find_client_by_id(int32_t id)
+{
+	return queue_find(gatt_clients, match_client_by_id, INT_TO_PTR(id));
+}
+
 static bool match_by_value(const void *data, const void *user_data)
 {
 	return data == user_data;
@@ -628,8 +633,7 @@ static void handle_client_scan(const void *buf, uint16_t len)
 
 	DBG("new state %d", cmd->start);
 
-	registered = queue_find(gatt_clients, match_client_by_id,
-						INT_TO_PTR(cmd->client_if));
+	registered = find_client_by_id(cmd->client_if);
 	if (!registered) {
 		error("gatt: Client not registered");
 		status = HAL_STATUS_FAILED;
@@ -736,8 +740,7 @@ static void handle_client_connect(const void *buf, uint16_t len)
 	DBG("");
 
 	/* Check if client is registered */
-	l = queue_find(gatt_clients, match_client_by_id,
-						INT_TO_PTR(cmd->client_if));
+	l = find_client_by_id(cmd->client_if);
 	if (!l) {
 		error("gatt: Client id %d not found", cmd->client_if);
 		status = HAL_STATUS_FAILED;
-- 
1.8.3.1

Re: [PATCHv2 02/11] android/gatt: Add helper to get client by id

[Szymon Janc]

Hi Marcin,

On Thursday 27 March 2014 22:13:00 Marcin Kraglak wrote:
> It will look for gatt_client by client_id.
> ---
>  android/gatt.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 7a3db27..1af7e33 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -148,6 +148,11 @@ static bool match_client_by_id(const void *data, const
> void *user_data) return client->id == exp_id;
>  }
> 
> +static struct gatt_client *find_client_by_id(int32_t id)
> +{
> +	return queue_find(gatt_clients, match_client_by_id, INT_TO_PTR(id));
> +}
> +
>  static bool match_by_value(const void *data, const void *user_data)
>  {
>  	return data == user_data;
> @@ -628,8 +633,7 @@ static void handle_client_scan(const void *buf, uint16_t
> len)
> 
>  	DBG("new state %d", cmd->start);
> 
> -	registered = queue_find(gatt_clients, match_client_by_id,
> -						INT_TO_PTR(cmd->client_if));
> +	registered = find_client_by_id(cmd->client_if);
>  	if (!registered) {
>  		error("gatt: Client not registered");
>  		status = HAL_STATUS_FAILED;
> @@ -736,8 +740,7 @@ static void handle_client_connect(const void *buf,
> uint16_t len) DBG("");
> 
>  	/* Check if client is registered */
> -	l = queue_find(gatt_clients, match_client_by_id,
> -						INT_TO_PTR(cmd->client_if));
> +	l = find_client_by_id(cmd->client_if);
>  	if (!l) {
>  		error("gatt: Client id %d not found", cmd->client_if);
>  		status = HAL_STATUS_FAILED;

This patch is now applied, thanks.

-- 
Szymon K. Janc
szymon.janc@gmail.com

[PATCHv2 03/11] android/gatt: Service error when client cannot be created

[Marcin Kraglak]

If memory allocation failed, return error from handle_client_register.
---
 android/gatt.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/android/gatt.c b/android/gatt.c
index 1af7e33..2e93ef7 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -239,6 +239,11 @@ static void handle_client_register(const void *buf, uint16_t len)
 	}
 
 	client = new0(struct gatt_client, 1);
+	if (!client) {
+		error("gatt: cannot allocate memory for registering client");
+		status = HAL_STATUS_FAILED;
+		goto failed;
+	}
 
 	memcpy(client->uuid, cmd->uuid, sizeof(client->uuid));
 
-- 
1.8.3.1

Re: [PATCHv2 03/11] android/gatt: Service error when client cannot be created

[Szymon Janc]

Hi Marcin,

On Thursday 27 March 2014 22:13:01 Marcin Kraglak wrote:
> If memory allocation failed, return error from handle_client_register.
> ---
>  android/gatt.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 1af7e33..2e93ef7 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -239,6 +239,11 @@ static void handle_client_register(const void *buf,
> uint16_t len) }
> 
>  	client = new0(struct gatt_client, 1);
> +	if (!client) {
> +		error("gatt: cannot allocate memory for registering client");
> +		status = HAL_STATUS_FAILED;
> +		goto failed;
> +	}
> 
>  	memcpy(client->uuid, cmd->uuid, sizeof(client->uuid));


This patch is now pushed, thanks.

-- 
Szymon K. Janc
szymon.janc@gmail.com

[PATCHv2 04/11] android/gatt: Add initial implementation of get_included_service

[Marcin Kraglak]

It will look for included service in range passed in command.
It call gatt_find_included(), but without result callback now,
as it is initial version.
---
 android/gatt.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/android/gatt.c b/android/gatt.c
index 2e93ef7..bf40162 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -955,13 +955,70 @@ reply:
 			HAL_OP_GATT_CLIENT_SEARCH_SERVICE, status);
 }
 
+static bool match_service_by_uuid(const void *data, const void *user_data)
+{
+	const struct service *service = data;
+	const bt_uuid_t *uuid = user_data;
+	bt_uuid_t service_uuid;
+
+	if (bt_string_to_uuid(&service_uuid, service->primary.uuid) < 0)
+		return false;
+
+	return !bt_uuid_cmp(uuid, &service_uuid);
+}
+
+static struct service *find_service_bu_uuid(struct gatt_device *device,
+								bt_uuid_t *uuid)
+{
+	return queue_find(device->services, match_service_by_uuid, uuid);
+}
+
 static void handle_client_get_included_service(const void *buf, uint16_t len)
 {
+	const struct hal_cmd_gatt_client_get_included_service *cmd = buf;
+	struct gatt_device *device;
+	struct service *service;
+	uint8_t status;
+
 	DBG("");
 
+	device = find_device_by_conn_id(cmd->conn_id);
+	if (!device) {
+		error("gatt: device with conn_id %d not found", cmd->conn_id);
+		status = HAL_STATUS_FAILED;
+		goto failed;
+	}
+
+	if (queue_isempty(device->services)) {
+		error("gatt: Search primary services first");
+		status = HAL_STATUS_FAILED;
+		goto failed;
+	}
+
+	if (!cmd->number) {
+		service = queue_peek_head(device->services);
+	} else {
+		bt_uuid_t uuid;
+
+		android2uuid(cmd->srvc_id->uuid, &uuid);
+		service = find_service_bu_uuid(device, &uuid);
+	}
+
+	if (!service) {
+		error("gatt: service not found");
+		status = HAL_STATUS_FAILED;
+		goto failed;
+	}
+
+	gatt_find_included(device->attrib, service->primary.range.start,
+					service->primary.range.end, NULL, NULL);
+
+	status = HAL_STATUS_SUCCESS;
+
+failed:
 	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
 					HAL_OP_GATT_CLIENT_GET_INCLUDED_SERVICE,
-					HAL_STATUS_FAILED);
+					status);
 }
 
 static void send_client_char_notify(const struct characteristic *ch,
-- 
1.8.3.1

Re: [PATCHv2 04/11] android/gatt: Add initial implementation of get_included_service

[Szymon Janc]

Hi Marcin,

On Thursday 27 March 2014 22:13:02 Marcin Kraglak wrote:
> It will look for included service in range passed in command.
> It call gatt_find_included(), but without result callback now,
> as it is initial version.
> ---
>  android/gatt.c | 59
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed,
> 58 insertions(+), 1 deletion(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 2e93ef7..bf40162 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -955,13 +955,70 @@ reply:
>  			HAL_OP_GATT_CLIENT_SEARCH_SERVICE, status);
>  }
> 
> +static bool match_service_by_uuid(const void *data, const void *user_data)
> +{
> +	const struct service *service = data;
> +	const bt_uuid_t *uuid = user_data;
> +	bt_uuid_t service_uuid;
> +
> +	if (bt_string_to_uuid(&service_uuid, service->primary.uuid) < 0)
> +		return false;
> +
> +	return !bt_uuid_cmp(uuid, &service_uuid);
> +}
> +
> +static struct service *find_service_bu_uuid(struct gatt_device *device,
> +								bt_uuid_t *uuid)
> +{
> +	return queue_find(device->services, match_service_by_uuid, uuid);
> +}
> +
>  static void handle_client_get_included_service(const void *buf, uint16_t
> len) {
> +	const struct hal_cmd_gatt_client_get_included_service *cmd = buf;
> +	struct gatt_device *device;
> +	struct service *service;
> +	uint8_t status;
> +
>  	DBG("");
> 
> +	device = find_device_by_conn_id(cmd->conn_id);
> +	if (!device) {
> +		error("gatt: device with conn_id %d not found", cmd->conn_id);

We don't need to log error for every failed condition while handling commands.
Returning error code is enough for this. Logging error is fine for internal 
errors (like memory allocation or connect failure etc.).

> +		status = HAL_STATUS_FAILED;
> +		goto failed;
> +	}
> +
> +	if (queue_isempty(device->services)) {
> +		error("gatt: Search primary services first");

ditto.

> +		status = HAL_STATUS_FAILED;
> +		goto failed;
> +	}
> +
> +	if (!cmd->number) {
> +		service = queue_peek_head(device->services);
> +	} else {
> +		bt_uuid_t uuid;
> +
> +		android2uuid(cmd->srvc_id->uuid, &uuid);
> +		service = find_service_bu_uuid(device, &uuid);
> +	}
> +
> +	if (!service) {
> +		error("gatt: service not found");

ditto.

> +		status = HAL_STATUS_FAILED;
> +		goto failed;
> +	}
> +
> +	gatt_find_included(device->attrib, service->primary.range.start,
> +					service->primary.range.end, NULL, NULL);
> +
> +	status = HAL_STATUS_SUCCESS;
> +
> +failed:
>  	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
>  					HAL_OP_GATT_CLIENT_GET_INCLUDED_SERVICE,
> -					HAL_STATUS_FAILED);
> +					status);
>  }
> 
>  static void send_client_char_notify(const struct characteristic *ch,

-- 
Szymon K. Janc
szymon.janc@gmail.com

[PATCHv2 05/11] android/gatt: Add get_included_data struct and callback

[Marcin Kraglak]

Create user data passed to callback get_included_cb.
It will handle included services and pass it to notification.
---
 android/gatt.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/android/gatt.c b/android/gatt.c
index bf40162..7eb867b 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -973,9 +973,23 @@ static struct service *find_service_bu_uuid(struct gatt_device *device,
 	return queue_find(device->services, match_service_by_uuid, uuid);
 }
 
+struct get_included_data {
+	struct service *service;
+	struct gatt_device *device;
+};
+
+static void get_included_cb(uint8_t status, GSList *included, void *user_data)
+{
+	struct get_included_data *data = user_data;
+
+	/* TODO pass included services to notification */
+	free(data);
+}
+
 static void handle_client_get_included_service(const void *buf, uint16_t len)
 {
 	const struct hal_cmd_gatt_client_get_included_service *cmd = buf;
+	struct get_included_data *data;
 	struct gatt_device *device;
 	struct service *service;
 	uint8_t status;
@@ -1010,8 +1024,19 @@ static void handle_client_get_included_service(const void *buf, uint16_t len)
 		goto failed;
 	}
 
+	data = new0(struct get_included_data, 1);
+	if (!data) {
+		error("gatt: failed to allocate memory for included_data");
+		status = HAL_STATUS_FAILED;
+		goto failed;
+	}
+
+	data->service = service;
+	data->device = device;
+
 	gatt_find_included(device->attrib, service->primary.range.start,
-					service->primary.range.end, NULL, NULL);
+				service->primary.range.end, get_included_cb,
+				data);
 
 	status = HAL_STATUS_SUCCESS;
 
-- 
1.8.3.1

[PATCHv2 06/11] android/gatt: Add implementation of get_included_cb

[Marcin Kraglak]

It will pass found services to upper layer with notification.
Caching included services should be done later.
---
 android/gatt.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/android/gatt.c b/android/gatt.c
index 7eb867b..e35b099 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -980,10 +980,53 @@ struct get_included_data {
 
 static void get_included_cb(uint8_t status, GSList *included, void *user_data)
 {
+	struct hal_ev_gatt_client_get_inc_service ev;
 	struct get_included_data *data = user_data;
+	struct gatt_device *device = data->device;
+	struct service *service = data->service;
+	GSList *list = included;
+	bt_uuid_t uuid;
+
+	DBG("");
 
-	/* TODO pass included services to notification */
 	free(data);
+
+	if (status) {
+		error("gatt: no included services found");
+		return;
+	}
+
+	bt_string_to_uuid(&uuid, service->primary.uuid);
+
+	/* TODO store included services in device->services list */
+	for (; list; list = list->next) {
+		struct gatt_included *included = list->data;
+		bt_uuid_t inc_uuid;
+
+		ev.conn_id = device->conn_id;
+		ev.status = HAL_STATUS_SUCCESS;
+
+		ev.srvc_id.inst_id = 0;
+		uuid2android(&uuid, ev.srvc_id.uuid);
+
+		ev.incl_srvc_id.inst_id = 0;
+		bt_string_to_uuid(&inc_uuid, included->uuid);
+		uuid2android(&uuid, ev.incl_srvc_id.uuid);
+
+		ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT ,
+					HAL_EV_GATT_CLIENT_GET_INC_SERVICE,
+					sizeof(ev), &ev);
+	}
+
+	/* Android expects notification with error status in the end */
+	ev.conn_id = device->conn_id;
+	ev.status = HAL_STATUS_FAILED;
+	ev.srvc_id.inst_id = 0;
+	uuid2android(&uuid, ev.srvc_id.uuid);
+
+	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT ,
+					HAL_EV_GATT_CLIENT_GET_INC_SERVICE,
+					sizeof(ev), &ev);
 }
 
 static void handle_client_get_included_service(const void *buf, uint16_t len)
-- 
1.8.3.1

Re: [PATCHv2 06/11] android/gatt: Add implementation of get_included_cb

[Szymon Janc]

Hi Marcin,

On Thursday 27 March 2014 22:13:04 Marcin Kraglak wrote:
> It will pass found services to upper layer with notification.
> Caching included services should be done later.
> ---
>  android/gatt.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 7eb867b..e35b099 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -980,10 +980,53 @@ struct get_included_data {
> 
>  static void get_included_cb(uint8_t status, GSList *included, void
> *user_data) {
> +	struct hal_ev_gatt_client_get_inc_service ev;
>  	struct get_included_data *data = user_data;
> +	struct gatt_device *device = data->device;
> +	struct service *service = data->service;
> +	GSList *list = included;

This is not needed, just use included (it is already a copy).

> +	bt_uuid_t uuid;
> +
> +	DBG("");
> 
> -	/* TODO pass included services to notification */
>  	free(data);
> +
> +	if (status) {
> +		error("gatt: no included services found");
> +		return;
> +	}
> +
> +	bt_string_to_uuid(&uuid, service->primary.uuid);
> +
> +	/* TODO store included services in device->services list */
> +	for (; list; list = list->next) {
> +		struct gatt_included *included = list->data;

You are shadowing variable here, please don't do that.

> +		bt_uuid_t inc_uuid;
> +
> +		ev.conn_id = device->conn_id;
> +		ev.status = HAL_STATUS_SUCCESS;
> +
> +		ev.srvc_id.inst_id = 0;
> +		uuid2android(&uuid, ev.srvc_id.uuid);
> +
> +		ev.incl_srvc_id.inst_id = 0;
> +		bt_string_to_uuid(&inc_uuid, included->uuid);
> +		uuid2android(&uuid, ev.incl_srvc_id.uuid);
> +
> +		ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT ,
> +					HAL_EV_GATT_CLIENT_GET_INC_SERVICE,
> +					sizeof(ev), &ev);
> +	}
> +
> +	/* Android expects notification with error status in the end */
> +	ev.conn_id = device->conn_id;
> +	ev.status = HAL_STATUS_FAILED;
> +	ev.srvc_id.inst_id = 0;
> +	uuid2android(&uuid, ev.srvc_id.uuid);
> +
> +	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT ,
> +					HAL_EV_GATT_CLIENT_GET_INC_SERVICE,
> +					sizeof(ev), &ev);
>  }
> 
>  static void handle_client_get_included_service(const void *buf, uint16_t
> len)

-- 
Szymon K. Janc
szymon.janc@gmail.com

[PATCHv2 07/11] android/gatt: Add notifications list to gatt_client

[Marcin Kraglak]

It will be used for storing informations about registered handlers.
These handlers will be registered for both ATT_OP_HANDLE_NOTIFY
and ATT_OP_HANDLE_IND. Added also new function to destroy gatt_client.
---
 android/gatt.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/android/gatt.c b/android/gatt.c
index e35b099..2cd0731 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -51,6 +51,7 @@
 struct gatt_client {
 	int32_t id;
 	uint8_t uuid[16];
+	struct queue *notifications;
 };
 
 struct element_id {
@@ -70,6 +71,16 @@ struct service {
 	struct queue *chars;
 };
 
+struct notification_data {
+	struct hal_gatt_srvc_id service;
+	struct hal_gatt_gatt_id ch;
+	struct gatt_client *client;
+	struct gatt_device *dev;
+	guint notif_id;
+	guint ind_id;
+	int ref;
+};
+
 struct gatt_device {
 	bdaddr_t bdaddr;
 	uint8_t bdaddr_type;
@@ -216,6 +227,16 @@ static void destroy_device(void *data)
 	free(dev);
 }
 
+static void destroy_gatt_client(void *data)
+{
+	struct gatt_client *client = data;
+
+	if (client->notifications)
+		queue_destroy(client->notifications, free);
+
+	free(client);
+}
+
 static void handle_client_register(const void *buf, uint16_t len)
 {
 	const struct hal_cmd_gatt_client_register *cmd = buf;
@@ -247,6 +268,14 @@ static void handle_client_register(const void *buf, uint16_t len)
 
 	memcpy(client->uuid, cmd->uuid, sizeof(client->uuid));
 
+	client->notifications = queue_new();
+	if (!client->notifications) {
+		error("gatt: couldn't allocate notifications queue");
+		destroy_gatt_client(client);
+		status = HAL_STATUS_FAILED;
+		goto failed;
+	}
+
 	client->id = client_cnt++;
 
 	queue_push_head(gatt_clients, client);
@@ -281,7 +310,7 @@ static void handle_client_unregister(const void *buf, uint16_t len)
 		goto failed;
 	}
 
-	free(cl);
+	destroy_gatt_client(cl);
 	status = HAL_STATUS_SUCCESS;
 
 failed:
-- 
1.8.3.1

Re: [PATCHv2 07/11] android/gatt: Add notifications list to gatt_client

[Szymon Janc]

Hi Marcin,

On Thursday 27 March 2014 22:13:05 Marcin Kraglak wrote:
> It will be used for storing informations about registered handlers.
> These handlers will be registered for both ATT_OP_HANDLE_NOTIFY
> and ATT_OP_HANDLE_IND. Added also new function to destroy gatt_client.
> ---
>  android/gatt.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index e35b099..2cd0731 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -51,6 +51,7 @@
>  struct gatt_client {
>  	int32_t id;
>  	uint8_t uuid[16];
> +	struct queue *notifications;
>  };
> 
>  struct element_id {
> @@ -70,6 +71,16 @@ struct service {
>  	struct queue *chars;
>  };
> 
> +struct notification_data {
> +	struct hal_gatt_srvc_id service;
> +	struct hal_gatt_gatt_id ch;
> +	struct gatt_client *client;
> +	struct gatt_device *dev;
> +	guint notif_id;
> +	guint ind_id;
> +	int ref;

I'd prefer if you add new members for this in patch that actually make use of 
it. It makes easier to judge if member is needed.

> +};
> +
>  struct gatt_device {
>  	bdaddr_t bdaddr;
>  	uint8_t bdaddr_type;
> @@ -216,6 +227,16 @@ static void destroy_device(void *data)
>  	free(dev);
>  }
> 
> +static void destroy_gatt_client(void *data)
> +{
> +	struct gatt_client *client = data;
> +
> +	if (client->notifications)
> +		queue_destroy(client->notifications, free);

No need to check if this pointer is not NULL. queue_destroy does it already.

> +
> +	free(client);
> +}
> +
>  static void handle_client_register(const void *buf, uint16_t len)
>  {
>  	const struct hal_cmd_gatt_client_register *cmd = buf;
> @@ -247,6 +268,14 @@ static void handle_client_register(const void *buf,
> uint16_t len)
> 
>  	memcpy(client->uuid, cmd->uuid, sizeof(client->uuid));
> 
> +	client->notifications = queue_new();
> +	if (!client->notifications) {
> +		error("gatt: couldn't allocate notifications queue");
> +		destroy_gatt_client(client);
> +		status = HAL_STATUS_FAILED;
> +		goto failed;
> +	}
> +
>  	client->id = client_cnt++;
> 
>  	queue_push_head(gatt_clients, client);
> @@ -281,7 +310,7 @@ static void handle_client_unregister(const void *buf,
> uint16_t len) goto failed;
>  	}
> 
> -	free(cl);
> +	destroy_gatt_client(cl);
>  	status = HAL_STATUS_SUCCESS;
> 
>  failed:

-- 
Szymon K. Janc
szymon.janc@gmail.com

[PATCHv2 08/11] android/gatt: Add initial implementation of register_for_notification

[Marcin Kraglak]

It will register notification and indication handler for given parameters.
Proper callback should be send with status of registration.
---
 android/gatt.c | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 157 insertions(+), 2 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 2cd0731..0a7bca0 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -215,6 +215,54 @@ static bool match_char_by_higher_inst_id(const void *data,
 	return inst_id < ch->id.instance;
 }
 
+static bool match_char_by_instance(const void *data, const void *user_data)
+{
+	const struct characteristic *ch = data;
+	uint8_t inst_id = PTR_TO_INT(user_data);
+
+	return inst_id == ch->id.instance;
+}
+
+static bool match_notification(const void *a, const void *b)
+{
+	const struct notification_data *a1 = a;
+	const struct notification_data *b1 = b;
+
+	if (bacmp(&a1->dev->bdaddr, &b1->dev->bdaddr))
+		return false;
+	if (memcmp(&a1->ch, &b1->ch, sizeof(a1->ch)))
+		return false;
+	if (memcmp(&a1->service, &b1->service, sizeof(a1->service)))
+		return false;
+
+	return true;
+}
+
+static void destroy_notification(void *data)
+{
+	struct notification_data *notification = data;
+
+	if (--notification->ref)
+		return;
+
+	queue_remove_if(notification->client->notifications, match_notification,
+								notification);
+	free(notification);
+}
+
+static void unregister_notification(void *data)
+{
+	struct notification_data *notification = data;
+
+	if (notification->notif_id)
+		g_attrib_unregister(notification->dev->attrib,
+							notification->notif_id);
+
+	if (notification->ind_id)
+		g_attrib_unregister(notification->dev->attrib,
+							notification->ind_id);
+}
+
 static void destroy_device(void *data)
 {
 	struct gatt_device *dev = data;
@@ -231,8 +279,16 @@ static void destroy_gatt_client(void *data)
 {
 	struct gatt_client *client = data;
 
-	if (client->notifications)
+	if (client->notifications) {
+		while (queue_peek_head(client->notifications)) {
+			struct notification_data *notification;
+
+			notification = queue_pop_head(client->notifications);
+			unregister_notification(notification);
+		}
+
 		queue_destroy(client->notifications, free);
+	}
 
 	free(client);
 }
@@ -1350,11 +1406,110 @@ static void handle_client_execute_write(const void *buf, uint16_t len)
 static void handle_client_register_for_notification(const void *buf,
 								uint16_t len)
 {
+	const struct hal_cmd_gatt_client_register_for_notification *cmd = buf;
+	struct notification_data *notification;
+	char uuid[MAX_LEN_UUID_STR];
+	struct gatt_client *client;
+	struct characteristic *c;
+	struct element_id match_id;
+	struct gatt_device *dev;
+	struct service *service;
+	char addr_str[18];
+	uint8_t status;
+	bdaddr_t addr;
+
 	DBG("");
 
+	client = find_client_by_id(cmd->client_if);
+	if (!client) {
+		error("gatt: client %d not registered", cmd->client_if);
+		status = HAL_STATUS_FAILED;
+		goto failed;
+	}
+
+	android2bdaddr((bdaddr_t *)&cmd->bdaddr, &addr);
+	ba2str(&addr, addr_str);
+
+	dev = queue_find(conn_list, match_dev_by_bdaddr, &addr);
+	if (!dev) {
+		error("gatt: device %s not found in connected devices list",
+								addr_str);
+		status = HAL_STATUS_FAILED;
+		goto failed;
+	}
+
+	hal_srvc_id_to_element_id(&cmd->srvc_id, &match_id);
+	service = queue_find(dev->services, match_srvc_by_element_id,
+								&match_id);
+	bt_uuid_to_string(&match_id.uuid, uuid, MAX_LEN_UUID_STR);
+	if (!service) {
+		error("gatt: can't register notification, service not found");
+		status = HAL_STATUS_FAILED;
+		goto failed;
+	}
+
+	c = queue_find(service->chars, match_char_by_instance,
+					INT_TO_PTR(cmd->char_id.inst_id));
+	if (!c) {
+		error("gatt: can't register notification: no characteristic");
+		status = HAL_STATUS_FAILED;
+		goto failed;
+	}
+
+	notification = new0(struct notification_data, 1);
+	if (!notification) {
+		error("gatt: failed to allocate memory for notification");
+		status = HAL_STATUS_FAILED;
+		goto failed;
+	}
+
+	memcpy(&notification->ch, &cmd->char_id, sizeof(notification->ch));
+	memcpy(&notification->service, &cmd->srvc_id,
+						sizeof(notification->service));
+	notification->dev = dev;
+	notification->client = client;
+
+	if (queue_find(client->notifications, match_notification,
+			notification)) {
+		DBG("can't register for notification, already registered");
+		free(notification);
+		status = HAL_STATUS_SUCCESS;
+		goto failed;
+	}
+
+	notification->notif_id = g_attrib_register(dev->attrib,
+							ATT_OP_HANDLE_NOTIFY,
+							c->ch.value_handle,
+							NULL, notification,
+							destroy_notification);
+	if (!notification->notif_id) {
+		free(notification);
+		status = HAL_STATUS_FAILED;
+		goto failed;
+	}
+
+	notification->ind_id = g_attrib_register(dev->attrib, ATT_OP_HANDLE_IND,
+							c->ch.value_handle,
+							NULL, notification,
+							destroy_notification);
+	if (!notification->ind_id) {
+		g_attrib_unregister(dev->attrib, notification->notif_id);
+		free(notification);
+		status = HAL_STATUS_FAILED;
+		goto failed;
+	}
+
+	notification->ref = 2;
+
+	queue_push_tail(client->notifications, notification);
+
+	status = HAL_STATUS_SUCCESS;
+
+failed:
+	/* TODO: send callback with notification enabled/disabled */
 	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
 				HAL_OP_GATT_CLIENT_REGISTER_FOR_NOTIFICATION,
-				HAL_STATUS_FAILED);
+				status);
 }
 
 static void handle_client_deregister_for_notification(const void *buf,
-- 
1.8.3.1

Re: [PATCHv2 08/11] android/gatt: Add initial implementation of register_for_notification

[Szymon Janc]

On Thursday 27 March 2014 22:13:06 Marcin Kraglak wrote:
> It will register notification and indication handler for given parameters.
> Proper callback should be send with status of registration.
> ---
>  android/gatt.c | 159
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed,
> 157 insertions(+), 2 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 2cd0731..0a7bca0 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -215,6 +215,54 @@ static bool match_char_by_higher_inst_id(const void
> *data, return inst_id < ch->id.instance;
>  }
> 
> +static bool match_char_by_instance(const void *data, const void *user_data)
> +{
> +	const struct characteristic *ch = data;
> +	uint8_t inst_id = PTR_TO_INT(user_data);
> +
> +	return inst_id == ch->id.instance;
> +}
> +
> +static bool match_notification(const void *a, const void *b)
> +{
> +	const struct notification_data *a1 = a;
> +	const struct notification_data *b1 = b;
> +
> +	if (bacmp(&a1->dev->bdaddr, &b1->dev->bdaddr))
> +		return false;
> +	if (memcmp(&a1->ch, &b1->ch, sizeof(a1->ch)))
> +		return false;
> +	if (memcmp(&a1->service, &b1->service, sizeof(a1->service)))
> +		return false;
> +
> +	return true;
> +}
> +
> +static void destroy_notification(void *data)
> +{
> +	struct notification_data *notification = data;
> +
> +	if (--notification->ref)
> +		return;
> +
> +	queue_remove_if(notification->client->notifications, match_notification,
> +								notification);
> +	free(notification);
> +}
> +
> +static void unregister_notification(void *data)
> +{
> +	struct notification_data *notification = data;
> +
> +	if (notification->notif_id)
> +		g_attrib_unregister(notification->dev->attrib,
> +							notification->notif_id);
> +
> +	if (notification->ind_id)
> +		g_attrib_unregister(notification->dev->attrib,
> +							notification->ind_id);
> +}
> +
>  static void destroy_device(void *data)
>  {
>  	struct gatt_device *dev = data;
> @@ -231,8 +279,16 @@ static void destroy_gatt_client(void *data)
>  {
>  	struct gatt_client *client = data;
> 
> -	if (client->notifications)
> +	if (client->notifications) {
> +		while (queue_peek_head(client->notifications)) {
> +			struct notification_data *notification;
> +
> +			notification = queue_pop_head(client->notifications);
> +			unregister_notification(notification);
> +		}
> +
>  		queue_destroy(client->notifications, free);
> +	}
> 
>  	free(client);
>  }
> @@ -1350,11 +1406,110 @@ static void handle_client_execute_write(const void
> *buf, uint16_t len) static void
> handle_client_register_for_notification(const void *buf, uint16_t len)
>  {
> +	const struct hal_cmd_gatt_client_register_for_notification *cmd = buf;
> +	struct notification_data *notification;
> +	char uuid[MAX_LEN_UUID_STR];
> +	struct gatt_client *client;
> +	struct characteristic *c;
> +	struct element_id match_id;
> +	struct gatt_device *dev;
> +	struct service *service;
> +	char addr_str[18];
> +	uint8_t status;
> +	bdaddr_t addr;
> +
>  	DBG("");
> 
> +	client = find_client_by_id(cmd->client_if);
> +	if (!client) {
> +		error("gatt: client %d not registered", cmd->client_if);
> +		status = HAL_STATUS_FAILED;
> +		goto failed;
> +	}
> +
> +	android2bdaddr((bdaddr_t *)&cmd->bdaddr, &addr);
> +	ba2str(&addr, addr_str);
> +
> +	dev = queue_find(conn_list, match_dev_by_bdaddr, &addr);
> +	if (!dev) {
> +		error("gatt: device %s not found in connected devices list",
> +								addr_str);
> +		status = HAL_STATUS_FAILED;
> +		goto failed;
> +	}
> +
> +	hal_srvc_id_to_element_id(&cmd->srvc_id, &match_id);
> +	service = queue_find(dev->services, match_srvc_by_element_id,
> +								&match_id);
> +	bt_uuid_to_string(&match_id.uuid, uuid, MAX_LEN_UUID_STR);
> +	if (!service) {
> +		error("gatt: can't register notification, service not found");
> +		status = HAL_STATUS_FAILED;
> +		goto failed;
> +	}
> +
> +	c = queue_find(service->chars, match_char_by_instance,
> +					INT_TO_PTR(cmd->char_id.inst_id));
> +	if (!c) {
> +		error("gatt: can't register notification: no characteristic");
> +		status = HAL_STATUS_FAILED;
> +		goto failed;
> +	}
> +
> +	notification = new0(struct notification_data, 1);
> +	if (!notification) {
> +		error("gatt: failed to allocate memory for notification");
> +		status = HAL_STATUS_FAILED;
> +		goto failed;
> +	}
> +
> +	memcpy(&notification->ch, &cmd->char_id, sizeof(notification->ch));
> +	memcpy(&notification->service, &cmd->srvc_id,
> +						sizeof(notification->service));
> +	notification->dev = dev;
> +	notification->client = client;
> +
> +	if (queue_find(client->notifications, match_notification,
> +			notification)) {
> +		DBG("can't register for notification, already registered");
> +		free(notification);
> +		status = HAL_STATUS_SUCCESS;
> +		goto failed;
> +	}
> +
> +	notification->notif_id = g_attrib_register(dev->attrib,
> +							ATT_OP_HANDLE_NOTIFY,
> +							c->ch.value_handle,
> +							NULL, notification,
> +							destroy_notification);
> +	if (!notification->notif_id) {
> +		free(notification);
> +		status = HAL_STATUS_FAILED;
> +		goto failed;
> +	}
> +
> +	notification->ind_id = g_attrib_register(dev->attrib, ATT_OP_HANDLE_IND,
> +							c->ch.value_handle,
> +							NULL, notification,
> +							destroy_notification);
> +	if (!notification->ind_id) {
> +		g_attrib_unregister(dev->attrib, notification->notif_id);
> +		free(notification);
> +		status = HAL_STATUS_FAILED;
> +		goto failed;
> +	}
> +
> +	notification->ref = 2;

As we discussed before, that needs comment why it is done this way.

> +
> +	queue_push_tail(client->notifications, notification);

This may fail.

> +
> +	status = HAL_STATUS_SUCCESS;
> +
> +failed:
> +	/* TODO: send callback with notification enabled/disabled */
>  	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
>  				HAL_OP_GATT_CLIENT_REGISTER_FOR_NOTIFICATION,
> -				HAL_STATUS_FAILED);
> +				status);
>  }
> 
>  static void handle_client_deregister_for_notification(const void *buf,

-- 
Szymon K. Janc
szymon.janc@gmail.com

[PATCHv2 09/11] android/gatt: Add client_register_for_notif_cb function

[Marcin Kraglak]

It will be called with register/deregister will be called.
It will send status, service id and characteristic id.
---
 android/gatt.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/android/gatt.c b/android/gatt.c
index 0a7bca0..61c35c3 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -1403,6 +1403,23 @@ static void handle_client_execute_write(const void *buf, uint16_t len)
 			HAL_OP_GATT_CLIENT_EXECUTE_WRITE, HAL_STATUS_FAILED);
 }
 
+static void client_register_for_notif_cb(int32_t conn_id, int32_t registered,
+					int32_t status,
+					const struct hal_gatt_srvc_id *srvc,
+					const struct hal_gatt_gatt_id *ch)
+{
+	struct hal_ev_gatt_client_reg_for_notif ev;
+
+	ev.conn_id = conn_id;
+	ev.status = status;
+	ev.registered = registered;
+	memcpy(&ev.srvc_id, srvc, sizeof(ev.srvc_id));
+	memcpy(&ev.char_id, ch, sizeof(ev.char_id));
+
+	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
+			HAL_EV_GATT_CLIENT_REGISTER_FOR_NOTIF, sizeof(ev), &ev);
+}
+
 static void handle_client_register_for_notification(const void *buf,
 								uint16_t len)
 {
@@ -1415,6 +1432,7 @@ static void handle_client_register_for_notification(const void *buf,
 	struct gatt_device *dev;
 	struct service *service;
 	char addr_str[18];
+	int32_t conn_id = 0;
 	uint8_t status;
 	bdaddr_t addr;
 
@@ -1438,6 +1456,8 @@ static void handle_client_register_for_notification(const void *buf,
 		goto failed;
 	}
 
+	conn_id = dev->conn_id;
+
 	hal_srvc_id_to_element_id(&cmd->srvc_id, &match_id);
 	service = queue_find(dev->services, match_srvc_by_element_id,
 								&match_id);
@@ -1506,7 +1526,8 @@ static void handle_client_register_for_notification(const void *buf,
 	status = HAL_STATUS_SUCCESS;
 
 failed:
-	/* TODO: send callback with notification enabled/disabled */
+	client_register_for_notif_cb(conn_id, 1, status, &cmd->srvc_id,
+								&cmd->char_id);
 	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
 				HAL_OP_GATT_CLIENT_REGISTER_FOR_NOTIFICATION,
 				status);
-- 
1.8.3.1

Re: [PATCHv2 09/11] android/gatt: Add client_register_for_notif_cb function

[Szymon Janc]

Hi Marcin,

On Thursday 27 March 2014 22:13:07 Marcin Kraglak wrote:
> It will be called with register/deregister will be called.
> It will send status, service id and characteristic id.
> ---
>  android/gatt.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 0a7bca0..61c35c3 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -1403,6 +1403,23 @@ static void handle_client_execute_write(const void
> *buf, uint16_t len) HAL_OP_GATT_CLIENT_EXECUTE_WRITE, HAL_STATUS_FAILED);
>  }
> 
> +static void client_register_for_notif_cb(int32_t conn_id, int32_t
> registered, +					int32_t status,
> +					const struct hal_gatt_srvc_id *srvc,
> +					const struct hal_gatt_gatt_id *ch)
> +{

This is not used as callback so name it send_client_register_for_notif_ev() or 
something similar (if you came up with shorter name).

You may also open code it if it will be used only from one place.

> +	struct hal_ev_gatt_client_reg_for_notif ev;
> +
> +	ev.conn_id = conn_id;
> +	ev.status = status;
> +	ev.registered = registered;
> +	memcpy(&ev.srvc_id, srvc, sizeof(ev.srvc_id));
> +	memcpy(&ev.char_id, ch, sizeof(ev.char_id));
> +
> +	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
> +			HAL_EV_GATT_CLIENT_REGISTER_FOR_NOTIF, sizeof(ev), &ev);
> +}
> +
>  static void handle_client_register_for_notification(const void *buf,
>  								uint16_t len)
>  {
> @@ -1415,6 +1432,7 @@ static void
> handle_client_register_for_notification(const void *buf, struct gatt_device
> *dev;
>  	struct service *service;
>  	char addr_str[18];
> +	int32_t conn_id = 0;
>  	uint8_t status;
>  	bdaddr_t addr;
> 
> @@ -1438,6 +1456,8 @@ static void
> handle_client_register_for_notification(const void *buf, goto failed;
>  	}
> 
> +	conn_id = dev->conn_id;
> +
>  	hal_srvc_id_to_element_id(&cmd->srvc_id, &match_id);
>  	service = queue_find(dev->services, match_srvc_by_element_id,
>  								&match_id);
> @@ -1506,7 +1526,8 @@ static void
> handle_client_register_for_notification(const void *buf, status =
> HAL_STATUS_SUCCESS;
> 
>  failed:
> -	/* TODO: send callback with notification enabled/disabled */
> +	client_register_for_notif_cb(conn_id, 1, status, &cmd->srvc_id,
> +								&cmd->char_id);
>  	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
>  				HAL_OP_GATT_CLIENT_REGISTER_FOR_NOTIFICATION,
>  				status);

-- 
Szymon K. Janc
szymon.janc@gmail.com

[PATCHv2 10/11] android/gatt: Add handler for ATT_OP_HANDLE_NOTIFY and ATT_OP_HANDLE_IND

[Marcin Kraglak]

Invoke callback notify_cb with received data. We set flag is_notify
properly to inform Android what type of notification has been received.
---
 android/gatt.c | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 61c35c3..d004e35 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -1403,6 +1403,41 @@ static void handle_client_execute_write(const void *buf, uint16_t len)
 			HAL_OP_GATT_CLIENT_EXECUTE_WRITE, HAL_STATUS_FAILED);
 }
 
+static void client_notify_cb(struct notification_data *notification,
+					const uint8_t *pdu, uint16_t len,
+					bool is_notify)
+{
+	uint8_t buf[IPC_MTU];
+	struct hal_ev_gatt_client_notify *ev = (void *) buf;
+
+	memcpy(&ev->char_id, &notification->ch, sizeof(ev->char_id));
+	memcpy(&ev->srvc_id, &notification->service, sizeof(ev->srvc_id));
+	bdaddr2android(&notification->dev->bdaddr, &ev->bda);
+	ev->conn_id = notification->dev->conn_id;
+	ev->is_notify = is_notify;
+	ev->len = len;
+	memcpy(ev->value, pdu, len);
+
+	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT, HAL_EV_GATT_CLIENT_NOTIFY,
+						sizeof(*ev) + ev->len, ev);
+}
+
+static void handle_notification(const uint8_t *pdu, uint16_t len,
+							gpointer user_data)
+{
+	struct notification_data *notification = user_data;
+
+	client_notify_cb(notification, pdu, len, true);
+}
+
+static void handle_indication(const uint8_t *pdu, uint16_t len,
+							gpointer user_data)
+{
+	struct notification_data *notification = user_data;
+
+	client_notify_cb(notification, pdu, len, false);
+}
+
 static void client_register_for_notif_cb(int32_t conn_id, int32_t registered,
 					int32_t status,
 					const struct hal_gatt_srvc_id *srvc,
@@ -1500,7 +1535,8 @@ static void handle_client_register_for_notification(const void *buf,
 	notification->notif_id = g_attrib_register(dev->attrib,
 							ATT_OP_HANDLE_NOTIFY,
 							c->ch.value_handle,
-							NULL, notification,
+							handle_notification,
+							notification,
 							destroy_notification);
 	if (!notification->notif_id) {
 		free(notification);
@@ -1510,7 +1546,8 @@ static void handle_client_register_for_notification(const void *buf,
 
 	notification->ind_id = g_attrib_register(dev->attrib, ATT_OP_HANDLE_IND,
 							c->ch.value_handle,
-							NULL, notification,
+							handle_indication,
+							notification,
 							destroy_notification);
 	if (!notification->ind_id) {
 		g_attrib_unregister(dev->attrib, notification->notif_id);
-- 
1.8.3.1

Re: [PATCHv2 10/11] android/gatt: Add handler for ATT_OP_HANDLE_NOTIFY and ATT_OP_HANDLE_IND

[Szymon Janc]

Hi Marcin,

On Thursday 27 March 2014 22:13:08 Marcin Kraglak wrote:
> Invoke callback notify_cb with received data. We set flag is_notify
> properly to inform Android what type of notification has been received.
> ---
>  android/gatt.c | 41 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 61c35c3..d004e35 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -1403,6 +1403,41 @@ static void handle_client_execute_write(const void
> *buf, uint16_t len) HAL_OP_GATT_CLIENT_EXECUTE_WRITE, HAL_STATUS_FAILED);
>  }
> 
> +static void client_notify_cb(struct notification_data *notification,
> +					const uint8_t *pdu, uint16_t len,
> +					bool is_notify)
> +{

This is not used as a callback so rename it.

> +	uint8_t buf[IPC_MTU];
> +	struct hal_ev_gatt_client_notify *ev = (void *) buf;
> +
> +	memcpy(&ev->char_id, &notification->ch, sizeof(ev->char_id));
> +	memcpy(&ev->srvc_id, &notification->service, sizeof(ev->srvc_id));
> +	bdaddr2android(&notification->dev->bdaddr, &ev->bda);
> +	ev->conn_id = notification->dev->conn_id;
> +	ev->is_notify = is_notify;
> +	ev->len = len;
> +	memcpy(ev->value, pdu, len);
> +
> +	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT, HAL_EV_GATT_CLIENT_NOTIFY,
> +						sizeof(*ev) + ev->len, ev);
> +}
> +
> +static void handle_notification(const uint8_t *pdu, uint16_t len,
> +							gpointer user_data)
> +{
> +	struct notification_data *notification = user_data;
> +
> +	client_notify_cb(notification, pdu, len, true);
> +}
> +
> +static void handle_indication(const uint8_t *pdu, uint16_t len,
> +							gpointer user_data)
> +{
> +	struct notification_data *notification = user_data;
> +
> +	client_notify_cb(notification, pdu, len, false);
> +}

We can use single callback if those are always handled in same way.

> +
>  static void client_register_for_notif_cb(int32_t conn_id, int32_t
> registered, int32_t status,
>  					const struct hal_gatt_srvc_id *srvc,
> @@ -1500,7 +1535,8 @@ static void
> handle_client_register_for_notification(const void *buf,
> notification->notif_id = g_attrib_register(dev->attrib,
>  							ATT_OP_HANDLE_NOTIFY,
>  							c->ch.value_handle,
> -							NULL, notification,
> +							handle_notification,
> +							notification,
>  							destroy_notification);
>  	if (!notification->notif_id) {
>  		free(notification);
> @@ -1510,7 +1546,8 @@ static void
> handle_client_register_for_notification(const void *buf,
> 
>  	notification->ind_id = g_attrib_register(dev->attrib, ATT_OP_HANDLE_IND,
>  							c->ch.value_handle,
> -							NULL, notification,
> +							handle_indication,
> +							notification,
>  							destroy_notification);
>  	if (!notification->ind_id) {
>  		g_attrib_unregister(dev->attrib, notification->notif_id);

-- 
Szymon K. Janc
szymon.janc@gmail.com

[PATCHv2 11/11] android/gatt: Add deregister_for_notification implementation

[Marcin Kraglak]

This will unregister handlers for notification and indications
and remove notification_data from client's list.
---
 android/gatt.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/android/gatt.c b/android/gatt.c
index d004e35..384dccf 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -1573,11 +1573,61 @@ failed:
 static void handle_client_deregister_for_notification(const void *buf,
 								uint16_t len)
 {
+	const struct hal_cmd_gatt_client_deregister_for_notification *cmd = buf;
+	struct notification_data *notification, notif;
+	struct gatt_client *client;
+	struct gatt_device *dev;
+	int32_t conn_id = 0;
+	char addr_str[18];
+	uint8_t status;
+	bdaddr_t addr;
+
 	DBG("");
 
+	client = queue_find(gatt_clients, match_client_by_id,
+						INT_TO_PTR(cmd->client_if));
+	if (!client) {
+		error("gatt: couldn't deregister: client not registered");
+		status = HAL_STATUS_FAILED;
+		goto failed;
+	}
+
+	android2bdaddr((bdaddr_t *)&cmd->bdaddr, &addr);
+	ba2str(&addr, addr_str);
+
+	dev = queue_find(conn_list, match_dev_by_bdaddr, &addr);
+	if (!dev) {
+		error("gatt: couldn't deregister: device %s not found",
+								addr_str);
+		status = HAL_STATUS_FAILED;
+		goto failed;
+	}
+
+	conn_id = dev->conn_id;
+	memcpy(&notif.ch, &cmd->char_id, sizeof(notif.ch));
+	memcpy(&notif.service, &cmd->srvc_id, sizeof(notif.service));
+	notif.dev = dev;
+
+	notification = queue_find(client->notifications,
+						match_notification, &notif);
+
+	if (!notification) {
+		error("gatt: couldn't deregister: notification not registered");
+		status = HAL_STATUS_FAILED;
+		goto failed;
+	}
+
+	unregister_notification(notification);
+
+	status = HAL_STATUS_SUCCESS;
+
+failed:
+	client_register_for_notif_cb(conn_id, 0, status, &cmd->srvc_id,
+								&cmd->char_id);
+
 	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
 				HAL_OP_GATT_CLIENT_DEREGISTER_FOR_NOTIFICATION,
-				HAL_STATUS_FAILED);
+				status);
 }
 
 static void handle_client_read_remote_rssi(const void *buf, uint16_t len)
-- 
1.8.3.1

Re: [PATCHv2 11/11] android/gatt: Add deregister_for_notification implementation

[Szymon Janc]

Hi Marcin,

On Thursday 27 March 2014 22:13:09 Marcin Kraglak wrote:
> This will unregister handlers for notification and indications
> and remove notification_data from client's list.
> ---
>  android/gatt.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index d004e35..384dccf 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -1573,11 +1573,61 @@ failed:
>  static void handle_client_deregister_for_notification(const void *buf,
>  								uint16_t len)
>  {
> +	const struct hal_cmd_gatt_client_deregister_for_notification *cmd = buf;
> +	struct notification_data *notification, notif;
> +	struct gatt_client *client;
> +	struct gatt_device *dev;
> +	int32_t conn_id = 0;
> +	char addr_str[18];
> +	uint8_t status;
> +	bdaddr_t addr;
> +
>  	DBG("");
> 
> +	client = queue_find(gatt_clients, match_client_by_id,
> +						INT_TO_PTR(cmd->client_if));
> +	if (!client) {
> +		error("gatt: couldn't deregister: client not registered");
> +		status = HAL_STATUS_FAILED;
> +		goto failed;
> +	}
> +
> +	android2bdaddr((bdaddr_t *)&cmd->bdaddr, &addr);

Casting is not needed here.

> +	ba2str(&addr, addr_str);
> +
> +	dev = queue_find(conn_list, match_dev_by_bdaddr, &addr);
> +	if (!dev) {
> +		error("gatt: couldn't deregister: device %s not found",
> +								addr_str);
> +		status = HAL_STATUS_FAILED;
> +		goto failed;
> +	}
> +
> +	conn_id = dev->conn_id;
> +	memcpy(&notif.ch, &cmd->char_id, sizeof(notif.ch));
> +	memcpy(&notif.service, &cmd->srvc_id, sizeof(notif.service));
> +	notif.dev = dev;
> +
> +	notification = queue_find(client->notifications,
> +						match_notification, &notif);
> +

Do we need to check that? Wouldn't g_attrib_unregister() failed if id is 
invalid?

And empty line is not needed here.

> +	if (!notification) {
> +		error("gatt: couldn't deregister: notification not registered");
> +		status = HAL_STATUS_FAILED;
> +		goto failed;
> +	}
> +
> +	unregister_notification(notification);
> +
> +	status = HAL_STATUS_SUCCESS;
> +
> +failed:
> +	client_register_for_notif_cb(conn_id, 0, status, &cmd->srvc_id,
> +								&cmd->char_id);
> +
>  	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
>  				HAL_OP_GATT_CLIENT_DEREGISTER_FOR_NOTIFICATION,
> -				HAL_STATUS_FAILED);
> +				status);
>  }
> 
>  static void handle_client_read_remote_rssi(const void *buf, uint16_t len)

-- 
Szymon K. Janc
szymon.janc@gmail.com

Re: [PATCHv2 01/11] android/gatt: Add helper for getting device by conn_id

[Szymon Janc]

Hi Marcin,

On Thursday 27 March 2014 22:12:59 Marcin Kraglak wrote:
> It will look for cgatt_device by conn_id.
> ---
>  android/gatt.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 81bfdc0..7a3db27 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -713,6 +713,17 @@ static struct gatt_device *find_device(bdaddr_t *addr)
>  	return NULL;
>  }
> 
> +static struct gatt_device *find_device_by_conn_id(int32_t conn_id)
> +{
> +	struct gatt_device *dev;
> +
> +	dev = queue_find(conn_list, match_dev_by_conn_id, INT_TO_PTR(conn_id));
> +	if (dev)
> +		return dev;
> +
> +	return NULL;

Wouldn't just return queue_find(); do the same here?
> +}
> +
>  static void handle_client_connect(const void *buf, uint16_t len)
>  {
>  	const struct hal_cmd_gatt_client_connect *cmd = buf;
> @@ -850,8 +861,7 @@ static void handle_client_disconnect(const void *buf,
> uint16_t len)
> 
>  	ba2str((bdaddr_t *)&cmd->bdaddr, addr);
> 
> -	dev = queue_find(conn_list, match_dev_by_conn_id,
> -						INT_TO_PTR(cmd->conn_id));
> +	dev = find_device_by_conn_id(cmd->conn_id);
>  	if (!dev) {
>  		error("gatt: dev %s with conn_id=%d not found",
>  							addr, cmd->conn_id);
> @@ -916,8 +926,7 @@ static void handle_client_search_service(const void
> *buf, uint16_t len)
> 
>  	DBG("");
> 
> -	dev = queue_find(conn_list, match_dev_by_conn_id,
> -						INT_TO_PTR(cmd->conn_id));
> +	dev = find_device_by_conn_id(cmd->conn_id);
>  	if (!dev) {
>  		error("gatt: dev with conn_id=%d not found", cmd->conn_id);
>  		status = HAL_STATUS_FAILED;
> @@ -1040,8 +1049,7 @@ static bool find_service(int32_t conn_id, struct
> element_id *service_id, struct gatt_device *device;
>  	struct service *service;
> 
> -	device = queue_find(conn_list, match_dev_by_conn_id,
> -							INT_TO_PTR(conn_id));
> +	device = find_device_by_conn_id(conn_id);
>  	if (!device) {
>  		error("gatt: conn_id=%d not found", conn_id);
>  		return false;

-- 
Szymon K. Janc
szymon.janc@gmail.com