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. 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.
> 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.
> 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.
--
Luiz Augusto von Dentz
--
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
