Bluetooth: host: Improve error reporting on gatt subscribe

[Yagoor]

• Return EALREADY when the subscription already exists but it is a different entry. This indicates to the application to not wait on subscribe callback. This is needed because the callback is called from the ccc_write which does not happen when another subscription exists

[jhedberg]

Since this is a change in public API behavior, shouldn't it be documented in the release notes or migration guide as well?

[Yagoor]

Since this is a change in public API behavior, shouldn't it be documented in the release notes or migration guide as well?

If the change is accepted, I can document it.

[jhedberg]

Since this is a change in public API behavior, shouldn't it be documented in the release notes or migration guide as well?

If the change is accepted, I can document it.

It seems reasonable to me. The migration guid would likely be the right place for it.

[Thalley]

I'm not 100% certain this is the right approach.

In this case we may return an error, while still adding the params to the list. That means this is a case where we return an error while still changing the state, which I'm not a fan off.

Did you consider the approach of calling the callback instead without the GATT write when it exists? (Although I'm not a fan of that approach either tbh).

[Yagoor]

I see your dilemma, it sounds wrong to change the state and still return an error.

I don't have a problem with that approach either.
The disadvantage in that case is that we will be indicating to the application that a ccc write happened. I am not sure if this can have any implication to some application.

[Thalley]

Another alternative is to still send the request regardless and remove the has_subscription completely.

We are actually returning a 0 for a write request, without actually writing anything. Maybe has_subscription should just be removed?

I assume it exists as an optimization, but if the remote device has for any reason removed the value itself (which wouldn't get notified IIRC), then we are actually preventing applications from sending another request (unless bt_gatt_unsubscribe is called).

[Yagoor]

Yeah, my assumption is that it is also a optimization.
I'll give it a try, but my assumption is that it will work without a problem.

[alwa-nordic]

This breaks stable API without a deprecation period. Can we discuss alternatives?

[alwa-nordic]

Alternative: Invoke the callback, emulating a successful response.

[jhedberg]

Discussed in the Bluetooth WG. Consensus seems (for now) to be toward the "safe approach" i.e. just do another write over the air, as proposed by @Thalley here: #82107 (comment)

[Thalley]

Changes look good to me, although I'm unaware if we have any tests that tests this?

[Yagoor]

Changes look good to me, although I'm unaware if we have any tests that tests this?

The subscription is indirectly tested in many bsims (specially the ones that involve audio)
So, the current behavior should not have changed if those tests are satisfied.

[alwa-nordic]

We must consider the case when the application subscribes to indications and then to notifications. I don't know what the original intention behind the API is for this case, but from just reading the code, I see the behavior has changed.

Before this change, it results in indications. After this change, it results in notifications.

[Yagoor]

We must consider the case when the application subscribes to indications and then to notifications. I don't know what the original intention behind the API is for this case, but from just reading the code, I see the behavior has changed.

Before this change, it results in indications. After this change, it results in notifications.

Good catch! If I propagate the value, it should be good right?

Application subscribes to indications and then to notifications.
Before this change, it results in indications.
After it would result in indications and notifications.

[alwa-nordic]

I found some relevant text in Core 3.G.3.3.3.3:

If both the Notification and Indication bits are set, then the server shall use the notification procedure (see Section 4.10) when an acknowledgment is not required by a higher-layer specification or shall use the indication procedure (see Section 4.11) when an acknowledgment is required. The server should not use both procedures to send the same characteristic value.

The server may reject a request to set both the Notification and Indication bits for the same characteristic.

I'm not sure how to interpret this. At first reading it seems like it may be important for higher-layers to have the ability to set both bits. But it also says the server may reject this, so it seems contradictory.

[Thalley]

The server may reject a request to set both the Notification and Indication bits for the same characteristic.

In nearly all GATT requests the server may reject the request for any reason, so this is a weird thing to write here. A GATT server may even reject a read request to the device name if it chooses.

In any case, if a GATT client user in Zephyr chooses to set both bits, then that's up to that user.

I think I'm missing the detail that you are discussing. Isn't the subscription value something that the client writes to the server, and then it is up to the server to decide what to do here? Are we, as the client, assuming some behavior?

[alwa-nordic]

I posted the quote from the spec because I think @Yagoor is asking whether he should update the code to set both bits when we have two subscriptions from the application, where one subscription is asking for notifications and the other is asking for indications. I think this is a subtly difficult question to answer. But we can put that discussion aside for now.

To maintain the previous behavior w.r.t. the value of the CCC, the value must be "indications" when the application subscribes to indications and notifications separately, and it must be "indications and notifications" if the application is requesting that in any single subscription.

In other words, we must bring back the loop removed by this PR, but instead of computing has_subscription, we compute ccc_value = MAX(all subscriptions, including the new one).

[Thalley]

In other words, we must bring back the loop removed by this PR, but instead of computing has_subscription, we compute ccc_value = MAX(all subscriptions, including the new one).

Ah, I understand it now.

So if application A has subscribed to indications and then application B later subscribes to notifications, then we (in current version) overwrite application A's value.

Not sure what your meant by MAX here, but shouldn't it be the union of all subscriptions?

So if A subscribes to indications and B later subscribes to notifications, then we send BT_GATT_CCC_NOTIFY | BT_GATT_CCC_INDICATE?

[alwa-nordic]

Not sure what your meant by MAX here

I mean the maximum given the ordering (notify & indicate) > indicate > notify > 0. It just happens that the CCC values for those are 3, 2, 1, 0, respectively, so the numerical maximum will give the same result. This is a little hidden and counts as smart-programming, I would not be against something more explicit.

shouldn't it be the union of all subscriptions?

So if A subscribes to indications and B later subscribes to notifications, then we send BT_GATT_CCC_NOTIFY | BT_GATT_CCC_INDICATE?

The old code does >=.

zephyr/subsys/bluetooth/host/gatt.c

Line 5471 in 2f583a8

tmp->value >= params->value) {

If we want to retain the old behavior, we must take the maximum. In your example, we send BT_GATT_CCC_INDICATE.

If someone wants to change the behavior, please say so, and please state your reasoning. I'm just assuming we want to keep old behavior until we intentionally decide to change it.

[Thalley]

If someone wants to change the behavior, please say so, and please state your reasoning. I'm just assuming we want to keep old behavior until we intentionally decide to change it.

Semantically I want to change it. It does not make sense to treat indications as a superset of notifications. If I subscribe to notifications and then my callback gives me the indication value, then I assume there's a bug.

Practically I don't think this will ever be an issue to be honest. Characteristics that have both indication and notifications are corner cases, and Zephyr applications where we want to subscribe to both, but individually, is equally much a corner case, so this ends up being a corner case squared :D

[Yagoor]

If someone wants to change the behavior, please say so, and please state your reasoning. I'm just assuming we want to keep old behavior until we intentionally decide to change it.

Semantically I want to change it. It does not make sense to treat indications as a superset of notifications. If I subscribe to notifications and then my callback gives me the indication value, then I assume there's a bug.

Practically I don't think this will ever be an issue to be honest. Characteristics that have both indication and notifications are corner cases, and Zephyr applications where we want to subscribe to both, but individually, is equally much a corner case, so this ends up being a corner case squared :D

Should I rollback to the simple change in which we just send the write even if another subscription exists?

[Thalley]

Should I rollback to the simple change in which we just send the write even if another subscription exists?

No I think the current solution is better.

if existing ccc == 0 (none) and params->value is 0 => Send 0
if existing ccc == 0 (none) and params->value is 1 => Send 1
if existing ccc == 0 (none) and params->value is 2 => Send 2
if existing ccc == 0 (none) and params->value is 3 => Send 3
if existing ccc == 1 (none) and params->value is 0 => Send 1
if existing ccc == 1 (none) and params->value is 1 => Send 1
if existing ccc == 1 (none) and params->value is 2 => Send 3
if existing ccc == 1 (none) and params->value is 3 => Send 3
if existing ccc == 2 (none) and params->value is 0 => Send 2
if existing ccc == 2 (none) and params->value is 1 => Send 3
if existing ccc == 2 (none) and params->value is 2 => Send 2
if existing ccc == 2 (none) and params->value is 3 => Send 3
if existing ccc == 3 (none) and params->value is 0 => Send 3
if existing ccc == 3 (none) and params->value is 1 => Send 3
if existing ccc == 3 (none) and params->value is 2 => Send 3
if existing ccc == 3 (none) and params->value is 3 => Send 3

It should however be documented that if a different user/application has already subscribed to the same handle, then the callback will contain the "sum" (or max or whatever) of all the users/applications. From a GATT client application standpoint, it shouldn't matter (unless perhaps for a few GATT services) whether a notification or indication was received.

[Thalley]

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

[Thalley]

@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.

[Thalley]

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