Hi Luiz,
On Mon, Dec 8, 2014 at 2:08 PM, Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
> Hi Arman,
>
> On Mon, Dec 8, 2014 at 9:50 PM, Arman Uguray <armansito@xxxxxxxxxxxx> wrote:
>> Hi Luiz & Jakub,
>>
>>> On Mon, Dec 8, 2014 at 3:02 AM, Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote:
>>> Hi Jakub,
>>>
>>> On Mon, Dec 8, 2014 at 12:17 PM, Tyszkowski Jakub
>>> <jakub.tyszkowski@xxxxxxxxx> wrote:
>>>> Hi,
>>>>
>>>>
>>>> On 12/08/2014 10:31 AM, Tyszkowski Jakub wrote:
>>>>>
>>>>> Hi Luiz,
>>>>>
>>>>> On 12/08/2014 10:22 AM, Luiz Augusto von Dentz wrote:
>>>>>>
>>>>>> Hi Jakub,
>>>>>>
>>>>>> On Mon, Dec 8, 2014 at 10:55 AM, Luiz Augusto von Dentz
>>>>>> <luiz.dentz@xxxxxxxxx> wrote:
>>>>>>>
>>>>>>> Hi Jakub,
>>>>>>>
>>>>>>> On Mon, Dec 8, 2014 at 10:22 AM, Jakub Tyszkowski
>>>>>>> <jakub.tyszkowski@xxxxxxxxx> wrote:
>>>>>>>>
>>>>>>>> Registering for GATTRIB_ALL_REQS now means only requests and not
>>>>>>>> commands
>>>>>>>> or other type of att operations. Those needs their own handlers to be
>>>>>>>> registered.
>>>>>>>
>>>>>>>
>>>>>>> Is this something we break while with the internal changes to use
>>>>>>> bt_gatt* internally? Maybe we should have it fixed there instead since
>>>>>>> it perhaps breaks the core daemon as well. Going forward this code
>>>>>>> will transition to use bt_gatt* directly.
>>>>>>
>>>>>>
>>>>>> Looks like this code is to blame:
>>>>>>
>>>>>> opcode_match:
>>>>>>
>>>>>> if (opcode == BT_ATT_ALL_REQUESTS &&
>>>>>> get_op_type(test_opcode) == ATT_OP_TYPE_REQ)
>>>>>> return true;
>>>>>>
>>>>>> Id say this turn BT_ATT_ALL_REQUESTS not that convenient for server
>>>>>> since, and looking at gatt-server.c you can clearly see how many
>>>>>> handlers we end up doing, perhaps we could something like this:
>>>>>>
>>>>>> diff --git a/src/shared/att.c b/src/shared/att.c
>>>>>> index bc01827..3b52607 100644
>>>>>> --- a/src/shared/att.c
>>>>>> +++ b/src/shared/att.c
>>>>>> @@ -661,7 +661,7 @@ struct notify_data {
>>>>>> static bool opcode_match(uint8_t opcode, uint8_t test_opcode)
>>>>>> {
>>>>>> if (opcode == BT_ATT_ALL_REQUESTS &&
>>>>>> - get_op_type(test_opcode) ==
>>>>>> ATT_OP_TYPE_REQ)
>>>>>> + get_op_type(test_opcode) !=
>>>>>> ATT_OP_TYPE_RSP)
>>>>>> return true;
>>>>
>>>>
>>>> If we do that, we are still left with other stuff like indications or even
>>>> confirmations which would still be quite quirky for using *ALL_REQESTS in
>>>> name.
>>>
>>> Perhaps we check ATT_OP_TYPE_CMD in addition to ATT_OP_TYPE_REQ.
>>>
>>>>>>
>>>>>> return opcode == test_opcode;
>>>>>>
>>>>> Yeap, that's the line. I assumed that this change was intended and we
>>>>> should make the daemon compatible with this new semantic.
>>>>> I've looked through the code where ALL_REQUESTS is used and I think
>>>>> there is no other place affected by this.
>>>>>
>>
>> The change in bt_att was really done to make GAttrib's ALL_REQUESTS
>> behavior work, otherwise we wouldn't have added it. In general, new
>> code should just register individual handler functions for the
>> specific opcodes it wants to handle. This leads to cleaner code, and
>> avoids the kind of huge switch/if statements you end up needing with
>> an ALL_* handler.
>
> I would classify a command as a request as well which apparently is
> not the case anymore, anyway the problem is that ALL_REQUESTS was
> create to map to GAttrib but we end up with something different thus
> breaking Android code.
Oh OK, if the GAttrib code groups commands under the ALL_REQUESTS flag
then bt_att should respect that for compatibility, at least until we
remove the dependencies. So feel free to make the fix to bt_att so
that it works with commands as well.
> ... For new code you actually should not register
> handler yourself, except as a client for notifications and
> indications, since bt_gatt_server should take care of those but I
> agree that is probably cleaner to have each handler registered
> separately.
>
Sorry, that's exactly what I meant. In general if new code needs to
handle specific opcodes then they should register a special handler
for it, except they won't need to for most things since bt_gatt_server
already does it as you said :)
>> So for notifications/indications, you should really just register a
>> handler with the associated opcodes.
>
> Yep, that should really be the only handlers outside bt_gatt_server scope.
>
Right. Actually, in the future, for incoming notifications/indications
new code should use bt_gatt_client_register_notify. This automatically
writes to the remote CCC (does reference counting so that
notifications don't accidentally get disabled), sets its own handler
with bt_att for notifications and indications and automatically sends
out a confirmation for the latter. So I guess new code generally won't
need to ever directly call bt_att_register as long as they use
bt_gatt_client/bt_gatt_server.
>> As a note: bt_att doesn't allow registering handlers for incoming
>> response PDUs. Instead, those are propagated to the upper layer via
>> the callback passed to bt_att_send when the request was originally
>> sent out. Basically, you don't get to know about a response unless you
>> sent out the request in the first place (and AFAIK this is the case
>> with GAttrib as well).
>
> I guess we were discussing how to get the same behavior as in GAttrib
> ALL_REQUESTS, this would probably be removed once we transition to
> bt_gatt_server.
>
I think so. I think it's basically just for backwards compatibility to
keep GAttrib working and we can remove it in the future. We should
probably add a note in shared/att-types.h to discourage new code from
using it.
Thanks,
Arman
--
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
