[PATCH 0/2] Implement Long reads for GATT/ATT

[PATCH 0/2] Implement Long reads for GATT/ATT

[Brian Gix]

These patches implement long attribute reads for GATT clients.

The first patch implements the ATT_OP_READ_BLOB_REQ opcode pkt in ATT.

The second patch modifies the gatt_read_char() API to recognize long
attributes, and build up the result over multiple ATT calls to both
ATT_OP_READ_REQ and ATT_OP_READ_BLOB_REQ, until the entire attribute
has been retrieved. It then forwards to the original requester.

The public GATT API is unchanged, and the input parameters, and returned
results are the same.

Over-the-air, the request-result sequence is unchanged unless a long
attribute is detected.

Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum 

[PATCH 1/1] Implempent READ_BLOB encoding for ATT.

[Brian Gix]

Added enc_read_blob_req() API to ATT transport, to enable
the reading of attributes that exceed the length of the
MTU.

The packet composed conforms to the Bluetooth Core v4.0 specification.

Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum 

---
 attrib/att.c |   18 ++++++++++++++++++
 attrib/att.h |    2 ++
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/attrib/att.c b/attrib/att.c
index 445b192..d0879f8 100644
--- a/attrib/att.c
+++ b/attrib/att.c
@@ -542,6 +542,24 @@ uint16_t enc_read_req(uint16_t handle, uint8_t *pdu, int len)
 	return min_len;
 }
 
+uint16_t enc_read_blob_req(uint16_t handle, uint16_t offset, uint8_t *pdu,
+			   						int len)
+{
+	const uint16_t min_len = sizeof(pdu[0]) + sizeof(handle) + sizeof(offset);
+
+	if (pdu == NULL)
+		return 0;
+
+	if (len < min_len)
+		return 0;
+
+	pdu[0] = ATT_OP_READ_BLOB_REQ;
+	att_put_u16(handle, &pdu[1]);
+	att_put_u16(offset, &pdu[3]);
+
+	return min_len;
+}
+
 uint16_t dec_read_req(const uint8_t *pdu, int len, uint16_t *handle)
 {
 	const uint16_t min_len = sizeof(pdu[0]) + sizeof(*handle);
diff --git a/attrib/att.h b/attrib/att.h
index 2c8c724..46bdad4 100644
--- a/attrib/att.h
+++ b/attrib/att.h
@@ -202,6 +202,8 @@ uint16_t enc_write_req(uint16_t handle, const uint8_t *value, int vlen,
 uint16_t dec_write_req(const uint8_t *pdu, int len, uint16_t *handle,
 						uint8_t *value, int *vlen);
 uint16_t enc_read_req(uint16_t handle, uint8_t *pdu, int len);
+uint16_t enc_read_blob_req(uint16_t handle, uint16_t offset, uint8_t *pdu,
+			   						int len);
 uint16_t dec_read_req(const uint8_t *pdu, int len, uint16_t *handle);
 uint16_t enc_read_resp(uint8_t *value, int vlen, uint8_t *pdu, int len);
 uint16_t dec_read_resp(const uint8_t *pdu, int len, uint8_t *value, int *vlen);
-- 
1.7.1

Re: [PATCH 1/1] Implempent READ_BLOB encoding for ATT.

[Vinicius Costa Gomes]

Hi Brian,

On 09:24 Mon 13 Dec, Brian Gix wrote:
> Added enc_read_blob_req() API to ATT transport, to enable
> the reading of attributes that exceed the length of the
> MTU.
> 
> The packet composed conforms to the Bluetooth Core v4.0 specification.
> 
> Brian Gix
> bgix@codeaurora.org
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum 

Not sure if this signature belongs in the patch comment.

> 
> ---
>  attrib/att.c |   18 ++++++++++++++++++
>  attrib/att.h |    2 ++
>  2 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/attrib/att.c b/attrib/att.c
> index 445b192..d0879f8 100644
> --- a/attrib/att.c
> +++ b/attrib/att.c
> @@ -542,6 +542,24 @@ uint16_t enc_read_req(uint16_t handle, uint8_t *pdu, int len)
>  	return min_len;
>  }
>  
> +uint16_t enc_read_blob_req(uint16_t handle, uint16_t offset, uint8_t *pdu,
> +			   						int len)

I couldn't apply the patches because there were a mix between tabs and spaces,
please use only tabs for indentation.

> +{
> +	const uint16_t min_len = sizeof(pdu[0]) + sizeof(handle) + sizeof(offset);
> +
> +	if (pdu == NULL)
> +		return 0;
> +
> +	if (len < min_len)
> +		return 0;
> +
> +	pdu[0] = ATT_OP_READ_BLOB_REQ;
> +	att_put_u16(handle, &pdu[1]);
> +	att_put_u16(offset, &pdu[3]);
> +
> +	return min_len;
> +}
> +
>  uint16_t dec_read_req(const uint8_t *pdu, int len, uint16_t *handle)
>  {
>  	const uint16_t min_len = sizeof(pdu[0]) + sizeof(*handle);
> diff --git a/attrib/att.h b/attrib/att.h
> index 2c8c724..46bdad4 100644
> --- a/attrib/att.h
> +++ b/attrib/att.h
> @@ -202,6 +202,8 @@ uint16_t enc_write_req(uint16_t handle, const uint8_t *value, int vlen,
>  uint16_t dec_write_req(const uint8_t *pdu, int len, uint16_t *handle,
>  						uint8_t *value, int *vlen);
>  uint16_t enc_read_req(uint16_t handle, uint8_t *pdu, int len);
> +uint16_t enc_read_blob_req(uint16_t handle, uint16_t offset, uint8_t *pdu,
> +			   						int len);
>  uint16_t dec_read_req(const uint8_t *pdu, int len, uint16_t *handle);
>  uint16_t enc_read_resp(uint8_t *value, int vlen, uint8_t *pdu, int len);
>  uint16_t dec_read_resp(const uint8_t *pdu, int len, uint8_t *value, int *vlen);
> -- 
> 1.7.1
> 
> --
> 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

-- 
Vinicius

[PATCH 1/1] Support for reading long Characteristic Values.

[Brian Gix]

Modify existing gatt_read_char() function support the reading of 
attributes (specifically Characteristic Values) that are longer than
the MTU would otherwise allow.  When a result to an ATT_OP_READ_REQ
is received, it will be passed to the requester as always if the total
result was shorter than the Default MTU (23). Any results equal to or
longer will cause a series of READ_BLOB requests to be made, with the
additional results built up until the end of the Attribute is detected.
The full result will then be passed to the original requester.

The end of the Attribute is detected by either a successful result to
the READ_BLOB request that is shorter than the Default MTU, or by an
error result that indicates that a read is being attempted beyond the
length of the attribute, or that the Attribute wasn't "Long".

This patch is dependant on the earlier patch:
0001-Implempent-READ_BLOB-encoding-for-ATT.patch

The packet composed conforms to the Bluetooth Core v4.0 specification.

Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum 

---
 attrib/gatt.c |  114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 112 insertions(+), 2 deletions(-)

diff --git a/attrib/gatt.c b/attrib/gatt.c
index bca8b49..d888f1d 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -97,15 +97,125 @@ guint gatt_read_char_by_uuid(GAttrib *attrib, uint16_t start, uint16_t end,
 					pdu, plen, func, user_data, NULL);
 }
 
+struct read_long_data {
+	GAttrib *attrib;
+	GAttribResultFunc func;
+	gpointer user_data;
+	guint8 *result_data;
+	guint16 result_len;
+	guint16 handle;
+};
+
+static void read_blob_helper(guint8 status, const guint8 *res_pdu,
+					guint16 res_len, gpointer user_data)
+{
+	struct read_long_data *long_read = user_data;
+	uint8_t pdu[ATT_DEFAULT_MTU];
+	guint8 *tmp;
+	guint16 plen;
+	guint ret_val;
+
+	if (status == ATT_ECODE_ATTR_NOT_LONG ||
+	    status == ATT_ECODE_INVALID_OFFSET) {
+		status = 0;
+		goto done;
+	}
+
+	if (status != 0 || res_len == 1)
+		goto done;
+
+	tmp = g_try_realloc(long_read->result_data,
+			    	long_read->result_len + res_len - 1);
+
+	if (tmp == NULL) {
+		status = ATT_ECODE_INSUFF_RESOURCES;
+		goto done;
+	}
+
+	memcpy(&tmp[long_read->result_len], &res_pdu[1], res_len-1);
+	long_read->result_data = tmp;
+	long_read->result_len += res_len-1;
+
+	if (res_len < ATT_DEFAULT_MTU)
+		goto done;
+
+	plen = enc_read_blob_req(long_read->handle, long_read->result_len-1,
+			 				pdu, sizeof(pdu));
+	ret_val = g_attrib_send(long_read->attrib, ATT_OP_READ_BLOB_REQ, pdu,
+				plen, read_blob_helper, long_read, NULL);
+
+	if (ret_val != 0)
+		return;
+
+	status = ATT_ECODE_INSUFF_RESOURCES;
+
+done:
+	long_read->func(status, long_read->result_data, long_read->result_len,
+							long_read->user_data);
+	g_free(long_read->result_data);
+	g_free(long_read);
+}
+
+static void read_char_helper(guint8 status, const guint8 *res_pdu,
+					guint16 res_len, gpointer user_data)
+{
+	struct read_long_data *long_read = user_data;
+	uint8_t pdu[ATT_DEFAULT_MTU];
+	guint16 plen;
+	guint ret_val;
+
+	if (status != 0 || res_len < ATT_DEFAULT_MTU)
+		goto done;
+
+	long_read->result_data = g_malloc(res_len);
+
+	if (long_read->result_data == NULL)
+		goto done;
+
+	memcpy(long_read->result_data, res_pdu, res_len);
+	long_read->result_len = res_len;
+
+	plen = enc_read_blob_req(long_read->handle, res_len-1, pdu,
+				 				sizeof(pdu));
+	ret_val = g_attrib_send(long_read->attrib, ATT_OP_READ_BLOB_REQ,
+			pdu, plen, read_blob_helper, long_read, NULL);
+
+	if (ret_val != 0)
+		return;
+
+	status = ATT_ECODE_INSUFF_RESOURCES;
+
+done:
+	long_read->func(status, res_pdu, res_len, long_read->user_data);
+	g_free(long_read);
+}
+
 guint gatt_read_char(GAttrib *attrib, uint16_t handle, GAttribResultFunc func,
 							gpointer user_data)
 {
 	uint8_t pdu[ATT_DEFAULT_MTU];
 	guint16 plen;
+	guint ret_val;
+	struct read_long_data *long_read;
+
+	long_read = g_try_new0(struct read_long_data, 1);
+
+	if (long_read == NULL)
+		return 0;
+
+	long_read->attrib = attrib;
+	long_read->func = func;
+	long_read->user_data = user_data;
+	long_read->handle = handle;
 
 	plen = enc_read_req(handle, pdu, sizeof(pdu));
-	return g_attrib_send(attrib, ATT_OP_READ_REQ, pdu, plen, func,
-							user_data, NULL);
+	ret_val = g_attrib_send(attrib, ATT_OP_READ_REQ, pdu, plen,
+			     		read_char_helper, long_read, NULL);
+
+	if (ret_val == 0)
+		g_free(long_read);
+
+	return ret_val;
 }
 
 guint gatt_write_char(GAttrib *attrib, uint16_t handle, uint8_t *value,
-- 
1.7.1

Re: [PATCH 1/1] Support for reading long Characteristic Values.

[Vinicius Costa Gomes]

On 09:24 Mon 13 Dec, Brian Gix wrote:
> Modify existing gatt_read_char() function support the reading of 
> attributes (specifically Characteristic Values) that are longer than
> the MTU would otherwise allow.  When a result to an ATT_OP_READ_REQ
> is received, it will be passed to the requester as always if the total
> result was shorter than the Default MTU (23). Any results equal to or
> longer will cause a series of READ_BLOB requests to be made, with the
> additional results built up until the end of the Attribute is detected.
> The full result will then be passed to the original requester.
> 
> The end of the Attribute is detected by either a successful result to
> the READ_BLOB request that is shorter than the Default MTU, or by an
> error result that indicates that a read is being attempted beyond the
> length of the attribute, or that the Attribute wasn't "Long".
> 
> This patch is dependant on the earlier patch:
> 0001-Implempent-READ_BLOB-encoding-for-ATT.patch
> 
> The packet composed conforms to the Bluetooth Core v4.0 specification.
> 
> Brian Gix
> bgix@codeaurora.org
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum 
> 
> ---
>  attrib/gatt.c |  114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 112 insertions(+), 2 deletions(-)
> 
> diff --git a/attrib/gatt.c b/attrib/gatt.c
> index bca8b49..d888f1d 100644
> --- a/attrib/gatt.c
> +++ b/attrib/gatt.c
> @@ -97,15 +97,125 @@ guint gatt_read_char_by_uuid(GAttrib *attrib, uint16_t start, uint16_t end,
>  					pdu, plen, func, user_data, NULL);
>  }
>  
> +struct read_long_data {
> +	GAttrib *attrib;
> +	GAttribResultFunc func;
> +	gpointer user_data;
> +	guint8 *result_data;
> +	guint16 result_len;

I would call result_data "buffer" (or something like it) and result_len "size"
(or total). What do you think?

> +	guint16 handle;
> +};
> +
> +static void read_blob_helper(guint8 status, const guint8 *res_pdu,
> +					guint16 res_len, gpointer user_data)
> +{
> +	struct read_long_data *long_read = user_data;
> +	uint8_t pdu[ATT_DEFAULT_MTU];
> +	guint8 *tmp;
> +	guint16 plen;
> +	guint ret_val;
> +
> +	if (status == ATT_ECODE_ATTR_NOT_LONG ||
> +	    status == ATT_ECODE_INVALID_OFFSET) {
> +		status = 0;
> +		goto done;
> +	}
> +
> +	if (status != 0 || res_len == 1)
> +		goto done;
> +
> +	tmp = g_try_realloc(long_read->result_data,
> +			    	long_read->result_len + res_len - 1);
> +
> +	if (tmp == NULL) {
> +		status = ATT_ECODE_INSUFF_RESOURCES;
> +		goto done;
> +	}
> +
> +	memcpy(&tmp[long_read->result_len], &res_pdu[1], res_len-1);

It should be "res - 1".

> +	long_read->result_data = tmp;
> +	long_read->result_len += res_len-1;

Same here.

> +
> +	if (res_len < ATT_DEFAULT_MTU)
> +		goto done;
> +
> +	plen = enc_read_blob_req(long_read->handle, long_read->result_len-1,

And here.

> +			 				pdu, sizeof(pdu));
> +	ret_val = g_attrib_send(long_read->attrib, ATT_OP_READ_BLOB_REQ, pdu,
> +				plen, read_blob_helper, long_read, NULL);
> +
> +	if (ret_val != 0)
> +		return;
> +
> +	status = ATT_ECODE_INSUFF_RESOURCES;
> +
> +done:
> +	long_read->func(status, long_read->result_data, long_read->result_len,
> +							long_read->user_data);
> +	g_free(long_read->result_data);
> +	g_free(long_read);
> +}
> +
> +static void read_char_helper(guint8 status, const guint8 *res_pdu,
> +					guint16 res_len, gpointer user_data)
> +{
> +	struct read_long_data *long_read = user_data;
> +	uint8_t pdu[ATT_DEFAULT_MTU];
> +	guint16 plen;
> +	guint ret_val;
> +
> +	if (status != 0 || res_len < ATT_DEFAULT_MTU)
> +		goto done;
> +
> +	long_read->result_data = g_malloc(res_len);
> +
> +	if (long_read->result_data == NULL)
> +		goto done;
> +
> +	memcpy(long_read->result_data, res_pdu, res_len);
> +	long_read->result_len = res_len;
> +
> +	plen = enc_read_blob_req(long_read->handle, res_len-1, pdu,
> +				 				sizeof(pdu));
> +	ret_val = g_attrib_send(long_read->attrib, ATT_OP_READ_BLOB_REQ,
> +			pdu, plen, read_blob_helper, long_read, NULL);

g_attrib_send() returns an command id (something that could be used to cancel
the command later, if needed), so I think it would make more sense to call
ret_val "id" or just "ret". And thinking about it, I guess that the "res_"
prefix doesn't add much meaning to "res_pdu" and "res_len". Do you agree?

> +
> +	if (ret_val != 0)
> +		return;
> +
> +	status = ATT_ECODE_INSUFF_RESOURCES;
> +
> +done:
> +	long_read->func(status, res_pdu, res_len, long_read->user_data);

If g_attrib_send() fails, load_read->result_data may be leaking here.

> +	g_free(long_read);
> +}
> +
>  guint gatt_read_char(GAttrib *attrib, uint16_t handle, GAttribResultFunc func,
>  							gpointer user_data)
>  {
>  	uint8_t pdu[ATT_DEFAULT_MTU];
>  	guint16 plen;
> +	guint ret_val;
> +	struct read_long_data *long_read;
> +
> +	long_read = g_try_new0(struct read_long_data, 1);
> +
> +	if (long_read == NULL)
> +		return 0;
> +
> +	long_read->attrib = attrib;
> +	long_read->func = func;
> +	long_read->user_data = user_data;
> +	long_read->handle = handle;
>  
>  	plen = enc_read_req(handle, pdu, sizeof(pdu));
> -	return g_attrib_send(attrib, ATT_OP_READ_REQ, pdu, plen, func,
> -							user_data, NULL);
> +	ret_val = g_attrib_send(attrib, ATT_OP_READ_REQ, pdu, plen,
> +			     		read_char_helper, long_read, NULL);
> +
> +	if (ret_val == 0)
> +		g_free(long_read);
> +
> +	return ret_val;
>  }
>  
>  guint gatt_write_char(GAttrib *attrib, uint16_t handle, uint8_t *value,
> -- 
> 1.7.1
> 
> --
> 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,
-- 
Vinicius

Re: [PATCH 1/1] Support for reading long Characteristic Values.

[Brian Gix]

Hi Vinicius,

On 12/13/2010 10:39 AM, Vinicius Costa Gomes wrote:
> On 09:24 Mon 13 Dec, Brian Gix wrote:
>> Modify existing gatt_read_char() function support the reading of
>> attributes (specifically Characteristic Values) that are longer than
>> the MTU would otherwise allow.  When a result to an ATT_OP_READ_REQ
>> is received, it will be passed to the requester as always if the total
>> result was shorter than the Default MTU (23). Any results equal to or
>> longer will cause a series of READ_BLOB requests to be made, with the
>> additional results built up until the end of the Attribute is detected.
>> The full result will then be passed to the original requester.
>>
>> The end of the Attribute is detected by either a successful result to
>> the READ_BLOB request that is shorter than the Default MTU, or by an
>> error result that indicates that a read is being attempted beyond the
>> length of the attribute, or that the Attribute wasn't "Long".
>>
>> This patch is dependant on the earlier patch:
>> 0001-Implempent-READ_BLOB-encoding-for-ATT.patch
>>
>> The packet composed conforms to the Bluetooth Core v4.0 specification.
>>
>> Brian Gix
>> bgix@codeaurora.org
>> Employee of Qualcomm Innovation Center, Inc.
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

I will omit the sig in the annotation in the future.

Also, I will examine the subsequent submissions for non-TAB ws.  I need 
to work on my gvim settings a bit, I think.

>>
>> ---
>>   attrib/gatt.c |  114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 files changed, 112 insertions(+), 2 deletions(-)
>>
>> diff --git a/attrib/gatt.c b/attrib/gatt.c
>> index bca8b49..d888f1d 100644
>> --- a/attrib/gatt.c
>> +++ b/attrib/gatt.c
>> @@ -97,15 +97,125 @@ guint gatt_read_char_by_uuid(GAttrib *attrib, uint16_t start, uint16_t end,
>>   					pdu, plen, func, user_data, NULL);
>>   }
>>
>> +struct read_long_data {
>> +	GAttrib *attrib;
>> +	GAttribResultFunc func;
>> +	gpointer user_data;
>> +	guint8 *result_data;
>> +	guint16 result_len;
>
> I would call result_data "buffer" (or something like it) and result_len "size"
> (or total). What do you think?

Will rename to buffer and size. I often avoid "size" to avoid keyword 
collisions, but I don't believe it applies here.

>
>> +	guint16 handle;
>> +};
>> +
>> +static void read_blob_helper(guint8 status, const guint8 *res_pdu,
>> +					guint16 res_len, gpointer user_data)
>> +{
>> +	struct read_long_data *long_read = user_data;
>> +	uint8_t pdu[ATT_DEFAULT_MTU];
>> +	guint8 *tmp;
>> +	guint16 plen;
>> +	guint ret_val;
>> +
>> +	if (status == ATT_ECODE_ATTR_NOT_LONG ||
>> +	    status == ATT_ECODE_INVALID_OFFSET) {
>> +		status = 0;
>> +		goto done;
>> +	}
>> +
>> +	if (status != 0 || res_len == 1)
>> +		goto done;
>> +
>> +	tmp = g_try_realloc(long_read->result_data,
>> +			    	long_read->result_len + res_len - 1);
>> +
>> +	if (tmp == NULL) {
>> +		status = ATT_ECODE_INSUFF_RESOURCES;
>> +		goto done;
>> +	}
>> +
>> +	memcpy(&tmp[long_read->result_len],&res_pdu[1], res_len-1);
>
> It should be "res - 1".
>
>> +	long_read->result_data = tmp;
>> +	long_read->result_len += res_len-1;
>
> Same here.
>
>> +
>> +	if (res_len<  ATT_DEFAULT_MTU)
>> +		goto done;
>> +
>> +	plen = enc_read_blob_req(long_read->handle, long_read->result_len-1,
>
> And here.

done, done, done.

>
>> +			 				pdu, sizeof(pdu));
>> +	ret_val = g_attrib_send(long_read->attrib, ATT_OP_READ_BLOB_REQ, pdu,
>> +				plen, read_blob_helper, long_read, NULL);
>> +
>> +	if (ret_val != 0)
>> +		return;
>> +
>> +	status = ATT_ECODE_INSUFF_RESOURCES;
>> +
>> +done:
>> +	long_read->func(status, long_read->result_data, long_read->result_len,
>> +							long_read->user_data);
>> +	g_free(long_read->result_data);
>> +	g_free(long_read);
>> +}
>> +
>> +static void read_char_helper(guint8 status, const guint8 *res_pdu,
>> +					guint16 res_len, gpointer user_data)
>> +{
>> +	struct read_long_data *long_read = user_data;
>> +	uint8_t pdu[ATT_DEFAULT_MTU];
>> +	guint16 plen;
>> +	guint ret_val;
>> +
>> +	if (status != 0 || res_len<  ATT_DEFAULT_MTU)
>> +		goto done;
>> +
>> +	long_read->result_data = g_malloc(res_len);
>> +
>> +	if (long_read->result_data == NULL)
>> +		goto done;
>> +
>> +	memcpy(long_read->result_data, res_pdu, res_len);
>> +	long_read->result_len = res_len;
>> +
>> +	plen = enc_read_blob_req(long_read->handle, res_len-1, pdu,
>> +				 				sizeof(pdu));
>> +	ret_val = g_attrib_send(long_read->attrib, ATT_OP_READ_BLOB_REQ,
>> +			pdu, plen, read_blob_helper, long_read, NULL);
>
> g_attrib_send() returns an command id (something that could be used to cancel
> the command later, if needed), so I think it would make more sense to call
> ret_val "id" or just "ret". And thinking about it, I guess that the "res_"
> prefix doesn't add much meaning to "res_pdu" and "res_len". Do you agree?

renamed res_pdu to "rpdu", to avoid name collision with outbound local 
pdu variable.

renamed res_len to "rlen" to keep the two together, and distinct from 
plen, which is also used for the outbound pdu.

renamed ret_val to "ret".  In two of the functions, it has lost the 
ability to be used to cancel the GATT procedure. It can cancel the 
original ATT_OP_READ_REQ opcodes, but not the subsequent 
ATT_OP_READ_BLOB_REQ opcodes.

>
>> +
>> +	if (ret_val != 0)
>> +		return;
>> +
>> +	status = ATT_ECODE_INSUFF_RESOURCES;
>> +
>> +done:
>> +	long_read->func(status, res_pdu, res_len, long_read->user_data);
>
> If g_attrib_send() fails, load_read->result_data may be leaking here.

Is there no guarantee that the GAttribResultFunc parameter will be 
invoked if there is a non-Zero return value from g_attrib_send?

If there is paths that could result in a non-response (abnormal link 
termination?) then you are correct, and I will need to rethink clean-up.

If I am correctly reading the code, I can pass a function to the 
GDestroyNotify parameter of g_attrib_send, which will alert me to the 
destruction of the command, at which point I could do clean-up. Is that 
path foolproof? This would also entail removing all g_free clean-ups 
from the helper functions, because the destroy function is always 
called, including after invoking the GAttribResultFunc function.

Also:
If we intend to use the ID of the (original) command to cancel the GATT
procedure, I propose the following:

Compound GATT procedure such as the Read Long procedure that I am 
implementing, should save the original ID returned by g_attrib_send in 
the nested user_data structure such as I have done. When subsequent ATT 
commands are sent as part of the same GATT procedure, the new command 
shall be assigned the same ID as the original.  This could be done with 
a function in gattrib.c with the prototype:

g_attrib_set_id(GAttrib *attrib, guint old_id, guint new_id);

This would allow the upper layer to cancel the entire GATT procedure, 
even if it is partially completed.

This same methodology could then be applied to long writes, and other 
compound GATT procedures.

What do you think?

>
>> +	g_free(long_read);
>> +}
>> +
>>   guint gatt_read_char(GAttrib *attrib, uint16_t handle, GAttribResultFunc func,
>>   							gpointer user_data)
>>   {
>>   	uint8_t pdu[ATT_DEFAULT_MTU];
>>   	guint16 plen;
>> +	guint ret_val;
>> +	struct read_long_data *long_read;
>> +
>> +	long_read = g_try_new0(struct read_long_data, 1);
>> +
>> +	if (long_read == NULL)
>> +		return 0;
>> +
>> +	long_read->attrib = attrib;
>> +	long_read->func = func;
>> +	long_read->user_data = user_data;
>> +	long_read->handle = handle;
>>
>>   	plen = enc_read_req(handle, pdu, sizeof(pdu));
>> -	return g_attrib_send(attrib, ATT_OP_READ_REQ, pdu, plen, func,
>> -							user_data, NULL);
>> +	ret_val = g_attrib_send(attrib, ATT_OP_READ_REQ, pdu, plen,
>> +			     		read_char_helper, long_read, NULL);
>> +
>> +	if (ret_val == 0)
>> +		g_free(long_read);
>> +
>> +	return ret_val;
>>   }
>>
>>   guint gatt_write_char(GAttrib *attrib, uint16_t handle, uint8_t *value,
>> --
>> 1.7.1
>>
>> --
>> 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,

Thanks,

-- 
Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 1/1] Support for reading long Characteristic Values.

[Vinicius Costa Gomes]

On 11:56 Mon 13 Dec, Brian Gix wrote:
> Hi Vinicius,
> 
> On 12/13/2010 10:39 AM, Vinicius Costa Gomes wrote:
> > On 09:24 Mon 13 Dec, Brian Gix wrote:
> >> Modify existing gatt_read_char() function support the reading of
> >> attributes (specifically Characteristic Values) that are longer than
> >> the MTU would otherwise allow.  When a result to an ATT_OP_READ_REQ
> >> is received, it will be passed to the requester as always if the total
> >> result was shorter than the Default MTU (23). Any results equal to or
> >> longer will cause a series of READ_BLOB requests to be made, with the
> >> additional results built up until the end of the Attribute is detected.
> >> The full result will then be passed to the original requester.
> >>
> >> The end of the Attribute is detected by either a successful result to
> >> the READ_BLOB request that is shorter than the Default MTU, or by an
> >> error result that indicates that a read is being attempted beyond the
> >> length of the attribute, or that the Attribute wasn't "Long".
> >>
> >> This patch is dependant on the earlier patch:
> >> 0001-Implempent-READ_BLOB-encoding-for-ATT.patch
> >>
> >> The packet composed conforms to the Bluetooth Core v4.0 specification.
> >>
> >> Brian Gix
> >> bgix@codeaurora.org
> >> Employee of Qualcomm Innovation Center, Inc.
> >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
> 
> I will omit the sig in the annotation in the future.
> 
> Also, I will examine the subsequent submissions for non-TAB ws.  I need 
> to work on my gvim settings a bit, I think.
> 
> >>
> >> ---
> >>   attrib/gatt.c |  114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>   1 files changed, 112 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/attrib/gatt.c b/attrib/gatt.c
> >> index bca8b49..d888f1d 100644
> >> --- a/attrib/gatt.c
> >> +++ b/attrib/gatt.c
> >> @@ -97,15 +97,125 @@ guint gatt_read_char_by_uuid(GAttrib *attrib, uint16_t start, uint16_t end,
> >>   					pdu, plen, func, user_data, NULL);
> >>   }
> >>
> >> +struct read_long_data {
> >> +	GAttrib *attrib;
> >> +	GAttribResultFunc func;
> >> +	gpointer user_data;
> >> +	guint8 *result_data;
> >> +	guint16 result_len;
> >
> > I would call result_data "buffer" (or something like it) and result_len "size"
> > (or total). What do you think?
> 
> Will rename to buffer and size. I often avoid "size" to avoid keyword 
> collisions, but I don't believe it applies here.
> 
> >
> >> +	guint16 handle;
> >> +};
> >> +
> >> +static void read_blob_helper(guint8 status, const guint8 *res_pdu,
> >> +					guint16 res_len, gpointer user_data)
> >> +{
> >> +	struct read_long_data *long_read = user_data;
> >> +	uint8_t pdu[ATT_DEFAULT_MTU];
> >> +	guint8 *tmp;
> >> +	guint16 plen;
> >> +	guint ret_val;
> >> +
> >> +	if (status == ATT_ECODE_ATTR_NOT_LONG ||
> >> +	    status == ATT_ECODE_INVALID_OFFSET) {
> >> +		status = 0;
> >> +		goto done;
> >> +	}
> >> +
> >> +	if (status != 0 || res_len == 1)
> >> +		goto done;
> >> +
> >> +	tmp = g_try_realloc(long_read->result_data,
> >> +			    	long_read->result_len + res_len - 1);
> >> +
> >> +	if (tmp == NULL) {
> >> +		status = ATT_ECODE_INSUFF_RESOURCES;
> >> +		goto done;
> >> +	}
> >> +
> >> +	memcpy(&tmp[long_read->result_len],&res_pdu[1], res_len-1);
> >
> > It should be "res - 1".
> >
> >> +	long_read->result_data = tmp;
> >> +	long_read->result_len += res_len-1;
> >
> > Same here.
> >
> >> +
> >> +	if (res_len<  ATT_DEFAULT_MTU)
> >> +		goto done;
> >> +
> >> +	plen = enc_read_blob_req(long_read->handle, long_read->result_len-1,
> >
> > And here.
> 
> done, done, done.
> 
> >
> >> +			 				pdu, sizeof(pdu));
> >> +	ret_val = g_attrib_send(long_read->attrib, ATT_OP_READ_BLOB_REQ, pdu,
> >> +				plen, read_blob_helper, long_read, NULL);
> >> +
> >> +	if (ret_val != 0)
> >> +		return;
> >> +
> >> +	status = ATT_ECODE_INSUFF_RESOURCES;
> >> +
> >> +done:
> >> +	long_read->func(status, long_read->result_data, long_read->result_len,
> >> +							long_read->user_data);
> >> +	g_free(long_read->result_data);
> >> +	g_free(long_read);
> >> +}
> >> +
> >> +static void read_char_helper(guint8 status, const guint8 *res_pdu,
> >> +					guint16 res_len, gpointer user_data)
> >> +{
> >> +	struct read_long_data *long_read = user_data;
> >> +	uint8_t pdu[ATT_DEFAULT_MTU];
> >> +	guint16 plen;
> >> +	guint ret_val;
> >> +
> >> +	if (status != 0 || res_len<  ATT_DEFAULT_MTU)
> >> +		goto done;
> >> +
> >> +	long_read->result_data = g_malloc(res_len);
> >> +
> >> +	if (long_read->result_data == NULL)
> >> +		goto done;
> >> +
> >> +	memcpy(long_read->result_data, res_pdu, res_len);
> >> +	long_read->result_len = res_len;
> >> +
> >> +	plen = enc_read_blob_req(long_read->handle, res_len-1, pdu,
> >> +				 				sizeof(pdu));
> >> +	ret_val = g_attrib_send(long_read->attrib, ATT_OP_READ_BLOB_REQ,
> >> +			pdu, plen, read_blob_helper, long_read, NULL);
> >
> > g_attrib_send() returns an command id (something that could be used to cancel
> > the command later, if needed), so I think it would make more sense to call
> > ret_val "id" or just "ret". And thinking about it, I guess that the "res_"
> > prefix doesn't add much meaning to "res_pdu" and "res_len". Do you agree?
> 
> renamed res_pdu to "rpdu", to avoid name collision with outbound local 
> pdu variable.
> 
> renamed res_len to "rlen" to keep the two together, and distinct from 
> plen, which is also used for the outbound pdu.
> 
> renamed ret_val to "ret".  In two of the functions, it has lost the 
> ability to be used to cancel the GATT procedure. It can cancel the 
> original ATT_OP_READ_REQ opcodes, but not the subsequent 
> ATT_OP_READ_BLOB_REQ opcodes.
> 
> >
> >> +
> >> +	if (ret_val != 0)
> >> +		return;
> >> +
> >> +	status = ATT_ECODE_INSUFF_RESOURCES;
> >> +
> >> +done:
> >> +	long_read->func(status, res_pdu, res_len, long_read->user_data);
> >
> > If g_attrib_send() fails, load_read->result_data may be leaking here.
> 
> Is there no guarantee that the GAttribResultFunc parameter will be 
> invoked if there is a non-Zero return value from g_attrib_send?
> 
> If there is paths that could result in a non-response (abnormal link 
> termination?) then you are correct, and I will need to rethink clean-up.

The case I was referring was when g_attrib_send() cannot allocate memory for
the command. 

> 
> If I am correctly reading the code, I can pass a function to the 
> GDestroyNotify parameter of g_attrib_send, which will alert me to the 
> destruction of the command, at which point I could do clean-up. Is that 
> path foolproof? This would also entail removing all g_free clean-ups 
> from the helper functions, because the destroy function is always 
> called, including after invoking the GAttribResultFunc function.
> 

Yeah, this path should be foolproof. The destroy function should be called
even for abnormal cases, the ResultFunc is called just when the command
finishes (with a response or an error).

> Also:
> If we intend to use the ID of the (original) command to cancel the GATT
> procedure, I propose the following:
> 
> Compound GATT procedure such as the Read Long procedure that I am 
> implementing, should save the original ID returned by g_attrib_send in 
> the nested user_data structure such as I have done. When subsequent ATT 
> commands are sent as part of the same GATT procedure, the new command 
> shall be assigned the same ID as the original.  This could be done with 
> a function in gattrib.c with the prototype:
> 
> g_attrib_set_id(GAttrib *attrib, guint old_id, guint new_id);
> 
> This would allow the upper layer to cancel the entire GATT procedure, 
> even if it is partially completed.
> 
> This same methodology could then be applied to long writes, and other 
> compound GATT procedures.
> 
> What do you think?

Sounds good, I just don't know about messing with the commands id's, 
maybe having some thing like command groups. 

For example, adding these funtions:

guint g_attrib_send_full(GAttrib *attrib, guint8 opcode, const guint8 *pdu,
				guint16 len, guint group, GAttribResultFunc func,
				gpointer user_data, GDestroyNotify notify);

gboolean g_attrib_cancel_group(GAttrib *attrib, guint group);

How do this look?

> 
> >
> >> +	g_free(long_read);
> >> +}
> >> +
> >>   guint gatt_read_char(GAttrib *attrib, uint16_t handle, GAttribResultFunc func,
> >>   							gpointer user_data)
> >>   {
> >>   	uint8_t pdu[ATT_DEFAULT_MTU];
> >>   	guint16 plen;
> >> +	guint ret_val;
> >> +	struct read_long_data *long_read;
> >> +
> >> +	long_read = g_try_new0(struct read_long_data, 1);
> >> +
> >> +	if (long_read == NULL)
> >> +		return 0;
> >> +
> >> +	long_read->attrib = attrib;
> >> +	long_read->func = func;
> >> +	long_read->user_data = user_data;
> >> +	long_read->handle = handle;
> >>
> >>   	plen = enc_read_req(handle, pdu, sizeof(pdu));
> >> -	return g_attrib_send(attrib, ATT_OP_READ_REQ, pdu, plen, func,
> >> -							user_data, NULL);
> >> +	ret_val = g_attrib_send(attrib, ATT_OP_READ_REQ, pdu, plen,
> >> +			     		read_char_helper, long_read, NULL);
> >> +
> >> +	if (ret_val == 0)
> >> +		g_free(long_read);
> >> +
> >> +	return ret_val;
> >>   }
> >>
> >>   guint gatt_write_char(GAttrib *attrib, uint16_t handle, uint8_t *value,
> >> --
> >> 1.7.1
> >>
> >> --
> >> 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,
> 
> Thanks,
> 
> -- 
> Brian Gix
> bgix@codeaurora.org
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Cheers,
-- 
Vinicius

Re: [PATCH 1/1] Support for reading long Characteristic Values.

[Brian Gix]

Hi Vinicius,

On 12/13/2010 12:33 PM, Vinicius Costa Gomes wrote:
> On 11:56 Mon 13 Dec, Brian Gix wrote:
> [trimmed]
 >
 >>>> +
 >>>> +	if (ret_val != 0)
 >>>> +		return;
 >>>> +
 >>>> +	status = ATT_ECODE_INSUFF_RESOURCES;
 >>>> +
 >>>> +done:
 >>>> +	long_read->func(status, res_pdu, res_len, long_read->user_data);
 >>>
 >>> If g_attrib_send() fails, load_read->result_data may be leaking here.
>>
>> Is there no guarantee that the GAttribResultFunc parameter will be
>> invoked if there is a non-Zero return value from g_attrib_send?
>>
>> If there is paths that could result in a non-response (abnormal link
>> termination?) then you are correct, and I will need to rethink clean-up.
>
> The case I was referring was when g_attrib_send() cannot allocate memory for
> the command.
>

I don't think this is an issue, because if g_attrib_send() cannot 
allocate memory, then it will return a 0, and fall through to the 
completion and clean-up. However, I like the idea of doing all clean-up 
in the destructor anyway, which resolves the memory leak potential.


>>>  [...]
>> Also:
>> If we intend to use the ID of the (original) command to cancel the GATT
>> procedure, I propose the following:
>>
>> Compound GATT procedure such as the Read Long procedure that I am
>> implementing, should save the original ID returned by g_attrib_send in
>> the nested user_data structure such as I have done. When subsequent ATT
>> commands are sent as part of the same GATT procedure, the new command
>> shall be assigned the same ID as the original.  This could be done with
>> a function in gattrib.c with the prototype:
>>
>> g_attrib_set_id(GAttrib *attrib, guint old_id, guint new_id);
>>
>> This would allow the upper layer to cancel the entire GATT procedure,
>> even if it is partially completed.
>>
>> This same methodology could then be applied to long writes, and other
>> compound GATT procedures.
>>
>> What do you think?
>
> Sounds good, I just don't know about messing with the commands id's,
> maybe having some thing like command groups.
>
> For example, adding these funtions:
>
> guint g_attrib_send_full(GAttrib *attrib, guint8 opcode, const guint8 *pdu,
> 				guint16 len, guint group, GAttribResultFunc func,
> 				gpointer user_data, GDestroyNotify notify);
>
> gboolean g_attrib_cancel_group(GAttrib *attrib, guint group);
>
> How do this look?
>

My biggest objection would be that to go this route, I think we would 
want to extend this "command group" to all GATT procedures, and not just 
the ones known or suspected to be Compound.  I don't think that 
canceling a "Characteristic Value Read" procedure should be any 
different (or use a different parameter type) than canceling "Find 
Primary Service", for example.

Do you think everything in gatt.c should use these "command groups"? 
And if so, by what mechanism would the "group number" remain unique?

It doesn't seem to me to be such a horrible overloading of the id. I do 
see that when a cancel is called, that it is important that only one 
command with that ID is in the queue. However, this reclaimation/reuse 
of the old ID would only occur after the prior instance is in the 
process of being destroyed. The danger I suppose would be the misuse of 
this renumbering capability by an entity other than gatt.c.


I'm sorry this is getting long winded BUT:

This also highlights the inherent danger of allowing multiple Requests 
to be queued up. If are to allow a Read-Value, Write-Value, Read-Value 
sequence of commands to be enqueued onto the attrib->queue, it will be 
impossible to correctly implement any of the compound Read or Write 
commands, because the subsequent "Read-Blob" would be added to the end 
of the queue, and the read could be split around a write.  GATT/ATT was 
always intended to be a single outstanding request (and procedure) at a 
time kind of protocol, and having a FIFO attrib->queue subverts that intent.

To get compound GATT procedures to work properly (while maintaining the 
Attrib queuing mechanism) we may need to restructure gattrib.c such that 
we can add the subsequent commands to the *head* of the queue, and not 
call wake_up_sender in the received_data() function until after the 
cmd->func() has been invoked, and given the opportunity to "continue the 
current procedure".

>
> Cheers,


-- 
Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum