Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GATT: bt_gatt_is_subscribed does not work as expected when called from bt_conn_cb->connected #42829

Closed
MariuszSkamra opened this issue Feb 15, 2022 · 3 comments · Fixed by #44015
Closed

GATT: bt_gatt_is_subscribed does not work as expected when called from bt_conn_cb->connected #42829

MariuszSkamra opened this issue Feb 15, 2022 · 3 comments · Fixed by #44015
Assignees
@MariuszSkamra @alwa-nordic
Labels
area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug

Comments

@MariuszSkamra
Copy link
Collaborator

MariuszSkamra commented Feb 15, 2022 •
edited
Loading

Describe the bug
CONFIG_BT_SETTINGS_CCC_LAZY_LOADING is set.
bt_gatt_is_subscribed will always return false when called from bt_conn_cb->connected callback.

It is because bt_gatt_connected is called after all other registered callbacks were called. The change that introduced such behavior: 6bb75a5

GATT as the stack layer shall be ready when the application is notified about connection.

To Reproduce

  1. Use peer device to connect and subscribe for notifications/indications.
  2. Disconnect
  3. Connect again and check the output of bt_gatt_is_subscribed called from bt_conn_cb->connected callback context is false.

Expected behavior
bt_gatt_is_subscribed shall return true.

Impact
App does not know whether the device is subscribed unless it stores the value itself (it's not preferred solution since the value is stored in BT_SETTINGS).
@MariuszSkamra MariuszSkamra added bug The issue is a bug, or the PR is fixing a bug area: Bluetooth Host Bluetooth Host (excluding BR/EDR) labels Feb 15, 2022
@dkalowsk dkalowsk added area: Bluetooth priority: low Low impact/importance bug labels Feb 15, 2022
@alwa-nordic
Copy link
Collaborator

alwa-nordic commented Feb 24, 2022 •
edited
Loading

I agree. Rationale:

  • CONFIG_BT_SETTINGS_CCC_LAZY_LOADING should not change API behavior. "Lazy" means on-demand.
  • My intuition says bt_gatt_is_subscribed could be a function of bt_addr_le_t instead of bt_conn. The subscription exist across connections.

Possible fixes:

  • On-demand load CCC settings when the CCC database is queried.
  • Revert Bluetooth: gatt: ccc changed cb after connection cb #13473. The rationale behind that change was that an application wants to know where it is asked to send notifications to when it receives a BT_GATT_CCC _changed event. But the unfinished discussion in the that issue says to me that this is not a use case of that API. The _changed event asks the application pause or resume broadcasting notifications. I.e. It should pause or resume any periodic bt_gatt_notify(NULL, ...) as a battery optimization.

@MariuszSkamra MariuszSkamra mentioned this issue Mar 7, 2022
Closed
@carlescufi
Copy link
Member

@MariuszSkamra are you planning to send a PR to fix this?

@MariuszSkamra MariuszSkamra self-assigned this Mar 21, 2022
@MariuszSkamra
Copy link
Collaborator Author

@alwa-nordic I would go with the revert, since the _changed callback does not even carry information about the bt_conn nor bt_addr_le_t so that it does not matter whether it's called before or after connection cb.

MariuszSkamra added a commit to MariuszSkamra/zephyr that referenced this issue Mar 21, 2022
@MariuszSkamra
Revert "Bluetooth: gatt: ccc changed cb after connection cb"
a8e17be 
This reverts commit 6bb75a5.
This fixes GATT that was uninitalized when application received
bt_conn_cb->connected callback. As the result, the bt_gatt_is_subscribed
was not working as expected when called from bt_conn_cb->connected.

The _bt_gatt_ccc->cfg_changed callback does not carry information about
the device that subscribed for notifications but rather is says the
app when it should start/stop broadcasting notifications.

This leads to the conclusion that the bt_gatt_ccc->cfg_changed can be
called before bt_conn_cb->connected callback.

Fixes: zephyrproject-rtos#42829
Signed-off-by: Mariusz Skamra <[email protected]>
@MariuszSkamra MariuszSkamra mentioned this issue Mar 21, 2022
Merged
carlescufi pushed a commit that referenced this issue Mar 22, 2022
@MariuszSkamra @carlescufi
Revert "Bluetooth: gatt: ccc changed cb after connection cb"
d0111f7 
This reverts commit 6bb75a5.
This fixes GATT that was uninitalized when application received
bt_conn_cb->connected callback. As the result, the bt_gatt_is_subscribed
was not working as expected when called from bt_conn_cb->connected.

The _bt_gatt_ccc->cfg_changed callback does not carry information about
the device that subscribed for notifications but rather is says the
app when it should start/stop broadcasting notifications.

This leads to the conclusion that the bt_gatt_ccc->cfg_changed can be
called before bt_conn_cb->connected callback.

Fixes: #42829
Signed-off-by: Mariusz Skamra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MariuszSkamra MariuszSkamra

@alwa-nordic alwa-nordic

Labels
area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Revert "Bluetooth: gatt: ccc changed cb after connection cb" MariuszSkamra/zephyr
4 participants
@dkalowsk @MariuszSkamra @carlescufi @alwa-nordic