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

[Vinicius Costa Gomes <vinicius.gomes@openbossa.org>]

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