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

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

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.

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

Regards