Re: [PATCH 3/5] android/gatt: Fix write commands being not handled

[Tyszkowski Jakub <jakub.tyszkowski@tieto.com>]

Hi Arman, Luiz,

On 12/08/2014 11:20 PM, Arman Uguray wrote:
> Hi Luiz,
>
> On Mon, Dec 8, 2014 at 2:08 PM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Arman,
>>
>> On Mon, Dec 8, 2014 at 9:50 PM, Arman Uguray <armansito@chromium.org> wrote:
>>> Hi Luiz & Jakub,
>>>
>>>> On Mon, Dec 8, 2014 at 3:02 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>>> Hi Jakub,
>>>>
>>>> On Mon, Dec 8, 2014 at 12:17 PM, Tyszkowski Jakub
>>>> <jakub.tyszkowski@tieto.com> 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@gmail.com> wrote:
>>>>>>>>
>>>>>>>> Hi Jakub,
>>>>>>>>
>>>>>>>> On Mon, Dec 8, 2014 at 10:22 AM, Jakub Tyszkowski
>>>>>>>> <jakub.tyszkowski@tieto.com> 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.

Sounds great. :)

>
>>> 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.

Right. I'll send patch enabling commands under ALL_REQUEST type, as 
attrib-server.c looks to also be affected. I'll redo this set so that 
write commands stays in the same handler as requests and only 
indications are handled separately.

>
> Thanks,
> Arman
>

Regards,
Jakub