Notification enabled before connection

[pdunaj]

Describe the bug
Bond peripheral device to Moto G5 (Android 8.1.0). Reboot device which will reconnect.
On the log I observe that before connection callback is received I get callback about notification being enabled.

CCC changes are notified from channel ops callback called from bt_l2cap_connected. This last function is called from bt_conn_set_state on BT_CONN_CONNECTED state. Now note that notify_connected is called from the same place but after bt_l2cap_connected. This explains why information about callback is sent to application after information about notification being enabled.

To Reproduce
Described above.

Impact
Application does not know which connection ccc change relates to as there is no conn argument in the callback.

Screenshots or console output

[00009340] <inf> ble_adv: Advertising started
[00065351] <err> bt_hci_core: PDUNAJ enh_conn_complete 839
[00065355] <err> bt_hci_core: PDUNAJ enh_conn_complete 958 conn=0x20002554
[00065356] <err> bt_gatt: PDUNAJ bt_gatt_connected 2429 conn 0x20002554
[00065357] <err> bt_gatt: PDUNAJ connected_cb 1078
[00065357] <err> bt_gatt: PDUNAJ gatt_ccc_changed 644
[00065357] <err> ble_gatt_hids: PDUNAJ hids_input_report_ccc_changed 440
[00065358] <err> hids: PDUNAJ mouse_notif_handler 135
[00065358] <err> hids: PDUNAJ notif_handler 125
[00065358] <err> hids: PDUNAJ broadcast_subscription_change 72 conn=0x00000000
[00065359] <inf> hids: Notifications enabled
[00065359] <err> bt_gatt: PDUNAJ connected_cb 1078
[00065359] <err> bt_gatt: PDUNAJ gatt_ccc_changed 644
[00065360] <err> ble_gatt_hids: PDUNAJ hids_boot_mouse_inp_rep_ccc_changed 494
[00065360] <err> hids: PDUNAJ boot_mouse_notif_handler 142
[00065360] <err> hids: PDUNAJ notif_handler 125
[00065361] <err> bt_conn: PDUNAJ notify_connected 136 conn=0x20002554 cb=0x00003f01
[00065363] <inf> ble_state: Connected to 80:58:f8:56:19:ce (public)
[00065364] <inf> ble_state: Set security level
[00065368] <inf> event_manager: e: hid_report_subscription_event MOUSE report notification enabled by 0x00000000
[00065370] <inf> event_manager: e: ble_peer_event id=0x20002554 CONNECTED

Environment (please complete the following information):
ncs zephyr 236ec2d2553536835c76dff84adcb188309eaf90
zephyr dee63e20

Additional context
I this it worked in the past... although I see no changes related to this done recently. Maybe something else changed or this was overlooked...

[jhedberg]

@Vudentz any thoughts on this one?

[Vudentz]

The L2CAP needs to be initialized properly in order to start receiving request on the channels, so changing the order is not really an option, besides the use of BT_GATT_CCC is recommended only when the user don't need to descriminate the connections thus why the callback don't have a connection parameter since the application shall be passing NULL to bt_gatt_notify/bt_gatt_indicate and the stack will properly propagate the notifications for the connected peers which had it enabled.

We could perhaps add the connection which first change the consolidated setting but the application won't see other connections which just happen to have the same configuration, it is just the first connection to enable and the last to disable that triggers the callback just to inform when the application shall be notifying/indicating or not.

A second option is to delay the enabling the changed callback, though with that applications won't be able to react to new connections as quickly as they used to be.

[kapi-no]

I tried solving this issue and came up with the following change:

kapi-no@9cd0c9e

Basically, it synchronizes connected callbacks with bt_gatt_connected function, which triggers CCC changed callbacks. @Vudentz, is it acceptable for you or do you see any side effects with this approach (apart from the mentioned delay)?

[Vudentz]

@kapi-no That pehaps is not too bad, I though bt_gatt_connected was doing some context initialization but that is done in bt_att_connected so all it is pushing back is CCC logic which perhaps is ok. We could perhaps add some code to initialize the CCC if the application start submitting GATT procedures, though perhaps that is no necessary since those procedures are most likely GATT client since notification/indiction are controlled via CCC.

That said Id like to understand why does the application needs to know who is connected on the CCC callback, perhaps that is something we didn't envision when creating that API, or it is the same situation of mesh control points which do need to know the connections in order to react properly.

[pdunaj]

@Vudentz , one example can a HID device that is simultaneously connected to several hosts (all of which enabled notifications) and allows user to select to which host HID reports will go to.

In short application must know which host enabled notifications (i.e. which hosts are ready to received data) and must be able to target a specific host on it's level (i.e. we cannot assume that stack will select a correct host for us).

[kapi-no]

Well, the proposed change only fixes the order of callbacks received by the application: the connected callback should precede ccc_changed callback. I opened a PR for it, where we can discuss further details:

#13473

As you also noticed, we would like to modify ccc_changed API to include connection object as an additional callback parameter. The reason for it is to better target notification's recipients (GATT clients). We would like to be able to choose a specific conn target and send the notification only to this single target even though other connected GATT clients may have notifications enabled. (In this case, we can't pass NULL to bt_gatt_notify and let the stack propagate notifications.) To be able to do that, it's necessary to keep track of connected peers that have notifications enabled for the given CCC.

[pdunaj]

@kapi-no , the issue is only for the order but all-in-all this two things are connected.

[Vudentz]

Well even if we introduce a conn to the CCC callback you had still do the mapping with the actual connection which are enabled to receive notifications.

@Vudentz , one example can a HID device that is simultaneously connected to several hosts (all of which enabled notifications) and allows user to select to which host HID reports will go to.

I supose this means the device would not even be connected, or there is a reason to connect if you won't send any data to them? Or there is some other limitation we are not aware of? Perhaps not supporting directed advertisements?

[pdunaj]

Hi @Vudentz , example can be a seamless transition between hosts. You start connection to host 2 while maintaining connection to host 1. You switch data flow to host 2 only after it is connected and ready to receive data. I know some companies may want to have this model.

[pdunaj]

@Vudentz, @kapi-no, just to clarify - at the moment we don't need to identify the conn from notification as we use only one. It may be that we won't need it for some time but it can come as a request at some point.

But we still need to have correct sequence connect->notify.

[Vudentz]

Hi @Vudentz , example can be a seamless transition between hosts. You start connection to host 2 while maintaining connection to host 1. You switch data flow to host 2 only after it is connected and ready to receive data. I know some companies may want to have this model.

Right, here are some options that I can think of:

1. Add an API to disable the subscription at server side which would then be checked by bt_gatt_notify/bt_gatt_indicate before they are sent out. One can already do something like this by changing the value on bt_gatt_ccc_cfg which actually comes from the application.

2. Add a callback bt_gatt_notify/bt_gatt_indicate which is called to authorize the notification sending.

Both 1 and 2 avoid having to have the application managing connections directly, but option 2 may break the API of bt_gatt_notify since there is no struct to append new fields like in bt_gatt_indicate.

[pdunaj]

We need to reopen this one as we see problem with it.
@kapi-no when gatt is notified about connection error is not checked. It can happen that connection failed and in this case notification will still be sent. We observe that when direct advertised connection fails.