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@xxxxxxxxx> wrote:
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.
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
--
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
