[PATCHv5 00/14] Included service discovery

[PATCHv5 00/14] Included service discovery

[Marcin Kraglak]

v3:
In this version after primary service discovery,
secondary services are discovered. Next included
services are resolved. With this approach we
don't have recursively search for included service,
like it was TODO in previous proposal.
There is also small coding style fix suggested by Arman.

v4:
If no secondary services found, continue include services search (fixed
in gatt-client.c).
Fixed wrong debug logs (primary->secondary).
Fixed searching descriptors

v5:
Ignore Unsupported Group Type Error in response to secondary service
discovery and continue included services discovery.

Marcin Kraglak (14):
  shared/gatt: Add discover_secondary_services()
  shared/gatt: Add initial implementation of discover_included_services
  shared/gatt: Discover included services 128 bit UUIDS
  shared/gatt: Add extra check in characteristic iterator
  shared/gatt: Add included service iterator
  shared/gatt: Remove not needed function parameter
  shared/gatt: Distinguish Primary from Secondary services
  tools/btgatt-client: Print type of service
  shared/gatt: Discover secondary services
  shared/gatt: Discover included services
  shared/gatt: Add gatt-client include service iterator
  tools/btgatt-client: Print found include services
  shared/gatt: Fix searching descriptors
  shared/gatt: Add function bt_gatt_result_included_count()

 src/shared/gatt-client.c  | 263 +++++++++++++++++++++++++++--
 src/shared/gatt-client.h  |  18 ++
 src/shared/gatt-helpers.c | 418 +++++++++++++++++++++++++++++++++++++++++++---
 src/shared/gatt-helpers.h |  10 +-
 tools/btgatt-client.c     |  17 +-
 5 files changed, 690 insertions(+), 36 deletions(-)

-- 
1.9.3

[PATCHv5 01/14] shared/gatt: Add discover_secondary_services()

[Marcin Kraglak]

This pach implements searching Secondary Services in given range.
---
 src/shared/gatt-helpers.c | 56 +++++++++++++++++++++++++++++++++--------------
 src/shared/gatt-helpers.h |  5 +++++
 2 files changed, 45 insertions(+), 16 deletions(-)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index 55e6992..4dc787f 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -175,6 +175,7 @@ struct discovery_op {
 	uint16_t end_handle;
 	int ref_count;
 	bt_uuid_t uuid;
+	uint16_t service_type;
 	struct bt_gatt_result *result_head;
 	struct bt_gatt_result *result_tail;
 	bt_gatt_discovery_callback_t callback;
@@ -487,7 +488,7 @@ static void read_by_grp_type_cb(uint8_t opcode, const void *pdu,
 
 		put_le16(last_end + 1, pdu);
 		put_le16(op->end_handle, pdu + 2);
-		put_le16(GATT_PRIM_SVC_UUID, pdu + 4);
+		put_le16(op->service_type, pdu + 4);
 
 		if (bt_att_send(op->att, BT_ATT_OP_READ_BY_GRP_TYPE_REQ,
 							pdu, sizeof(pdu),
@@ -569,7 +570,7 @@ static void find_by_type_val_cb(uint8_t opcode, const void *pdu,
 
 		put_le16(last_end + 1, pdu);
 		put_le16(op->end_handle, pdu + 2);
-		put_le16(GATT_PRIM_SVC_UUID, pdu + 4);
+		put_le16(op->service_type, pdu + 4);
 		put_uuid_le(&op->uuid, pdu + 6);
 
 		if (bt_att_send(op->att, BT_ATT_OP_FIND_BY_TYPE_VAL_REQ,
@@ -594,21 +595,12 @@ done:
 		op->callback(success, att_ecode, final_result, op->user_data);
 }
 
-bool bt_gatt_discover_all_primary_services(struct bt_att *att, bt_uuid_t *uuid,
-					bt_gatt_discovery_callback_t callback,
-					void *user_data,
-					bt_gatt_destroy_func_t destroy)
-{
-	return bt_gatt_discover_primary_services(att, uuid, 0x0001, 0xffff,
-							callback, user_data,
-							destroy);
-}
-
-bool bt_gatt_discover_primary_services(struct bt_att *att, bt_uuid_t *uuid,
+static bool discover_services(struct bt_att *att, bt_uuid_t *uuid,
 					uint16_t start, uint16_t end,
 					bt_gatt_discovery_callback_t callback,
 					void *user_data,
-					bt_gatt_destroy_func_t destroy)
+					bt_gatt_destroy_func_t destroy,
+					bool primary)
 {
 	struct discovery_op *op;
 	bool result;
@@ -625,6 +617,8 @@ bool bt_gatt_discover_primary_services(struct bt_att *att, bt_uuid_t *uuid,
 	op->callback = callback;
 	op->user_data = user_data;
 	op->destroy = destroy;
+	/* set service uuid to primary or secondary */
+	op->service_type = primary ? GATT_PRIM_SVC_UUID : GATT_SND_SVC_UUID;
 
 	/* If UUID is NULL, then discover all primary services */
 	if (!uuid) {
@@ -632,7 +626,7 @@ bool bt_gatt_discover_primary_services(struct bt_att *att, bt_uuid_t *uuid,
 
 		put_le16(start, pdu);
 		put_le16(end, pdu + 2);
-		put_le16(GATT_PRIM_SVC_UUID, pdu + 4);
+		put_le16(op->service_type, pdu + 4);
 
 		result = bt_att_send(att, BT_ATT_OP_READ_BY_GRP_TYPE_REQ,
 							pdu, sizeof(pdu),
@@ -652,7 +646,7 @@ bool bt_gatt_discover_primary_services(struct bt_att *att, bt_uuid_t *uuid,
 
 		put_le16(start, pdu);
 		put_le16(end, pdu + 2);
-		put_le16(GATT_PRIM_SVC_UUID, pdu + 4);
+		put_le16(op->service_type, pdu + 4);
 		put_uuid_le(&op->uuid, pdu + 6);
 
 		result = bt_att_send(att, BT_ATT_OP_FIND_BY_TYPE_VAL_REQ,
@@ -668,6 +662,36 @@ bool bt_gatt_discover_primary_services(struct bt_att *att, bt_uuid_t *uuid,
 	return result;
 }
 
+bool bt_gatt_discover_all_primary_services(struct bt_att *att, bt_uuid_t *uuid,
+					bt_gatt_discovery_callback_t callback,
+					void *user_data,
+					bt_gatt_destroy_func_t destroy)
+{
+	return bt_gatt_discover_primary_services(att, uuid, 0x0001, 0xffff,
+							callback, user_data,
+							destroy);
+}
+
+bool bt_gatt_discover_primary_services(struct bt_att *att, bt_uuid_t *uuid,
+					uint16_t start, uint16_t end,
+					bt_gatt_discovery_callback_t callback,
+					void *user_data,
+					bt_gatt_destroy_func_t destroy)
+{
+	return discover_services(att, uuid, start, end, callback, user_data,
+								destroy, true);
+}
+
+bool bt_gatt_discover_secondary_services(struct bt_att *att, bt_uuid_t *uuid,
+					uint16_t start, uint16_t end,
+					bt_gatt_discovery_callback_t callback,
+					void *user_data,
+					bt_gatt_destroy_func_t destroy)
+{
+	return discover_services(att, uuid, start, end, callback, user_data,
+								destroy, false);
+}
+
 bool bt_gatt_discover_included_services(struct bt_att *att,
 					uint16_t start, uint16_t end,
 					bt_uuid_t *uuid,
diff --git a/src/shared/gatt-helpers.h b/src/shared/gatt-helpers.h
index f6f4b62..8a25dea 100644
--- a/src/shared/gatt-helpers.h
+++ b/src/shared/gatt-helpers.h
@@ -72,6 +72,11 @@ bool bt_gatt_discover_primary_services(struct bt_att *att, bt_uuid_t *uuid,
 					bt_gatt_discovery_callback_t callback,
 					void *user_data,
 					bt_gatt_destroy_func_t destroy);
+bool bt_gatt_discover_secondary_services(struct bt_att *att, bt_uuid_t *uuid,
+					uint16_t start, uint16_t end,
+					bt_gatt_discovery_callback_t callback,
+					void *user_data,
+					bt_gatt_destroy_func_t destroy);
 bool bt_gatt_discover_included_services(struct bt_att *att,
 					uint16_t start, uint16_t end,
 					bt_uuid_t *uuid,
-- 
1.9.3

Re: [PATCHv5 01/14] shared/gatt: Add discover_secondary_services()

[Szymon Janc]

Hi Marcin,

On Thursday 16 of October 2014 12:17:13 Marcin Kraglak wrote:
> This pach implements searching Secondary Services in given range.

typo pach -> patch

> ---
>  src/shared/gatt-helpers.c | 56 +++++++++++++++++++++++++++++++++--------------
>  src/shared/gatt-helpers.h |  5 +++++
>  2 files changed, 45 insertions(+), 16 deletions(-)
> 
> diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
> index 55e6992..4dc787f 100644
> --- a/src/shared/gatt-helpers.c
> +++ b/src/shared/gatt-helpers.c
> @@ -175,6 +175,7 @@ struct discovery_op {
>  	uint16_t end_handle;
>  	int ref_count;
>  	bt_uuid_t uuid;
> +	uint16_t service_type;
>  	struct bt_gatt_result *result_head;
>  	struct bt_gatt_result *result_tail;
>  	bt_gatt_discovery_callback_t callback;
> @@ -487,7 +488,7 @@ static void read_by_grp_type_cb(uint8_t opcode, const void *pdu,
>  
>  		put_le16(last_end + 1, pdu);
>  		put_le16(op->end_handle, pdu + 2);
> -		put_le16(GATT_PRIM_SVC_UUID, pdu + 4);
> +		put_le16(op->service_type, pdu + 4);
>  
>  		if (bt_att_send(op->att, BT_ATT_OP_READ_BY_GRP_TYPE_REQ,
>  							pdu, sizeof(pdu),
> @@ -569,7 +570,7 @@ static void find_by_type_val_cb(uint8_t opcode, const void *pdu,
>  
>  		put_le16(last_end + 1, pdu);
>  		put_le16(op->end_handle, pdu + 2);
> -		put_le16(GATT_PRIM_SVC_UUID, pdu + 4);
> +		put_le16(op->service_type, pdu + 4);
>  		put_uuid_le(&op->uuid, pdu + 6);
>  
>  		if (bt_att_send(op->att, BT_ATT_OP_FIND_BY_TYPE_VAL_REQ,
> @@ -594,21 +595,12 @@ done:
>  		op->callback(success, att_ecode, final_result, op->user_data);
>  }
>  
> -bool bt_gatt_discover_all_primary_services(struct bt_att *att, bt_uuid_t *uuid,
> -					bt_gatt_discovery_callback_t callback,
> -					void *user_data,
> -					bt_gatt_destroy_func_t destroy)
> -{
> -	return bt_gatt_discover_primary_services(att, uuid, 0x0001, 0xffff,
> -							callback, user_data,
> -							destroy);
> -}
> -
> -bool bt_gatt_discover_primary_services(struct bt_att *att, bt_uuid_t *uuid,
> +static bool discover_services(struct bt_att *att, bt_uuid_t *uuid,
>  					uint16_t start, uint16_t end,
>  					bt_gatt_discovery_callback_t callback,
>  					void *user_data,
> -					bt_gatt_destroy_func_t destroy)
> +					bt_gatt_destroy_func_t destroy,
> +					bool primary)
>  {
>  	struct discovery_op *op;
>  	bool result;
> @@ -625,6 +617,8 @@ bool bt_gatt_discover_primary_services(struct bt_att *att, bt_uuid_t *uuid,
>  	op->callback = callback;
>  	op->user_data = user_data;
>  	op->destroy = destroy;
> +	/* set service uuid to primary or secondary */
> +	op->service_type = primary ? GATT_PRIM_SVC_UUID : GATT_SND_SVC_UUID;
>  
>  	/* If UUID is NULL, then discover all primary services */
>  	if (!uuid) {
> @@ -632,7 +626,7 @@ bool bt_gatt_discover_primary_services(struct bt_att *att, bt_uuid_t *uuid,
>  
>  		put_le16(start, pdu);
>  		put_le16(end, pdu + 2);
> -		put_le16(GATT_PRIM_SVC_UUID, pdu + 4);
> +		put_le16(op->service_type, pdu + 4);
>  
>  		result = bt_att_send(att, BT_ATT_OP_READ_BY_GRP_TYPE_REQ,
>  							pdu, sizeof(pdu),
> @@ -652,7 +646,7 @@ bool bt_gatt_discover_primary_services(struct bt_att *att, bt_uuid_t *uuid,
>  
>  		put_le16(start, pdu);
>  		put_le16(end, pdu + 2);
> -		put_le16(GATT_PRIM_SVC_UUID, pdu + 4);
> +		put_le16(op->service_type, pdu + 4);
>  		put_uuid_le(&op->uuid, pdu + 6);
>  
>  		result = bt_att_send(att, BT_ATT_OP_FIND_BY_TYPE_VAL_REQ,
> @@ -668,6 +662,36 @@ bool bt_gatt_discover_primary_services(struct bt_att *att, bt_uuid_t *uuid,
>  	return result;
>  }
>  
> +bool bt_gatt_discover_all_primary_services(struct bt_att *att, bt_uuid_t *uuid,
> +					bt_gatt_discovery_callback_t callback,
> +					void *user_data,
> +					bt_gatt_destroy_func_t destroy)
> +{
> +	return bt_gatt_discover_primary_services(att, uuid, 0x0001, 0xffff,
> +							callback, user_data,
> +							destroy);
> +}
> +
> +bool bt_gatt_discover_primary_services(struct bt_att *att, bt_uuid_t *uuid,
> +					uint16_t start, uint16_t end,
> +					bt_gatt_discovery_callback_t callback,
> +					void *user_data,
> +					bt_gatt_destroy_func_t destroy)
> +{
> +	return discover_services(att, uuid, start, end, callback, user_data,
> +								destroy, true);
> +}
> +
> +bool bt_gatt_discover_secondary_services(struct bt_att *att, bt_uuid_t *uuid,
> +					uint16_t start, uint16_t end,
> +					bt_gatt_discovery_callback_t callback,
> +					void *user_data,
> +					bt_gatt_destroy_func_t destroy)
> +{
> +	return discover_services(att, uuid, start, end, callback, user_data,
> +								destroy, false);
> +}
> +
>  bool bt_gatt_discover_included_services(struct bt_att *att,
>  					uint16_t start, uint16_t end,
>  					bt_uuid_t *uuid,
> diff --git a/src/shared/gatt-helpers.h b/src/shared/gatt-helpers.h
> index f6f4b62..8a25dea 100644
> --- a/src/shared/gatt-helpers.h
> +++ b/src/shared/gatt-helpers.h
> @@ -72,6 +72,11 @@ bool bt_gatt_discover_primary_services(struct bt_att *att, bt_uuid_t *uuid,
>  					bt_gatt_discovery_callback_t callback,
>  					void *user_data,
>  					bt_gatt_destroy_func_t destroy);
> +bool bt_gatt_discover_secondary_services(struct bt_att *att, bt_uuid_t *uuid,
> +					uint16_t start, uint16_t end,
> +					bt_gatt_discovery_callback_t callback,
> +					void *user_data,
> +					bt_gatt_destroy_func_t destroy);
>  bool bt_gatt_discover_included_services(struct bt_att *att,
>  					uint16_t start, uint16_t end,
>  					bt_uuid_t *uuid,

This is not related to this patch only but I have a feeling that those
functions names are getting to long. Maybe we should think of something
more compact?

-- 
Best regards, 
Szymon Janc

[PATCHv5 02/14] shared/gatt: Add initial implementation of discover_included_services

[Marcin Kraglak]

Current implementation allow to discover included services with
16 bit UUID only.
---
 src/shared/gatt-helpers.c | 108 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 106 insertions(+), 2 deletions(-)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index 4dc787f..dcb2a2f 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -692,6 +692,82 @@ bool bt_gatt_discover_secondary_services(struct bt_att *att, bt_uuid_t *uuid,
 								destroy, false);
 }
 
+static void discover_included_cb(uint8_t opcode, const void *pdu,
+					uint16_t length, void *user_data)
+{
+	struct bt_gatt_result *final_result = NULL;
+	struct discovery_op *op = user_data;
+	struct bt_gatt_result *cur_result;
+	uint8_t att_ecode = 0;
+	uint16_t last_handle;
+	size_t data_length;
+	bool success;
+
+	if (opcode == BT_ATT_OP_ERROR_RSP) {
+		success = false;
+		att_ecode = process_error(pdu, length);
+
+		if (att_ecode == BT_ATT_ERROR_ATTRIBUTE_NOT_FOUND &&
+							op->result_head)
+			goto success;
+
+		goto done;
+	}
+
+	if (opcode != BT_ATT_OP_READ_BY_TYPE_RSP || !pdu || length < 6) {
+		success = false;
+		goto done;
+	}
+
+	data_length = ((uint8_t *) pdu)[0];
+
+	if ((length - 1) % data_length || data_length != 8) {
+		success = false;
+		goto done;
+	}
+
+	cur_result = result_create(opcode, pdu + 1, length - 1,
+							data_length, op);
+	if (!cur_result) {
+		success = false;
+		goto done;
+	}
+
+	if (!op->result_head)
+		op->result_head = op->result_tail = cur_result;
+	else {
+		op->result_tail->next = cur_result;
+		op->result_tail = cur_result;
+	}
+
+	last_handle = get_le16(pdu + length - data_length);
+	if (last_handle != op->end_handle) {
+		uint8_t pdu[6];
+
+		put_le16(last_handle + 1, pdu);
+		put_le16(op->end_handle, pdu + 2);
+		put_le16(GATT_INCLUDE_UUID, pdu + 4);
+
+		if (bt_att_send(op->att, BT_ATT_OP_READ_BY_TYPE_REQ,
+							pdu, sizeof(pdu),
+							discover_included_cb,
+							discovery_op_ref(op),
+							discovery_op_unref))
+			return;
+
+		discovery_op_unref(op);
+		success = false;
+	}
+
+success:
+	success = true;
+	final_result = op->result_head;
+
+done:
+	if (op->callback)
+		op->callback(success, att_ecode, final_result, op->user_data);
+}
+
 bool bt_gatt_discover_included_services(struct bt_att *att,
 					uint16_t start, uint16_t end,
 					bt_uuid_t *uuid,
@@ -699,8 +775,36 @@ bool bt_gatt_discover_included_services(struct bt_att *att,
 					void *user_data,
 					bt_gatt_destroy_func_t destroy)
 {
-	/* TODO */
-	return false;
+	struct discovery_op *op;
+	uint8_t pdu[6];
+
+	if (!att)
+		return false;
+
+	op = new0(struct discovery_op, 1);
+	if (!op)
+		return false;
+
+	op->att = att;
+	op->callback = callback;
+	op->user_data = user_data;
+	op->destroy = destroy;
+	op->end_handle = end;
+
+	put_le16(start, pdu);
+	put_le16(end, pdu + 2);
+	put_le16(GATT_INCLUDE_UUID, pdu + 4);
+
+	if (!bt_att_send(att, BT_ATT_OP_READ_BY_TYPE_REQ,
+					pdu, sizeof(pdu),
+					discover_included_cb,
+					discovery_op_ref(op),
+					discovery_op_unref)) {
+		free(op);
+		return false;
+	}
+
+	return true;
 }
 
 static void discover_chrcs_cb(uint8_t opcode, const void *pdu,
-- 
1.9.3

Re: [PATCHv5 02/14] shared/gatt: Add initial implementation of discover_included_services

[Szymon Janc]

Hi Marcin,

On Thursday 16 of October 2014 12:17:14 Marcin Kraglak wrote:
> Current implementation allow to discover included services with
> 16 bit UUID only.
> ---
>  src/shared/gatt-helpers.c | 108 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 106 insertions(+), 2 deletions(-)
> 
> diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
> index 4dc787f..dcb2a2f 100644
> --- a/src/shared/gatt-helpers.c
> +++ b/src/shared/gatt-helpers.c
> @@ -692,6 +692,82 @@ bool bt_gatt_discover_secondary_services(struct bt_att *att, bt_uuid_t *uuid,
>  								destroy, false);
>  }
>  
> +static void discover_included_cb(uint8_t opcode, const void *pdu,
> +					uint16_t length, void *user_data)
> +{
> +	struct bt_gatt_result *final_result = NULL;
> +	struct discovery_op *op = user_data;
> +	struct bt_gatt_result *cur_result;
> +	uint8_t att_ecode = 0;
> +	uint16_t last_handle;
> +	size_t data_length;
> +	bool success;
> +
> +	if (opcode == BT_ATT_OP_ERROR_RSP) {
> +		success = false;

set this right before goto , otherwise it looks strange to
set success to false and then jump to success label.

> +		att_ecode = process_error(pdu, length);
> +
> +		if (att_ecode == BT_ATT_ERROR_ATTRIBUTE_NOT_FOUND &&
> +							op->result_head)
> +			goto success;
> +
> +		goto done;
> +	}
> +
> +	if (opcode != BT_ATT_OP_READ_BY_TYPE_RSP || !pdu || length < 6) {
> +		success = false;
> +		goto done;
> +	}
> +
> +	data_length = ((uint8_t *) pdu)[0];

pdu is const pointer so cast it to (const uint8_t *).
Also I'm not entirely sure about this byte pdu processing, maybe we should have
proper packed structs for those (not sure how feasible that would be, though)

> +

Please comment this in code.

> +	if ((length - 1) % data_length || data_length != 8) {
> +		success = false;
> +		goto done;
> +	}
> +
> +	cur_result = result_create(opcode, pdu + 1, length - 1,
> +							data_length, op);
> +	if (!cur_result) {
> +		success = false;
> +		goto done;
> +	}
> +
> +	if (!op->result_head)
> +		op->result_head = op->result_tail = cur_result;

Both branches should in braces.

> +	else {
> +		op->result_tail->next = cur_result;
> +		op->result_tail = cur_result;
> +	}
> +
> +	last_handle = get_le16(pdu + length - data_length);
> +	if (last_handle != op->end_handle) {
> +		uint8_t pdu[6];
> +
> +		put_le16(last_handle + 1, pdu);
> +		put_le16(op->end_handle, pdu + 2);
> +		put_le16(GATT_INCLUDE_UUID, pdu + 4);
> +
> +		if (bt_att_send(op->att, BT_ATT_OP_READ_BY_TYPE_REQ,
> +							pdu, sizeof(pdu),
> +							discover_included_cb,
> +							discovery_op_ref(op),
> +							discovery_op_unref))
> +			return;
> +
> +		discovery_op_unref(op);
> +		success = false;

goto done: missing maybe?

also I'd change labels' names a bit:
done -> failed
success -> done

> +	}
> +
> +success:
> +	success = true;
> +	final_result = op->result_head;
> +
> +done:
> +	if (op->callback)
> +		op->callback(success, att_ecode, final_result, op->user_data);
> +}
> +
>  bool bt_gatt_discover_included_services(struct bt_att *att,
>  					uint16_t start, uint16_t end,
>  					bt_uuid_t *uuid,
> @@ -699,8 +775,36 @@ bool bt_gatt_discover_included_services(struct bt_att *att,
>  					void *user_data,
>  					bt_gatt_destroy_func_t destroy)
>  {
> -	/* TODO */
> -	return false;
> +	struct discovery_op *op;
> +	uint8_t pdu[6];
> +
> +	if (!att)
> +		return false;
> +
> +	op = new0(struct discovery_op, 1);
> +	if (!op)
> +		return false;
> +
> +	op->att = att;
> +	op->callback = callback;
> +	op->user_data = user_data;
> +	op->destroy = destroy;
> +	op->end_handle = end;
> +
> +	put_le16(start, pdu);
> +	put_le16(end, pdu + 2);
> +	put_le16(GATT_INCLUDE_UUID, pdu + 4);
> +
> +	if (!bt_att_send(att, BT_ATT_OP_READ_BY_TYPE_REQ,
> +					pdu, sizeof(pdu),
> +					discover_included_cb,
> +					discovery_op_ref(op),
> +					discovery_op_unref)) {
> +		free(op);
> +		return false;
> +	}
> +
> +	return true;
>  }
>  
>  static void discover_chrcs_cb(uint8_t opcode, const void *pdu,
> 

-- 
Best regards, 
Szymon Janc

[PATCHv5 03/14] shared/gatt: Discover included services 128 bit UUIDS

[Marcin Kraglak]

If included services has 128 bit UUID, it won't be returned in
READ_BY_TYPE_RSP. To get UUID READ_REQUEST is used.
This procedure is described in CORE SPEC 4.5.1 "Find Included Services".
---
 src/shared/gatt-helpers.c | 170 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 169 insertions(+), 1 deletion(-)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index dcb2a2f..c18f738 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -692,6 +692,160 @@ bool bt_gatt_discover_secondary_services(struct bt_att *att, bt_uuid_t *uuid,
 								destroy, false);
 }
 
+struct read_incl_data {
+	struct discovery_op *op;
+	struct bt_gatt_result *result;
+	int pos;
+	int ref_count;
+};
+
+static struct read_incl_data *new_read_included(struct bt_gatt_result *res)
+{
+	struct read_incl_data *data;
+
+	data = new0(struct read_incl_data, 1);
+	if (!data)
+		return NULL;
+
+	data->op = discovery_op_ref(res->op);
+	data->result = res;
+
+	return data;
+};
+
+static struct read_incl_data *read_included_ref(struct read_incl_data *data)
+{
+	__sync_fetch_and_add(&data->ref_count, 1);
+
+	return data;
+}
+
+static void read_included_unref(void *data)
+{
+	struct read_incl_data *read_data = data;
+
+	if (__sync_sub_and_fetch(&read_data->ref_count, 1))
+		return;
+
+	discovery_op_unref(read_data->op);
+
+	free(read_data);
+}
+
+static void discover_included_cb(uint8_t opcode, const void *pdu,
+					uint16_t length, void *user_data);
+
+static void read_included_cb(uint8_t opcode, const void *pdu,
+					uint16_t length, void *user_data)
+{
+	struct read_incl_data *data = user_data;
+	struct bt_gatt_result *final_result = NULL;
+	struct discovery_op *op = data->op;
+	struct bt_gatt_result *cur_result;
+	uint8_t att_ecode = 0;
+	uint16_t handle;
+	uint8_t read_pdu[2];
+	bool success;
+
+	if (opcode == BT_ATT_OP_ERROR_RSP) {
+		success = false;
+		att_ecode = process_error(pdu, length);
+		goto done;
+	}
+
+	if (opcode != BT_ATT_OP_READ_RSP || (!pdu && length)) {
+		success = false;
+		goto done;
+	}
+
+	if (length != 16) {
+		success = false;
+		goto done;
+	}
+	cur_result = result_create(opcode, pdu, length, length, op);
+	if (!cur_result) {
+		success = false;
+		goto done;
+	}
+
+	if (!op->result_head)
+		op->result_head = op->result_tail = cur_result;
+	else {
+		op->result_tail->next = cur_result;
+		op->result_tail = cur_result;
+	}
+
+	if (data->pos == data->result->pdu_len) {
+		uint16_t last_handle, data_len;
+		uint8_t pdu[6];
+
+		data_len = data->result->data_len;
+		last_handle = get_le16(data->result->pdu + data->pos -
+								data_len);
+		if (last_handle == op->end_handle) {
+			final_result = op->result_head;
+			success = true;
+			goto done;
+		}
+
+		put_le16(last_handle + 1, pdu);
+		put_le16(op->end_handle, pdu + 2);
+		put_le16(GATT_INCLUDE_UUID, pdu + 4);
+
+		if (bt_att_send(op->att, BT_ATT_OP_READ_BY_TYPE_REQ,
+						pdu, sizeof(pdu),
+						discover_included_cb,
+						discovery_op_ref(op),
+						discovery_op_unref))
+			return;
+
+		discovery_op_unref(op);
+		success = false;
+		goto done;
+	}
+
+	handle = get_le16(data->result->pdu + data->pos + 2);
+	put_le16(handle, read_pdu);
+
+	data->pos += data->result->data_len;
+
+	if (bt_att_send(op->att, BT_ATT_OP_READ_REQ, read_pdu, sizeof(read_pdu),
+							read_included_cb,
+							read_included_ref(data),
+							read_included_unref))
+		return;
+
+	read_included_unref(data);
+	success = false;
+
+done:
+	if (op->callback)
+		op->callback(success, att_ecode, final_result, op->user_data);
+}
+
+static void read_included(struct read_incl_data *data)
+{
+	struct discovery_op *op = data->op;
+	uint16_t handle;
+	uint8_t pdu[2];
+
+	handle = get_le16(data->result->pdu + 2);
+	put_le16(handle, pdu);
+
+	data->pos += data->result->data_len;
+
+	if (bt_att_send(op->att, BT_ATT_OP_READ_REQ, pdu, sizeof(pdu),
+							read_included_cb,
+							read_included_ref(data),
+							read_included_unref))
+		return;
+
+	read_included_unref(data);
+
+	if (op->callback)
+		op->callback(false, 0, NULL, data->op->user_data);
+}
+
 static void discover_included_cb(uint8_t opcode, const void *pdu,
 					uint16_t length, void *user_data)
 {
@@ -721,7 +875,8 @@ static void discover_included_cb(uint8_t opcode, const void *pdu,
 
 	data_length = ((uint8_t *) pdu)[0];
 
-	if ((length - 1) % data_length || data_length != 8) {
+	if (((length - 1) % data_length) ||
+				(data_length != 8 && data_length != 6)) {
 		success = false;
 		goto done;
 	}
@@ -740,6 +895,19 @@ static void discover_included_cb(uint8_t opcode, const void *pdu,
 		op->result_tail = cur_result;
 	}
 
+	if (data_length == 6) {
+		struct read_incl_data *data;
+
+		data = new_read_included(cur_result);
+		if (!data) {
+			success = false;
+			goto done;
+		}
+
+		read_included(data);
+		return;
+	}
+
 	last_handle = get_le16(pdu + length - data_length);
 	if (last_handle != op->end_handle) {
 		uint8_t pdu[6];
-- 
1.9.3

Re: [PATCHv5 03/14] shared/gatt: Discover included services 128 bit UUIDS

[Szymon Janc]

Hi Marcin,

On Thursday 16 October 2014 12:17:15 Marcin Kraglak wrote:
> If included services has 128 bit UUID, it won't be returned in
> READ_BY_TYPE_RSP. To get UUID READ_REQUEST is used.
> This procedure is described in CORE SPEC 4.5.1 "Find Included Services".
> ---
>  src/shared/gatt-helpers.c | 170
> +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 169
> insertions(+), 1 deletion(-)
> 
> diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
> index dcb2a2f..c18f738 100644
> --- a/src/shared/gatt-helpers.c
> +++ b/src/shared/gatt-helpers.c
> @@ -692,6 +692,160 @@ bool bt_gatt_discover_secondary_services(struct bt_att
> *att, bt_uuid_t *uuid, destroy, false);
>  }
> 
> +struct read_incl_data {
> +	struct discovery_op *op;
> +	struct bt_gatt_result *result;
> +	int pos;
> +	int ref_count;
> +};
> +
> +static struct read_incl_data *new_read_included(struct bt_gatt_result *res)
> +{
> +	struct read_incl_data *data;
> +
> +	data = new0(struct read_incl_data, 1);
> +	if (!data)
> +		return NULL;
> +
> +	data->op = discovery_op_ref(res->op);
> +	data->result = res;
> +
> +	return data;
> +};
> +
> +static struct read_incl_data *read_included_ref(struct read_incl_data
> *data) +{
> +	__sync_fetch_and_add(&data->ref_count, 1);
> +
> +	return data;
> +}
> +
> +static void read_included_unref(void *data)
> +{
> +	struct read_incl_data *read_data = data;
> +
> +	if (__sync_sub_and_fetch(&read_data->ref_count, 1))
> +		return;
> +
> +	discovery_op_unref(read_data->op);
> +
> +	free(read_data);
> +}
> +
> +static void discover_included_cb(uint8_t opcode, const void *pdu,
> +					uint16_t length, void *user_data);
> +
> +static void read_included_cb(uint8_t opcode, const void *pdu,
> +					uint16_t length, void *user_data)
> +{
> +	struct read_incl_data *data = user_data;
> +	struct bt_gatt_result *final_result = NULL;
> +	struct discovery_op *op = data->op;
> +	struct bt_gatt_result *cur_result;
> +	uint8_t att_ecode = 0;
> +	uint16_t handle;
> +	uint8_t read_pdu[2];
> +	bool success;
> +
> +	if (opcode == BT_ATT_OP_ERROR_RSP) {
> +		success = false;
> +		att_ecode = process_error(pdu, length);
> +		goto done;
> +	}
> +
> +	if (opcode != BT_ATT_OP_READ_RSP || (!pdu && length)) {
> +		success = false;
> +		goto done;
> +	}
> +
> +	if (length != 16) {

I think we should have those magic numbers defined (this is more general 
comment since more code here uses magic values). It might be clear for you now 
why this must be 16 but won't be in 2 months :)

> +		success = false;
> +		goto done;
> +	}



Empty line here.

> +	cur_result = result_create(opcode, pdu, length, length, op);
> +	if (!cur_result) {
> +		success = false;
> +		goto done;
> +	}
> +
> +	if (!op->result_head)
> +		op->result_head = op->result_tail = cur_result;

Braces on both branches.

> +	else {
> +		op->result_tail->next = cur_result;
> +		op->result_tail = cur_result;
> +	}
> +
> +	if (data->pos == data->result->pdu_len) {
> +		uint16_t last_handle, data_len;
> +		uint8_t pdu[6];
> +
> +		data_len = data->result->data_len;
> +		last_handle = get_le16(data->result->pdu + data->pos -
> +								data_len);

This data_len variable doesn't make code clearer.

> +		if (last_handle == op->end_handle) {
> +			final_result = op->result_head;
> +			success = true;
> +			goto done;
> +		}
> +
> +		put_le16(last_handle + 1, pdu);
> +		put_le16(op->end_handle, pdu + 2);
> +		put_le16(GATT_INCLUDE_UUID, pdu + 4);
> +
> +		if (bt_att_send(op->att, BT_ATT_OP_READ_BY_TYPE_REQ,
> +						pdu, sizeof(pdu),
> +						discover_included_cb,
> +						discovery_op_ref(op),
> +						discovery_op_unref))

This should fit just in 2 lines, instead of 4.

> +			return;
> +
> +		discovery_op_unref(op);
> +		success = false;
> +		goto done;
> +	}
> +
> +	handle = get_le16(data->result->pdu + data->pos + 2);
> +	put_le16(handle, read_pdu);

So if both are LE value why not just memcpy() here?

> +
> +	data->pos += data->result->data_len;
> +
> +	if (bt_att_send(op->att, BT_ATT_OP_READ_REQ, read_pdu, sizeof(read_pdu),
> +							read_included_cb,
> +							read_included_ref(data),
> +							read_included_unref))

Ditto.

> +		return;
> +
> +	read_included_unref(data);
> +	success = false;
> +
> +done:
> +	if (op->callback)
> +		op->callback(success, att_ecode, final_result, op->user_data);
> +}
> +
> +static void read_included(struct read_incl_data *data)
> +{
> +	struct discovery_op *op = data->op;
> +	uint16_t handle;
> +	uint8_t pdu[2];
> +
> +	handle = get_le16(data->result->pdu + 2);
> +	put_le16(handle, pdu);

memcpy

> +
> +	data->pos += data->result->data_len;
> +
> +	if (bt_att_send(op->att, BT_ATT_OP_READ_REQ, pdu, sizeof(pdu),
> +							read_included_cb,
> +							read_included_ref(data),
> +							read_included_unref))
> +		return;
> +
> +	read_included_unref(data);
> +
> +	if (op->callback)
> +		op->callback(false, 0, NULL, data->op->user_data);
> +}
> +
>  static void discover_included_cb(uint8_t opcode, const void *pdu,
>  					uint16_t length, void *user_data)
>  {
> @@ -721,7 +875,8 @@ static void discover_included_cb(uint8_t opcode, const
> void *pdu,
> 
>  	data_length = ((uint8_t *) pdu)[0];
> 
> -	if ((length - 1) % data_length || data_length != 8) {
> +	if (((length - 1) % data_length) ||
> +				(data_length != 8 && data_length != 6)) {
>  		success = false;
>  		goto done;
>  	}
> @@ -740,6 +895,19 @@ static void discover_included_cb(uint8_t opcode, const
> void *pdu, op->result_tail = cur_result;
>  	}
> 
> +	if (data_length == 6) {
> +		struct read_incl_data *data;
> +
> +		data = new_read_included(cur_result);
> +		if (!data) {
> +			success = false;
> +			goto done;
> +		}
> +
> +		read_included(data);
> +		return;
> +	}
> +
>  	last_handle = get_le16(pdu + length - data_length);
>  	if (last_handle != op->end_handle) {
>  		uint8_t pdu[6];

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

[PATCHv5 04/14] shared/gatt: Add extra check in characteristic iterator

[Marcin Kraglak]

Avoid incorrect reading of included service discovery results.
---
 src/shared/gatt-helpers.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index c18f738..58104d9 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -123,6 +123,9 @@ unsigned int bt_gatt_result_characteristic_count(struct bt_gatt_result *result)
 	if (result->opcode != BT_ATT_OP_READ_BY_TYPE_RSP)
 		return 0;
 
+	if (result->data_len != 21 && result->data_len != 7)
+		return 0;
+
 	return result_element_count(result);
 }
 
@@ -239,6 +242,9 @@ bool bt_gatt_iter_next_characteristic(struct bt_gatt_iter *iter,
 	if (iter->result->opcode != BT_ATT_OP_READ_BY_TYPE_RSP)
 		return false;
 
+	if (iter->result->data_len != 21 && iter->result->data_len != 7)
+		return false;
+
 	op = iter->result->op;
 	pdu_ptr = iter->result->pdu + iter->pos;
 
-- 
1.9.3

Re: [PATCHv5 04/14] shared/gatt: Add extra check in characteristic iterator

[Szymon Janc]

Hi Marcin,

On Thursday 16 October 2014 12:17:16 Marcin Kraglak wrote:
> Avoid incorrect reading of included service discovery results.
> ---
>  src/shared/gatt-helpers.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
> index c18f738..58104d9 100644
> --- a/src/shared/gatt-helpers.c
> +++ b/src/shared/gatt-helpers.c
> @@ -123,6 +123,9 @@ unsigned int bt_gatt_result_characteristic_count(struct
> bt_gatt_result *result) if (result->opcode != BT_ATT_OP_READ_BY_TYPE_RSP)
>  		return 0;
> 
> +	if (result->data_len != 21 && result->data_len != 7)
> +		return 0;

This really needs to be commented (or better use meaningful defines instead of 
magic values).

> +
>  	return result_element_count(result);
>  }
> 
> @@ -239,6 +242,9 @@ bool bt_gatt_iter_next_characteristic(struct
> bt_gatt_iter *iter, if (iter->result->opcode != BT_ATT_OP_READ_BY_TYPE_RSP)
>  		return false;
> 
> +	if (iter->result->data_len != 21 && iter->result->data_len != 7)
> +		return false;

Ditto.

> +
>  	op = iter->result->op;
>  	pdu_ptr = iter->result->pdu + iter->pos;

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

[PATCHv5 05/14] shared/gatt: Add included service iterator

[Marcin Kraglak]

It will fetch included services from result.
---
 src/shared/gatt-helpers.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/gatt-helpers.h |  3 +++
 2 files changed, 61 insertions(+)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index 58104d9..fd8d06c 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -186,6 +186,64 @@ struct discovery_op {
 	bt_gatt_destroy_func_t destroy;
 };
 
+bool bt_gatt_iter_next_included_service(struct bt_gatt_iter *iter,
+				uint16_t *handle, uint16_t *start_handle,
+				uint16_t *end_handle, uint8_t uuid[16])
+{
+	struct bt_gatt_result *read_result;
+	const void *pdu_ptr;
+	int i = 0;
+
+	if (!iter || !iter->result || !handle || !start_handle || !end_handle
+								|| !uuid)
+		return false;
+
+	if (iter->result->opcode != BT_ATT_OP_READ_BY_TYPE_RSP)
+		return false;
+
+	if (iter->result->data_len != 8 && iter->result->data_len != 6)
+		return false;
+
+	pdu_ptr = iter->result->pdu + iter->pos;
+
+	if (iter->result->data_len == 8) {
+		*handle = get_le16(pdu_ptr);
+		*start_handle = get_le16(pdu_ptr + 2);
+		*end_handle = get_le16(pdu_ptr + 4);
+		convert_uuid_le(pdu_ptr + 6, 2, uuid);
+
+		iter->pos += iter->result->data_len;
+
+		if (iter->pos == iter->result->pdu_len) {
+			iter->result = iter->result->next;
+			iter->pos = 0;
+		}
+
+		return true;
+	}
+
+	*handle = get_le16(pdu_ptr);
+	*start_handle = get_le16(pdu_ptr + 2);
+	*end_handle = get_le16(pdu_ptr + 4);
+	read_result = iter->result;
+
+	do {
+		read_result = read_result->next;
+	} while (read_result && i++ < (iter->pos / iter->result->data_len));
+
+	if (!read_result)
+		return false;
+
+	convert_uuid_le(read_result->pdu, read_result->data_len, uuid);
+	iter->pos += iter->result->data_len;
+	if (iter->pos == iter->result->pdu_len) {
+		iter->result = read_result->next;
+		iter->pos = 0;
+	}
+
+	return true;
+}
+
 bool bt_gatt_iter_next_service(struct bt_gatt_iter *iter,
 				uint16_t *start_handle, uint16_t *end_handle,
 				uint8_t uuid[16])
diff --git a/src/shared/gatt-helpers.h b/src/shared/gatt-helpers.h
index 8a25dea..8c434c1 100644
--- a/src/shared/gatt-helpers.h
+++ b/src/shared/gatt-helpers.h
@@ -49,6 +49,9 @@ bool bt_gatt_iter_next_characteristic(struct bt_gatt_iter *iter,
 				uint8_t uuid[16]);
 bool bt_gatt_iter_next_descriptor(struct bt_gatt_iter *iter, uint16_t *handle,
 							uint8_t uuid[16]);
+bool bt_gatt_iter_next_included_service(struct bt_gatt_iter *iter,
+				uint16_t *handle, uint16_t *start_handle,
+				uint16_t *end_handle, uint8_t uuid[16]);
 
 typedef void (*bt_gatt_destroy_func_t)(void *user_data);
 
-- 
1.9.3

Re: [PATCHv5 05/14] shared/gatt: Add included service iterator

[Szymon Janc]

Hi Marcin,

On Thursday 16 October 2014 12:17:17 Marcin Kraglak wrote:
> It will fetch included services from result.
> ---
>  src/shared/gatt-helpers.c | 58
> +++++++++++++++++++++++++++++++++++++++++++++++ src/shared/gatt-helpers.h |
>  3 +++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
> index 58104d9..fd8d06c 100644
> --- a/src/shared/gatt-helpers.c
> +++ b/src/shared/gatt-helpers.c
> @@ -186,6 +186,64 @@ struct discovery_op {
>  	bt_gatt_destroy_func_t destroy;
>  };
> 
> +bool bt_gatt_iter_next_included_service(struct bt_gatt_iter *iter,
> +				uint16_t *handle, uint16_t *start_handle,
> +				uint16_t *end_handle, uint8_t uuid[16])
> +{
> +	struct bt_gatt_result *read_result;
> +	const void *pdu_ptr;
> +	int i = 0;
> +
> +	if (!iter || !iter->result || !handle || !start_handle || !end_handle
> +								|| !uuid)
> +		return false;
> +
> +	if (iter->result->opcode != BT_ATT_OP_READ_BY_TYPE_RSP)
> +		return false;
> +
> +	if (iter->result->data_len != 8 && iter->result->data_len != 6)
> +		return false;
> +
> +	pdu_ptr = iter->result->pdu + iter->pos;
> +
> +	if (iter->result->data_len == 8) {
> +		*handle = get_le16(pdu_ptr);
> +		*start_handle = get_le16(pdu_ptr + 2);
> +		*end_handle = get_le16(pdu_ptr + 4);
> +		convert_uuid_le(pdu_ptr + 6, 2, uuid);
> +
> +		iter->pos += iter->result->data_len;
> +
> +		if (iter->pos == iter->result->pdu_len) {
> +			iter->result = iter->result->next;
> +			iter->pos = 0;
> +		}
> +
> +		return true;
> +	}
> +
> +	*handle = get_le16(pdu_ptr);
> +	*start_handle = get_le16(pdu_ptr + 2);
> +	*end_handle = get_le16(pdu_ptr + 4);
> +	read_result = iter->result;
> +
> +	do {
> +		read_result = read_result->next;
> +	} while (read_result && i++ < (iter->pos / iter->result->data_len));

This loop looks quite unusual. Maybe use for and break inside?
Also please put some comment about those calculations.

> +
> +	if (!read_result)
> +		return false;
> +
> +	convert_uuid_le(read_result->pdu, read_result->data_len, uuid);
> +	iter->pos += iter->result->data_len;
> +	if (iter->pos == iter->result->pdu_len) {
> +		iter->result = read_result->next;
> +		iter->pos = 0;
> +	}
> +
> +	return true;
> +}
> +
>  bool bt_gatt_iter_next_service(struct bt_gatt_iter *iter,
>  				uint16_t *start_handle, uint16_t *end_handle,
>  				uint8_t uuid[16])
> diff --git a/src/shared/gatt-helpers.h b/src/shared/gatt-helpers.h
> index 8a25dea..8c434c1 100644
> --- a/src/shared/gatt-helpers.h
> +++ b/src/shared/gatt-helpers.h
> @@ -49,6 +49,9 @@ bool bt_gatt_iter_next_characteristic(struct bt_gatt_iter
> *iter, uint8_t uuid[16]);
>  bool bt_gatt_iter_next_descriptor(struct bt_gatt_iter *iter, uint16_t
> *handle, uint8_t uuid[16]);
> +bool bt_gatt_iter_next_included_service(struct bt_gatt_iter *iter,



> +				uint16_t *handle, uint16_t *start_handle,
> +				uint16_t *end_handle, uint8_t uuid[16]);
> 
>  typedef void (*bt_gatt_destroy_func_t)(void *user_data);

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

[PATCHv5 06/14] shared/gatt: Remove not needed function parameter

[Marcin Kraglak]

According to SPEC 4.1 4.5.1 "Find Included Services" there is no need
to pass UUID to discover_included_services() function.
---
 src/shared/gatt-helpers.c | 1 -
 src/shared/gatt-helpers.h | 1 -
 2 files changed, 2 deletions(-)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index fd8d06c..48e7379 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -1002,7 +1002,6 @@ done:
 
 bool bt_gatt_discover_included_services(struct bt_att *att,
 					uint16_t start, uint16_t end,
-					bt_uuid_t *uuid,
 					bt_gatt_discovery_callback_t callback,
 					void *user_data,
 					bt_gatt_destroy_func_t destroy)
diff --git a/src/shared/gatt-helpers.h b/src/shared/gatt-helpers.h
index 8c434c1..f2c7ec5 100644
--- a/src/shared/gatt-helpers.h
+++ b/src/shared/gatt-helpers.h
@@ -82,7 +82,6 @@ bool bt_gatt_discover_secondary_services(struct bt_att *att, bt_uuid_t *uuid,
 					bt_gatt_destroy_func_t destroy);
 bool bt_gatt_discover_included_services(struct bt_att *att,
 					uint16_t start, uint16_t end,
-					bt_uuid_t *uuid,
 					bt_gatt_discovery_callback_t callback,
 					void *user_data,
 					bt_gatt_destroy_func_t destroy);
-- 
1.9.3

[PATCHv5 07/14] shared/gatt: Distinguish Primary from Secondary services

[Marcin Kraglak]

Add flag primary to distinguish service as primary or secondary.
---
 src/shared/gatt-client.c | 7 +++++--
 src/shared/gatt-client.h | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 03d722f..724fde2 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -202,7 +202,8 @@ static void mark_notify_data_invalid_if_in_range(void *data, void *user_data)
 
 static bool service_list_add_service(struct service_list **head,
 						struct service_list **tail,
-						uint16_t start, uint16_t end,
+						bool primary, uint16_t start,
+						uint16_t end,
 						uint8_t uuid[BT_GATT_UUID_SIZE])
 {
 	struct service_list *list;
@@ -211,6 +212,7 @@ static bool service_list_add_service(struct service_list **head,
 	if (!list)
 		return false;
 
+	list->service.primary = primary;
 	list->service.start_handle = start;
 	list->service.end_handle = end;
 	memcpy(list->service.uuid, uuid, UUID_BYTES);
@@ -668,7 +670,8 @@ static void discover_primary_cb(bool success, uint8_t att_ecode,
 
 		/* Store the service */
 		if (!service_list_add_service(&op->result_head,
-					&op->result_tail, start, end, uuid)) {
+					&op->result_tail, true, start, end,
+					uuid)) {
 			util_debug(client->debug_callback, client->debug_data,
 						"Failed to store service");
 			success = false;
diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
index 6807f6b..22d4dc0 100644
--- a/src/shared/gatt-client.h
+++ b/src/shared/gatt-client.h
@@ -75,6 +75,7 @@ bool bt_gatt_client_set_debug(struct bt_gatt_client *client,
 					bt_gatt_client_destroy_func_t destroy);
 
 typedef struct {
+	bool primary;
 	uint16_t start_handle;
 	uint16_t end_handle;
 	uint8_t uuid[BT_GATT_UUID_SIZE];
-- 
1.9.3

[PATCHv5 08/14] tools/btgatt-client: Print type of service

[Marcin Kraglak]

---
 tools/btgatt-client.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
index d900e08..5c692ff 100644
--- a/tools/btgatt-client.c
+++ b/tools/btgatt-client.c
@@ -182,8 +182,9 @@ static void print_service(const bt_gatt_service_t *service)
 		return;
 	}
 
-	printf(COLOR_RED "service" COLOR_OFF " - start: 0x%04x, "
+	printf(COLOR_RED "service %s" COLOR_OFF " - start: 0x%04x, "
 				"end: 0x%04x, uuid: ",
+				service->primary ? "primary" : "second.",
 				service->start_handle, service->end_handle);
 	print_uuid(service->uuid);
 
-- 
1.9.3

Re: [PATCHv5 08/14] tools/btgatt-client: Print type of service

[Szymon Janc]

Hi Marcin,

On Thursday 16 October 2014 12:17:20 Marcin Kraglak wrote:
> ---
>  tools/btgatt-client.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
> index d900e08..5c692ff 100644
> --- a/tools/btgatt-client.c
> +++ b/tools/btgatt-client.c
> @@ -182,8 +182,9 @@ static void print_service(const bt_gatt_service_t
> *service) return;
>  	}
> 
> -	printf(COLOR_RED "service" COLOR_OFF " - start: 0x%04x, "
> +	printf(COLOR_RED "service %s" COLOR_OFF " - start: 0x%04x, "
>  				"end: 0x%04x, uuid: ",
> +				service->primary ? "primary" : "second.",
>  				service->start_handle, service->end_handle);

As we discussed offline, maybe having "type: foo" would be nicer instead of 
"serivce second." ?

>  	print_uuid(service->uuid);

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

[PATCHv5 09/14] shared/gatt: Discover secondary services

[Marcin Kraglak]

Add searching of secondary services in gatt-client.
---
 src/shared/gatt-client.c | 117 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 104 insertions(+), 13 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 724fde2..e04724c 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -200,23 +200,36 @@ static void mark_notify_data_invalid_if_in_range(void *data, void *user_data)
 		notify_data->invalid = true;
 }
 
-static bool service_list_add_service(struct service_list **head,
-						struct service_list **tail,
-						bool primary, uint16_t start,
-						uint16_t end,
+static struct service_list *new_service_list(uint16_t start, uint16_t end,
+						bool primary,
 						uint8_t uuid[BT_GATT_UUID_SIZE])
 {
 	struct service_list *list;
 
 	list = new0(struct service_list, 1);
 	if (!list)
-		return false;
+		return NULL;
 
 	list->service.primary = primary;
 	list->service.start_handle = start;
 	list->service.end_handle = end;
 	memcpy(list->service.uuid, uuid, UUID_BYTES);
 
+	return list;
+}
+
+static bool service_list_add_service(struct service_list **head,
+						struct service_list **tail,
+						bool primary, uint16_t start,
+						uint16_t end,
+						uint8_t uuid[BT_GATT_UUID_SIZE])
+{
+	struct service_list *list;
+
+	list = new_service_list(start, end, primary, uuid);
+	if (!list)
+		return false;
+
 	if (!(*head))
 		*head = *tail = list;
 	else {
@@ -364,6 +377,8 @@ struct discovery_op {
 	struct bt_gatt_client *client;
 	struct service_list *result_head, *result_tail, *cur_service;
 	struct chrc_data *cur_chrc;
+	uint16_t start;
+	uint16_t end;
 	int cur_chrc_index;
 	int ref_count;
 	void (*complete_func)(struct discovery_op *op, bool success,
@@ -634,6 +649,79 @@ done:
 	op->complete_func(op, success, att_ecode);
 }
 
+static void discover_secondary_cb(bool success, uint8_t att_ecode,
+						struct bt_gatt_result *result,
+						void *user_data)
+{
+	struct discovery_op *op = user_data;
+	struct bt_gatt_client *client = op->client;
+	struct bt_gatt_iter iter;
+	uint16_t start, end;
+	uint8_t uuid[BT_GATT_UUID_SIZE];
+	char uuid_str[MAX_LEN_UUID_STR];
+	struct service_list *service;
+
+	if (!success) {
+		util_debug(client->debug_callback, client->debug_data,
+					"Secondary service discovery failed."
+					" ATT ECODE: 0x%02x", att_ecode);
+		switch (att_ecode) {
+		case BT_ATT_ERROR_ATTRIBUTE_NOT_FOUND:
+		case BT_ATT_ERROR_UNSUPPORTED_GROUP_TYPE:
+			goto next;
+		default:
+			goto done;
+		}
+	}
+
+	if (!result || !bt_gatt_iter_init(&iter, result)) {
+		success = false;
+		goto done;
+	}
+
+	util_debug(client->debug_callback, client->debug_data,
+					"Secondary services found: %u",
+					bt_gatt_result_service_count(result));
+
+	while (bt_gatt_iter_next_service(&iter, &start, &end, uuid)) {
+		uuid_to_string(uuid, uuid_str);
+		util_debug(client->debug_callback, client->debug_data,
+				"start: 0x%04x, end: 0x%04x, uuid: %s",
+				start, end, uuid_str);
+
+		/* Store the service */
+		service = new_service_list(start, end, false, uuid);
+		if (!service) {
+			util_debug(client->debug_callback, client->debug_data,
+						"Failed to create service");
+			success = false;
+			goto done;
+		}
+
+		service_list_insert_services(&op->result_head, &op->result_tail,
+							service, service);
+	}
+
+next:
+	/* Sequentially discover the characteristics of all services */
+	op->cur_service = op->result_head;
+	if (bt_gatt_discover_characteristics(client->att,
+					op->cur_service->service.start_handle,
+					op->cur_service->service.end_handle,
+					discover_chrcs_cb,
+					discovery_op_ref(op),
+					discovery_op_unref))
+		return;
+
+	util_debug(client->debug_callback, client->debug_data,
+				"Failed to start characteristic discovery");
+	discovery_op_unref(op);
+	success = false;
+
+done:
+	op->complete_func(op, success, att_ecode);
+}
+
 static void discover_primary_cb(bool success, uint8_t att_ecode,
 						struct bt_gatt_result *result,
 						void *user_data)
@@ -686,18 +774,17 @@ static void discover_primary_cb(bool success, uint8_t att_ecode,
 	if (!op->result_head)
 		goto done;
 
-	/* Sequentially discover the characteristics of all services */
+	/* Discover secondary services */
 	op->cur_service = op->result_head;
-	if (bt_gatt_discover_characteristics(client->att,
-					op->cur_service->service.start_handle,
-					op->cur_service->service.end_handle,
-					discover_chrcs_cb,
-					discovery_op_ref(op),
-					discovery_op_unref))
+	if (bt_gatt_discover_secondary_services(client->att, NULL,
+							op->start, op->end,
+							discover_secondary_cb,
+							discovery_op_ref(op),
+							discovery_op_unref))
 		return;
 
 	util_debug(client->debug_callback, client->debug_data,
-				"Failed to start characteristic discovery");
+				"Failed to start secondary service discovery");
 	discovery_op_unref(op);
 	success = false;
 
@@ -870,6 +957,8 @@ static void process_service_changed(struct bt_gatt_client *client,
 
 	op->client = client;
 	op->complete_func = service_changed_complete;
+	op->start = start_handle;
+	op->end = end_handle;
 
 	if (!bt_gatt_discover_primary_services(client->att, NULL,
 						start_handle, end_handle,
@@ -1002,6 +1091,8 @@ static bool gatt_client_init(struct bt_gatt_client *client, uint16_t mtu)
 
 	op->client = client;
 	op->complete_func = init_complete;
+	op->start = 0x0001;
+	op->end = 0xffff;
 
 	/* Configure the MTU */
 	if (!bt_gatt_exchange_mtu(client->att, MAX(BT_ATT_DEFAULT_LE_MTU, mtu),
-- 
1.9.3

[PATCHv5 10/14] shared/gatt: Discover included services

[Marcin Kraglak]

---
 src/shared/gatt-client.c | 114 +++++++++++++++++++++++++++++++++++++++++++++--
 src/shared/gatt-client.h |   7 +++
 2 files changed, 117 insertions(+), 4 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index e04724c..971788c 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -69,6 +69,8 @@ struct service_list {
 	bt_gatt_service_t service;
 	struct chrc_data *chrcs;
 	size_t num_chrcs;
+	bt_gatt_included_service_t *includes;
+	size_t num_includes;
 	struct service_list *next;
 };
 
@@ -253,6 +255,14 @@ static void service_destroy_characteristics(struct service_list *service)
 	free(service->chrcs);
 }
 
+static void service_destroy_includes(struct service_list *service)
+{
+	free(service->includes);
+
+	service->includes = NULL;
+	service->num_includes = 0;
+}
+
 static void service_list_clear(struct service_list **head,
 						struct service_list **tail)
 {
@@ -265,6 +275,7 @@ static void service_list_clear(struct service_list **head,
 
 	while (l) {
 		service_destroy_characteristics(l);
+		service_destroy_includes(l);
 		tmp = l;
 		l = tmp->next;
 		free(tmp);
@@ -293,6 +304,7 @@ static void service_list_clear_range(struct service_list **head,
 		}
 
 		service_destroy_characteristics(cur);
+		service_destroy_includes(cur);
 
 		if (!prev)
 			*head = cur->next;
@@ -428,6 +440,99 @@ static int uuid_cmp(const uint8_t uuid128[16], uint16_t uuid16)
 	return memcmp(uuid128, rhs_uuid, sizeof(rhs_uuid));
 }
 
+static void discover_incl_cb(bool success, uint8_t att_ecode,
+		struct bt_gatt_result *result,
+		void *user_data)
+{
+	struct discovery_op *op = user_data;
+	struct bt_gatt_client *client = op->client;
+	struct bt_gatt_iter iter;
+	char uuid_str[MAX_LEN_UUID_STR];
+	bt_gatt_included_service_t *includes;
+	unsigned int includes_count, i;
+
+	if (!success) {
+		if (att_ecode == BT_ATT_ERROR_ATTRIBUTE_NOT_FOUND) {
+			success = true;
+			goto next;
+		}
+
+		goto done;
+	}
+
+	if (!result || !bt_gatt_iter_init(&iter, result)) {
+		success = false;
+		goto done;
+	}
+
+	includes_count = bt_gatt_result_included_count(result);
+	if (includes_count == 0) {
+		success = false;
+		goto done;
+	}
+
+	includes = new0(bt_gatt_included_service_t, includes_count);
+	if (!includes) {
+		success = false;
+		goto done;
+	}
+
+	util_debug(client->debug_callback, client->debug_data,
+						"Included services found: %u",
+						includes_count);
+
+	i = 0;
+	while (bt_gatt_iter_next_included_service(&iter, &includes[i].handle,
+						&includes[i].start_handle,
+						&includes[i].end_handle,
+						includes[i].uuid)) {
+		uuid_to_string(includes[i].uuid, uuid_str);
+		util_debug(client->debug_callback, client->debug_data,
+				"handle: 0x%04x, start: 0x%04x, end: 0x%04x,"
+				"uuid: %s", includes[i].handle,
+				includes[i].start_handle,
+				includes[i].end_handle, uuid_str);
+		i++;
+	}
+
+	op->cur_service->includes = includes;
+	op->cur_service->num_includes = includes_count;
+
+next:
+	if (!op->cur_service->next) {
+		op->cur_service = op->result_head;
+		if (bt_gatt_discover_characteristics(client->att,
+					op->cur_service->service.start_handle,
+					op->cur_service->service.end_handle,
+					discover_chrcs_cb,
+					discovery_op_ref(op),
+					discovery_op_unref))
+			return;
+
+		util_debug(client->debug_callback, client->debug_data,
+				"Failed to start characteristic discovery");
+		discovery_op_unref(op);
+		success = false;
+		goto done;
+	}
+
+	op->cur_service = op->cur_service->next;
+	if (bt_gatt_discover_included_services(client->att,
+				op->cur_service->service.start_handle,
+				op->cur_service->service.end_handle,
+				discover_incl_cb,
+				discovery_op_ref(op),
+				discovery_op_unref))
+		return;
+	util_debug(client->debug_callback, client->debug_data,
+					"Failed to start included discovery");
+	discovery_op_unref(op);
+	success = false;
+
+done:
+	op->complete_func(op, success, att_ecode);
+}
+
 static void discover_descs_cb(bool success, uint8_t att_ecode,
 						struct bt_gatt_result *result,
 						void *user_data)
@@ -532,6 +637,7 @@ done:
 	op->complete_func(op, success, att_ecode);
 }
 
+
 static void discover_chrcs_cb(bool success, uint8_t att_ecode,
 						struct bt_gatt_result *result,
 						void *user_data)
@@ -703,18 +809,18 @@ static void discover_secondary_cb(bool success, uint8_t att_ecode,
 	}
 
 next:
-	/* Sequentially discover the characteristics of all services */
+	/* Sequentially discover included services */
 	op->cur_service = op->result_head;
-	if (bt_gatt_discover_characteristics(client->att,
+	if (bt_gatt_discover_included_services(client->att,
 					op->cur_service->service.start_handle,
 					op->cur_service->service.end_handle,
-					discover_chrcs_cb,
+					discover_incl_cb,
 					discovery_op_ref(op),
 					discovery_op_unref))
 		return;
 
 	util_debug(client->debug_callback, client->debug_data,
-				"Failed to start characteristic discovery");
+				"Failed to start included services discovery");
 	discovery_op_unref(op);
 	success = false;
 
diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
index 22d4dc0..05b4838 100644
--- a/src/shared/gatt-client.h
+++ b/src/shared/gatt-client.h
@@ -96,6 +96,13 @@ typedef struct {
 	size_t num_descs;
 } bt_gatt_characteristic_t;
 
+typedef struct {
+	uint16_t handle;
+	uint16_t start_handle;
+	uint16_t end_handle;
+	uint8_t uuid[BT_GATT_UUID_SIZE];
+} bt_gatt_included_service_t;
+
 struct bt_gatt_service_iter {
 	struct bt_gatt_client *client;
 	void *ptr;
-- 
1.9.3

Re: [PATCHv5 10/14] shared/gatt: Discover included services

[Szymon Janc]

Hi Marcin,

On Thursday 16 October 2014 12:17:22 Marcin Kraglak wrote:
> ---
>  src/shared/gatt-client.c | 114
> +++++++++++++++++++++++++++++++++++++++++++++-- src/shared/gatt-client.h | 
>  7 +++
>  2 files changed, 117 insertions(+), 4 deletions(-)
> 
> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> index e04724c..971788c 100644
> --- a/src/shared/gatt-client.c
> +++ b/src/shared/gatt-client.c
> @@ -69,6 +69,8 @@ struct service_list {
>  	bt_gatt_service_t service;
>  	struct chrc_data *chrcs;
>  	size_t num_chrcs;
> +	bt_gatt_included_service_t *includes;
> +	size_t num_includes;
>  	struct service_list *next;
>  };
> 
> @@ -253,6 +255,14 @@ static void service_destroy_characteristics(struct
> service_list *service) free(service->chrcs);
>  }
> 
> +static void service_destroy_includes(struct service_list *service)
> +{
> +	free(service->includes);
> +
> +	service->includes = NULL;
> +	service->num_includes = 0;
> +}
> +
>  static void service_list_clear(struct service_list **head,
>  						struct service_list **tail)
>  {
> @@ -265,6 +275,7 @@ static void service_list_clear(struct service_list
> **head,
> 
>  	while (l) {
>  		service_destroy_characteristics(l);
> +		service_destroy_includes(l);
>  		tmp = l;
>  		l = tmp->next;
>  		free(tmp);
> @@ -293,6 +304,7 @@ static void service_list_clear_range(struct service_list
> **head, }
> 
>  		service_destroy_characteristics(cur);
> +		service_destroy_includes(cur);
> 
>  		if (!prev)
>  			*head = cur->next;
> @@ -428,6 +440,99 @@ static int uuid_cmp(const uint8_t uuid128[16], uint16_t
> uuid16) return memcmp(uuid128, rhs_uuid, sizeof(rhs_uuid));
>  }
> 
> +static void discover_incl_cb(bool success, uint8_t att_ecode,
> +		struct bt_gatt_result *result,
> +		void *user_data)

Indentation is broken here and those likely fit in single line.

> +{
> +	struct discovery_op *op = user_data;
> +	struct bt_gatt_client *client = op->client;
> +	struct bt_gatt_iter iter;
> +	char uuid_str[MAX_LEN_UUID_STR];
> +	bt_gatt_included_service_t *includes;
> +	unsigned int includes_count, i;
> +
> +	if (!success) {
> +		if (att_ecode == BT_ATT_ERROR_ATTRIBUTE_NOT_FOUND) {
> +			success = true;
> +			goto next;
> +		}
> +
> +		goto done;
> +	}
> +
> +	if (!result || !bt_gatt_iter_init(&iter, result)) {
> +		success = false;
> +		goto done;
> +	}
> +
> +	includes_count = bt_gatt_result_included_count(result);
> +	if (includes_count == 0) {
> +		success = false;
> +		goto done;
> +	}
> +
> +	includes = new0(bt_gatt_included_service_t, includes_count);
> +	if (!includes) {
> +		success = false;
> +		goto done;
> +	}
> +
> +	util_debug(client->debug_callback, client->debug_data,
> +						"Included services found: %u",
> +						includes_count);
> +
> +	i = 0;
> +	while (bt_gatt_iter_next_included_service(&iter, &includes[i].handle,
> +						&includes[i].start_handle,
> +						&includes[i].end_handle,
> +						includes[i].uuid)) {
> +		uuid_to_string(includes[i].uuid, uuid_str);
> +		util_debug(client->debug_callback, client->debug_data,
> +				"handle: 0x%04x, start: 0x%04x, end: 0x%04x,"
> +				"uuid: %s", includes[i].handle,
> +				includes[i].start_handle,
> +				includes[i].end_handle, uuid_str);
> +		i++;
> +	}

I'd use for(;;) + break here. Also should we verify if this loop iterated 
includes_count times?

> +
> +	op->cur_service->includes = includes;
> +	op->cur_service->num_includes = includes_count;
> +
> +next:
> +	if (!op->cur_service->next) {
> +		op->cur_service = op->result_head;
> +		if (bt_gatt_discover_characteristics(client->att,
> +					op->cur_service->service.start_handle,
> +					op->cur_service->service.end_handle,
> +					discover_chrcs_cb,
> +					discovery_op_ref(op),
> +					discovery_op_unref))
> +			return;
> +
> +		util_debug(client->debug_callback, client->debug_data,
> +				"Failed to start characteristic discovery");
> +		discovery_op_unref(op);
> +		success = false;
> +		goto done;
> +	}
> +
> +	op->cur_service = op->cur_service->next;
> +	if (bt_gatt_discover_included_services(client->att,
> +				op->cur_service->service.start_handle,
> +				op->cur_service->service.end_handle,
> +				discover_incl_cb,
> +				discovery_op_ref(op),
> +				discovery_op_unref))
> +		return;

Empty line here.

> +	util_debug(client->debug_callback, client->debug_data,
> +					"Failed to start included discovery");
> +	discovery_op_unref(op);
> +	success = false;
> +
> +done:
> +	op->complete_func(op, success, att_ecode);

This is always called with success == false so maybe label should be called 
failed and called with hardcoded false?  Or there is some codepath missing for 
success case?

> +}
> +
>  static void discover_descs_cb(bool success, uint8_t att_ecode,
>  						struct bt_gatt_result *result,
>  						void *user_data)
> @@ -532,6 +637,7 @@ done:
>  	op->complete_func(op, success, att_ecode);
>  }
> 
> +
>  static void discover_chrcs_cb(bool success, uint8_t att_ecode,
>  						struct bt_gatt_result *result,
>  						void *user_data)
> @@ -703,18 +809,18 @@ static void discover_secondary_cb(bool success,
> uint8_t att_ecode, }
> 
>  next:
> -	/* Sequentially discover the characteristics of all services */
> +	/* Sequentially discover included services */
>  	op->cur_service = op->result_head;
> -	if (bt_gatt_discover_characteristics(client->att,
> +	if (bt_gatt_discover_included_services(client->att,
>  					op->cur_service->service.start_handle,
>  					op->cur_service->service.end_handle,
> -					discover_chrcs_cb,
> +					discover_incl_cb,
>  					discovery_op_ref(op),
>  					discovery_op_unref))
>  		return;
> 
>  	util_debug(client->debug_callback, client->debug_data,
> -				"Failed to start characteristic discovery");
> +				"Failed to start included services discovery");
>  	discovery_op_unref(op);
>  	success = false;
> 
> diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
> index 22d4dc0..05b4838 100644
> --- a/src/shared/gatt-client.h
> +++ b/src/shared/gatt-client.h
> @@ -96,6 +96,13 @@ typedef struct {
>  	size_t num_descs;
>  } bt_gatt_characteristic_t;
> 
> +typedef struct {
> +	uint16_t handle;
> +	uint16_t start_handle;
> +	uint16_t end_handle;
> +	uint8_t uuid[BT_GATT_UUID_SIZE];
> +} bt_gatt_included_service_t;
> +
>  struct bt_gatt_service_iter {
>  	struct bt_gatt_client *client;
>  	void *ptr;

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

[PATCHv5 11/14] shared/gatt: Add gatt-client include service iterator

[Marcin Kraglak]

It will allow user to take value, handle, start and end handle
of included service.
---
 src/shared/gatt-client.c | 30 ++++++++++++++++++++++++++++++
 src/shared/gatt-client.h | 10 ++++++++++
 2 files changed, 40 insertions(+)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 971788c..28865da 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -1674,6 +1674,36 @@ bool bt_gatt_characteristic_iter_next(struct bt_gatt_characteristic_iter *iter,
 	return true;
 }
 
+bool bt_gatt_include_service_iter_init(struct bt_gatt_incl_service_iter *iter,
+					const bt_gatt_service_t *service)
+{
+	if (!iter || !service)
+		return false;
+
+	memset(iter, 0, sizeof(*iter));
+	iter->service = (struct service_list *) service;
+
+	return true;
+}
+
+bool bt_gatt_include_service_iter_next(struct bt_gatt_incl_service_iter *iter,
+					const bt_gatt_included_service_t **incl)
+{
+	struct service_list *service;
+
+	if (!iter || !incl)
+		return false;
+
+	service = iter->service;
+
+	if (iter->pos >= service->num_includes)
+		return false;
+
+	*incl = &service->includes[iter->pos++];
+
+	return true;
+}
+
 struct read_op {
 	bt_gatt_client_read_callback_t callback;
 	void *user_data;
diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
index 05b4838..bf4e7bb 100644
--- a/src/shared/gatt-client.h
+++ b/src/shared/gatt-client.h
@@ -113,6 +113,11 @@ struct bt_gatt_characteristic_iter {
 	size_t pos;
 };
 
+struct bt_gatt_incl_service_iter {
+	void *service;
+	size_t pos;
+};
+
 bool bt_gatt_service_iter_init(struct bt_gatt_service_iter *iter,
 						struct bt_gatt_client *client);
 bool bt_gatt_service_iter_next(struct bt_gatt_service_iter *iter,
@@ -129,6 +134,11 @@ bool bt_gatt_characteristic_iter_init(struct bt_gatt_characteristic_iter *iter,
 bool bt_gatt_characteristic_iter_next(struct bt_gatt_characteristic_iter *iter,
 					const bt_gatt_characteristic_t **chrc);
 
+bool bt_gatt_include_service_iter_init(struct bt_gatt_incl_service_iter *iter,
+					const bt_gatt_service_t *service);
+bool bt_gatt_include_service_iter_next(struct bt_gatt_incl_service_iter *iter,
+					const bt_gatt_included_service_t **inc);
+
 typedef void (*bt_gatt_client_read_callback_t)(bool success, uint8_t att_ecode,
 					const uint8_t *value, uint16_t length,
 					void *user_data);
-- 
1.9.3

Re: [PATCHv5 11/14] shared/gatt: Add gatt-client include service iterator

[Szymon Janc]

Hi Marcin,

On Thursday 16 October 2014 12:17:23 Marcin Kraglak wrote:
> It will allow user to take value, handle, start and end handle
> of included service.
> ---
>  src/shared/gatt-client.c | 30 ++++++++++++++++++++++++++++++
>  src/shared/gatt-client.h | 10 ++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> index 971788c..28865da 100644
> --- a/src/shared/gatt-client.c
> +++ b/src/shared/gatt-client.c
> @@ -1674,6 +1674,36 @@ bool bt_gatt_characteristic_iter_next(struct
> bt_gatt_characteristic_iter *iter, return true;
>  }
> 
> +bool bt_gatt_include_service_iter_init(struct bt_gatt_incl_service_iter
> *iter, +					const bt_gatt_service_t *service)
> +{
> +	if (!iter || !service)
> +		return false;
> +
> +	memset(iter, 0, sizeof(*iter));
> +	iter->service = (struct service_list *) service;


I know that this is based on existing code but I'll comment on it here :)

This cast is really confusing. Either it should be casted to void* as service 
field inside iter or service should be const in iter if we are passing const 
data there. Or it should be of type service_list and named services?

This list interface is kinda hard to read and I don't really like it without 
proper macros (like in kernel code)...

> +
> +	return true;
> +}
> +
> +bool bt_gatt_include_service_iter_next(struct bt_gatt_incl_service_iter
> *iter, +					const bt_gatt_included_service_t **incl)
> +{
> +	struct service_list *service;
> +
> +	if (!iter || !incl)
> +		return false;
> +
> +	service = iter->service;
> +
> +	if (iter->pos >= service->num_includes)
> +		return false;
> +
> +	*incl = &service->includes[iter->pos++];
> +
> +	return true;
> +}
> +
>  struct read_op {
>  	bt_gatt_client_read_callback_t callback;
>  	void *user_data;
> diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
> index 05b4838..bf4e7bb 100644
> --- a/src/shared/gatt-client.h
> +++ b/src/shared/gatt-client.h
> @@ -113,6 +113,11 @@ struct bt_gatt_characteristic_iter {
>  	size_t pos;
>  };
> 
> +struct bt_gatt_incl_service_iter {
> +	void *service;
> +	size_t pos;
> +};
> +
>  bool bt_gatt_service_iter_init(struct bt_gatt_service_iter *iter,
>  						struct bt_gatt_client *client);
>  bool bt_gatt_service_iter_next(struct bt_gatt_service_iter *iter,
> @@ -129,6 +134,11 @@ bool bt_gatt_characteristic_iter_init(struct
> bt_gatt_characteristic_iter *iter, bool
> bt_gatt_characteristic_iter_next(struct bt_gatt_characteristic_iter *iter,
> const bt_gatt_characteristic_t **chrc);
> 
> +bool bt_gatt_include_service_iter_init(struct bt_gatt_incl_service_iter
> *iter, +					const bt_gatt_service_t *service);
> +bool bt_gatt_include_service_iter_next(struct bt_gatt_incl_service_iter
> *iter, +					const bt_gatt_included_service_t **inc);
> +
>  typedef void (*bt_gatt_client_read_callback_t)(bool success, uint8_t
> att_ecode, const uint8_t *value, uint16_t length,
>  					void *user_data);

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

[PATCHv5 12/14] tools/btgatt-client: Print found include services

[Marcin Kraglak]

---
 tools/btgatt-client.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
index 5c692ff..5465d53 100644
--- a/tools/btgatt-client.c
+++ b/tools/btgatt-client.c
@@ -174,7 +174,9 @@ static void print_uuid(const uint8_t uuid[16])
 static void print_service(const bt_gatt_service_t *service)
 {
 	struct bt_gatt_characteristic_iter iter;
+	struct bt_gatt_incl_service_iter include_iter;
 	const bt_gatt_characteristic_t *chrc;
+	const bt_gatt_included_service_t *incl;
 	size_t i;
 
 	if (!bt_gatt_characteristic_iter_init(&iter, service)) {
@@ -182,12 +184,24 @@ static void print_service(const bt_gatt_service_t *service)
 		return;
 	}
 
+	if (!bt_gatt_include_service_iter_init(&include_iter, service)) {
+		PRLOG("Failed to initialize include service iterator\n");
+		return;
+	}
 	printf(COLOR_RED "service %s" COLOR_OFF " - start: 0x%04x, "
 				"end: 0x%04x, uuid: ",
 				service->primary ? "primary" : "second.",
 				service->start_handle, service->end_handle);
 	print_uuid(service->uuid);
 
+	while (bt_gatt_include_service_iter_next(&include_iter, &incl)) {
+		printf("\t  " COLOR_GREEN "include" COLOR_OFF " - handle: "
+					"0x%04x, - start: 0x%04x, end: 0x%04x,"
+					"uuid: ", incl->handle,
+					incl->start_handle, incl->end_handle);
+			print_uuid(incl->uuid);
+	}
+
 	while (bt_gatt_characteristic_iter_next(&iter, &chrc)) {
 		printf("\t  " COLOR_YELLOW "charac" COLOR_OFF
 				" - start: 0x%04x, end: 0x%04x, "
-- 
1.9.3

[PATCHv5 13/14] shared/gatt: Fix searching descriptors

[Marcin Kraglak]

Descriptor discovery range started from characteristic value handle + 1
and end with characteristic end handle. If characteristic value handle
is 0xffff, then discovery range was set to 0x0000-0xffff.
Found during PTS test case TC_GAD_CL_BV_03_C.
---
 src/shared/gatt-client.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 28865da..8089302 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -714,8 +714,8 @@ static void discover_chrcs_cb(bool success, uint8_t att_ecode,
 	for (i = 0; i < chrc_count; i++) {
 		op->cur_chrc_index = i;
 		op->cur_chrc = chrcs + i;
-		desc_start = chrcs[i].chrc_external.value_handle + 1;
-		if (desc_start > chrcs[i].chrc_external.end_handle)
+		desc_start = chrcs[i].chrc_external.value_handle;
+		if (desc_start++ == chrcs[i].chrc_external.end_handle)
 			continue;
 
 		if (bt_gatt_discover_descriptors(client->att, desc_start,
-- 
1.9.3

[PATCHv5 14/14] shared/gatt: Add function bt_gatt_result_included_count()

[Marcin Kraglak]

It will return number of included services in result.
---
 src/shared/gatt-helpers.c | 21 +++++++++++++++++++++
 src/shared/gatt-helpers.h |  1 +
 2 files changed, 22 insertions(+)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index 48e7379..92d0696 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -140,6 +140,27 @@ unsigned int bt_gatt_result_descriptor_count(struct bt_gatt_result *result)
 	return result_element_count(result);
 }
 
+unsigned int bt_gatt_result_included_count(struct bt_gatt_result *result)
+{
+	struct bt_gatt_result *cur;
+	unsigned int count = 0;
+
+	if (!result)
+		return 0;
+
+	if (result->opcode != BT_ATT_OP_READ_BY_TYPE_RSP)
+		return 0;
+
+	if (result->data_len != 6 && result->data_len != 8)
+		return 0;
+
+	for (cur = result; cur; cur = cur->next)
+		if (cur->opcode == BT_ATT_OP_READ_BY_TYPE_RSP)
+			count += cur->pdu_len / cur->data_len;
+
+	return count;
+}
+
 bool bt_gatt_iter_init(struct bt_gatt_iter *iter, struct bt_gatt_result *result)
 {
 	if (!iter || !result)
diff --git a/src/shared/gatt-helpers.h b/src/shared/gatt-helpers.h
index f2c7ec5..07c145d 100644
--- a/src/shared/gatt-helpers.h
+++ b/src/shared/gatt-helpers.h
@@ -38,6 +38,7 @@ struct bt_gatt_iter {
 unsigned int bt_gatt_result_service_count(struct bt_gatt_result *result);
 unsigned int bt_gatt_result_characteristic_count(struct bt_gatt_result *result);
 unsigned int bt_gatt_result_descriptor_count(struct bt_gatt_result *result);
+unsigned int bt_gatt_result_included_count(struct bt_gatt_result *result);
 
 bool bt_gatt_iter_init(struct bt_gatt_iter *iter, struct bt_gatt_result *result);
 bool bt_gatt_iter_next_service(struct bt_gatt_iter *iter,
-- 
1.9.3

Re: [PATCHv5 00/14] Included service discovery

[Marcin Kraglak]

On 16 October 2014 12:17, Marcin Kraglak <marcin.kraglak@tieto.com> wrote:
> v3:
> In this version after primary service discovery,
> secondary services are discovered. Next included
> services are resolved. With this approach we
> don't have recursively search for included service,
> like it was TODO in previous proposal.
> There is also small coding style fix suggested by Arman.
>
> v4:
> If no secondary services found, continue include services search (fixed
> in gatt-client.c).
> Fixed wrong debug logs (primary->secondary).
> Fixed searching descriptors
>
> v5:
> Ignore Unsupported Group Type Error in response to secondary service
> discovery and continue included services discovery.
>
> Marcin Kraglak (14):
>   shared/gatt: Add discover_secondary_services()
>   shared/gatt: Add initial implementation of discover_included_services
>   shared/gatt: Discover included services 128 bit UUIDS
>   shared/gatt: Add extra check in characteristic iterator
>   shared/gatt: Add included service iterator
>   shared/gatt: Remove not needed function parameter
>   shared/gatt: Distinguish Primary from Secondary services
>   tools/btgatt-client: Print type of service
>   shared/gatt: Discover secondary services
>   shared/gatt: Discover included services
>   shared/gatt: Add gatt-client include service iterator
>   tools/btgatt-client: Print found include services
>   shared/gatt: Fix searching descriptors
>   shared/gatt: Add function bt_gatt_result_included_count()
>
>  src/shared/gatt-client.c  | 263 +++++++++++++++++++++++++++--
>  src/shared/gatt-client.h  |  18 ++
>  src/shared/gatt-helpers.c | 418 +++++++++++++++++++++++++++++++++++++++++++---
>  src/shared/gatt-helpers.h |  10 +-
>  tools/btgatt-client.c     |  17 +-
>  5 files changed, 690 insertions(+), 36 deletions(-)
>
> --
> 1.9.3
>

Ping

Re: [PATCHv5 00/14] Included service discovery

[Luiz Augusto von Dentz]

Hi Marcin,

On Wed, Oct 22, 2014 at 9:25 AM, Marcin Kraglak
<marcin.kraglak@tieto.com> wrote:
> On 16 October 2014 12:17, Marcin Kraglak <marcin.kraglak@tieto.com> wrote:
>> v3:
>> In this version after primary service discovery,
>> secondary services are discovered. Next included
>> services are resolved. With this approach we
>> don't have recursively search for included service,
>> like it was TODO in previous proposal.
>> There is also small coding style fix suggested by Arman.
>>
>> v4:
>> If no secondary services found, continue include services search (fixed
>> in gatt-client.c).
>> Fixed wrong debug logs (primary->secondary).
>> Fixed searching descriptors
>>
>> v5:
>> Ignore Unsupported Group Type Error in response to secondary service
>> discovery and continue included services discovery.
>>
>> Marcin Kraglak (14):
>>   shared/gatt: Add discover_secondary_services()
>>   shared/gatt: Add initial implementation of discover_included_services
>>   shared/gatt: Discover included services 128 bit UUIDS
>>   shared/gatt: Add extra check in characteristic iterator
>>   shared/gatt: Add included service iterator
>>   shared/gatt: Remove not needed function parameter
>>   shared/gatt: Distinguish Primary from Secondary services
>>   tools/btgatt-client: Print type of service
>>   shared/gatt: Discover secondary services
>>   shared/gatt: Discover included services
>>   shared/gatt: Add gatt-client include service iterator
>>   tools/btgatt-client: Print found include services
>>   shared/gatt: Fix searching descriptors
>>   shared/gatt: Add function bt_gatt_result_included_count()
>>
>>  src/shared/gatt-client.c  | 263 +++++++++++++++++++++++++++--
>>  src/shared/gatt-client.h  |  18 ++
>>  src/shared/gatt-helpers.c | 418 +++++++++++++++++++++++++++++++++++++++++++---
>>  src/shared/gatt-helpers.h |  10 +-
>>  tools/btgatt-client.c     |  17 +-
>>  5 files changed, 690 insertions(+), 36 deletions(-)
>>
>> --
>> 1.9.3
>>

Patches looks fine to me, but I would like Arman to ack before applying.

Btw, there is one thing I would like to see addressed, right now
whenever we disconnect the cache is lost which doesn't use the CCC
ability to persist across connections, this can cause us to loose
notifications when reconnecting because the code don't remember any
handles and this is specially bad for profiles such as HoG since we
may loose some input events while rediscovering, in fact we should
probably store the cache whenever the remote supports service changed
and be able to reload them so we only have to do the discover again if
something has changed.


-- 
Luiz Augusto von Dentz

Re: [PATCHv5 00/14] Included service discovery

[Arman Uguray]

Hi Luiz,

On Wed, Oct 22, 2014 at 7:54 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Marcin,
>
> On Wed, Oct 22, 2014 at 9:25 AM, Marcin Kraglak
> <marcin.kraglak@tieto.com> wrote:
>> On 16 October 2014 12:17, Marcin Kraglak <marcin.kraglak@tieto.com> wrote:
>>> v3:
>>> In this version after primary service discovery,
>>> secondary services are discovered. Next included
>>> services are resolved. With this approach we
>>> don't have recursively search for included service,
>>> like it was TODO in previous proposal.
>>> There is also small coding style fix suggested by Arman.
>>>
>>> v4:
>>> If no secondary services found, continue include services search (fixed
>>> in gatt-client.c).
>>> Fixed wrong debug logs (primary->secondary).
>>> Fixed searching descriptors
>>>
>>> v5:
>>> Ignore Unsupported Group Type Error in response to secondary service
>>> discovery and continue included services discovery.
>>>
>>> Marcin Kraglak (14):
>>>   shared/gatt: Add discover_secondary_services()
>>>   shared/gatt: Add initial implementation of discover_included_services
>>>   shared/gatt: Discover included services 128 bit UUIDS
>>>   shared/gatt: Add extra check in characteristic iterator
>>>   shared/gatt: Add included service iterator
>>>   shared/gatt: Remove not needed function parameter
>>>   shared/gatt: Distinguish Primary from Secondary services
>>>   tools/btgatt-client: Print type of service
>>>   shared/gatt: Discover secondary services
>>>   shared/gatt: Discover included services
>>>   shared/gatt: Add gatt-client include service iterator
>>>   tools/btgatt-client: Print found include services
>>>   shared/gatt: Fix searching descriptors
>>>   shared/gatt: Add function bt_gatt_result_included_count()
>>>
>>>  src/shared/gatt-client.c  | 263 +++++++++++++++++++++++++++--
>>>  src/shared/gatt-client.h  |  18 ++
>>>  src/shared/gatt-helpers.c | 418 +++++++++++++++++++++++++++++++++++++++++++---
>>>  src/shared/gatt-helpers.h |  10 +-
>>>  tools/btgatt-client.c     |  17 +-
>>>  5 files changed, 690 insertions(+), 36 deletions(-)
>>>
>>> --
>>> 1.9.3
>>>
>
> Patches looks fine to me, but I would like Arman to ack before applying.
>

I'm fine with the patches as they are, though I saw that Szymon has
left comments on some of them (I'm guessing he's a doing a pass of the
patch set now). I'm good with it as long as his comments are
addressed.


> Btw, there is one thing I would like to see addressed, right now
> whenever we disconnect the cache is lost which doesn't use the CCC
> ability to persist across connections, this can cause us to loose
> notifications when reconnecting because the code don't remember any
> handles and this is specially bad for profiles such as HoG since we
> may loose some input events while rediscovering, in fact we should
> probably store the cache whenever the remote supports service changed
> and be able to reload them so we only have to do the discover again if
> something has changed.
>

There's an item in the top-level TODO for long-term caching of
attributes, so I guess this falls under that?


>
> --
> Luiz Augusto von Dentz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Cheers,
-Arman

Re: [PATCHv5 00/14] Included service discovery

[Szymon Janc]

Hi,

On Wednesday 22 October 2014 08:35:05 Arman Uguray wrote:
> Hi Luiz,
> 
> On Wed, Oct 22, 2014 at 7:54 AM, Luiz Augusto von Dentz
> 
> <luiz.dentz@gmail.com> wrote:
> > Hi Marcin,
> > 
> > On Wed, Oct 22, 2014 at 9:25 AM, Marcin Kraglak
> > 
> > <marcin.kraglak@tieto.com> wrote:
> >> On 16 October 2014 12:17, Marcin Kraglak <marcin.kraglak@tieto.com> 
wrote:
> >>> v3:
> >>> In this version after primary service discovery,
> >>> secondary services are discovered. Next included
> >>> services are resolved. With this approach we
> >>> don't have recursively search for included service,
> >>> like it was TODO in previous proposal.
> >>> There is also small coding style fix suggested by Arman.
> >>> 
> >>> v4:
> >>> If no secondary services found, continue include services search (fixed
> >>> in gatt-client.c).
> >>> Fixed wrong debug logs (primary->secondary).
> >>> Fixed searching descriptors
> >>> 
> >>> v5:
> >>> Ignore Unsupported Group Type Error in response to secondary service
> >>> discovery and continue included services discovery.
> >>> 
> >>> Marcin Kraglak (14):
> >>>   shared/gatt: Add discover_secondary_services()
> >>>   shared/gatt: Add initial implementation of discover_included_services
> >>>   shared/gatt: Discover included services 128 bit UUIDS
> >>>   shared/gatt: Add extra check in characteristic iterator
> >>>   shared/gatt: Add included service iterator
> >>>   shared/gatt: Remove not needed function parameter
> >>>   shared/gatt: Distinguish Primary from Secondary services
> >>>   tools/btgatt-client: Print type of service
> >>>   shared/gatt: Discover secondary services
> >>>   shared/gatt: Discover included services
> >>>   shared/gatt: Add gatt-client include service iterator
> >>>   tools/btgatt-client: Print found include services
> >>>   shared/gatt: Fix searching descriptors
> >>>   shared/gatt: Add function bt_gatt_result_included_count()
> >>>  
> >>>  src/shared/gatt-client.c  | 263 +++++++++++++++++++++++++++--
> >>>  src/shared/gatt-client.h  |  18 ++
> >>>  src/shared/gatt-helpers.c | 418
> >>>  +++++++++++++++++++++++++++++++++++++++++++---
> >>>  src/shared/gatt-helpers.h |  10 +-
> >>>  tools/btgatt-client.c     |  17 +-
> >>>  5 files changed, 690 insertions(+), 36 deletions(-)
> >>> 
> >>> --
> >>> 1.9.3
> > 
> > Patches looks fine to me, but I would like Arman to ack before applying.
> 
> I'm fine with the patches as they are, though I saw that Szymon has
> left comments on some of them (I'm guessing he's a doing a pass of the
> patch set now). I'm good with it as long as his comments are
> addressed.

I really think we should have more comments and less magic numbers there.
Otherwise this code will be hard to maintain in long term.

> 
> > Btw, there is one thing I would like to see addressed, right now
> > whenever we disconnect the cache is lost which doesn't use the CCC
> > ability to persist across connections, this can cause us to loose
> > notifications when reconnecting because the code don't remember any
> > handles and this is specially bad for profiles such as HoG since we
> > may loose some input events while rediscovering, in fact we should
> > probably store the cache whenever the remote supports service changed
> > and be able to reload them so we only have to do the discover again if
> > something has changed.
> 
> There's an item in the top-level TODO for long-term caching of
> attributes, so I guess this falls under that?
> 
> > --
> > Luiz Augusto von Dentz
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
> > in the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> Cheers,
> -Arman
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCHv5 00/14] Included service discovery

[Luiz Augusto von Dentz]

Hi Szymon,

On Wed, Oct 22, 2014 at 9:39 PM, Szymon Janc <szymon.janc@gmail.com> wrote:
> Hi,
>
> On Wednesday 22 October 2014 08:35:05 Arman Uguray wrote:
>> Hi Luiz,
>>
>> On Wed, Oct 22, 2014 at 7:54 AM, Luiz Augusto von Dentz
>>
>> <luiz.dentz@gmail.com> wrote:
>> > Hi Marcin,
>> >
>> > On Wed, Oct 22, 2014 at 9:25 AM, Marcin Kraglak
>> >
>> > <marcin.kraglak@tieto.com> wrote:
>> >> On 16 October 2014 12:17, Marcin Kraglak <marcin.kraglak@tieto.com>
> wrote:
>> >>> v3:
>> >>> In this version after primary service discovery,
>> >>> secondary services are discovered. Next included
>> >>> services are resolved. With this approach we
>> >>> don't have recursively search for included service,
>> >>> like it was TODO in previous proposal.
>> >>> There is also small coding style fix suggested by Arman.
>> >>>
>> >>> v4:
>> >>> If no secondary services found, continue include services search (fixed
>> >>> in gatt-client.c).
>> >>> Fixed wrong debug logs (primary->secondary).
>> >>> Fixed searching descriptors
>> >>>
>> >>> v5:
>> >>> Ignore Unsupported Group Type Error in response to secondary service
>> >>> discovery and continue included services discovery.
>> >>>
>> >>> Marcin Kraglak (14):
>> >>>   shared/gatt: Add discover_secondary_services()
>> >>>   shared/gatt: Add initial implementation of discover_included_services
>> >>>   shared/gatt: Discover included services 128 bit UUIDS
>> >>>   shared/gatt: Add extra check in characteristic iterator
>> >>>   shared/gatt: Add included service iterator
>> >>>   shared/gatt: Remove not needed function parameter
>> >>>   shared/gatt: Distinguish Primary from Secondary services
>> >>>   tools/btgatt-client: Print type of service
>> >>>   shared/gatt: Discover secondary services
>> >>>   shared/gatt: Discover included services
>> >>>   shared/gatt: Add gatt-client include service iterator
>> >>>   tools/btgatt-client: Print found include services
>> >>>   shared/gatt: Fix searching descriptors
>> >>>   shared/gatt: Add function bt_gatt_result_included_count()
>> >>>
>> >>>  src/shared/gatt-client.c  | 263 +++++++++++++++++++++++++++--
>> >>>  src/shared/gatt-client.h  |  18 ++
>> >>>  src/shared/gatt-helpers.c | 418
>> >>>  +++++++++++++++++++++++++++++++++++++++++++---
>> >>>  src/shared/gatt-helpers.h |  10 +-
>> >>>  tools/btgatt-client.c     |  17 +-
>> >>>  5 files changed, 690 insertions(+), 36 deletions(-)
>> >>>
>> >>> --
>> >>> 1.9.3
>> >
>> > Patches looks fine to me, but I would like Arman to ack before applying.
>>
>> I'm fine with the patches as they are, though I saw that Szymon has
>> left comments on some of them (I'm guessing he's a doing a pass of the
>> patch set now). I'm good with it as long as his comments are
>> addressed.
>
> I really think we should have more comments and less magic numbers there.
> Otherwise this code will be hard to maintain in long term.

Yep, we could actually start creating packed structs for PDU as it was
done for AVRCP so we could do sizeof etc, it makes the code much
easier to understand and maintain.


-- 
Luiz Augusto von Dentz

Re: [PATCHv5 00/14] Included service discovery

[Arman Uguray]

Hi Luiz,

On Thu, Oct 23, 2014 at 12:55 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Szymon,
>
> On Wed, Oct 22, 2014 at 9:39 PM, Szymon Janc <szymon.janc@gmail.com> wrote:
>> Hi,
>>
>> On Wednesday 22 October 2014 08:35:05 Arman Uguray wrote:
>>> Hi Luiz,
>>>
>>> On Wed, Oct 22, 2014 at 7:54 AM, Luiz Augusto von Dentz
>>>
>>> <luiz.dentz@gmail.com> wrote:
>>> > Hi Marcin,
>>> >
>>> > On Wed, Oct 22, 2014 at 9:25 AM, Marcin Kraglak
>>> >
>>> > <marcin.kraglak@tieto.com> wrote:
>>> >> On 16 October 2014 12:17, Marcin Kraglak <marcin.kraglak@tieto.com>
>> wrote:
>>> >>> v3:
>>> >>> In this version after primary service discovery,
>>> >>> secondary services are discovered. Next included
>>> >>> services are resolved. With this approach we
>>> >>> don't have recursively search for included service,
>>> >>> like it was TODO in previous proposal.
>>> >>> There is also small coding style fix suggested by Arman.
>>> >>>
>>> >>> v4:
>>> >>> If no secondary services found, continue include services search (fixed
>>> >>> in gatt-client.c).
>>> >>> Fixed wrong debug logs (primary->secondary).
>>> >>> Fixed searching descriptors
>>> >>>
>>> >>> v5:
>>> >>> Ignore Unsupported Group Type Error in response to secondary service
>>> >>> discovery and continue included services discovery.
>>> >>>
>>> >>> Marcin Kraglak (14):
>>> >>>   shared/gatt: Add discover_secondary_services()
>>> >>>   shared/gatt: Add initial implementation of discover_included_services
>>> >>>   shared/gatt: Discover included services 128 bit UUIDS
>>> >>>   shared/gatt: Add extra check in characteristic iterator
>>> >>>   shared/gatt: Add included service iterator
>>> >>>   shared/gatt: Remove not needed function parameter
>>> >>>   shared/gatt: Distinguish Primary from Secondary services
>>> >>>   tools/btgatt-client: Print type of service
>>> >>>   shared/gatt: Discover secondary services
>>> >>>   shared/gatt: Discover included services
>>> >>>   shared/gatt: Add gatt-client include service iterator
>>> >>>   tools/btgatt-client: Print found include services
>>> >>>   shared/gatt: Fix searching descriptors
>>> >>>   shared/gatt: Add function bt_gatt_result_included_count()
>>> >>>
>>> >>>  src/shared/gatt-client.c  | 263 +++++++++++++++++++++++++++--
>>> >>>  src/shared/gatt-client.h  |  18 ++
>>> >>>  src/shared/gatt-helpers.c | 418
>>> >>>  +++++++++++++++++++++++++++++++++++++++++++---
>>> >>>  src/shared/gatt-helpers.h |  10 +-
>>> >>>  tools/btgatt-client.c     |  17 +-
>>> >>>  5 files changed, 690 insertions(+), 36 deletions(-)
>>> >>>
>>> >>> --
>>> >>> 1.9.3
>>> >
>>> > Patches looks fine to me, but I would like Arman to ack before applying.
>>>
>>> I'm fine with the patches as they are, though I saw that Szymon has
>>> left comments on some of them (I'm guessing he's a doing a pass of the
>>> patch set now). I'm good with it as long as his comments are
>>> addressed.
>>
>> I really think we should have more comments and less magic numbers there.
>> Otherwise this code will be hard to maintain in long term.
>
> Yep, we could actually start creating packed structs for PDU as it was
> done for AVRCP so we could do sizeof etc, it makes the code much
> easier to understand and maintain.
>

I agree, though I'd be happier if we added unit tests first. We can
then introduce packed structs and convert all code, though the unit
tests would at least give us confidence that we didn't accidentally
break any of the PDU encoding.

Cheers,
Arman