This set of patches improves handling GATT request which requires more
then one ATT request to complete full GATT request, like service search or similar.
Now it is possible to track such GATT request with one request id.
Lukasz Rymanowski (5):
attrib/gatt: Fix minor coding style
shared/gatt: Extract send_att function
shared/gatt: Add bt_send_att_with_id functions
attrib/gattrib: Fix gattrib send command
attrib/gatt: Fix for search searvices
attrib/gatt.c | 43 ++++++++++++++++++++++++++---------
attrib/gattrib.c | 9 +++++---
src/shared/att.c | 68 +++++++++++++++++++++++++++++++++++++++++---------------
src/shared/att.h | 6 +++++
4 files changed, 94 insertions(+), 32 deletions(-)
--
1.8.4
Hi Arman,
>>>>>> With this patch it is possible to send ATT request with a given id
>>>>>> request. It might be useful for ATT user for example to keep track of
>>>>>> the GATT request which requires more then one ATT request e.g. search
>>>>>> services
>>>>>> ---
>>>>>> src/shared/att.c | 26 ++++++++++++++++++++++++++
>>>>>> src/shared/att.h | 6 ++++++
>>>>>> 2 files changed, 32 insertions(+)
>>>>>>
>>>>>> diff --git a/src/shared/att.c b/src/shared/att.c
>>>>>> index 2a131e0..f51f893 100644
>>>>>> --- a/src/shared/att.c
>>>>>> +++ b/src/shared/att.c
>>>>>> @@ -1083,6 +1083,32 @@ static unsigned int send_att(struct bt_att *att, struct att_send_op *op)
>>>>>> return op->id;
>>>>>> }
>>>>>>
>>>>>> +unsigned int bt_att_send_with_id(struct bt_att *att, unsigned int id,
>>>>>> + uint8_t opcode, const void *pdu,
>>>>>> + uint16_t length,
>>>>>> + bt_att_response_func_t callback,
>>>>>> + void *user_data,
>>>>>> + bt_att_destroy_func_t destroy)
>>>>>> +{
>>>>>> + struct att_send_op *op;
>>>>>> +
>>>>>> + if (!att || !att->io)
>>>>>> + return 0;
>>>>>
>>>>> I guess we need to check for invalid id here, or we can do the
>>>>> opposite and let 0 id be used for self assign an id so bt_att_send
>>>>> could just bt_att_send_with_id so we reuse more code.
>>>>>
>>>>>> + op = create_att_send_op(opcode, pdu, length, att->mtu, callback,
>>>>>> + user_data, destroy);
>>>>>> + if (!op)
>>>>>> + return 0;
>>>>>>
>>>>>> + /*
>>>>>> + * TODO: Some verification might be needed here. For now we
>>>>>> + * believe that user know what he is doing.
>>>>>> + */
>>>>>> + op->id = id;
>>>>>
>>>>> I think we should prevent multiple entries using the same id and Im
>>>>> also not sure why this is not being queue like the rest of operations?
>>>>
>>>> or we are not doing this at all since this sounds like a crazy API.
>>>>
>>>> If the caller wants to keep track of something, then it can keep track of it, but we are not allowing the caller to mess with internals.
>>>
>>> Well this is done internally so the caller can cancel operations such
>>> as discovery, we could add another id but the caller would have no
>>> idea the id has changed perhaps we could add an id mapping between
>>> gatt and att so the id on gatt won't change but the att id would, but
>>> with that we would need to change gattrib to do the same.
>>
>> ...and attrib/gatt.c is state less so it's not that easy to add that
>> tracking there ...
>
> I agree with Marcel here, in that it's probably better for the upper
> layer to keep track of which ATT request id maps to the overall
> operation at a given time. This is something that shared/gatt-helpers
> should be doing also but isn't, then again this hasn't hurt things
> that much so far, and all of those functions just return bool anyway.
> Generally you need this id to cancel an on-going operation in the case
> of a protocol error, and this is generally better served by something
> like bt_att_cancel_all already, where you just want to stop
> everything.
>
> Eitherway, maybe keep track of the operation states in GAttrib? It
> seems like your patch set is mainly addressing the fact that the id
> returned by g_attrib_* becomes invalid, so it sounds like the fix
> generally belongs in attrib/gatt?
this is just for reference since I am not advocating to implement this until we really need to. And with bt_gatt_client being the primary interface, I highly doubt we ever need to. However we have solved these kind of user facing use cases before.
The problem is that you want to use bt_att_cancel_all(), but only have it cancel the requests that you made and not requests that a different profile might have made. Since the ATT channel is a shared resource you need to play nice with others. One obvious solution is of course to just track all bt_att_send() call and cancel them individually via bt_att_cancel().
The only way we can ever use bt_att_cancel_all() in multi user/profile scenarios would be if we introduce struct bt_att *bt_att_clone(struct bt_att *). What this means is that you would create your own private clone of an existing struct bt_att and that all bt_att_send() and bt_att_register() are bound to your clone. Let me try to give an example.
master_att = bt_att_new();
att1 = bt_att_clone(master_att);
att2 = bt_att_clone(master_att);
bt_att_send(att1, ..)
bt_att_send(att1, ..)
bt_att_send(att2, ..)
bt_att_cancel_all(att1, ..)
So this would only cancel the two bt_att_send() from att1 and the one from att2 will be happily executed.
We have used this concept before in GAtChat of oFono. However it is some heavy lifting involved internally to get this done. We did it because it made the drivers simpler and one driver was no longer affect the other even if the run over the same physical line.
The only reason to do this in BlueZ would be that the caller code becomes so complicated when it has to track all the bt_att_send() ids that it is unreasonable. Since this is only GAttrib right now, I would not go this route. It overhead that is involved with supported clones.
Also us abstracting things behind bt_gatt_client should not bring us into the situation that we need to have to create bt_att_clone() support. Anyway, this is just reference in case someone has to look this up in the future.
Regards
Marcel
Hi Lukasz,
> On Mon, Dec 15, 2014 at 8:28 AM, Lukasz Rymanowski <lukasz.rymanowski@tie=
to.com> wrote:
> Hi Arman,
>
> On 15 December 2014 at 16:56, Arman Uguray <[email protected]> wrote:
>> Hi Lukasz,
>>
>>> On Mon, Dec 15, 2014 at 3:05 AM, Lukasz Rymanowski <lukasz.rymanowski@t=
ieto.com> wrote:
>>> Hi Marcel, Luiz,
>>>
>>> On 15 December 2014 at 12:00, Luiz Augusto von Dentz
>>> <[email protected]> wrote:
>>>> Hi Marcel,
>>>>
>>>> On Mon, Dec 15, 2014 at 12:53 PM, Marcel Holtmann <[email protected]=
> wrote:
>>>>> Hi Luiz,
>>>>>
>>>>>
>>>>>>> With this patch it is possible to send ATT request with a given id
>>>>>>> request. It might be useful for ATT user for example to keep track =
of
>>>>>>> the GATT request which requires more then one ATT request e.g. sear=
ch
>>>>>>> services
>>>>>>> ---
>>>>>>> src/shared/att.c | 26 ++++++++++++++++++++++++++
>>>>>>> src/shared/att.h | 6 ++++++
>>>>>>> 2 files changed, 32 insertions(+)
>>>>>>>
>>>>>>> diff --git a/src/shared/att.c b/src/shared/att.c
>>>>>>> index 2a131e0..f51f893 100644
>>>>>>> --- a/src/shared/att.c
>>>>>>> +++ b/src/shared/att.c
>>>>>>> @@ -1083,6 +1083,32 @@ static unsigned int send_att(struct bt_att *=
att, struct att_send_op *op)
>>>>>>> return op->id;
>>>>>>> }
>>>>>>>
>>>>>>> +unsigned int bt_att_send_with_id(struct bt_att *att, unsigned int =
id,
>>>>>>> + uint8_t opcode, const void =
*pdu,
>>>>>>> + uint16_t length,
>>>>>>> + bt_att_response_func_t call=
back,
>>>>>>> + void *user_data,
>>>>>>> + bt_att_destroy_func_t destr=
oy)
>>>>>>> +{
>>>>>>> + struct att_send_op *op;
>>>>>>> +
>>>>>>> + if (!att || !att->io)
>>>>>>> + return 0;
>>>>>>
>>>>>> I guess we need to check for invalid id here, or we can do the
>>>>>> opposite and let 0 id be used for self assign an id so bt_att_send
>>>>>> could just bt_att_send_with_id so we reuse more code.
>>>>>>
>>>>>>> + op =3D create_att_send_op(opcode, pdu, length, att->mtu, ca=
llback,
>>>>>>> + user_data, =
destroy);
>>>>>>> + if (!op)
>>>>>>> + return 0;
>>>>>>>
>>>>>>> + /*
>>>>>>> + * TODO: Some verification might be needed here. For now we
>>>>>>> + * believe that user know what he is doing.
>>>>>>> + */
>>>>>>> + op->id =3D id;
>>>>>>
>>>>>> I think we should prevent multiple entries using the same id and Im
>>>>>> also not sure why this is not being queue like the rest of operation=
s?
>>>>>
>>>>> or we are not doing this at all since this sounds like a crazy API.
>>>>>
>>>>> If the caller wants to keep track of something, then it can keep trac=
k of it, but we are not allowing the caller to mess with internals.
>>>>
>>>> Well this is done internally so the caller can cancel operations such
>>>> as discovery, we could add another id but the caller would have no
>>>> idea the id has changed perhaps we could add an id mapping between
>>>> gatt and att so the id on gatt won't change but the att id would, but
>>>> with that we would need to change gattrib to do the same.
>>>
>>> ...and attrib/gatt.c is state less so it's not that easy to add that
>>> tracking there ...
>>
>> I agree with Marcel here, in that it's probably better for the upper
>> layer to keep track of which ATT request id maps to the overall
>> operation at a given time. This is something that shared/gatt-helpers
>> should be doing also but isn't, then again this hasn't hurt things
>> that much so far, and all of those functions just return bool anyway.
>> Generally you need this id to cancel an on-going operation in the case
>> of a protocol error, and this is generally better served by something
>> like bt_att_cancel_all already, where you just want to stop
>> everything.
>>
>> Eitherway, maybe keep track of the operation states in GAttrib? It
>> seems like your patch set is mainly addressing the fact that the id
>> returned by g_attrib_* becomes invalid, so it sounds like the fix
>> generally belongs in attrib/gatt?
>
> Yes, fix belongs to attrib/gatt, but to solve it you need to cover two th=
ings.
> One thing is to have request id tracked by attrib/gatt and second thing
> is to let user of gattrib information about ongoing request id.
> Without that problem is still there
> because user of gatrrib is out of control of ongoing att request
> because of outdated request id.
> Unless we reuse existing req_id what is done in this set but is not welco=
me
>
> Anyway, after discussion on IRC we decide to remove gattrib from
> Android GATT and HOG and start use gatt-client.
> We need to do it anyway so instead of fixing gattrib we move to gatt/clie=
nt.
>
Ah OK, I missed that part of the conversation. Yes, please start using
shared/gatt-client. With more sets of eyes on that code we can
hopefully flesh out any remaining bugs and any missing features.
> \=C5=81ukasz
>
>
>
>>
>>>
>>> \Lukasz
>>>>
>>>>
>>>> --
>>>> Luiz Augusto von Dentz
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetoo=
th" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>> Cheers,
>> Arman
Thanks,
Arman
Hi Arman,
On 15 December 2014 at 16:56, Arman Uguray <[email protected]> wrote:
> Hi Lukasz,
>
>> On Mon, Dec 15, 2014 at 3:05 AM, Lukasz Rymanowski <[email protected]> wrote:
>> Hi Marcel, Luiz,
>>
>> On 15 December 2014 at 12:00, Luiz Augusto von Dentz
>> <[email protected]> wrote:
>>> Hi Marcel,
>>>
>>> On Mon, Dec 15, 2014 at 12:53 PM, Marcel Holtmann <[email protected]> wrote:
>>>> Hi Luiz,
>>>>
>>>>
>>>>>> With this patch it is possible to send ATT request with a given id
>>>>>> request. It might be useful for ATT user for example to keep track of
>>>>>> the GATT request which requires more then one ATT request e.g. search
>>>>>> services
>>>>>> ---
>>>>>> src/shared/att.c | 26 ++++++++++++++++++++++++++
>>>>>> src/shared/att.h | 6 ++++++
>>>>>> 2 files changed, 32 insertions(+)
>>>>>>
>>>>>> diff --git a/src/shared/att.c b/src/shared/att.c
>>>>>> index 2a131e0..f51f893 100644
>>>>>> --- a/src/shared/att.c
>>>>>> +++ b/src/shared/att.c
>>>>>> @@ -1083,6 +1083,32 @@ static unsigned int send_att(struct bt_att *att, struct att_send_op *op)
>>>>>> return op->id;
>>>>>> }
>>>>>>
>>>>>> +unsigned int bt_att_send_with_id(struct bt_att *att, unsigned int id,
>>>>>> + uint8_t opcode, const void *pdu,
>>>>>> + uint16_t length,
>>>>>> + bt_att_response_func_t callback,
>>>>>> + void *user_data,
>>>>>> + bt_att_destroy_func_t destroy)
>>>>>> +{
>>>>>> + struct att_send_op *op;
>>>>>> +
>>>>>> + if (!att || !att->io)
>>>>>> + return 0;
>>>>>
>>>>> I guess we need to check for invalid id here, or we can do the
>>>>> opposite and let 0 id be used for self assign an id so bt_att_send
>>>>> could just bt_att_send_with_id so we reuse more code.
>>>>>
>>>>>> + op = create_att_send_op(opcode, pdu, length, att->mtu, callback,
>>>>>> + user_data, destroy);
>>>>>> + if (!op)
>>>>>> + return 0;
>>>>>>
>>>>>> + /*
>>>>>> + * TODO: Some verification might be needed here. For now we
>>>>>> + * believe that user know what he is doing.
>>>>>> + */
>>>>>> + op->id = id;
>>>>>
>>>>> I think we should prevent multiple entries using the same id and Im
>>>>> also not sure why this is not being queue like the rest of operations?
>>>>
>>>> or we are not doing this at all since this sounds like a crazy API.
>>>>
>>>> If the caller wants to keep track of something, then it can keep track of it, but we are not allowing the caller to mess with internals.
>>>
>>> Well this is done internally so the caller can cancel operations such
>>> as discovery, we could add another id but the caller would have no
>>> idea the id has changed perhaps we could add an id mapping between
>>> gatt and att so the id on gatt won't change but the att id would, but
>>> with that we would need to change gattrib to do the same.
>>
>> ...and attrib/gatt.c is state less so it's not that easy to add that
>> tracking there ...
>
> I agree with Marcel here, in that it's probably better for the upper
> layer to keep track of which ATT request id maps to the overall
> operation at a given time. This is something that shared/gatt-helpers
> should be doing also but isn't, then again this hasn't hurt things
> that much so far, and all of those functions just return bool anyway.
> Generally you need this id to cancel an on-going operation in the case
> of a protocol error, and this is generally better served by something
> like bt_att_cancel_all already, where you just want to stop
> everything.
>
> Eitherway, maybe keep track of the operation states in GAttrib? It
> seems like your patch set is mainly addressing the fact that the id
> returned by g_attrib_* becomes invalid, so it sounds like the fix
> generally belongs in attrib/gatt?
Yes, fix belongs to attrib/gatt, but to solve it you need to cover two things.
One thing is to have request id tracked by attrib/gatt and second thing
is to let user of gattrib information about ongoing request id.
Without that problem is still there
because user of gatrrib is out of control of ongoing att request
because of outdated request id.
Unless we reuse existing req_id what is done in this set but is not welcome
Anyway, after discussion on IRC we decide to remove gattrib from
Android GATT and HOG and start use gatt-client.
We need to do it anyway so instead of fixing gattrib we move to gatt/client.
\Łukasz
>
>>
>> \Lukasz
>>>
>>>
>>> --
>>> Luiz Augusto von Dentz
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> Cheers,
> Arman
Hi Lukasz,
> On Mon, Dec 15, 2014 at 3:05 AM, Lukasz Rymanowski <[email protected]> wrote:
> Hi Marcel, Luiz,
>
> On 15 December 2014 at 12:00, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> Hi Marcel,
>>
>> On Mon, Dec 15, 2014 at 12:53 PM, Marcel Holtmann <[email protected]> wrote:
>>> Hi Luiz,
>>>
>>>
>>>>> With this patch it is possible to send ATT request with a given id
>>>>> request. It might be useful for ATT user for example to keep track of
>>>>> the GATT request which requires more then one ATT request e.g. search
>>>>> services
>>>>> ---
>>>>> src/shared/att.c | 26 ++++++++++++++++++++++++++
>>>>> src/shared/att.h | 6 ++++++
>>>>> 2 files changed, 32 insertions(+)
>>>>>
>>>>> diff --git a/src/shared/att.c b/src/shared/att.c
>>>>> index 2a131e0..f51f893 100644
>>>>> --- a/src/shared/att.c
>>>>> +++ b/src/shared/att.c
>>>>> @@ -1083,6 +1083,32 @@ static unsigned int send_att(struct bt_att *att, struct att_send_op *op)
>>>>> return op->id;
>>>>> }
>>>>>
>>>>> +unsigned int bt_att_send_with_id(struct bt_att *att, unsigned int id,
>>>>> + uint8_t opcode, const void *pdu,
>>>>> + uint16_t length,
>>>>> + bt_att_response_func_t callback,
>>>>> + void *user_data,
>>>>> + bt_att_destroy_func_t destroy)
>>>>> +{
>>>>> + struct att_send_op *op;
>>>>> +
>>>>> + if (!att || !att->io)
>>>>> + return 0;
>>>>
>>>> I guess we need to check for invalid id here, or we can do the
>>>> opposite and let 0 id be used for self assign an id so bt_att_send
>>>> could just bt_att_send_with_id so we reuse more code.
>>>>
>>>>> + op = create_att_send_op(opcode, pdu, length, att->mtu, callback,
>>>>> + user_data, destroy);
>>>>> + if (!op)
>>>>> + return 0;
>>>>>
>>>>> + /*
>>>>> + * TODO: Some verification might be needed here. For now we
>>>>> + * believe that user know what he is doing.
>>>>> + */
>>>>> + op->id = id;
>>>>
>>>> I think we should prevent multiple entries using the same id and Im
>>>> also not sure why this is not being queue like the rest of operations?
>>>
>>> or we are not doing this at all since this sounds like a crazy API.
>>>
>>> If the caller wants to keep track of something, then it can keep track of it, but we are not allowing the caller to mess with internals.
>>
>> Well this is done internally so the caller can cancel operations such
>> as discovery, we could add another id but the caller would have no
>> idea the id has changed perhaps we could add an id mapping between
>> gatt and att so the id on gatt won't change but the att id would, but
>> with that we would need to change gattrib to do the same.
>
> ...and attrib/gatt.c is state less so it's not that easy to add that
> tracking there ...
I agree with Marcel here, in that it's probably better for the upper
layer to keep track of which ATT request id maps to the overall
operation at a given time. This is something that shared/gatt-helpers
should be doing also but isn't, then again this hasn't hurt things
that much so far, and all of those functions just return bool anyway.
Generally you need this id to cancel an on-going operation in the case
of a protocol error, and this is generally better served by something
like bt_att_cancel_all already, where you just want to stop
everything.
Eitherway, maybe keep track of the operation states in GAttrib? It
seems like your patch set is mainly addressing the fact that the id
returned by g_attrib_* becomes invalid, so it sounds like the fix
generally belongs in attrib/gatt?
>
> \Lukasz
>>
>>
>> --
>> Luiz Augusto von Dentz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers,
Arman
Hi Lukasz,
On Mon, Dec 15, 2014 at 11:59 AM, Lukasz Rymanowski
<[email protected]> wrote:
> ---
> attrib/gattrib.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/attrib/gattrib.c b/attrib/gattrib.c
> index 7fe7647..3ce6748 100644
> --- a/attrib/gattrib.c
> +++ b/attrib/gattrib.c
> @@ -224,7 +224,6 @@ static void attrib_callback_result(uint8_t opcode, const void *pdu,
> free(buf);
> }
>
> -
> static void attrib_callback_notify(uint8_t opcode, const void *pdu,
> uint16_t length, void *user_data)
> {
> --
> 1.8.4
Applied just this one, lets see what directions we should go with the
rest of the set.
--
Luiz Augusto von Dentz
Hi Marcel, Luiz,
On 15 December 2014 at 12:00, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Marcel,
>
> On Mon, Dec 15, 2014 at 12:53 PM, Marcel Holtmann <[email protected]> wrote:
>> Hi Luiz,
>>
>>
>>>> With this patch it is possible to send ATT request with a given id
>>>> request. It might be useful for ATT user for example to keep track of
>>>> the GATT request which requires more then one ATT request e.g. search
>>>> services
>>>> ---
>>>> src/shared/att.c | 26 ++++++++++++++++++++++++++
>>>> src/shared/att.h | 6 ++++++
>>>> 2 files changed, 32 insertions(+)
>>>>
>>>> diff --git a/src/shared/att.c b/src/shared/att.c
>>>> index 2a131e0..f51f893 100644
>>>> --- a/src/shared/att.c
>>>> +++ b/src/shared/att.c
>>>> @@ -1083,6 +1083,32 @@ static unsigned int send_att(struct bt_att *att, struct att_send_op *op)
>>>> return op->id;
>>>> }
>>>>
>>>> +unsigned int bt_att_send_with_id(struct bt_att *att, unsigned int id,
>>>> + uint8_t opcode, const void *pdu,
>>>> + uint16_t length,
>>>> + bt_att_response_func_t callback,
>>>> + void *user_data,
>>>> + bt_att_destroy_func_t destroy)
>>>> +{
>>>> + struct att_send_op *op;
>>>> +
>>>> + if (!att || !att->io)
>>>> + return 0;
>>>
>>> I guess we need to check for invalid id here, or we can do the
>>> opposite and let 0 id be used for self assign an id so bt_att_send
>>> could just bt_att_send_with_id so we reuse more code.
>>>
>>>> + op = create_att_send_op(opcode, pdu, length, att->mtu, callback,
>>>> + user_data, destroy);
>>>> + if (!op)
>>>> + return 0;
>>>>
>>>> + /*
>>>> + * TODO: Some verification might be needed here. For now we
>>>> + * believe that user know what he is doing.
>>>> + */
>>>> + op->id = id;
>>>
>>> I think we should prevent multiple entries using the same id and Im
>>> also not sure why this is not being queue like the rest of operations?
>>
>> or we are not doing this at all since this sounds like a crazy API.
>>
>> If the caller wants to keep track of something, then it can keep track of it, but we are not allowing the caller to mess with internals.
>
> Well this is done internally so the caller can cancel operations such
> as discovery, we could add another id but the caller would have no
> idea the id has changed perhaps we could add an id mapping between
> gatt and att so the id on gatt won't change but the att id would, but
> with that we would need to change gattrib to do the same.
...and attrib/gatt.c is state less so it's not that easy to add that
tracking there ...
\Lukasz
>
>
> --
> Luiz Augusto von Dentz
Hi Marcel,
On Mon, Dec 15, 2014 at 12:53 PM, Marcel Holtmann <[email protected]> wrote:
> Hi Luiz,
>
>
>>> With this patch it is possible to send ATT request with a given id
>>> request. It might be useful for ATT user for example to keep track of
>>> the GATT request which requires more then one ATT request e.g. search
>>> services
>>> ---
>>> src/shared/att.c | 26 ++++++++++++++++++++++++++
>>> src/shared/att.h | 6 ++++++
>>> 2 files changed, 32 insertions(+)
>>>
>>> diff --git a/src/shared/att.c b/src/shared/att.c
>>> index 2a131e0..f51f893 100644
>>> --- a/src/shared/att.c
>>> +++ b/src/shared/att.c
>>> @@ -1083,6 +1083,32 @@ static unsigned int send_att(struct bt_att *att, struct att_send_op *op)
>>> return op->id;
>>> }
>>>
>>> +unsigned int bt_att_send_with_id(struct bt_att *att, unsigned int id,
>>> + uint8_t opcode, const void *pdu,
>>> + uint16_t length,
>>> + bt_att_response_func_t callback,
>>> + void *user_data,
>>> + bt_att_destroy_func_t destroy)
>>> +{
>>> + struct att_send_op *op;
>>> +
>>> + if (!att || !att->io)
>>> + return 0;
>>
>> I guess we need to check for invalid id here, or we can do the
>> opposite and let 0 id be used for self assign an id so bt_att_send
>> could just bt_att_send_with_id so we reuse more code.
>>
>>> + op = create_att_send_op(opcode, pdu, length, att->mtu, callback,
>>> + user_data, destroy);
>>> + if (!op)
>>> + return 0;
>>>
>>> + /*
>>> + * TODO: Some verification might be needed here. For now we
>>> + * believe that user know what he is doing.
>>> + */
>>> + op->id = id;
>>
>> I think we should prevent multiple entries using the same id and Im
>> also not sure why this is not being queue like the rest of operations?
>
> or we are not doing this at all since this sounds like a crazy API.
>
> If the caller wants to keep track of something, then it can keep track of it, but we are not allowing the caller to mess with internals.
Well this is done internally so the caller can cancel operations such
as discovery, we could add another id but the caller would have no
idea the id has changed perhaps we could add an id mapping between
gatt and att so the id on gatt won't change but the att id would, but
with that we would need to change gattrib to do the same.
--
Luiz Augusto von Dentz
Hi Luiz,
>> With this patch it is possible to send ATT request with a given id
>> request. It might be useful for ATT user for example to keep track of
>> the GATT request which requires more then one ATT request e.g. search
>> services
>> ---
>> src/shared/att.c | 26 ++++++++++++++++++++++++++
>> src/shared/att.h | 6 ++++++
>> 2 files changed, 32 insertions(+)
>>
>> diff --git a/src/shared/att.c b/src/shared/att.c
>> index 2a131e0..f51f893 100644
>> --- a/src/shared/att.c
>> +++ b/src/shared/att.c
>> @@ -1083,6 +1083,32 @@ static unsigned int send_att(struct bt_att *att, struct att_send_op *op)
>> return op->id;
>> }
>>
>> +unsigned int bt_att_send_with_id(struct bt_att *att, unsigned int id,
>> + uint8_t opcode, const void *pdu,
>> + uint16_t length,
>> + bt_att_response_func_t callback,
>> + void *user_data,
>> + bt_att_destroy_func_t destroy)
>> +{
>> + struct att_send_op *op;
>> +
>> + if (!att || !att->io)
>> + return 0;
>
> I guess we need to check for invalid id here, or we can do the
> opposite and let 0 id be used for self assign an id so bt_att_send
> could just bt_att_send_with_id so we reuse more code.
>
>> + op = create_att_send_op(opcode, pdu, length, att->mtu, callback,
>> + user_data, destroy);
>> + if (!op)
>> + return 0;
>>
>> + /*
>> + * TODO: Some verification might be needed here. For now we
>> + * believe that user know what he is doing.
>> + */
>> + op->id = id;
>
> I think we should prevent multiple entries using the same id and Im
> also not sure why this is not being queue like the rest of operations?
or we are not doing this at all since this sounds like a crazy API.
If the caller wants to keep track of something, then it can keep track of it, but we are not allowing the caller to mess with internals.
Regards
Marcel
Hi Lukasz,
On Mon, Dec 15, 2014 at 11:59 AM, Lukasz Rymanowski
<[email protected]> wrote:
> With this patch it is possible to send ATT request with a given id
> request. It might be useful for ATT user for example to keep track of
> the GATT request which requires more then one ATT request e.g. search
> services
> ---
> src/shared/att.c | 26 ++++++++++++++++++++++++++
> src/shared/att.h | 6 ++++++
> 2 files changed, 32 insertions(+)
>
> diff --git a/src/shared/att.c b/src/shared/att.c
> index 2a131e0..f51f893 100644
> --- a/src/shared/att.c
> +++ b/src/shared/att.c
> @@ -1083,6 +1083,32 @@ static unsigned int send_att(struct bt_att *att, struct att_send_op *op)
> return op->id;
> }
>
> +unsigned int bt_att_send_with_id(struct bt_att *att, unsigned int id,
> + uint8_t opcode, const void *pdu,
> + uint16_t length,
> + bt_att_response_func_t callback,
> + void *user_data,
> + bt_att_destroy_func_t destroy)
> +{
> + struct att_send_op *op;
> +
> + if (!att || !att->io)
> + return 0;
I guess we need to check for invalid id here, or we can do the
opposite and let 0 id be used for self assign an id so bt_att_send
could just bt_att_send_with_id so we reuse more code.
> + op = create_att_send_op(opcode, pdu, length, att->mtu, callback,
> + user_data, destroy);
> + if (!op)
> + return 0;
>
> + /*
> + * TODO: Some verification might be needed here. For now we
> + * believe that user know what he is doing.
> + */
> + op->id = id;
I think we should prevent multiple entries using the same id and Im
also not sure why this is not being queue like the rest of operations?
> + return send_att(att, op);
> +}
> +
> unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
> const void *pdu, uint16_t length,
> bt_att_response_func_t callback,
> diff --git a/src/shared/att.h b/src/shared/att.h
> index 99b5a5b..1210ddc 100644
> --- a/src/shared/att.h
> +++ b/src/shared/att.h
> @@ -60,6 +60,12 @@ unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
> bt_att_response_func_t callback,
> void *user_data,
> bt_att_destroy_func_t destroy);
> +unsigned int bt_att_send_with_id(struct bt_att *att, unsigned int id,
> + uint8_t opcode, const void *pdu,
> + uint16_t length,
> + bt_att_response_func_t callback,
> + void *user_data,
> + bt_att_destroy_func_t destroy);
> bool bt_att_cancel(struct bt_att *att, unsigned int id);
> bool bt_att_cancel_all(struct bt_att *att);
>
> --
> 1.8.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Luiz Augusto von Dentz
If client provides id to the g_attrib_send function it should be taken
into account and appropriate bt_att_send* should be called
---
attrib/gattrib.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index 3ce6748..cf0bf3b 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -276,8 +276,12 @@ guint g_attrib_send(GAttrib *attrib, guint id, const guint8 *pdu, guint16 len,
destroy_cb = attrib_callbacks_remove;
}
- return bt_att_send(attrib->att, pdu[0], (void *)pdu + 1, len - 1,
- response_cb, cb, destroy_cb);
+ if (id == 0)
+ return bt_att_send(attrib->att, pdu[0], (void *)pdu + 1,
+ len - 1, response_cb, cb, destroy_cb);
+
+ return bt_att_send_with_id(attrib->att, id, pdu[0], (void *)pdu + 1,
+ len - 1, response_cb, cb, destroy_cb);
}
gboolean g_attrib_cancel(GAttrib *attrib, guint id)
--
1.8.4
This patch adds means to reuse att request id for GATT operations
which might need more then one ATT req for complete reguest. Meaning
discover primary\included services and discover
characteristics/descriptors
This is needed for user of gattib, to make sure that att request id he
holds is valid during whole gatt operation.
So far, it could happen that gattrib did additional ATT request without
user knowledge which leads to situation that user had outdated att
request id.
Note that request id is used by the user for canceling request.
---
attrib/gatt.c | 43 ++++++++++++++++++++++++++++++++-----------
1 file changed, 32 insertions(+), 11 deletions(-)
diff --git a/attrib/gatt.c b/attrib/gatt.c
index b4be25a..aafd3f7 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -41,6 +41,7 @@
struct discover_primary {
int ref;
GAttrib *attrib;
+ unsigned int id;
bt_uuid_t uuid;
GSList *primaries;
gatt_cb_t cb;
@@ -50,6 +51,7 @@ struct discover_primary {
/* Used for the Included Services Discovery (ISD) procedure */
struct included_discovery {
GAttrib *attrib;
+ unsigned int id;
int refs;
int err;
uint16_t end_handle;
@@ -66,6 +68,7 @@ struct included_uuid_query {
struct discover_char {
int ref;
GAttrib *attrib;
+ unsigned int id;
bt_uuid_t *uuid;
uint16_t end;
GSList *characteristics;
@@ -76,6 +79,7 @@ struct discover_char {
struct discover_desc {
int ref;
GAttrib *attrib;
+ unsigned int id;
bt_uuid_t *uuid;
uint16_t end;
GSList *descriptors;
@@ -258,7 +262,7 @@ static void primary_by_uuid_cb(guint8 status, const guint8 *ipdu,
if (oplen == 0)
goto done;
- g_attrib_send(dp->attrib, 0, buf, oplen, primary_by_uuid_cb,
+ g_attrib_send(dp->attrib, dp->id, buf, oplen, primary_by_uuid_cb,
discover_primary_ref(dp), discover_primary_unref);
return;
@@ -327,7 +331,7 @@ static void primary_all_cb(guint8 status, const guint8 *ipdu, guint16 iplen,
guint16 oplen = encode_discover_primary(end + 1, 0xffff, NULL,
buf, buflen);
- g_attrib_send(dp->attrib, 0, buf, oplen, primary_all_cb,
+ g_attrib_send(dp->attrib, dp->id, buf, oplen, primary_all_cb,
discover_primary_ref(dp),
discover_primary_unref);
@@ -365,9 +369,11 @@ guint gatt_discover_primary(GAttrib *attrib, bt_uuid_t *uuid, gatt_cb_t func,
} else
cb = primary_all_cb;
- return g_attrib_send(attrib, 0, buf, plen, cb,
+ dp->id = g_attrib_send(attrib, 0, buf, plen, cb,
discover_primary_ref(dp),
discover_primary_unref);
+
+ return dp->id;
}
static void resolve_included_uuid_cb(uint8_t status, const uint8_t *pdu,
@@ -422,7 +428,7 @@ static guint resolve_included_uuid(struct included_discovery *isd,
query->isd = isd_ref(isd);
query->included = incl;
- return g_attrib_send(isd->attrib, 0, buf, oplen,
+ return g_attrib_send(isd->attrib, query->isd->id, buf, oplen,
resolve_included_uuid_cb, query,
inc_query_free);
}
@@ -459,8 +465,17 @@ static guint find_included(struct included_discovery *isd, uint16_t start)
oplen = enc_read_by_type_req(start, isd->end_handle, &uuid,
buf, buflen);
- return g_attrib_send(isd->attrib, 0, buf, oplen, find_included_cb,
+ /* If id != 0 it means we are in the middle of include search */
+ if (isd->id)
+ return g_attrib_send(isd->attrib, isd->id, buf, oplen,
+ find_included_cb, isd_ref(isd),
+ (GDestroyNotify) isd_unref);
+
+ /* This is first call from the gattrib user */
+ isd->id = g_attrib_send(isd->attrib, 0, buf, oplen, find_included_cb,
isd_ref(isd), (GDestroyNotify) isd_unref);
+
+ return isd->id;
}
static void find_included_cb(uint8_t status, const uint8_t *pdu, uint16_t len,
@@ -599,8 +614,9 @@ static void char_discovered_cb(guint8 status, const guint8 *ipdu, guint16 iplen,
if (oplen == 0)
return;
- g_attrib_send(dc->attrib, 0, buf, oplen, char_discovered_cb,
- discover_char_ref(dc), discover_char_unref);
+ g_attrib_send(dc->attrib, dc->id, buf, oplen,
+ char_discovered_cb, discover_char_ref(dc),
+ discover_char_unref);
return;
}
@@ -636,8 +652,10 @@ guint gatt_discover_char(GAttrib *attrib, uint16_t start, uint16_t end,
dc->end = end;
dc->uuid = g_memdup(uuid, sizeof(bt_uuid_t));
- return g_attrib_send(attrib, 0, buf, plen, char_discovered_cb,
+ dc->id = g_attrib_send(attrib, 0, buf, plen, char_discovered_cb,
discover_char_ref(dc), discover_char_unref);
+
+ return dc->id;
}
guint gatt_read_char_by_uuid(GAttrib *attrib, uint16_t start, uint16_t end,
@@ -1017,8 +1035,9 @@ static void desc_discovered_cb(guint8 status, const guint8 *ipdu,
if (oplen == 0)
return;
- g_attrib_send(dd->attrib, 0, buf, oplen, desc_discovered_cb,
- discover_desc_ref(dd), discover_desc_unref);
+ g_attrib_send(dd->attrib, dd->id, buf, oplen,
+ desc_discovered_cb, discover_desc_ref(dd),
+ discover_desc_unref);
return;
}
@@ -1051,8 +1070,10 @@ guint gatt_discover_desc(GAttrib *attrib, uint16_t start, uint16_t end,
dd->end = end;
dd->uuid = g_memdup(uuid, sizeof(bt_uuid_t));
- return g_attrib_send(attrib, 0, buf, plen, desc_discovered_cb,
+ dd->id = g_attrib_send(attrib, 0, buf, plen, desc_discovered_cb,
discover_desc_ref(dd), discover_desc_unref);
+
+ return dd->id;
}
guint gatt_write_cmd(GAttrib *attrib, uint16_t handle, const uint8_t *value,
--
1.8.4
With this patch it is possible to send ATT request with a given id
request. It might be useful for ATT user for example to keep track of
the GATT request which requires more then one ATT request e.g. search
services
---
src/shared/att.c | 26 ++++++++++++++++++++++++++
src/shared/att.h | 6 ++++++
2 files changed, 32 insertions(+)
diff --git a/src/shared/att.c b/src/shared/att.c
index 2a131e0..f51f893 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -1083,6 +1083,32 @@ static unsigned int send_att(struct bt_att *att, struct att_send_op *op)
return op->id;
}
+unsigned int bt_att_send_with_id(struct bt_att *att, unsigned int id,
+ uint8_t opcode, const void *pdu,
+ uint16_t length,
+ bt_att_response_func_t callback,
+ void *user_data,
+ bt_att_destroy_func_t destroy)
+{
+ struct att_send_op *op;
+
+ if (!att || !att->io)
+ return 0;
+
+ op = create_att_send_op(opcode, pdu, length, att->mtu, callback,
+ user_data, destroy);
+ if (!op)
+ return 0;
+
+ /*
+ * TODO: Some verification might be needed here. For now we
+ * believe that user know what he is doing.
+ */
+ op->id = id;
+
+ return send_att(att, op);
+}
+
unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
const void *pdu, uint16_t length,
bt_att_response_func_t callback,
diff --git a/src/shared/att.h b/src/shared/att.h
index 99b5a5b..1210ddc 100644
--- a/src/shared/att.h
+++ b/src/shared/att.h
@@ -60,6 +60,12 @@ unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
bt_att_response_func_t callback,
void *user_data,
bt_att_destroy_func_t destroy);
+unsigned int bt_att_send_with_id(struct bt_att *att, unsigned int id,
+ uint8_t opcode, const void *pdu,
+ uint16_t length,
+ bt_att_response_func_t callback,
+ void *user_data,
+ bt_att_destroy_func_t destroy);
bool bt_att_cancel(struct bt_att *att, unsigned int id);
bool bt_att_cancel_all(struct bt_att *att);
--
1.8.4
This function sends already prepared buffer. We need this helper
function in following patch
---
src/shared/att.c | 42 ++++++++++++++++++++++++------------------
1 file changed, 24 insertions(+), 18 deletions(-)
diff --git a/src/shared/att.c b/src/shared/att.c
index 26b6c5b..2a131e0 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -1050,27 +1050,10 @@ bool bt_att_unregister_disconnect(struct bt_att *att, unsigned int id)
return true;
}
-unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
- const void *pdu, uint16_t length,
- bt_att_response_func_t callback, void *user_data,
- bt_att_destroy_func_t destroy)
+static unsigned int send_att(struct bt_att *att, struct att_send_op *op)
{
- struct att_send_op *op;
bool result;
- if (!att || !att->io)
- return 0;
-
- op = create_att_send_op(opcode, pdu, length, att->mtu, callback,
- user_data, destroy);
- if (!op)
- return 0;
-
- if (att->next_send_id < 1)
- att->next_send_id = 1;
-
- op->id = att->next_send_id++;
-
/* Add the op to the correct queue based on its type */
switch (op->type) {
case ATT_OP_TYPE_REQ:
@@ -1100,6 +1083,29 @@ unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
return op->id;
}
+unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
+ const void *pdu, uint16_t length,
+ bt_att_response_func_t callback,
+ void *user_data, bt_att_destroy_func_t destroy)
+{
+ struct att_send_op *op;
+
+ if (!att || !att->io)
+ return 0;
+
+ op = create_att_send_op(opcode, pdu, length, att->mtu, callback,
+ user_data, destroy);
+ if (!op)
+ return 0;
+
+ if (att->next_send_id < 1)
+ att->next_send_id = 1;
+
+ op->id = att->next_send_id++;
+
+ return send_att(att, op);
+}
+
static bool match_op_id(const void *a, const void *b)
{
const struct att_send_op *op = a;
--
1.8.4
---
attrib/gattrib.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index 7fe7647..3ce6748 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -224,7 +224,6 @@ static void attrib_callback_result(uint8_t opcode, const void *pdu,
free(buf);
}
-
static void attrib_callback_notify(uint8_t opcode, const void *pdu,
uint16_t length, void *user_data)
{
--
1.8.4