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

Bluetooth: host: Improve error reporting on gatt subscribe #82107

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+24 −16
Open

Bluetooth: host: Improve error reporting on gatt subscribe #82107

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8854543
Bluetooth: host: Improve gatt subscribe
Nov 29, 2024
7d2c6bd
Update subsys/bluetooth/host/gatt.c
Yagoor Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/releases/migration-guide-4.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,10 @@ Bluetooth Audio
Bluetooth Host
==============

* :c:func:`bt_gatt_subscribe` behavior has changed to if a different user/application has already
subscribed to the same handle, then the callback will contain the values aggregated of all the
users/applications.
Comment on lines +407 to +409
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change also needs to added to the documentation of bt_gatt_subscribe if not already

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yagoor please be careful about resolving review comments yourself: https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#workflow-suggestions-that-help-reviewers

Unless they applied the reviewer’s recommendation exactly, authors must not resolve and hide comments, they must let the initial reviewer do it.


Automatic advertiser resumption is deprecated
---------------------------------------------

Expand Down
5 changes: 5 additions & 0 deletions include/zephyr/bluetooth/gatt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2076,6 +2076,9 @@ struct bt_gatt_subscribe_params {
* notified value. One may then decide whether to unsubscribe directly from
* this callback. Notification callback with NULL data will not be called if
* subscription was removed by this method.
* If a different user/application has already subscribed to the same handle,
* then the callback will contain the values aggregated of all the
* users/applications.
Comment on lines +2079 to +2081
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to have an example of this?

There are 2 callbacks involved here.
One is the bt_gatt_subscribe_params.notify which contains the data as well as the params when we receive a notification or indication. The callback itself does not tell the application if it received an indication or notification (which is probably should have).

The other is the bt_gatt_subscribe_params.subscribe which is basically the write response. This just gives back the params.

So none of the callbacks actually contain information about whether it was a notification or indication.

But the documentation you've added here is wrong since the callback does not contain information about the type. What can happen is that App A subscribes to notifications and App B subscribes to indications, and looking at att_notify, att_indicate and bt_gatt_notification, these are just handled in the same way. So even if App A only subscribes to notifications, it may receive indications, or vice versa.

This is what the documentation should say, unless we want to take it a step further and change the behavior so that App A only gets notifications and App B only gets indications (so that the callback behavior matches the params->value). The change should be fairly trivial.

@jhedberg @alwa-nordic what are you thoughts about this?

And sorry that this is becoming a bigger thing that you probably intended @Yagoor :D

*
* The Response comes in callback @p params->subscribe. The callback is run from
* the context specified by 'config BT_RECV_CONTEXT'.
Expand All @@ -2095,6 +2098,8 @@ struct bt_gatt_subscribe_params {
* @retval 0 Successfully queued request. Will call @p params->write on
* resolution.
*
* @retval -EINVAL if the @p params->value is out of range
*
* @retval -ENOMEM ATT request queue is full and blocking would cause deadlock.
* Allow a pending request to resolve before retrying, or call this function
* outside the BT RX thread to get blocking behavior. Queue size is controlled
Expand Down
31 changes: 15 additions & 16 deletions subsys/bluetooth/host/gatt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5429,7 +5429,7 @@ int bt_gatt_subscribe(struct bt_conn *conn,
{
struct gatt_sub *sub;
struct bt_gatt_subscribe_params *tmp;
bool has_subscription = false;
int err;

__ASSERT(conn, "invalid parameters\n");
__ASSERT(params && params->notify, "invalid parameters\n");
Expand All @@ -5446,6 +5446,11 @@ int bt_gatt_subscribe(struct bt_conn *conn,
return -ENOTCONN;
}

if (!IN_RANGE(params->value, BT_GATT_CCC_NOTIFY,
BT_GATT_CCC_NOTIFY | BT_GATT_CCC_INDICATE)) {
Yagoor marked this conversation as resolved.
Show resolved Hide resolved
return -EINVAL;
}

sub = gatt_sub_add(conn);
if (!sub) {
return -ENOMEM;
Expand All @@ -5467,26 +5472,20 @@ int bt_gatt_subscribe(struct bt_conn *conn,
}

/* Check if another subscription exists */
if (tmp->value_handle == params->value_handle &&
tmp->value >= params->value) {
has_subscription = true;
if (tmp->value_handle == params->value_handle) {
params->value |= tmp->value;
Yagoor marked this conversation as resolved.
Show resolved Hide resolved
}
}

/* Skip write if already subscribed */
if (!has_subscription) {
int err;

#if defined(CONFIG_BT_GATT_AUTO_DISCOVER_CCC)
if (params->ccc_handle == BT_GATT_AUTO_DISCOVER_CCC_HANDLE) {
return gatt_ccc_discover(conn, params);
}
if (params->ccc_handle == BT_GATT_AUTO_DISCOVER_CCC_HANDLE) {
return gatt_ccc_discover(conn, params);
}
#endif
err = gatt_write_ccc(conn, params, gatt_write_ccc_rsp);
if (err) {
gatt_sub_remove(conn, sub, NULL, NULL);
return err;
}
err = gatt_write_ccc(conn, params, gatt_write_ccc_rsp);
if (err) {
gatt_sub_remove(conn, sub, NULL, NULL);
return err;
}

/*
Expand Down
Loading