Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1418026960-21104-1-git-send-email-jakub.tyszkowski@tieto.com> <1418026960-21104-3-git-send-email-jakub.tyszkowski@tieto.com> <54856FE3.7030808@tieto.com> <54857AB3.4090309@tieto.com> Date: Mon, 8 Dec 2014 14:20:35 -0800 Message-ID: Subject: Re: [PATCH 3/5] android/gatt: Fix write commands being not handled From: Arman Uguray To: Luiz Augusto von Dentz Cc: Tyszkowski Jakub , "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Luiz, On Mon, Dec 8, 2014 at 2:08 PM, Luiz Augusto von Dentz wrote: > Hi Arman, > > On Mon, Dec 8, 2014 at 9:50 PM, Arman Uguray wrote: >> Hi Luiz & Jakub, >> >>> On Mon, Dec 8, 2014 at 3:02 AM, Luiz Augusto von Dentz wrote: >>> Hi Jakub, >>> >>> On Mon, Dec 8, 2014 at 12:17 PM, Tyszkowski Jakub >>> 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 >>>>>> wrote: >>>>>>> >>>>>>> Hi Jakub, >>>>>>> >>>>>>> On Mon, Dec 8, 2014 at 10:22 AM, Jakub Tyszkowski >>>>>>> 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