Bluetooth: Improve ccc.cfg_changed #61170

alwa-nordic · 2023-08-04T13:51:34Z

In this PR we improve _bt_gatt_ccc.value to use a more natural definition: bitwise-or instead of max. We ensure that only currently connected peers affect this value, so that the definitions stays independent of the CCC lazy loading kconfig. We document the use case for _bt_gatt_ccc.value and clarify the documentation of cfg_changed.

Fixes: #61260
Resolves: #55373

jori-nordic · 2023-08-08T08:58:30Z

could you add/extend a test for this? ie subscribing to indications, checking the CCC value on the server and then doing the same for notifications

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]>

Thalley · 2023-08-14T11:55:02Z

tests/bsim/bluetooth/host/gatt/ccc_value/client/main.c

+			break;
+		}
+
+		__ASSERT_NO_MSG(!err);


Since ASSERTS can be disabled via Kconfig, wouldn't it be better to use regular ifs?

Thalley · 2023-08-14T11:55:30Z

tests/bsim/bluetooth/host/gatt/ccc_value/client/main.c

+	__ASSERT_NO_MSG(!err);
+
+	while (bt_eatt_count(conn) == 0) {
+		LOG_DBG("E..");


Is this DBG intended?

Thalley · 2023-08-14T11:57:26Z

tests/bsim/bluetooth/host/gatt/ccc_value/testlib/adv.c

I think it is a good idea with some common testlib functionality. Is this something that we can perhaps apply to multiple tests, rather than keep in this single test case, or do the tests differ too much to be able to use a common testlib?

Thalley · 2023-08-14T11:58:54Z

tests/bsim/bluetooth/host/gatt/ccc_value/testlib/adv.c

+	}
+	k_condvar_signal(&g_ctx->done);


Suggested change

}

k_condvar_signal(&g_ctx->done);

}

k_condvar_signal(&g_ctx->done);

Thalley · 2023-08-14T11:59:32Z

tests/bsim/bluetooth/host/gatt/ccc_value/testlib/adv.c

+
+int bt_testlib_adv_conn(struct bt_conn **conn, int id, uint32_t adv_options)
+{
+	int api_err;


Is it necessary to prefix this with api?

Suggested change

int api_err;

int err;

Thalley · 2023-08-14T12:02:22Z

tests/bsim/bluetooth/host/gatt/ccc_value/testlib/att_read.c

+	api_err = bt_gatt_read(ctx->conn, &ctx->params);
+
+	if (!api_err) {


Suggested change

api_err = bt_gatt_read(ctx->conn, &ctx->params);

if (!api_err) {

api_err = bt_gatt_read(ctx->conn, &ctx->params);

if (!api_err) {

Thalley · 2023-08-14T12:04:32Z

tests/bsim/bluetooth/host/gatt/ccc_value/testlib/att_read.c

+		.params = {.by_uuid = {.uuid = type,
+				       .start_handle = start_handle,
+				       .end_handle = end_handle},
+			   IF_ENABLED(CONFIG_BT_EATT, (.chan_opt = bearer))},


This looks like it could be indented differently to be more readable. If using clang-format, try adding , after the last element

Thalley · 2023-08-14T12:04:41Z

tests/bsim/bluetooth/host/gatt/ccc_value/testlib/att_read.c

+		.params = {.handle_count = 1,
+			   .single = {.handle = handle, .offset = offset},
+			   IF_ENABLED(CONFIG_BT_EATT, (.chan_opt = bearer))},


Thalley · 2023-08-14T12:05:27Z

tests/bsim/bluetooth/host/gatt/ccc_value/testlib/connect.c

+	api_err = bt_conn_le_create(peer, BT_CONN_LE_CREATE_CONN, BT_LE_CONN_PARAM_DEFAULT, conn);
+
+	if (!api_err) {


Suggested change

api_err = bt_conn_le_create(peer, BT_CONN_LE_CREATE_CONN, BT_LE_CONN_PARAM_DEFAULT, conn);

if (!api_err) {

api_err = bt_conn_le_create(peer, BT_CONN_LE_CREATE_CONN, BT_LE_CONN_PARAM_DEFAULT, conn);

if (!api_err) {

Thalley · 2023-08-14T12:07:36Z

tests/bsim/bluetooth/host/gatt/ccc_value/testlib/scan.c

+{
+	int api_err;
+	struct bt_scan_find_name_closure ctx = {
+		.wanted_name = name,


The simple pointer assignment assignment here indicates that name shall have a long life time (at least until the device is found), but that isn't really that obvious from the header file. Suggest to either do a copy, or to somehow indicate in the header file that the name argument cannot just be stack allocated.

github-actions · 2023-10-14T00:29:33Z

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 assigned jori-nordic Aug 4, 2023

alwa-nordic marked this pull request as ready for review August 4, 2023 14:02

alwa-nordic requested review from Vudentz, jhedberg, nashif and sjanc as code owners August 4, 2023 14:02

zephyrbot added Release Notes To be mentioned in the release notes area: Bluetooth area: Bluetooth Host Bluetooth Host (excluding BR/EDR) labels Aug 4, 2023

zephyrbot requested review from Thalley, asbjornsabo, fabiobaltieri, hermabe, jori-nordic, kruithofa and theob-pro August 4, 2023 14:02

alwa-nordic added 4 commits August 8, 2023 15:23

Bluetooth: Host: Document _bt_gatt_ccc.cfg_changed

df3dc18

This documents the behavior and a suggested use case. Signed-off-by: Aleksander Wasaznik <[email protected]>

wip tests

dceb291

alwa-nordic force-pushed the bitwise-or-ccc-value branch from 5fc8c4f to dceb291 Compare August 8, 2023 13:39

alwa-nordic requested a review from wopu-ot as a code owner August 8, 2023 13:39

alwa-nordic marked this pull request as draft August 8, 2023 13:39

Thalley requested changes Aug 14, 2023

View reviewed changes

alwa-nordic mentioned this pull request Oct 12, 2023

Bluetooth: Host: Fix GATT Long Read for EATT #62782

Merged

github-actions bot added the Stale label Oct 14, 2023

github-actions bot closed this Oct 29, 2023

		api_err = bt_gatt_read(ctx->conn, &ctx->params);

		if (!api_err) {

		api_err = bt_conn_le_create(peer, BT_CONN_LE_CREATE_CONN, BT_LE_CONN_PARAM_DEFAULT, conn);

		if (!api_err) {

Bluetooth: Improve ccc.cfg_changed #61170

Bluetooth: Improve ccc.cfg_changed #61170

Uh oh!

Conversation

alwa-nordic commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jori-nordic commented Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alwa-nordic commented Aug 4, 2023 •

edited

Loading

jori-nordic commented Aug 8, 2023 •

edited

Loading