[PATCH 0/8] Fix reading attribute value in database

[PATCH 0/8] Fix reading attribute value in database

[Marcin Kraglak]

This is refactor of existing server and database.
Now all functions in gatt_db for processing ATT commands are changed
to fill queue with handles of found attributes only, and no specific
data is used. There are also getters for attribute type and end
group type. Gatt_db_read is also refactored to return value of attribute
if exists or call read_cb if registered. In first case we can fill response,
otherwise we have collect data from callbacks in pending_requests queue
(in device struct) and then send response.
We also consider passing read_result_cb to gatt_db_read(), it can simplify
collecting attributes values (data will be always passed to read_result_cb).
Also functions like gatt_db_find_by_type can be replaced in future with
one "filtering" function.

Jakub Tyszkowski (2):
  android/gatt: Fix sending att responses
  shared/gatt: Make 'find_by_type_value' callback compatible

Marcin Kraglak (6):
  shared/gatt: Extend gatt_db_read function
  android/gatt: Refactor ATT read operations
  shared/gatt: Make read by type use response queue
  shared/gatt: Refactor find information
  shared/gatt: Add function to get end group handle
  shared/gatt: Refactor read_by_group_type

 android/gatt.c       | 445 ++++++++++++++++++++++++++++++++++-----------------
 src/shared/gatt-db.c | 128 +++++++--------
 src/shared/gatt-db.h |  42 ++---
 3 files changed, 372 insertions(+), 243 deletions(-)

-- 
1.8.5.3

[PATCH 1/8] android/gatt: Fix sending att responses

[Marcin Kraglak]

From: Jakub Tyszkowski <jakub.tyszkowski@tieto.com>

In case of error we should respond and not fail silently.
Before sending we should also check length value as, opdu is filled
only for some ATT operations and some are handled by read callbacks,
sending their own responses. We should be gradually moving to the later,
using response data queue.
---
 android/gatt.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 157ebe6..d8bcfb5 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4321,18 +4321,12 @@ static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
 		break;
 	case ATT_OP_WRITE_REQ:
 		status = write_req_request(ipdu, len, dev);
-		if (!status)
-			return;
 		break;
 	case ATT_OP_WRITE_CMD:
 		status = write_cmd_request(ipdu, len, dev);
-		if (!status)
-			return;
 		break;
 	case ATT_OP_PREP_WRITE_REQ:
 		status = write_prep_request(ipdu, len, dev);
-		if (!status)
-			return;
 		break;
 	case ATT_OP_EXEC_WRITE_REQ:
 		/* TODO */
@@ -4344,7 +4338,7 @@ static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
 	default:
 		DBG("Unsupported request 0x%02x", ipdu[0]);
 		status = ATT_ECODE_REQ_NOT_SUPP;
-		goto done;
+		break;
 	}
 
 done:
@@ -4352,7 +4346,8 @@ done:
 		length = enc_error_resp(ipdu[0], 0x0000, status, opdu,
 							ATT_DEFAULT_LE_MTU);
 
-	g_attrib_send(dev->attrib, 0, opdu, length, NULL, NULL, NULL);
+	if (length)
+		g_attrib_send(dev->attrib, 0, opdu, length, NULL, NULL, NULL);
 }
 
 static void create_listen_connections(void *data, void *user_data)
-- 
1.8.5.3

Re: [PATCH 1/8] android/gatt: Fix sending att responses

[Szymon Janc]

Hi,

On Monday 12 of May 2014 15:02:31 Marcin Kraglak wrote:
> From: Jakub Tyszkowski <jakub.tyszkowski@tieto.com>
> 
> In case of error we should respond and not fail silently.
> Before sending we should also check length value as, opdu is filled
> only for some ATT operations and some are handled by read callbacks,
> sending their own responses. We should be gradually moving to the later,
> using response data queue.
> ---
>  android/gatt.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 157ebe6..d8bcfb5 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -4321,18 +4321,12 @@ static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
>  		break;
>  	case ATT_OP_WRITE_REQ:
>  		status = write_req_request(ipdu, len, dev);
> -		if (!status)
> -			return;
>  		break;
>  	case ATT_OP_WRITE_CMD:
>  		status = write_cmd_request(ipdu, len, dev);
> -		if (!status)
> -			return;
>  		break;
>  	case ATT_OP_PREP_WRITE_REQ:
>  		status = write_prep_request(ipdu, len, dev);
> -		if (!status)
> -			return;
>  		break;
>  	case ATT_OP_EXEC_WRITE_REQ:
>  		/* TODO */
> @@ -4344,7 +4338,7 @@ static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
>  	default:
>  		DBG("Unsupported request 0x%02x", ipdu[0]);
>  		status = ATT_ECODE_REQ_NOT_SUPP;
> -		goto done;
> +		break;
>  	}
>  
>  done:
> @@ -4352,7 +4346,8 @@ done:
>  		length = enc_error_resp(ipdu[0], 0x0000, status, opdu,
>  							ATT_DEFAULT_LE_MTU);
>  
> -	g_attrib_send(dev->attrib, 0, opdu, length, NULL, NULL, NULL);
> +	if (length)
> +		g_attrib_send(dev->attrib, 0, opdu, length, NULL, NULL, NULL);
>  }
>  
>  static void create_listen_connections(void *data, void *user_data)
> 

This patch doesn't change function flow, I suppose it would matter later in
series. Please put patches in proper order.

-- 
Best regards, 
Szymon Janc

[PATCH 2/8] shared/gatt: Extend gatt_db_read function

[Marcin Kraglak]

If value exists in database, return pointer to it instead of returning
false. It is needed because some attributes don't have read_cb callback
and their value can be read directly from database. If read_cb is
registered, it will be called. In case of error false will be returned.
---
 android/gatt.c       |  3 ++-
 src/shared/gatt-db.c | 17 ++++++++++++++---
 src/shared/gatt-db.h |  3 ++-
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index d8bcfb5..0a2abe3 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4105,7 +4105,8 @@ static uint8_t read_request(const uint8_t *cmd, uint16_t cmd_len,
 		return ATT_ECODE_REQ_NOT_SUPP;
 	}
 
-	if (!gatt_db_read(gatt_db, handle, offset, cmd[0], &dev->bdaddr))
+	if (!gatt_db_read(gatt_db, handle, offset, cmd[0], &dev->bdaddr, NULL,
+									NULL))
 		return ATT_ECODE_UNLIKELY;
 
 	return 0;
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index ebfd586..adce153 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -684,12 +684,16 @@ static bool find_service_for_handle(const void *data, const void *user_data)
 }
 
 bool gatt_db_read(struct gatt_db *db, uint16_t handle, uint16_t offset,
-				uint8_t att_opcode, bdaddr_t *bdaddr)
+				uint8_t att_opcode, bdaddr_t *bdaddr,
+				uint8_t **value, uint16_t *length)
 {
 	struct gatt_db_service *service;
 	uint16_t service_handle;
 	struct gatt_db_attribute *a;
 
+	if (!value || !length)
+		return false;
+
 	service = queue_find(db->services, find_service_for_handle,
 						INT_TO_PTR(handle));
 	if (!service)
@@ -698,10 +702,17 @@ bool gatt_db_read(struct gatt_db *db, uint16_t handle, uint16_t offset,
 	service_handle = service->attributes[0]->handle;
 
 	a = service->attributes[handle - service_handle];
-	if (!a || !a->read_func)
+	if (!a || offset > a->value_len)
 		return false;
 
-	a->read_func(handle, offset, att_opcode, bdaddr, a->user_data);
+	if (!a->read_func) {
+		*value = &a->value[offset];
+		*length = a->value_len - offset;
+	} else {
+		*value = NULL;
+		*length = 0;
+		a->read_func(handle, offset, att_opcode, bdaddr, a->user_data);
+	}
 
 	return true;
 }
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index f704b8e..a5a5f41 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -106,7 +106,8 @@ void gatt_db_find_information(struct gatt_db *db, uint16_t start_handle,
 							struct queue *queue);
 
 bool gatt_db_read(struct gatt_db *db, uint16_t handle, uint16_t offset,
-					uint8_t att_opcode, bdaddr_t *bdaddr);
+					uint8_t att_opcode, bdaddr_t *bdaddr,
+					uint8_t **value, uint16_t *length);
 
 bool gatt_db_write(struct gatt_db *db, uint16_t handle, uint16_t offset,
 					const uint8_t *value, size_t len,
-- 
1.8.5.3

Re: [PATCH 2/8] shared/gatt: Extend gatt_db_read function

[Szymon Janc]

Hi,

On Monday 12 of May 2014 15:02:32 Marcin Kraglak wrote:
> If value exists in database, return pointer to it instead of returning
> false. It is needed because some attributes don't have read_cb callback
> and their value can be read directly from database. If read_cb is
> registered, it will be called. In case of error false will be returned.
> ---
>  android/gatt.c       |  3 ++-
>  src/shared/gatt-db.c | 17 ++++++++++++++---
>  src/shared/gatt-db.h |  3 ++-
>  3 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index d8bcfb5..0a2abe3 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -4105,7 +4105,8 @@ static uint8_t read_request(const uint8_t *cmd, uint16_t cmd_len,
>  		return ATT_ECODE_REQ_NOT_SUPP;
>  	}
>  
> -	if (!gatt_db_read(gatt_db, handle, offset, cmd[0], &dev->bdaddr))
> +	if (!gatt_db_read(gatt_db, handle, offset, cmd[0], &dev->bdaddr, NULL,
> +									NULL))
>  		return ATT_ECODE_UNLIKELY;
>  
>  	return 0;
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index ebfd586..adce153 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -684,12 +684,16 @@ static bool find_service_for_handle(const void *data, const void *user_data)
>  }
>  
>  bool gatt_db_read(struct gatt_db *db, uint16_t handle, uint16_t offset,
> -				uint8_t att_opcode, bdaddr_t *bdaddr)
> +				uint8_t att_opcode, bdaddr_t *bdaddr,
> +				uint8_t **value, uint16_t *length)
>  {
>  	struct gatt_db_service *service;
>  	uint16_t service_handle;
>  	struct gatt_db_attribute *a;
>  
> +	if (!value || !length)
> +		return false;
> +
>  	service = queue_find(db->services, find_service_for_handle,
>  						INT_TO_PTR(handle));
>  	if (!service)
> @@ -698,10 +702,17 @@ bool gatt_db_read(struct gatt_db *db, uint16_t handle, uint16_t offset,
>  	service_handle = service->attributes[0]->handle;
>  
>  	a = service->attributes[handle - service_handle];
> -	if (!a || !a->read_func)
> +	if (!a || offset > a->value_len)
>  		return false;
>  
> -	a->read_func(handle, offset, att_opcode, bdaddr, a->user_data);
> +	if (!a->read_func) {
> +		*value = &a->value[offset];
> +		*length = a->value_len - offset;
> +	} else {
> +		*value = NULL;
> +		*length = 0;
> +		a->read_func(handle, offset, att_opcode, bdaddr, a->user_data);
> +	}

Please keep following convention:

if (func)
  func();

>  
>  	return true;
>  }
> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
> index f704b8e..a5a5f41 100644
> --- a/src/shared/gatt-db.h
> +++ b/src/shared/gatt-db.h
> @@ -106,7 +106,8 @@ void gatt_db_find_information(struct gatt_db *db, uint16_t start_handle,
>  							struct queue *queue);
>  
>  bool gatt_db_read(struct gatt_db *db, uint16_t handle, uint16_t offset,
> -					uint8_t att_opcode, bdaddr_t *bdaddr);
> +					uint8_t att_opcode, bdaddr_t *bdaddr,
> +					uint8_t **value, uint16_t *length);

I think length should be signed. Otherwise if might be clumsy to distinguish
between 0 length data and callback called.

Also please add a comment about that in code.

>  
>  bool gatt_db_write(struct gatt_db *db, uint16_t handle, uint16_t offset,
>  					const uint8_t *value, size_t len,
> 

-- 
Best regards, 
Szymon Janc

[PATCH 3/8] android/gatt: Refactor ATT read operations

[Marcin Kraglak]

This changes device info and gap services callbacks to use response queue.
It will allow them work with plain read and read by type as well.

This starts transition to single response data queue, which should be
filled by various read type functions and processed in one place. This
will unify the way that responses are send, regardless of data source
(value taken directly from database, returned by read callback or sent
from upper layers asynchronously).

We will also introduce 'getter' type functions, using handles to
retrieve data from database. This will make various read and find
operations return handles instead of their own custom structures,
different for every operation performed.
---
 android/gatt.c | 210 +++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 181 insertions(+), 29 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 0a2abe3..f8539d2 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -156,6 +156,8 @@ struct gatt_device {
 
 	int ref;
 	int conn_cnt;
+
+	struct queue *pending_requests;
 };
 
 struct app_connection {
@@ -748,6 +750,21 @@ static void disconnect_notify_by_device(void *data, void *user_data)
 		send_app_connect_notify(conn, GATT_FAILURE);
 }
 
+struct response_data {
+	uint16_t handle;
+	uint16_t length;
+	uint8_t *value;
+	uint16_t offset;
+};
+
+static void destroy_response_data(void *data)
+{
+	struct response_data *entry = data;
+
+	free(entry->value);
+	free(entry);
+}
+
 static void destroy_device(void *data)
 {
 	struct gatt_device *dev = data;
@@ -756,6 +773,7 @@ static void destroy_device(void *data)
 		return;
 
 	queue_destroy(dev->services, destroy_service);
+	queue_destroy(dev->pending_requests, destroy_response_data);
 
 	free(dev);
 }
@@ -1138,6 +1156,13 @@ static struct gatt_device *create_device(const bdaddr_t *addr)
 		return NULL;
 	}
 
+	dev->pending_requests = queue_new();
+	if (!dev->pending_requests) {
+		error("gatt: Failed to allocate memory for client");
+		destroy_device(dev);
+		return NULL;
+	}
+
 	if (!queue_push_head(gatt_devices, dev)) {
 		error("gatt: Cannot push device to queue");
 		destroy_device(dev);
@@ -4027,6 +4052,62 @@ static uint8_t read_by_group_type(const uint8_t *cmd, uint16_t cmd_len,
 	return 0;
 }
 
+static void send_pending_response(uint8_t opcode, struct gatt_device *device)
+{
+	uint8_t rsp[ATT_DEFAULT_LE_MTU];
+	struct response_data *val;
+	uint16_t len = 0;
+
+	if (queue_isempty(device->pending_requests))
+		goto done;
+
+	switch (opcode) {
+	case ATT_OP_READ_BLOB_REQ:
+		val = queue_pop_head(device->pending_requests);
+		if (!val)
+			goto done;
+
+		len = enc_read_blob_resp(val->value, val->length, val->offset,
+							rsp, sizeof(rsp));
+		break;
+	case ATT_OP_READ_REQ:
+		val = queue_pop_head(device->pending_requests);
+		if (!val)
+			goto done;
+
+		len = enc_read_resp(val->value, val->length, rsp, sizeof(rsp));
+		break;
+	default:
+		break;
+	}
+
+done:
+	if (!len)
+		len = enc_error_resp(opcode, 0x0000, ATT_ECODE_UNLIKELY,
+						rsp, ATT_DEFAULT_LE_MTU);
+
+	g_attrib_send(device->attrib, 0, rsp, len, NULL, NULL, NULL);
+
+	queue_remove_all(device->pending_requests, NULL, NULL,
+							destroy_response_data);
+}
+
+static bool match_handle_val_by_empty_len(const void *data,
+							const void *user_data)
+{
+	const struct response_data *handle_data = data;
+
+	return !handle_data->length;
+}
+
+static bool match_handle_val_by_handle(const void *data, const void *user_data)
+{
+	const struct response_data *handle_data = data;
+	uint16_t handle = PTR_TO_UINT(user_data);
+
+	return handle_data->handle == handle;
+}
+
 static uint8_t read_by_type(const uint8_t *cmd, uint16_t cmd_len,
 						uint8_t *rsp, size_t rsp_size,
 						uint16_t *length)
@@ -4086,6 +4167,9 @@ static uint8_t read_request(const uint8_t *cmd, uint16_t cmd_len,
 	uint16_t handle;
 	uint16_t len;
 	uint16_t offset = 0;
+	uint8_t *value;
+	uint16_t length;
+	struct response_data *data;
 
 	DBG("");
 
@@ -4105,10 +4189,29 @@ static uint8_t read_request(const uint8_t *cmd, uint16_t cmd_len,
 		return ATT_ECODE_REQ_NOT_SUPP;
 	}
 
-	if (!gatt_db_read(gatt_db, handle, offset, cmd[0], &dev->bdaddr, NULL,
-									NULL))
+	data = new0(struct response_data, 1);
+	if (!data)
+		return ATT_ECODE_INSUFF_RESOURCES;
+
+	data->handle = handle;
+
+	if (!gatt_db_read(gatt_db, handle, offset, cmd[0], &dev->bdaddr, &value,
+								&length))
 		return ATT_ECODE_UNLIKELY;
 
+	if (length) {
+		data->value = malloc0(length);
+		if (!data->value) {
+			free(data);
+			return ATT_ECODE_INSUFF_RESOURCES;
+	}
+
+	data->length = length;
+	memcpy(data->value, value, length);
+
+	send_pending_response(cmd[0], dev);
+	}
+
 	return 0;
 }
 
@@ -4308,9 +4411,6 @@ static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
 	case ATT_OP_READ_REQ:
 	case ATT_OP_READ_BLOB_REQ:
 		status = read_request(ipdu, len, dev);
-		if (!status)
-			/* Response will be sent in callback */
-			return;
 		break;
 	case ATT_OP_MTU_REQ:
 		status = mtu_att_handle(ipdu, len, opdu, sizeof(opdu), dev,
@@ -4449,8 +4549,7 @@ static struct gap_srvc_handles gap_srvc_data;
 static void gap_read_cb(uint16_t handle, uint16_t offset, uint8_t att_opcode,
 					bdaddr_t *bdaddr, void *user_data)
 {
-	uint8_t pdu[ATT_DEFAULT_LE_MTU];
-	uint16_t len;
+	struct response_data *entry;
 	struct gatt_device *dev;
 
 	DBG("");
@@ -4461,33 +4560,61 @@ static void gap_read_cb(uint16_t handle, uint16_t offset, uint8_t att_opcode,
 		return;
 	}
 
+	entry = queue_find(dev->pending_requests, match_handle_val_by_handle,
+							UINT_TO_PTR(handle));
+
+	/*
+	 * There will already be such record if its batch read
+	 * (i.e. read by type), else - its normal read.
+	 */
+	if (!entry) {
+		entry = new0(struct response_data, 1);
+		if (!entry)
+			goto done;
+
+		queue_push_tail(dev->pending_requests, entry);
+	}
+
 	if (handle == gap_srvc_data.dev_name) {
 		const char *name = bt_get_adapter_name();
 
-		len = enc_read_resp((uint8_t *)name, strlen(name),
-							pdu, sizeof(pdu));
-		goto done;
-	}
+		entry->value = malloc0(strlen(name));
+		if (!entry->value) {
+			queue_remove(dev->pending_requests, entry);
+			free(entry);
+			goto done;
+		}
 
-	if (handle == gap_srvc_data.appear) {
-		uint8_t val[2];
-		put_le16(APPEARANCE_GENERIC_PHONE, val);
-		len = enc_read_resp(val, sizeof(val), pdu, sizeof(pdu));
-		goto done;
-	}
+		entry->length = strlen(name);
+		memcpy(entry->value, bt_get_adapter_name(), entry->length);
+	} else if (handle == gap_srvc_data.appear) {
+		entry->value = malloc0(2);
+		if (!entry->value) {
+			queue_remove(dev->pending_requests, entry);
+			free(entry);
+			goto done;
+		}
 
-	if (handle == gap_srvc_data.priv) {
-		uint8_t val = PERIPHERAL_PRIVACY_DISABLE;
-		len = enc_read_resp(&val, sizeof(val), pdu, sizeof(pdu));
-		goto done;
+		put_le16(APPEARANCE_GENERIC_PHONE, entry->value);
+		entry->length = sizeof(uint8_t) * 2;
+	} else if (handle == gap_srvc_data.priv) {
+		entry->value = malloc0(1);
+		if (!entry->value) {
+			queue_remove(dev->pending_requests, entry);
+			free(entry);
+			goto done;
+		}
+
+		*entry->value = PERIPHERAL_PRIVACY_DISABLE;
+		entry->length = sizeof(uint8_t);
 	}
 
-	error("gatt: Unknown handle 0x%02x", handle);
-	len = enc_error_resp(ATT_OP_READ_REQ, handle,
-					ATT_ECODE_UNLIKELY, pdu, sizeof(pdu));
 
+	entry->offset = offset;
 done:
-	g_attrib_send(dev->attrib, 0, pdu, len, NULL, NULL, NULL);
+	if (!queue_find(dev->pending_requests, match_handle_val_by_empty_len,
+									NULL))
+		send_pending_response(att_opcode, dev);
 }
 
 static void register_gap_service(void)
@@ -4551,10 +4678,9 @@ static void device_info_read_cb(uint16_t handle, uint16_t offset,
 					uint8_t att_opcode, bdaddr_t *bdaddr,
 					void *user_data)
 {
-	uint8_t pdu[ATT_DEFAULT_LE_MTU];
+	struct response_data *entry;
 	struct gatt_device *dev;
 	char *buf = user_data;
-	uint16_t len;
 
 	dev = find_device_by_addr(bdaddr);
 	if (!dev) {
@@ -4562,9 +4688,35 @@ static void device_info_read_cb(uint16_t handle, uint16_t offset,
 		return;
 	}
 
-	len = enc_read_resp((uint8_t *) buf, strlen(buf), pdu, sizeof(pdu));
+	entry = queue_find(dev->pending_requests, match_handle_val_by_handle,
+							UINT_TO_PTR(handle));
 
-	g_attrib_send(dev->attrib, 0, pdu, len, NULL, NULL, NULL);
+	/* there will already be such record if its batch read
+	 * (i.e. read by type), else - its normal read.
+	 */
+	if (!entry) {
+		entry = new0(struct response_data, 1);
+		if (!entry)
+			goto done;
+
+		queue_push_tail(dev->pending_requests, entry);
+	}
+
+	entry->value = malloc0(strlen(buf));
+	if (!entry->value) {
+		queue_remove(dev->pending_requests, entry);
+		free(entry);
+		goto done;
+	}
+
+	entry->length = strlen(buf);
+	memcpy(entry->value, buf, entry->length);
+	entry->offset = offset;
+
+done:
+	if (!queue_find(dev->pending_requests, match_handle_val_by_empty_len,
+									NULL))
+		send_pending_response(att_opcode, dev);
 }
 
 static void register_device_info_service(void)
-- 
1.8.5.3

Re: [PATCH 3/8] android/gatt: Refactor ATT read operations

[Szymon Janc]

Hi,

On Monday 12 of May 2014 15:02:33 Marcin Kraglak wrote:
> This changes device info and gap services callbacks to use response queue.
> It will allow them work with plain read and read by type as well.
> 
> This starts transition to single response data queue, which should be
> filled by various read type functions and processed in one place. This
> will unify the way that responses are send, regardless of data source
> (value taken directly from database, returned by read callback or sent
> from upper layers asynchronously).
> 
> We will also introduce 'getter' type functions, using handles to
> retrieve data from database. This will make various read and find
> operations return handles instead of their own custom structures,
> different for every operation performed.
> ---
>  android/gatt.c | 210 +++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 181 insertions(+), 29 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 0a2abe3..f8539d2 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -156,6 +156,8 @@ struct gatt_device {
>  
>  	int ref;
>  	int conn_cnt;
> +
> +	struct queue *pending_requests;
>  };
>  
>  struct app_connection {
> @@ -748,6 +750,21 @@ static void disconnect_notify_by_device(void *data, void *user_data)
>  		send_app_connect_notify(conn, GATT_FAILURE);
>  }
>  
> +struct response_data {
> +	uint16_t handle;
> +	uint16_t length;
> +	uint8_t *value;
> +	uint16_t offset;
> +};
> +
> +static void destroy_response_data(void *data)
> +{
> +	struct response_data *entry = data;
> +
> +	free(entry->value);
> +	free(entry);
> +}
> +
>  static void destroy_device(void *data)
>  {
>  	struct gatt_device *dev = data;
> @@ -756,6 +773,7 @@ static void destroy_device(void *data)
>  		return;
>  
>  	queue_destroy(dev->services, destroy_service);
> +	queue_destroy(dev->pending_requests, destroy_response_data);
>  
>  	free(dev);
>  }
> @@ -1138,6 +1156,13 @@ static struct gatt_device *create_device(const bdaddr_t *addr)
>  		return NULL;
>  	}
>  
> +	dev->pending_requests = queue_new();
> +	if (!dev->pending_requests) {
> +		error("gatt: Failed to allocate memory for client");
> +		destroy_device(dev);
> +		return NULL;
> +	}
> +
>  	if (!queue_push_head(gatt_devices, dev)) {
>  		error("gatt: Cannot push device to queue");
>  		destroy_device(dev);
> @@ -4027,6 +4052,62 @@ static uint8_t read_by_group_type(const uint8_t *cmd, uint16_t cmd_len,
>  	return 0;
>  }
>  
> +static void send_pending_response(uint8_t opcode, struct gatt_device *device)
> +{
> +	uint8_t rsp[ATT_DEFAULT_LE_MTU];
> +	struct response_data *val;
> +	uint16_t len = 0;
> +
> +	if (queue_isempty(device->pending_requests))
> +		goto done;
> +
> +	switch (opcode) {
> +	case ATT_OP_READ_BLOB_REQ:
> +		val = queue_pop_head(device->pending_requests);
> +		if (!val)
> +			goto done;
> +
> +		len = enc_read_blob_resp(val->value, val->length, val->offset,
> +							rsp, sizeof(rsp));
> +		break;
> +	case ATT_OP_READ_REQ:
> +		val = queue_pop_head(device->pending_requests);
> +		if (!val)
> +			goto done;
> +
> +		len = enc_read_resp(val->value, val->length, rsp, sizeof(rsp));
> +		break;
> +	default:
> +		break;
> +	}
> +
> +done:

This label is not really needed. Just break where needed.

> +	if (!len)
> +		len = enc_error_resp(opcode, 0x0000, ATT_ECODE_UNLIKELY,
> +						rsp, ATT_DEFAULT_LE_MTU);
> +
> +	g_attrib_send(device->attrib, 0, rsp, len, NULL, NULL, NULL);
> +
> +	queue_remove_all(device->pending_requests, NULL, NULL,
> +							destroy_response_data);
> +}
> +
> +static bool match_handle_val_by_empty_len(const void *data,
> +							const void *user_data)
> +{
> +	const struct response_data *handle_data = data;
> +
> +	return !handle_data->length;
> +}
> +
> +static bool match_handle_val_by_handle(const void *data, const void *user_data)
> +{
> +	const struct response_data *handle_data = data;
> +	uint16_t handle = PTR_TO_UINT(user_data);
> +
> +	return handle_data->handle == handle;
> +}
> +
>  static uint8_t read_by_type(const uint8_t *cmd, uint16_t cmd_len,
>  						uint8_t *rsp, size_t rsp_size,
>  						uint16_t *length)
> @@ -4086,6 +4167,9 @@ static uint8_t read_request(const uint8_t *cmd, uint16_t cmd_len,
>  	uint16_t handle;
>  	uint16_t len;
>  	uint16_t offset = 0;
> +	uint8_t *value;
> +	uint16_t length;
> +	struct response_data *data;
>  
>  	DBG("");
>  
> @@ -4105,10 +4189,29 @@ static uint8_t read_request(const uint8_t *cmd, uint16_t cmd_len,
>  		return ATT_ECODE_REQ_NOT_SUPP;
>  	}
>  
> -	if (!gatt_db_read(gatt_db, handle, offset, cmd[0], &dev->bdaddr, NULL,
> -									NULL))
> +	data = new0(struct response_data, 1);
> +	if (!data)
> +		return ATT_ECODE_INSUFF_RESOURCES;
> +
> +	data->handle = handle;
> +
> +	if (!gatt_db_read(gatt_db, handle, offset, cmd[0], &dev->bdaddr, &value,
> +								&length))
>  		return ATT_ECODE_UNLIKELY;
>  
> +	if (length) {
> +		data->value = malloc0(length);
> +		if (!data->value) {
> +			free(data);
> +			return ATT_ECODE_INSUFF_RESOURCES;
> +	}
> +
> +	data->length = length;
> +	memcpy(data->value, value, length);

Put this memcpy inside if

> +
> +	send_pending_response(cmd[0], dev);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -4308,9 +4411,6 @@ static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
>  	case ATT_OP_READ_REQ:
>  	case ATT_OP_READ_BLOB_REQ:
>  		status = read_request(ipdu, len, dev);
> -		if (!status)
> -			/* Response will be sent in callback */
> -			return;
>  		break;
>  	case ATT_OP_MTU_REQ:
>  		status = mtu_att_handle(ipdu, len, opdu, sizeof(opdu), dev,
> @@ -4449,8 +4549,7 @@ static struct gap_srvc_handles gap_srvc_data;
>  static void gap_read_cb(uint16_t handle, uint16_t offset, uint8_t att_opcode,
>  					bdaddr_t *bdaddr, void *user_data)
>  {
> -	uint8_t pdu[ATT_DEFAULT_LE_MTU];
> -	uint16_t len;
> +	struct response_data *entry;
>  	struct gatt_device *dev;
>  
>  	DBG("");
> @@ -4461,33 +4560,61 @@ static void gap_read_cb(uint16_t handle, uint16_t offset, uint8_t att_opcode,
>  		return;
>  	}
>  
> +	entry = queue_find(dev->pending_requests, match_handle_val_by_handle,
> +							UINT_TO_PTR(handle));
> +
> +	/*
> +	 * There will already be such record if its batch read
> +	 * (i.e. read by type), else - its normal read.
> +	 */
> +	if (!entry) {
> +		entry = new0(struct response_data, 1);
> +		if (!entry)
> +			goto done;
> +
> +		queue_push_tail(dev->pending_requests, entry);
> +	}
> +
>  	if (handle == gap_srvc_data.dev_name) {
>  		const char *name = bt_get_adapter_name();
>  
> -		len = enc_read_resp((uint8_t *)name, strlen(name),
> -							pdu, sizeof(pdu));
> -		goto done;
> -	}
> +		entry->value = malloc0(strlen(name));
> +		if (!entry->value) {
> +			queue_remove(dev->pending_requests, entry);
> +			free(entry);
> +			goto done;
> +		}
>  
> -	if (handle == gap_srvc_data.appear) {
> -		uint8_t val[2];
> -		put_le16(APPEARANCE_GENERIC_PHONE, val);
> -		len = enc_read_resp(val, sizeof(val), pdu, sizeof(pdu));
> -		goto done;
> -	}
> +		entry->length = strlen(name);
> +		memcpy(entry->value, bt_get_adapter_name(), entry->length);
> +	} else if (handle == gap_srvc_data.appear) {
> +		entry->value = malloc0(2);

A bit OT here..

This...

> +		if (!entry->value) {
> +			queue_remove(dev->pending_requests, entry);
> +			free(entry);
> +			goto done;
> +		}
>  
> -	if (handle == gap_srvc_data.priv) {
> -		uint8_t val = PERIPHERAL_PRIVACY_DISABLE;
> -		len = enc_read_resp(&val, sizeof(val), pdu, sizeof(pdu));
> -		goto done;
> +		put_le16(APPEARANCE_GENERIC_PHONE, entry->value);
> +		entry->length = sizeof(uint8_t) * 2;
> +	} else if (handle == gap_srvc_data.priv) {
> +		entry->value = malloc0(1);
> +		if (!entry->value) {
> +			queue_remove(dev->pending_requests, entry);
> +			free(entry);
> +			goto done;
> +		}

... and this makes me wonder if we should consider having abort policy
for OOM scenarios (at least for fixed size allocations) like GLib APIs do.

> +
> +		*entry->value = PERIPHERAL_PRIVACY_DISABLE;
> +		entry->length = sizeof(uint8_t);
>  	}
>  
> -	error("gatt: Unknown handle 0x%02x", handle);
> -	len = enc_error_resp(ATT_OP_READ_REQ, handle,
> -					ATT_ECODE_UNLIKELY, pdu, sizeof(pdu));
>  
> +	entry->offset = offset;
>  done:
> -	g_attrib_send(dev->attrib, 0, pdu, len, NULL, NULL, NULL);
> +	if (!queue_find(dev->pending_requests, match_handle_val_by_empty_len,
> +									NULL))
> +		send_pending_response(att_opcode, dev);
>  }
>  
>  static void register_gap_service(void)
> @@ -4551,10 +4678,9 @@ static void device_info_read_cb(uint16_t handle, uint16_t offset,
>  					uint8_t att_opcode, bdaddr_t *bdaddr,
>  					void *user_data)
>  {
> -	uint8_t pdu[ATT_DEFAULT_LE_MTU];
> +	struct response_data *entry;
>  	struct gatt_device *dev;
>  	char *buf = user_data;
> -	uint16_t len;
>  
>  	dev = find_device_by_addr(bdaddr);
>  	if (!dev) {
> @@ -4562,9 +4688,35 @@ static void device_info_read_cb(uint16_t handle, uint16_t offset,
>  		return;
>  	}
>  
> -	len = enc_read_resp((uint8_t *) buf, strlen(buf), pdu, sizeof(pdu));
> +	entry = queue_find(dev->pending_requests, match_handle_val_by_handle,
> +							UINT_TO_PTR(handle));
>  
> -	g_attrib_send(dev->attrib, 0, pdu, len, NULL, NULL, NULL);
> +	/* there will already be such record if its batch read
> +	 * (i.e. read by type), else - its normal read.
> +	 */

/*
 * foo
 * bar
 */

Lets keep this consistent.

> +	if (!entry) {
> +		entry = new0(struct response_data, 1);
> +		if (!entry)
> +			goto done;
> +
> +		queue_push_tail(dev->pending_requests, entry);
> +	}
> +
> +	entry->value = malloc0(strlen(buf));
> +	if (!entry->value) {
> +		queue_remove(dev->pending_requests, entry);
> +		free(entry);
> +		goto done;
> +	}
> +
> +	entry->length = strlen(buf);
> +	memcpy(entry->value, buf, entry->length);
> +	entry->offset = offset;
> +
> +done:
> +	if (!queue_find(dev->pending_requests, match_handle_val_by_empty_len,
> +									NULL))
> +		send_pending_response(att_opcode, dev);
>  }
>  
>  static void register_device_info_service(void)
> 

-- 
Best regards, 
Szymon Janc

[PATCH 4/8] shared/gatt: Make read by type use response queue

[Marcin Kraglak]

This makes read by type use response queue as plain read do.
---
 android/gatt.c       | 99 ++++++++++++++++++++++++++++++++++------------------
 src/shared/gatt-db.c | 15 ++------
 src/shared/gatt-db.h |  6 ----
 3 files changed, 67 insertions(+), 53 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index f8539d2..f107b41 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -3963,19 +3963,6 @@ static void copy_to_att_list(void *data, void *user_data)
 	memcpy(&value[4], group->value, group->len);
 }
 
-static void copy_to_att_list_type(void *data, void *user_data)
-{
-	struct copy_att_list_data *l = user_data;
-	struct gatt_db_handle_value *hdl_val = data;
-	uint8_t *value;
-
-	value = l->adl->data[l->iterator++];
-
-	put_le16(hdl_val->handle, value);
-
-	memcpy(&value[2], hdl_val->value, hdl_val->length);
-}
-
 static void copy_to_att_list_info(void *data, void *user_data)
 {
 	struct copy_att_list_data *l = user_data;
@@ -4055,6 +4042,7 @@ static uint8_t read_by_group_type(const uint8_t *cmd, uint16_t cmd_len,
 static void send_pending_response(uint8_t opcode, struct gatt_device *device)
 {
 	uint8_t rsp[ATT_DEFAULT_LE_MTU];
+	struct att_data_list *adl;
 	struct response_data *val;
 	uint16_t len = 0;
 
@@ -4062,6 +4050,33 @@ static void send_pending_response(uint8_t opcode, struct gatt_device *device)
 		goto done;
 
 	switch (opcode) {
+	case ATT_OP_READ_BY_TYPE_REQ: {
+		int iterator = 0;
+
+		/* FIXME: do proper allocation as val->length may vary */
+		val = queue_peek_head(device->pending_requests);
+		adl = att_data_list_alloc(queue_length(
+						device->pending_requests),
+						sizeof(uint16_t) + val->length);
+
+		val = queue_pop_head(device->pending_requests);
+		while (val) {
+			uint8_t *value = adl->data[iterator++];
+
+			put_le16(val->handle, value);
+			memcpy(&value[2], val->value, val->length);
+
+			destroy_response_data(val);
+
+			val = queue_pop_head(device->pending_requests);
+		}
+
+		len = enc_read_by_type_resp(adl, rsp, sizeof(rsp));
+
+		att_data_list_free(adl);
+
+		break;
+	}
 	case ATT_OP_READ_BLOB_REQ:
 		val = queue_pop_head(device->pending_requests);
 		if (!val)
@@ -4109,16 +4124,12 @@ static bool match_handle_val_by_handle(const void *data, const void *user_data)
 }
 
 static uint8_t read_by_type(const uint8_t *cmd, uint16_t cmd_len,
-						uint8_t *rsp, size_t rsp_size,
-						uint16_t *length)
+						struct gatt_device *device)
 {
 	uint16_t start, end;
 	uint16_t len;
 	bt_uuid_t uuid;
 	struct queue *q;
-	struct att_data_list *adl;
-	struct copy_att_list_data l;
-	struct gatt_db_handle_value *h;
 
 	DBG("");
 
@@ -4137,26 +4148,46 @@ static uint8_t read_by_type(const uint8_t *cmd, uint16_t cmd_len,
 		return ATT_ECODE_ATTR_NOT_FOUND;
 	}
 
-	len = queue_length(q);
-	h = queue_peek_tail(q);
+	while (queue_peek_head(q)) {
+		struct response_data *data;
+		uint16_t handle = PTR_TO_UINT(queue_pop_head(q));
+		uint8_t *value;
+		uint16_t value_len;
 
-	/* Element here is handle + value*/
-	adl = att_data_list_alloc(len, sizeof(uint16_t) + h->length);
-	if (!adl) {
-		queue_destroy(q, free);
-		return ATT_ECODE_INSUFF_RESOURCES;
-	}
+		data = new0(struct response_data, 1);
+		if (!data) {
+			queue_destroy(q, NULL);
+			return ATT_ECODE_INSUFF_RESOURCES;
+		}
 
-	l.iterator = 0;
-	l.adl = adl;
+		data->handle = handle;
+		queue_push_tail(device->pending_requests, data);
 
-	queue_foreach(q, copy_to_att_list_type, &l);
+		if (!gatt_db_read(gatt_db, handle, 0, ATT_OP_READ_BY_TYPE_REQ,
+					&device->bdaddr, &value, &value_len)) {
+			queue_remove(device->pending_requests, data);
+			free(data);
 
-	len = enc_read_by_type_resp(adl, rsp, rsp_size);
-	*length = len;
+			continue;
+		}
 
-	att_data_list_free(adl);
-	queue_destroy(q, free);
+		/* We have value here already if no callback will be called */
+		if (value_len) {
+			data->value = malloc0(value_len);
+			if (!data->value) {
+				queue_destroy(q, NULL);
+				return ATT_ECODE_INSUFF_RESOURCES;
+			}
+
+			memcpy(data->value, value, value_len);
+			data->length = value_len;
+		}
+	}
+
+	/* We send immediate if no data left to be filled by async callbacks */
+	if (!queue_find(device->pending_requests, match_handle_val_by_empty_len,
+									NULL))
+		send_pending_response(ATT_OP_READ_BY_TYPE_REQ, device);
 
 	return 0;
 }
@@ -4406,7 +4437,7 @@ static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
 								&length);
 		break;
 	case ATT_OP_READ_BY_TYPE_REQ:
-		status = read_by_type(ipdu, len, opdu, sizeof(opdu), &length);
+		status = read_by_type(ipdu, len, dev);
 		break;
 	case ATT_OP_READ_REQ:
 	case ATT_OP_READ_BLOB_REQ:
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index adce153..324a532 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -564,7 +564,6 @@ static void read_by_type(void *data, void *user_data)
 	struct read_by_type_data *search_data = user_data;
 	struct gatt_db_service *service = data;
 	struct gatt_db_attribute *attribute;
-	struct gatt_db_handle_value *value;
 	int i;
 
 	if (!service->active)
@@ -584,18 +583,8 @@ static void read_by_type(void *data, void *user_data)
 		if (bt_uuid_cmp(&search_data->uuid, &attribute->uuid))
 			continue;
 
-		value = malloc0(sizeof(struct gatt_db_handle_value) +
-							attribute->value_len);
-		if (!value)
-			return;
-
-		value->handle = attribute->handle;
-		value->length = attribute->value_len;
-		if (attribute->value_len)
-			memcpy(value->value, attribute->value, value->length);
-
-		if (!queue_push_tail(search_data->queue, value))
-			free(value);
+		queue_push_tail(search_data->queue,
+						UINT_TO_PTR(attribute->handle));
 	}
 }
 
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index a5a5f41..6c9216d 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -85,12 +85,6 @@ void gatt_db_find_by_type_value(struct gatt_db *db, uint16_t start_handle,
 							uint16_t length,
 							struct queue *queue);
 
-struct gatt_db_handle_value {
-	uint16_t handle;
-	uint16_t length;
-	uint8_t value[0];
-};
-
 void gatt_db_read_by_type(struct gatt_db *db, uint16_t start_handle,
 							uint16_t end_handle,
 							const bt_uuid_t type,
-- 
1.8.5.3

Re: [PATCH 4/8] shared/gatt: Make read by type use response queue

[Szymon Janc]

Hi,

On Monday 12 of May 2014 15:02:34 Marcin Kraglak wrote:
> This makes read by type use response queue as plain read do.
> ---
>  android/gatt.c       | 99 ++++++++++++++++++++++++++++++++++------------------
>  src/shared/gatt-db.c | 15 ++------
>  src/shared/gatt-db.h |  6 ----
>  3 files changed, 67 insertions(+), 53 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index f8539d2..f107b41 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -3963,19 +3963,6 @@ static void copy_to_att_list(void *data, void *user_data)
>  	memcpy(&value[4], group->value, group->len);
>  }
>  
> -static void copy_to_att_list_type(void *data, void *user_data)
> -{
> -	struct copy_att_list_data *l = user_data;
> -	struct gatt_db_handle_value *hdl_val = data;
> -	uint8_t *value;
> -
> -	value = l->adl->data[l->iterator++];
> -
> -	put_le16(hdl_val->handle, value);
> -
> -	memcpy(&value[2], hdl_val->value, hdl_val->length);
> -}
> -
>  static void copy_to_att_list_info(void *data, void *user_data)
>  {
>  	struct copy_att_list_data *l = user_data;
> @@ -4055,6 +4042,7 @@ static uint8_t read_by_group_type(const uint8_t *cmd, uint16_t cmd_len,
>  static void send_pending_response(uint8_t opcode, struct gatt_device *device)
>  {
>  	uint8_t rsp[ATT_DEFAULT_LE_MTU];
> +	struct att_data_list *adl;
>  	struct response_data *val;
>  	uint16_t len = 0;
>  
> @@ -4062,6 +4050,33 @@ static void send_pending_response(uint8_t opcode, struct gatt_device *device)
>  		goto done;
>  
>  	switch (opcode) {
> +	case ATT_OP_READ_BY_TYPE_REQ: {
> +		int iterator = 0;
> +
> +		/* FIXME: do proper allocation as val->length may vary */

As discussed offline, if this vary just send response.

> +		val = queue_peek_head(device->pending_requests);
> +		adl = att_data_list_alloc(queue_length(
> +						device->pending_requests),
> +						sizeof(uint16_t) + val->length);
> +
> +		val = queue_pop_head(device->pending_requests);
> +		while (val) {
> +			uint8_t *value = adl->data[iterator++];
> +
> +			put_le16(val->handle, value);
> +			memcpy(&value[2], val->value, val->length);
> +
> +			destroy_response_data(val);
> +
> +			val = queue_pop_head(device->pending_requests);
> +		}
> +
> +		len = enc_read_by_type_resp(adl, rsp, sizeof(rsp));
> +
> +		att_data_list_free(adl);
> +
> +		break;
> +	}
>  	case ATT_OP_READ_BLOB_REQ:
>  		val = queue_pop_head(device->pending_requests);
>  		if (!val)
> @@ -4109,16 +4124,12 @@ static bool match_handle_val_by_handle(const void *data, const void *user_data)
>  }
>  
>  static uint8_t read_by_type(const uint8_t *cmd, uint16_t cmd_len,
> -						uint8_t *rsp, size_t rsp_size,
> -						uint16_t *length)
> +						struct gatt_device *device)
>  {
>  	uint16_t start, end;
>  	uint16_t len;
>  	bt_uuid_t uuid;
>  	struct queue *q;
> -	struct att_data_list *adl;
> -	struct copy_att_list_data l;
> -	struct gatt_db_handle_value *h;
>  
>  	DBG("");
>  
> @@ -4137,26 +4148,46 @@ static uint8_t read_by_type(const uint8_t *cmd, uint16_t cmd_len,
>  		return ATT_ECODE_ATTR_NOT_FOUND;
>  	}
>  
> -	len = queue_length(q);
> -	h = queue_peek_tail(q);
> +	while (queue_peek_head(q)) {
> +		struct response_data *data;
> +		uint16_t handle = PTR_TO_UINT(queue_pop_head(q));
> +		uint8_t *value;
> +		uint16_t value_len;
>  
> -	/* Element here is handle + value*/
> -	adl = att_data_list_alloc(len, sizeof(uint16_t) + h->length);
> -	if (!adl) {
> -		queue_destroy(q, free);
> -		return ATT_ECODE_INSUFF_RESOURCES;
> -	}
> +		data = new0(struct response_data, 1);
> +		if (!data) {
> +			queue_destroy(q, NULL);
> +			return ATT_ECODE_INSUFF_RESOURCES;
> +		}
>  
> -	l.iterator = 0;
> -	l.adl = adl;
> +		data->handle = handle;
> +		queue_push_tail(device->pending_requests, data);
>  
> -	queue_foreach(q, copy_to_att_list_type, &l);
> +		if (!gatt_db_read(gatt_db, handle, 0, ATT_OP_READ_BY_TYPE_REQ,
> +					&device->bdaddr, &value, &value_len)) {
> +			queue_remove(device->pending_requests, data);
> +			free(data);
>  
> -	len = enc_read_by_type_resp(adl, rsp, rsp_size);
> -	*length = len;
> +			continue;
> +		}
>  
> -	att_data_list_free(adl);
> -	queue_destroy(q, free);
> +		/* We have value here already if no callback will be called */
> +		if (value_len) {
> +			data->value = malloc0(value_len);
> +			if (!data->value) {
> +				queue_destroy(q, NULL);
> +				return ATT_ECODE_INSUFF_RESOURCES;
> +			}
> +
> +			memcpy(data->value, value, value_len);
> +			data->length = value_len;
> +		}
> +	}
> +
> +	/* We send immediate if no data left to be filled by async callbacks */
> +	if (!queue_find(device->pending_requests, match_handle_val_by_empty_len,
> +									NULL))
> +		send_pending_response(ATT_OP_READ_BY_TYPE_REQ, device);
>  
>  	return 0;
>  }
> @@ -4406,7 +4437,7 @@ static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
>  								&length);
>  		break;
>  	case ATT_OP_READ_BY_TYPE_REQ:
> -		status = read_by_type(ipdu, len, opdu, sizeof(opdu), &length);
> +		status = read_by_type(ipdu, len, dev);
>  		break;
>  	case ATT_OP_READ_REQ:
>  	case ATT_OP_READ_BLOB_REQ:
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index adce153..324a532 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -564,7 +564,6 @@ static void read_by_type(void *data, void *user_data)
>  	struct read_by_type_data *search_data = user_data;
>  	struct gatt_db_service *service = data;
>  	struct gatt_db_attribute *attribute;
> -	struct gatt_db_handle_value *value;
>  	int i;
>  
>  	if (!service->active)
> @@ -584,18 +583,8 @@ static void read_by_type(void *data, void *user_data)
>  		if (bt_uuid_cmp(&search_data->uuid, &attribute->uuid))
>  			continue;
>  
> -		value = malloc0(sizeof(struct gatt_db_handle_value) +
> -							attribute->value_len);
> -		if (!value)
> -			return;
> -
> -		value->handle = attribute->handle;
> -		value->length = attribute->value_len;
> -		if (attribute->value_len)
> -			memcpy(value->value, attribute->value, value->length);
> -
> -		if (!queue_push_tail(search_data->queue, value))
> -			free(value);
> +		queue_push_tail(search_data->queue,
> +						UINT_TO_PTR(attribute->handle));
>  	}
>  }
>  
> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
> index a5a5f41..6c9216d 100644
> --- a/src/shared/gatt-db.h
> +++ b/src/shared/gatt-db.h
> @@ -85,12 +85,6 @@ void gatt_db_find_by_type_value(struct gatt_db *db, uint16_t start_handle,
>  							uint16_t length,
>  							struct queue *queue);
>  
> -struct gatt_db_handle_value {
> -	uint16_t handle;
> -	uint16_t length;
> -	uint8_t value[0];
> -};
> -

Please keep changes to shared/ folder in separate patches if possible (ie. here
you can remove unused type in separate patch with proper commit msg)

>  void gatt_db_read_by_type(struct gatt_db *db, uint16_t start_handle,
>  							uint16_t end_handle,
>  							const bt_uuid_t type,
> 

-- 
Best regards, 
Szymon Janc

[PATCH 5/8] shared/gatt: Refactor find information

[Marcin Kraglak]

Now find information will fill queue with handles of found attributes.
---
 android/gatt.c       | 66 ++++++++++++++++++----------------------------------
 src/shared/gatt-db.c | 32 ++++++++++++++++++-------
 src/shared/gatt-db.h | 12 ++++------
 3 files changed, 50 insertions(+), 60 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index f107b41..e53787e 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -3963,31 +3963,6 @@ static void copy_to_att_list(void *data, void *user_data)
 	memcpy(&value[4], group->value, group->len);
 }
 
-static void copy_to_att_list_info(void *data, void *user_data)
-{
-	struct copy_att_list_data *l = user_data;
-	struct gatt_db_find_information *info = data;
-	uint8_t *value;
-
-	value = l->adl->data[l->iterator++];
-
-	put_le16(info->handle, value);
-
-	switch (info->uuid.type) {
-	case BT_UUID16:
-		memcpy(&value[2], &info->uuid.value.u16,
-						bt_uuid_len(&info->uuid));
-		break;
-	case BT_UUID128:
-		memcpy(&value[2], &info->uuid.value.u128,
-						bt_uuid_len(&info->uuid));
-		break;
-	default:
-		error("gatt: Unexpected UUID type");
-		break;
-	}
-}
-
 static uint8_t read_by_group_type(const uint8_t *cmd, uint16_t cmd_len,
 						uint8_t *rsp, size_t rsp_size,
 						uint16_t *length)
@@ -4296,12 +4271,9 @@ static uint8_t find_info_handle(const uint8_t *cmd, uint16_t cmd_len,
 						uint16_t *length)
 {
 	struct queue *q;
-	struct gatt_db_find_information *last_element;
-	struct copy_att_list_data l;
 	struct att_data_list *adl;
+	int iterator = 0;
 	uint16_t start, end;
-	uint16_t num;
-	uint8_t format;
 	uint16_t len;
 
 	DBG("");
@@ -4322,29 +4294,35 @@ static uint8_t find_info_handle(const uint8_t *cmd, uint16_t cmd_len,
 	}
 
 	len = queue_length(q);
+	adl = att_data_list_alloc(len, 2 * sizeof(uint16_t));
+	if (!adl) {
+		queue_destroy(q, NULL);
+		return ATT_ECODE_INSUFF_RESOURCES;
+	}
+
+	while (queue_peek_head(q)) {
+		uint8_t *value;
+		const bt_uuid_t *type;
+		uint16_t handle = PTR_TO_UINT(queue_pop_head(q));
 
-	last_element = queue_peek_head(q);
+		type = gatt_db_get_attribute_type(gatt_db, handle);
+		if (!type)
+			break;
+
+		value = adl->data[iterator++];
+
+		put_le16(handle, value);
+		memcpy(&value[2], &type->value.u16, bt_uuid_len(type));
 
-	if (last_element->uuid.type == BT_UUID16) {
-		num = sizeof(uint16_t);
-		format = ATT_FIND_INFO_RESP_FMT_16BIT;
-	} else {
-		num = sizeof(uint128_t);
-		format = ATT_FIND_INFO_RESP_FMT_128BIT;
 	}
 
-	adl = att_data_list_alloc(len, num + sizeof(uint16_t));
 	if (!adl) {
-		queue_destroy(q, free);
+		queue_destroy(q, NULL);
 		return ATT_ECODE_INSUFF_RESOURCES;
 	}
 
-	l.iterator = 0;
-	l.adl = adl;
-
-	queue_foreach(q, copy_to_att_list_info, &l);
-
-	len = enc_find_info_resp(format, adl, rsp, rsp_size);
+	len = enc_find_info_resp(ATT_FIND_INFO_RESP_FMT_16BIT, adl, rsp,
+								rsp_size);
 	if (!len)
 		return ATT_ECODE_UNLIKELY;
 
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 324a532..532e0b7 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -614,7 +614,6 @@ static void find_information(void *data, void *user_data)
 	struct find_information_data *search_data = user_data;
 	struct gatt_db_service *service = data;
 	struct gatt_db_attribute *attribute;
-	struct gatt_db_find_information *value;
 	int i;
 
 	if (!service->active)
@@ -636,14 +635,8 @@ static void find_information(void *data, void *user_data)
 		if (attribute->handle > search_data->end_handle)
 			return;
 
-		value = new0(struct gatt_db_find_information, 1);
-		if (!value)
-			return;
-
-		value->handle = attribute->handle;
-		value->uuid = attribute->uuid;
-		if (!queue_push_tail(search_data->queue, value))
-			free(value);
+		queue_push_tail(search_data->queue,
+						UINT_TO_PTR(attribute->handle));
 	}
 }
 
@@ -730,3 +723,24 @@ bool gatt_db_write(struct gatt_db *db, uint16_t handle, uint16_t offset,
 
 	return true;
 }
+
+const bt_uuid_t *gatt_db_get_attribute_type(struct gatt_db *db,
+							uint16_t handle)
+{
+	struct gatt_db_service *service;
+	struct gatt_db_attribute *attribute;
+	uint16_t service_handle;
+
+	service = queue_find(db->services, find_service_for_handle,
+						INT_TO_PTR(handle));
+	if (!service)
+		return NULL;
+
+	service_handle = service->attributes[0]->handle;
+
+	attribute = service->attributes[handle - service_handle];
+	if (!attribute)
+		return NULL;
+
+	return &attribute->uuid;
+}
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index 6c9216d..cc9120c 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -84,17 +84,12 @@ void gatt_db_find_by_type_value(struct gatt_db *db, uint16_t start_handle,
 							const uint8_t *value,
 							uint16_t length,
 							struct queue *queue);
-
+/* List of handles */
 void gatt_db_read_by_type(struct gatt_db *db, uint16_t start_handle,
 							uint16_t end_handle,
 							const bt_uuid_t type,
 							struct queue *queue);
-
-struct gatt_db_find_information {
-	uint16_t handle;
-	bt_uuid_t uuid;
-};
-
+/* List of handles */
 void gatt_db_find_information(struct gatt_db *db, uint16_t start_handle,
 							uint16_t end_handle,
 							struct queue *queue);
@@ -106,3 +101,6 @@ bool gatt_db_read(struct gatt_db *db, uint16_t handle, uint16_t offset,
 bool gatt_db_write(struct gatt_db *db, uint16_t handle, uint16_t offset,
 					const uint8_t *value, size_t len,
 					uint8_t att_opcode, bdaddr_t *bdaddr);
+
+const bt_uuid_t *gatt_db_get_attribute_type(struct gatt_db *db,
+							uint16_t handle);
-- 
1.8.5.3

Re: [PATCH 5/8] shared/gatt: Refactor find information

[Szymon Janc]

Hi,

On Monday 12 of May 2014 15:02:35 Marcin Kraglak wrote:
> Now find information will fill queue with handles of found attributes.
> ---
>  android/gatt.c       | 66 ++++++++++++++++++----------------------------------
>  src/shared/gatt-db.c | 32 ++++++++++++++++++-------
>  src/shared/gatt-db.h | 12 ++++------

Please keep android and shared changes separate patches.

>  3 files changed, 50 insertions(+), 60 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index f107b41..e53787e 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -3963,31 +3963,6 @@ static void copy_to_att_list(void *data, void *user_data)
>  	memcpy(&value[4], group->value, group->len);
>  }
>  
> -static void copy_to_att_list_info(void *data, void *user_data)
> -{
> -	struct copy_att_list_data *l = user_data;
> -	struct gatt_db_find_information *info = data;
> -	uint8_t *value;
> -
> -	value = l->adl->data[l->iterator++];
> -
> -	put_le16(info->handle, value);
> -
> -	switch (info->uuid.type) {
> -	case BT_UUID16:
> -		memcpy(&value[2], &info->uuid.value.u16,
> -						bt_uuid_len(&info->uuid));
> -		break;
> -	case BT_UUID128:
> -		memcpy(&value[2], &info->uuid.value.u128,
> -						bt_uuid_len(&info->uuid));
> -		break;
> -	default:
> -		error("gatt: Unexpected UUID type");
> -		break;
> -	}
> -}
> -
>  static uint8_t read_by_group_type(const uint8_t *cmd, uint16_t cmd_len,
>  						uint8_t *rsp, size_t rsp_size,
>  						uint16_t *length)
> @@ -4296,12 +4271,9 @@ static uint8_t find_info_handle(const uint8_t *cmd, uint16_t cmd_len,
>  						uint16_t *length)
>  {
>  	struct queue *q;
> -	struct gatt_db_find_information *last_element;
> -	struct copy_att_list_data l;
>  	struct att_data_list *adl;
> +	int iterator = 0;
>  	uint16_t start, end;
> -	uint16_t num;
> -	uint8_t format;
>  	uint16_t len;
>  
>  	DBG("");
> @@ -4322,29 +4294,35 @@ static uint8_t find_info_handle(const uint8_t *cmd, uint16_t cmd_len,
>  	}
>  
>  	len = queue_length(q);
> +	adl = att_data_list_alloc(len, 2 * sizeof(uint16_t));
> +	if (!adl) {
> +		queue_destroy(q, NULL);
> +		return ATT_ECODE_INSUFF_RESOURCES;
> +	}
> +
> +	while (queue_peek_head(q)) {
> +		uint8_t *value;
> +		const bt_uuid_t *type;
> +		uint16_t handle = PTR_TO_UINT(queue_pop_head(q));
>  
> -	last_element = queue_peek_head(q);
> +		type = gatt_db_get_attribute_type(gatt_db, handle);
> +		if (!type)
> +			break;
> +
> +		value = adl->data[iterator++];
> +
> +		put_le16(handle, value);
> +		memcpy(&value[2], &type->value.u16, bt_uuid_len(type));
>  
> -	if (last_element->uuid.type == BT_UUID16) {
> -		num = sizeof(uint16_t);
> -		format = ATT_FIND_INFO_RESP_FMT_16BIT;
> -	} else {
> -		num = sizeof(uint128_t);
> -		format = ATT_FIND_INFO_RESP_FMT_128BIT;
>  	}
>  
> -	adl = att_data_list_alloc(len, num + sizeof(uint16_t));
>  	if (!adl) {
> -		queue_destroy(q, free);
> +		queue_destroy(q, NULL);
>  		return ATT_ECODE_INSUFF_RESOURCES;
>  	}
>  
> -	l.iterator = 0;
> -	l.adl = adl;
> -
> -	queue_foreach(q, copy_to_att_list_info, &l);
> -
> -	len = enc_find_info_resp(format, adl, rsp, rsp_size);
> +	len = enc_find_info_resp(ATT_FIND_INFO_RESP_FMT_16BIT, adl, rsp,
> +								rsp_size);
>  	if (!len)
>  		return ATT_ECODE_UNLIKELY;
>  
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index 324a532..532e0b7 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -614,7 +614,6 @@ static void find_information(void *data, void *user_data)
>  	struct find_information_data *search_data = user_data;
>  	struct gatt_db_service *service = data;
>  	struct gatt_db_attribute *attribute;
> -	struct gatt_db_find_information *value;
>  	int i;
>  
>  	if (!service->active)
> @@ -636,14 +635,8 @@ static void find_information(void *data, void *user_data)
>  		if (attribute->handle > search_data->end_handle)
>  			return;
>  
> -		value = new0(struct gatt_db_find_information, 1);
> -		if (!value)
> -			return;
> -
> -		value->handle = attribute->handle;
> -		value->uuid = attribute->uuid;
> -		if (!queue_push_tail(search_data->queue, value))
> -			free(value);
> +		queue_push_tail(search_data->queue,
> +						UINT_TO_PTR(attribute->handle));
>  	}
>  }
>  
> @@ -730,3 +723,24 @@ bool gatt_db_write(struct gatt_db *db, uint16_t handle, uint16_t offset,
>  
>  	return true;
>  }
> +
> +const bt_uuid_t *gatt_db_get_attribute_type(struct gatt_db *db,
> +							uint16_t handle)
> +{
> +	struct gatt_db_service *service;
> +	struct gatt_db_attribute *attribute;
> +	uint16_t service_handle;
> +
> +	service = queue_find(db->services, find_service_for_handle,
> +						INT_TO_PTR(handle));
> +	if (!service)
> +		return NULL;
> +
> +	service_handle = service->attributes[0]->handle;
> +
> +	attribute = service->attributes[handle - service_handle];
> +	if (!attribute)
> +		return NULL;
> +
> +	return &attribute->uuid;
> +}
> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
> index 6c9216d..cc9120c 100644
> --- a/src/shared/gatt-db.h
> +++ b/src/shared/gatt-db.h
> @@ -84,17 +84,12 @@ void gatt_db_find_by_type_value(struct gatt_db *db, uint16_t start_handle,
>  							const uint8_t *value,
>  							uint16_t length,
>  							struct queue *queue);
> -
> +/* List of handles */

Use full sentence here as now this is a bit unclear what this comment refers to.

>  void gatt_db_read_by_type(struct gatt_db *db, uint16_t start_handle,
>  							uint16_t end_handle,
>  							const bt_uuid_t type,
>  							struct queue *queue);
> -
> -struct gatt_db_find_information {
> -	uint16_t handle;
> -	bt_uuid_t uuid;
> -};
> -

This should be in separate patch.

> +/* List of handles */
>  void gatt_db_find_information(struct gatt_db *db, uint16_t start_handle,
>  							uint16_t end_handle,
>  							struct queue *queue);
> @@ -106,3 +101,6 @@ bool gatt_db_read(struct gatt_db *db, uint16_t handle, uint16_t offset,
>  bool gatt_db_write(struct gatt_db *db, uint16_t handle, uint16_t offset,
>  					const uint8_t *value, size_t len,
>  					uint8_t att_opcode, bdaddr_t *bdaddr);
> +
> +const bt_uuid_t *gatt_db_get_attribute_type(struct gatt_db *db,
> +							uint16_t handle);
> 

-- 
Best regards, 
Szymon Janc

[PATCH 6/8] shared/gatt: Add function to get end group handle

[Marcin Kraglak]

This helper will return end group handle of attribute.
---
 src/shared/gatt-db.c | 12 ++++++++++++
 src/shared/gatt-db.h |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 532e0b7..41457e2 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -744,3 +744,15 @@ const bt_uuid_t *gatt_db_get_attribute_type(struct gatt_db *db,
 
 	return &attribute->uuid;
 }
+
+uint16_t gatt_db_get_end_handle(struct gatt_db *db, uint16_t handle)
+{
+	struct gatt_db_service *service;
+
+	service = queue_find(db->services, find_service_for_handle,
+						INT_TO_PTR(handle));
+	if (!service)
+		return 0;
+
+	return service->attributes[0]->handle + service->num_handles - 1;
+}
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index cc9120c..ddb40da 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -104,3 +104,5 @@ bool gatt_db_write(struct gatt_db *db, uint16_t handle, uint16_t offset,
 
 const bt_uuid_t *gatt_db_get_attribute_type(struct gatt_db *db,
 							uint16_t handle);
+
+uint16_t gatt_db_get_end_handle(struct gatt_db *db, uint16_t handle);
-- 
1.8.5.3

[PATCH 7/8] shared/gatt: Make 'find_by_type_value' callback compatible

[Marcin Kraglak]

From: Jakub Tyszkowski <jakub.tyszkowski@tieto.com>

'Find by type and value' was handling only values written directly to
database and not those returned by callbacks or by Android Framework.
This replaces it with 'find by type' and leaves value verification to
the user.
---
 src/shared/gatt-db.c | 36 +++++++-----------------------------
 src/shared/gatt-db.h | 11 ++---------
 2 files changed, 9 insertions(+), 38 deletions(-)

diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 41457e2..3869d4f 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -485,16 +485,13 @@ struct find_by_type_value_data {
 	bt_uuid_t uuid;
 	uint16_t start_handle;
 	uint16_t end_handle;
-	uint16_t value_length;
-	const uint8_t *value;
 };
 
-static void find_by_type_value(void *data, void *user_data)
+static void find_by_type(void *data, void *user_data)
 {
 	struct find_by_type_value_data *search_data = user_data;
 	struct gatt_db_service *service = data;
 	struct gatt_db_attribute *attribute;
-	struct gatt_db_range *range;
 	int i;
 
 	if (!service->active)
@@ -513,43 +510,24 @@ static void find_by_type_value(void *data, void *user_data)
 		if (bt_uuid_cmp(&search_data->uuid, &attribute->uuid))
 			continue;
 
-		if (attribute->value_len != search_data->value_length)
-			continue;
-
-		if (!memcmp(attribute->value, search_data->value,
-							attribute->value_len))
-			continue;
-
-		range = new0(struct gatt_db_range, 1);
-		if (!range)
-			return;
-
-		range->handle = attribute->handle;
-		range->end_group = service->attributes[0]->handle +
-						service->num_handles - 1;
-
-		if (!queue_push_tail(search_data->queue, range))
-			free(range);
+		queue_push_tail(search_data->queue,
+						UINT_TO_PTR(attribute->handle));
 	}
 }
 
-void gatt_db_find_by_type_value(struct gatt_db *db, uint16_t start_handle,
+void gatt_db_find_by_type(struct gatt_db *db, uint16_t start_handle,
 							uint16_t end_handle,
-							const bt_uuid_t type,
-							const uint8_t *value,
-							uint16_t length,
+							const bt_uuid_t *type,
 							struct queue *queue)
 {
 	struct find_by_type_value_data data;
 
-	data.uuid = type;
+	data.uuid = *type;
 	data.start_handle = start_handle;
 	data.end_handle = end_handle;
 	data.queue = queue;
-	data.value_length = length;
-	data.value = value;
 
-	queue_foreach(db->services, find_by_type_value, &data);
+	queue_foreach(db->services, find_by_type, &data);
 }
 
 struct read_by_type_data {
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index ddb40da..5b2a17c 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -73,16 +73,9 @@ void gatt_db_read_by_group_type(struct gatt_db *db, uint16_t start_handle,
 							const bt_uuid_t type,
 							struct queue *queue);
 
-struct gatt_db_range {
-	uint16_t handle;
-	uint16_t end_group;
-};
-
-void gatt_db_find_by_type_value(struct gatt_db *db, uint16_t start_handle,
+void gatt_db_find_by_type(struct gatt_db *db, uint16_t start_handle,
 							uint16_t end_handle,
-							const bt_uuid_t type,
-							const uint8_t *value,
-							uint16_t length,
+							const bt_uuid_t *type,
 							struct queue *queue);
 /* List of handles */
 void gatt_db_read_by_type(struct gatt_db *db, uint16_t start_handle,
-- 
1.8.5.3

[PATCH 8/8] shared/gatt: Refactor read_by_group_type

[Marcin Kraglak]

It will return list of service's handles which have given type
and are placed in given range.
---
 android/gatt.c       | 60 ++++++++++++++++++++++++++--------------------------
 src/shared/gatt-db.c | 16 ++------------
 src/shared/gatt-db.h |  8 -------
 3 files changed, 32 insertions(+), 52 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index e53787e..c9a16cf 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -3944,36 +3944,17 @@ static const struct ipc_handler cmd_handlers[] = {
 		sizeof(struct hal_cmd_gatt_server_send_response) },
 };
 
-struct copy_att_list_data {
-	int iterator;
-	struct att_data_list *adl;
-};
-
-static void copy_to_att_list(void *data, void *user_data)
-{
-	struct copy_att_list_data *l = user_data;
-	struct gatt_db_group *group = data;
-	uint8_t *value;
-
-	value = l->adl->data[l->iterator++];
-
-	put_le16(group->handle, value);
-	put_le16(group->end_group, &value[2]);
-
-	memcpy(&value[4], group->value, group->len);
-}
-
 static uint8_t read_by_group_type(const uint8_t *cmd, uint16_t cmd_len,
 						uint8_t *rsp, size_t rsp_size,
-						uint16_t *length)
+						uint16_t *length,
+						struct gatt_device *device)
 {
 	uint16_t start, end;
 	uint16_t len;
 	bt_uuid_t uuid;
 	struct queue *q;
 	struct att_data_list *adl;
-	struct copy_att_list_data l;
-	struct gatt_db_group *d;
+	int iterator = 0;
 
 	len = dec_read_by_grp_req(cmd, cmd_len, &start, &end, &uuid);
 	if (!len)
@@ -3991,25 +3972,44 @@ static uint8_t read_by_group_type(const uint8_t *cmd, uint16_t cmd_len,
 	}
 
 	len = queue_length(q);
-	d = queue_peek_head(q);
 
 	/* Element contains start/end handle + size of uuid */
-	adl = att_data_list_alloc(len, 2 * sizeof(uint16_t) + d->len);
+	adl = att_data_list_alloc(len, 3 * sizeof(uint16_t));
 	if (!adl) {
-		queue_destroy(q, free);
+		queue_destroy(q, NULL);
 		return ATT_ECODE_INSUFF_RESOURCES;
 	}
 
-	l.iterator = 0;
-	l.adl = adl;
+	while (queue_peek_head(q)) {
+		uint16_t handle = PTR_TO_UINT(queue_pop_head(q));
+		uint16_t end_handle;
+		uint8_t *value;
+		uint16_t value_len;
+		uint8_t *val;
+
+		if (!gatt_db_read(gatt_db, handle, 0, ATT_OP_READ_BY_TYPE_REQ,
+					&device->bdaddr, &value, &value_len))
+			break;
 
-	queue_foreach(q, copy_to_att_list, &l);
+		if (!value_len) {
+			/* It should never happen. service's attribut value
+			 * must be set when creating service */
+			break;
+		}
+
+		end_handle = gatt_db_get_end_handle(gatt_db, handle);
+
+		val = adl->data[iterator++];
+		put_le16(handle, val);
+		put_le16(end_handle, &val[2]);
+		memcpy(&val[4], value, value_len);
+	}
 
 	len = enc_read_by_grp_resp(adl, rsp, rsp_size);
 	*length = len;
 
 	att_data_list_free(adl);
-	queue_destroy(q, free);
+	queue_destroy(q, NULL);
 
 	return 0;
 }
@@ -4412,7 +4412,7 @@ static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
 	switch (ipdu[0]) {
 	case ATT_OP_READ_BY_GROUP_REQ:
 		status = read_by_group_type(ipdu, len, opdu, sizeof(opdu),
-								&length);
+								&length, dev);
 		break;
 	case ATT_OP_READ_BY_TYPE_REQ:
 		status = read_by_type(ipdu, len, dev);
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 3869d4f..d44e70c 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -423,7 +423,6 @@ static void read_by_group_type(void *data, void *user_data)
 {
 	struct read_by_group_type_data *search_data = user_data;
 	struct gatt_db_service *service = data;
-	struct gatt_db_group *group;
 
 	if (!service->active)
 		return;
@@ -448,19 +447,8 @@ static void read_by_group_type(void *data, void *user_data)
 		return;
 	}
 
-	group = malloc0(sizeof(struct gatt_db_group) +
-					service->attributes[0]->value_len);
-	if (!group)
-		return;
-
-	group->len = service->attributes[0]->value_len;
-	memcpy(group->value, service->attributes[0]->value, group->len);
-	group->handle = service->attributes[0]->handle;
-	group->end_group = service->attributes[0]->handle +
-						service->num_handles - 1;
-
-	if (!queue_push_tail(search_data->queue, group))
-		free(group);
+	queue_push_tail(search_data->queue,
+			UINT_TO_PTR(service->attributes[0]->handle));
 }
 
 void gatt_db_read_by_group_type(struct gatt_db *db, uint16_t start_handle,
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index 5b2a17c..747c73b 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -60,14 +60,6 @@ uint16_t gatt_db_add_included_service(struct gatt_db *db, uint16_t handle,
 bool gatt_db_service_set_active(struct gatt_db *db, uint16_t handle,
 								bool active);
 
-struct gatt_db_group {
-	uint16_t handle;
-	uint16_t end_group;
-	uint16_t len;
-	uint8_t value[0];
-};
-
-/* Returns queue with struct gatt_db_group */
 void gatt_db_read_by_group_type(struct gatt_db *db, uint16_t start_handle,
 							uint16_t end_handle,
 							const bt_uuid_t type,
-- 
1.8.5.3

Re: [PATCH 8/8] shared/gatt: Refactor read_by_group_type

[Szymon Janc]

Hi,

On Monday 12 of May 2014 15:02:38 Marcin Kraglak wrote:
> It will return list of service's handles which have given type
> and are placed in given range.
> ---
>  android/gatt.c       | 60 ++++++++++++++++++++++++++--------------------------
>  src/shared/gatt-db.c | 16 ++------------
>  src/shared/gatt-db.h |  8 -------

Keep android and shared changes separated if possible.

>  3 files changed, 32 insertions(+), 52 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index e53787e..c9a16cf 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -3944,36 +3944,17 @@ static const struct ipc_handler cmd_handlers[] = {
>  		sizeof(struct hal_cmd_gatt_server_send_response) },
>  };
>  
> -struct copy_att_list_data {
> -	int iterator;
> -	struct att_data_list *adl;
> -};
> -
> -static void copy_to_att_list(void *data, void *user_data)
> -{
> -	struct copy_att_list_data *l = user_data;
> -	struct gatt_db_group *group = data;
> -	uint8_t *value;
> -
> -	value = l->adl->data[l->iterator++];
> -
> -	put_le16(group->handle, value);
> -	put_le16(group->end_group, &value[2]);
> -
> -	memcpy(&value[4], group->value, group->len);
> -}
> -
>  static uint8_t read_by_group_type(const uint8_t *cmd, uint16_t cmd_len,
>  						uint8_t *rsp, size_t rsp_size,
> -						uint16_t *length)
> +						uint16_t *length,
> +						struct gatt_device *device)
>  {
>  	uint16_t start, end;
>  	uint16_t len;
>  	bt_uuid_t uuid;
>  	struct queue *q;
>  	struct att_data_list *adl;
> -	struct copy_att_list_data l;
> -	struct gatt_db_group *d;
> +	int iterator = 0;
>  
>  	len = dec_read_by_grp_req(cmd, cmd_len, &start, &end, &uuid);
>  	if (!len)
> @@ -3991,25 +3972,44 @@ static uint8_t read_by_group_type(const uint8_t *cmd, uint16_t cmd_len,
>  	}
>  
>  	len = queue_length(q);
> -	d = queue_peek_head(q);
>  
>  	/* Element contains start/end handle + size of uuid */
> -	adl = att_data_list_alloc(len, 2 * sizeof(uint16_t) + d->len);
> +	adl = att_data_list_alloc(len, 3 * sizeof(uint16_t));

Is this change due to UUID being always 16bit here? If so please
update comment.

>  	if (!adl) {
> -		queue_destroy(q, free);
> +		queue_destroy(q, NULL);
>  		return ATT_ECODE_INSUFF_RESOURCES;
>  	}
>  
> -	l.iterator = 0;
> -	l.adl = adl;
> +	while (queue_peek_head(q)) {
> +		uint16_t handle = PTR_TO_UINT(queue_pop_head(q));
> +		uint16_t end_handle;
> +		uint8_t *value;
> +		uint16_t value_len;
> +		uint8_t *val;
> +
> +		if (!gatt_db_read(gatt_db, handle, 0, ATT_OP_READ_BY_TYPE_REQ,
> +					&device->bdaddr, &value, &value_len))
> +			break;
>  
> -	queue_foreach(q, copy_to_att_list, &l);
> +		if (!value_len) {
> +			/* It should never happen. service's attribut value
> +			 * must be set when creating service */

/*
 * foo
 * bar
 */


> +			break;
> +		}
> +
> +		end_handle = gatt_db_get_end_handle(gatt_db, handle);
> +
> +		val = adl->data[iterator++];
> +		put_le16(handle, val);
> +		put_le16(end_handle, &val[2]);
> +		memcpy(&val[4], value, value_len);
> +	}
>  
>  	len = enc_read_by_grp_resp(adl, rsp, rsp_size);
>  	*length = len;
>  
>  	att_data_list_free(adl);
> -	queue_destroy(q, free);
> +	queue_destroy(q, NULL);
>  
>  	return 0;
>  }
> @@ -4412,7 +4412,7 @@ static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
>  	switch (ipdu[0]) {
>  	case ATT_OP_READ_BY_GROUP_REQ:
>  		status = read_by_group_type(ipdu, len, opdu, sizeof(opdu),
> -								&length);
> +								&length, dev);
>  		break;
>  	case ATT_OP_READ_BY_TYPE_REQ:
>  		status = read_by_type(ipdu, len, dev);
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index 3869d4f..d44e70c 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -423,7 +423,6 @@ static void read_by_group_type(void *data, void *user_data)
>  {
>  	struct read_by_group_type_data *search_data = user_data;
>  	struct gatt_db_service *service = data;
> -	struct gatt_db_group *group;
>  
>  	if (!service->active)
>  		return;
> @@ -448,19 +447,8 @@ static void read_by_group_type(void *data, void *user_data)
>  		return;
>  	}
>  
> -	group = malloc0(sizeof(struct gatt_db_group) +
> -					service->attributes[0]->value_len);
> -	if (!group)
> -		return;
> -
> -	group->len = service->attributes[0]->value_len;
> -	memcpy(group->value, service->attributes[0]->value, group->len);
> -	group->handle = service->attributes[0]->handle;
> -	group->end_group = service->attributes[0]->handle +
> -						service->num_handles - 1;
> -
> -	if (!queue_push_tail(search_data->queue, group))
> -		free(group);
> +	queue_push_tail(search_data->queue,
> +			UINT_TO_PTR(service->attributes[0]->handle));
>  }
>  
>  void gatt_db_read_by_group_type(struct gatt_db *db, uint16_t start_handle,
> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
> index 5b2a17c..747c73b 100644
> --- a/src/shared/gatt-db.h
> +++ b/src/shared/gatt-db.h
> @@ -60,14 +60,6 @@ uint16_t gatt_db_add_included_service(struct gatt_db *db, uint16_t handle,
>  bool gatt_db_service_set_active(struct gatt_db *db, uint16_t handle,
>  								bool active);
>  
> -struct gatt_db_group {
> -	uint16_t handle;
> -	uint16_t end_group;
> -	uint16_t len;
> -	uint8_t value[0];
> -};
> -
> -/* Returns queue with struct gatt_db_group */
>  void gatt_db_read_by_group_type(struct gatt_db *db, uint16_t start_handle,
>  							uint16_t end_handle,
>  							const bt_uuid_t type,
> 

-- 
Best regards, 
Szymon Janc