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.

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

Bluetooth: CCC connected change event #81056

Closed
alwa-nordic opened this issue Nov 7, 2024 · 10 comments
Closed

Bluetooth: CCC connected change event #81056

alwa-nordic opened this issue Nov 7, 2024 · 10 comments
Assignees
Labels
area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth Enhancement Changes/Updates/Additions to existing features

Comments

@alwa-nordic
Copy link
Collaborator

alwa-nordic commented Nov 7, 2024

As a developer of a GATT service making use of notifications, I may need to send notifications continuously when a client is connected and subscribed. The notifications may be tailored to the connections.

I would like a signal telling me when a client is connected and subscribed to notifications, and likewise for indications.

Alternatives:

  • Poll _bt_gatt_ccc.cfg periodically. There may be some happens-to-work solution to avoid the periodic scanning by listening for _bt_gatt_ccc.write, bt_conn_cb.connected, bt_conn_cb.disconnected, but AFAIK these are not ordered with respect to the change in _bt_gatt_ccc.cfg.

Non-alternatives:

  • _bt_gatt_ccc.cfg_changed: Signals a change to _bt_gatt_ccc.value, which is a summary.
  • _bt_gatt_ccc.cfg_write: Callback for the ATT write. Does not react to a bonded subscribed client connecting. Does not react to disconnection.

Possibly relevant discussion: #79822

@alwa-nordic alwa-nordic added Enhancement Changes/Updates/Additions to existing features area: Bluetooth area: Bluetooth Host Bluetooth Host (excluding BR/EDR) labels Nov 7, 2024
@Thalley
Copy link
Collaborator

Thalley commented Nov 7, 2024

Not sure I understand the use case. Can you elaborate or give an example?

@alwa-nordic
Copy link
Collaborator Author

I was trying to motivate something like the change you propose in #80988 (comment). Let's see if we can characterize this problem together, and come up with a concrete use case. Do you have one in mind? Otherwise I will try to invent one.

@Thalley
Copy link
Collaborator

Thalley commented Nov 7, 2024

I see :)

That comment was an alternative to getting the max of Notify and Indicate (or a bitfield of all the clients' values). The callback seems to be based on the assumption that these will all be the same, hence why the conn was omitted in the first place. So my comment was more related to the change, rather than to an issue (which is also why I mentioned that the PR was not solving any issues).

The issue itself is already solved by bt_gatt_is_subscribed which gives applications exactly what they need to handle notifications and indications on a per-client basis.

However if I should try to motivate the change, it would be something like:
Since CCC values exist in the GATT database on a per-client basis, and not a global value, then changes to the CCC value should be signaled to the user containing the information about which client's value changed. Today we only get the combined value of all connected clients.

@alwa-nordic
Copy link
Collaborator Author

Can we say that we need an event for when bt_gatt_is_subscribed has a new value?

@Thalley
Copy link
Collaborator

Thalley commented Nov 7, 2024

Can we say that we need an event for when bt_gatt_is_subscribed has a new value?

Maybe, but not sure what value that adds.
If an application needs to know if it can/should send an notification, indication or nothing, then it will need to apply a check. It can either do this by using bt_gatt_is_subscribed (like our HAS implementation does), or it can store some information locally to perform this check if we add a new callback.

But I'm not sure it adds any value to the application to get more information via callbacks and store information itself, rather than just calling bt_gatt_is_subscribed. An application can combine bt_conn_foreach and bt_gatt_is_subscribed to get any information it needs for any connection. bt_gatt_is_subscribed has the advantage of not needing to store duplicate information that the stack already provides.

Do we have any examples of applications using the cfg_changed values in a way that cannot easily be replaced by bt_gatt_is_subscribed?

@alwa-nordic
Copy link
Collaborator Author

alwa-nordic commented Nov 8, 2024

I'm envisioning an application that needs to send a single "hello" indication to every peer that connects, when they subscribe or are bonded and already subscribed. It has to work with concurrent connections. How would I implement this?

Is this even a valid use-case? Arguably it's simpler that the client reads the "hello" instead.

@Thalley
Copy link
Collaborator

Thalley commented Nov 8, 2024

I'm envisioning an application that needs to send a single "hello" indication to every peer that connects, when they subscribe or are bonded and already subscribed. It has to work with concurrent connections. How would I implement this?

In that case, wouldn't there be a call guarded by bt_gatt_is_subscribed in the connected callback, rather than the cfg_changed? Or if it requires encryption, in the security_changed callback?
I wasn't aware, but it looks like cfg_changed is called on connect, which is questionable. What does "changed" here mean, because if a device reconnects, the CCC value hasn't really changed. It only changes for write requests to the characteristic, but I guess we have decided that "restore" also means "changed", if you can even call it a "restore" when applying the CCC value for bonded devices.

Is this even a valid use-case? Arguably it's simpler that the client reads the "hello" instead.

Arguably it is, but it's a use case already supported by bt_gatt_is_subscribed.

The way that most GATT services work is that

  1. Some characteristic value changed
  2. Notify/indicate subscribed peers

For that use case, the cfg_changed callback isn't useful, and you can also see that most of the implemented of that callback in Zephyr are just for logging purposes.

For the case where a service has to notify on (bonded) device reconnects, it is as easy to use the connected/security changed callbacks where you also get the conn pointer.

I think the main reason why I think the conn pointer should be added to the cfg_changed callback is simply because the value exist per client, and the change is done per client. Getting the max or a combined bitfield of the value simply isn't useful in most cases, but it may be useful (if not just for logging purposes) to know which conn modified its CCC value.

@alwa-nordic
Copy link
Collaborator Author

I wasn't aware, but it looks like cfg_changed is called on connect, which is questionable. What does "changed" here mean, because if a device reconnects, the CCC value hasn't really changed. It only changes for write requests to the characteristic, but I guess we have decided that "restore" also means "changed", if you can even call it a "restore" when applying the CCC value for bonded devices.

This behavior makes sense, if we define that _bt_gatt_ccc.cfg_changed signals a change to _bt_gatt_ccc.value. _bt_gatt_ccc.value reflects only the values of connected peers, per its documentation.

@alwa-nordic
Copy link
Collaborator Author

alwa-nordic commented Nov 11, 2024

I'm envisioning an application that needs to send a single "hello" indication to every peer that connects, when they subscribe or are bonded and already subscribed. It has to work with concurrent connections. How would I implement this?

In that case, wouldn't there be a call guarded by bt_gatt_is_subscribed in the connected callback, rather than the cfg_changed? Or if it requires encryption, in the security_changed callback?

We load CCC values from flash as a reaction to the connected event, so I'm was not sure value bt_gatt_is_subscribed reads is reliable in all connected callbacks. But, you are right, it's reasonable to expect this to work. I think it works out in the application's connected callbacks because our CCC implementation happens to get the callback first and blocks until it's loaded. It is reasonable that a read of the CCC should block until the value is available. BTW, this means bt_gatt_is_subscribed is potentially blocking.

I think we can close this issue, at least until someone asks for it.

I also noticed that bt_gatt_is_subscribed returns false if conn is disconnected. I think we should be document this so that bt_gatt_is_subscribed is "is connected and subscribed".

@Thalley
Copy link
Collaborator

Thalley commented Nov 11, 2024

I think we can close this issue until, at least until someone asks for it.

Agreed. I don't think we have a use case for it that isn't covered by bt_gatt_is_subscribed. I'd still like to see the conn pointer in the cfg_changed callback though.

I also noticed that bt_gatt_is_subscribed returns false if conn is disconnected. I think we should be document this so that bt_gatt_is_subscribed is "is connected and subscribed".

That is arguably explicit given that it takes a conn pointer, but adding more explicit documentation is fine :)

@Thalley Thalley closed this as not planned Won't fix, can't repro, duplicate, stale Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

No branches or pull requests

3 participants