Bluetooth: Host: Ensure only connected peers affect CCC value

[HaavardRei]

Only consider connected peers when summing CCC values instead of
connected or bonded.

[HaavardRei]

Picks up the thread of two of the commits from #44136

[alwa-nordic]

I would have liked if I was kept as author or co-author of the commits, but no matter. The important thing is it gets in :)

The two commits should ideally come in two PRs, since they are two separate behavior changes.

[HaavardRei]

I would have liked if I was kept as author or co-author of the commits, but no matter. The important thing is it gets in :)

The two commits should ideally come in two PRs, since they are two separate behavior changes.

Added you as co-author, didn't mean to steal your glory :)
I think the two separate commits are sufficient, but can fix if it's important!

[Thalley]

I think this has been attempted before.

Isn't it dangerous to use values that are not defined as a bitfield in the BT core spec as a bitfield?

For example if the BT core spec ever introduces a new value 0x0003 then value here would be the same as BT_GATT_CCC_NOTIFY | BT_GATT_CCC_INDICATE.

This change feels less future-proof than the existing, and does not fix any issues. The value here would also be 0x0003 which is not defined in Zephyr, and if applications are testing for == BT_GATT_CCC_NOTIFY or == BT_GATT_CCC_INDICATE (which we have several cases of in Zephyr), then those checks will break.

The real issue is that the cfg_changed callback does not consider that individual clients have different values.

	/** @brief CCC attribute changed callback
	 *
	 *  @param attr   The attribute that's changed value
	 *  @param value  New value
	 */
	void (*cfg_changed)(const struct bt_gatt_attr *attr, uint16_t value);

should really be

	/** @brief CCC attribute changed callback
	 *
	 *  @param conn   The connection that updated it's CCC value
	 *  @param attr   The attribute that's changed value
	 *  @param value  New value
	 */
	void (*cfg_changed)(struct bt_conn *conn, const struct bt_gatt_attr *attr, uint16_t value);

I would suggest to leave the existing callback as is, and introduce the new callback (and possibly deprecate the old one).

It feels like this is trying to fix a fundamental issue by applying a band-aid, while also introducing a breaking change as the callback value of 0x0003 (BT_GATT_CCC_NOTIFY | BT_GATT_CCC_INDICATE) is undefined.

[alwa-nordic]

It is a bit field.

[HaavardRei]

I think this has been attempted before.

Isn't it dangerous to use values that are not defined as a bitfield in the BT core spec as a bitfield?

For example if the BT core spec ever introduces a new value 0x0003 then value here would be the same as BT_GATT_CCC_NOTIFY | BT_GATT_CCC_INDICATE.

But it is defined as a bitfield (Core 6.0):

[Thalley]

I stand corrected :)

Thanks.
Should we consider redefining the values in Zephyr from 0x0001 and 0x0002 to BIT(0) and BIT(2) then?

In Zephyr we have quite a few cases where we treat it not as a bitfield:

They should be updated to use & instead of == then, as otherwise if the GATT clients subscribe to both indication and notification, then those checks will now fail (granted that all the == BT_GATT_CCC_NOTIFY would fail anyways due to the current version using the max of the 2.

I would also prefer if we somewhere (either in the values or in the documentation for cfg_changed or somewhere else) make it clear that these are now bitfields in Zephyr, which they have not been so far.

Not sure if this change counts as a breaking stable API change, as they have been treated as non-bitfield for 9+ years in Zephyr. I'll leave that up to the maintainers of the host to decide.

[HaavardRei]

Thanks. Should we consider redefining the values in Zephyr from 0x0001 and 0x0002toBIT(0)andBIT(2)` then?

I agree! And I'll look into switching the checks to bitfields.
As for the breaking/nonbreaking, we should at least have a release note/changelog. Thoughts @alwa-nordic?

(Guess it's BIT(0) and BIT(1), but the point is the same)

[alwa-nordic]

Good examples. In that case, this really is a breaking change. To be on the safe side, we should go with a deprecation, would you agree?

[alwa-nordic]

We should remove the problematic change from this PR and evaluate how to move forward. I think we should either leave _bt_gatt_ccc.cfg_changed as it is, or deprecate it without replacement. And we should, as @Thalley points out, address the lack of an event for changes in subscription. The only way to do that without breaking API is to add a new callback.

[alwa-nordic]

Adding issues:

• Bluetooth: CCC values should be treated as bitfields #81004
• Bluetooth: Host: CCC write should check characteristic properties #81005

[HaavardRei]

We should remove the problematic change from this PR and evaluate how to move forward.

Meaning keeping only the commit that checks whether the peer is connected or not?

[alwa-nordic]

I added another issue:

• Bluetooth: CCC connected change event #81056