Bluetooth: Host: Bitwise-or CCC values #44136
Conversation
| * This event will not trigger if only the @c cfg field changes but the | ||
| * @c value field remains the same. Combine the use of this event and | ||
| * @c cfg_write to catch all changes to the @c cfg field. |
There was a problem hiding this comment.
I would like to change this to trigger on any change to the cfg field. RFC
There was a problem hiding this comment.
On the other hand, it would be awkward because only value is passed as a parameter here.
There was a problem hiding this comment.
I see your point.
Technically a device keeps a CCCD for each client which is of course abstracted away by the API, so it appears as just a single value.
It does sound a bit odd to call the callback every time there is a change, since the value may not actually change here, and the application would thus not really be able to use that information for anything.
I think in the case where an application needs/wants more specific knowledge or handling of CCCD writes, it would just just the BT_GATT_CCC_MANAGED to do that.
There was a problem hiding this comment.
I think the bit-wise approach will give a better API to be honest. I also assume that for 99% of use cases this will not affect existing applications - Besides HAS I am not away of any services that even allow both indications and notifications on the same characteristic.
I think it will be worth going ahead with this change, and of course also add an entry in the release notes.
alwa-nordic
force-pushed
the
bitwise-or-ccc-value
branch
from
3e446a6 to
d081dae
Compare
github-actions
bot
added
area: API
alwa-nordic
force-pushed
the
bitwise-or-ccc-value
branch
from
d081dae to
6fc3e8a
Compare
The doc on `_bt_gatt_ccc.value` specifies that only connected peers contribute to that value. But before this change, it was all computed from all entries in `_bt_gatt_ccc.cfg`, which include bonded but not connected peers when `CONFIG_BT_SETTINGS_CCC_LAZY_LOADING` is set. Signed-off-by: Aleksander Wasaznik <[email protected]>
Summarize the value of the CCC using bitwise-or instead of max. This is more natural and affords distinguishing between when only indications are enabled and when both notifications and indications are enabled. It is not expected that this change will affect any applications. It will not affect attributes that allow only one of notifications and indications. It will also not affect applications that only compare this value to zero. But it simplifies the interface of `_bt_gatt_ccc.value` and `_bt_gatt_ccc.cfg_changed`. Signed-off-by: Aleksander Wasaznik <[email protected]>
This documents the behavior and a suggested use case. Signed-off-by: Aleksander Wasaznik <[email protected]>
alwa-nordic
force-pushed
the
bitwise-or-ccc-value
branch
from
6fc3e8a to
8fa7c03
Compare
| value |= ccc->cfg[i].value; | ||
|
|
||
| bt_conn_unref(conn); | ||
| conn = NULL; |
There was a problem hiding this comment.
This setting to NULL doesn't really give you anything, as the function returns immediately thereafter and the variable goes out of scope
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
|
@alwa-nordic please follow-up and rebase |
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
This PR fixes several issues:
connected or bonded.
cfg_changedevent.