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@xxxxxxxxxxxxxx
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers,
Thanks,
--
Brian Gix
bgix@xxxxxxxxxxxxxx
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
