Return-Path: MIME-Version: 1.0 In-Reply-To: <54857AB3.4090309@tieto.com> 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 13:02:59 +0200 Message-ID: Subject: Re: [PATCH 3/5] android/gatt: Fix write commands being not handled From: Luiz Augusto von Dentz To: Tyszkowski Jakub Cc: "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: 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. >> >> Regards > > > Regards > -- Luiz Augusto von Dentz