GATT ccc_changed should be called on value change of a connection, not a combined value of all peers.

[asteriskSF]

I am porting a product's code based from Nordic NRF SDK to zephyr. I noticed something odd in gatt.c function gatt_ccc_changed.

This particular device has a Notification-Only GATT characteristic. The action of enabling notification in the CCC activates the asynchronous flow of data from this characteristic. In some cases this device may operate with multiple different peers. In some future use cases there may also be simultaneous connection with multiple different peers. Enabling and disabling notifications to activate data generation can act as a form of flow control or power management.

When a bluetooth device supporting multiple connections has a GATT server characteristic with a CCC, the CCC should function independently for each peer/client. However the code currently performs the ccc_changed callback in gatt_ccc_changed only when the greatest value of all clients has changed. I don't believe this makes sense.

Here is the gatt.c code
`static void gatt_ccc_changed(const struct bt_gatt_attr *attr,
struct _bt_gatt_ccc *ccc)
{
int i;
uint16_t value = 0x0000;

for (i = 0; i < ARRAY_SIZE(ccc->cfg); i++) {
	if (ccc->cfg[i].value > value) {
		value = ccc->cfg[i].value;
	}
}

LOG_DBG("ccc %p value 0x%04x", ccc, value);

if (value != ccc->value) {
	ccc->value = value;
	if (ccc->cfg_changed) {
		ccc->cfg_changed(attr, value);
	}
}`

First, even if a combined ccc value for all connections was desired, shouldn't it be based only on currently connected peers? If a previously connected peer disconnected while the ccc was enabling notifications and subsequently a new peer connected and enabled notifications, shouldn't the ccc_changed callback be called?

In addition, upon a connection where the saved ccc value indicates notifications are enabled, the GATT may need to re-start code that generates the notifications. I'm not seeing an existing API to support checking the ccc value specific to a peer, for example a peer which has just connected. Similarly upon a disconnect of a peer, how can my code stop the asynchronous generation of data for that peer?

@theob-pro you worked on this code in the past.. do you have an opinion?
I would also like to mention aleksander.wasaznik@nordicsemi.no and jonathan.rico@nordicsemi.no who touched related code.

[asteriskSF]

One correction:

It appears I can check the status of the ccc upon connect/disconnect with

      bool bt_gatt_is_subscribed(struct bt_conn *conn,
   		   const struct bt_gatt_attr *attr, uint16_t ccc_type)

However I'm still not able to get a callback if a new peer connects and enable notifications after a previous peer enabled notifications.

[alwa-nordic]

It's especially bad because whether or not disconnected peers are considered depends on the value of CONFIG_BT_SETTINGS_CCC_LAZY_LOADING, a config that sounds like it should not affect behavior, only performance.

I have an old PR on this, that never went in:

• Bluetooth: Host: Bitwise-or CCC values #44136

These as well were an attempt at formalizing the documentation:

• Bluetooth: Host: Document _bt_gatt_ccc.cfg_changed #44030
• Bluetooth: Improve ccc.cfg_changed  #61170

The problem here lies in making changes to an old interface. There may be applications that rely on the old behavior. It would be better to start from scratch on a new CCC implementation, with its own API. My team is considering doing this. In principle it is possible for the application to define its own CCC implementation and use it with the Zephyr Host. The CCC attribute is not that special. The new implementation would not be part of the core Host.

[asteriskSF]

The concept of combining the cccd from multiple peers into a single value dates back to 2016 and was submitted from intel. I have never seen this type of abstraction in a bluetooth implementation before. I've been implementing bluetooth devices since 2013 and I have created devices with multiple active connections as well as devices that were gatt clients as well as servers I also suspect that it diverges from the original intent of the standard.

For my own development I'm going to proceed with patching gatt.c/gatt.h to add an additional callback which includes the connection parameter in the callback and to callback on every change in the cccd value related to the specific peer in the connection. I personally don't have any use case where cccd values from unrelated peers should be merged into a single value. Even if I did, that would be an exception which I would handle with custom code, not the other way around.

I also think the merged cccd value should be eliminated altogether, and it should always be retrieved based on an active connection. I however I am hesitant to put that into a PR since it potentially breaks other developers who have come to rely on this feature.

[alwa-nordic]

Relevant issue: #81118

[alwa-nordic]

Possibly relevant issue: #81056