LE Audio: Add support for dynamically setting the CSIP SIRK #67548

Thalley · 2024-01-12T13:19:43Z

Add support for dynamically setting and notifying the SIRK in CSIP.

Fixes #52516

kruithofa

Some nitpicks regarding the output for shell commands, which may be fixed in a later PR

subsys/bluetooth/audio/shell/cap_acceptor.c

fredrikdanebjer · 2024-02-02T13:54:39Z

tests/bsim/bluetooth/audio/src/csip_set_coordinator_test.c

@@ -110,6 +112,13 @@ static void csip_lock_changed_cb(struct bt_csip_set_coordinator_csis_inst *inst,
 	printk("Inst %p %s\n", inst, locked ? "locked" : "released");
 }

+static void csip_sirk_changed_cb(struct bt_csip_set_coordinator_csis_inst *inst)


Isn´t this only checking that we receive sirk_changed once? I.e. we are setting the flag on the first notification?
Correct me if I'm wrong - but if I interpret this right, shouldnt we wait until all connections have sent the notify?

Isn´t this only checking that we receive sirk_changed once? I.e. we are setting the flag on the first notification?

Correct me if I'm wrong - but if I interpret this right, shouldnt we wait until all connections have sent the notify?

Arguably yes. It doesn't really change the behavior of the test as they are running the same software: If one works, they should all work. If you prefer I can modify this to expect N number of notifications.

I would prefer this yes, even if you certainly are technically correct, I think that this would make the test look more logical and easier to interpret as to what should happen.

@fredrikdanebjer Mind if we do that in a follow up? I'd rather try to get this into the release now, rather than postponing it due to a test update :)

@Thalley that is fair enough

pin-zephyr

a couple of minor doc issues and an open question.

Rest looks good and nothing blocking 👍

pin-zephyr · 2024-02-02T21:30:20Z

doc/connectivity/bluetooth/api/shell/cap.rst

+
+This command can modify the currently used SIRK. To get the new RSI to advertise on air,
+:code:`bt adv-data`` or :code:`bt advertise` must be called again to set the new advertising data.
+If :code:`CONFIG_BT_CSIP_SET_MEMBER_NOTIFIABLE`` is enabled, this will also notify connected


a ` too much.

Suggested change

If :code:`CONFIG_BT_CSIP_SET_MEMBER_NOTIFIABLE`` is enabled, this will also notify connected

If :code:`CONFIG_BT_CSIP_SET_MEMBER_NOTIFIABLE` is enabled, this will also notify connected

@Thalley you have not addressed comments from @pin-zephyr ?

pin-zephyr · 2024-02-02T21:32:47Z

doc/connectivity/bluetooth/api/shell/csip.rst

+
+This command can modify the currently used SIRK. To get the new RSI to advertise on air,
+:code:`bt adv-data`` or :code:`bt advertise` must be called again to set the new advertising data.
+If :code:`CONFIG_BT_CSIP_SET_MEMBER_NOTIFIABLE`` is enabled, this will also notify connected


one more ` too much.

pin-zephyr · 2024-02-02T21:49:01Z

subsys/bluetooth/audio/csip_set_coordinator.c

+static void sirk_changed(struct bt_csip_set_coordinator_csis_inst *inst)
+{
+	struct bt_csip_set_coordinator_cb *listener;
+
+	SYS_SLIST_FOR_EACH_CONTAINER(&csip_set_coordinator_cbs, listener, _node) {
+		if (listener->sirk_changed) {
+			listener->sirk_changed(inst);


can inst be NULL ?

and if it can, should we check for that before looping each container ?

It cannot :) And even if it can, then it would provide NULL to the callback (which isn't documented, but that's because it isn't intentionally possible).

cvinayak · 2024-02-03T06:45:45Z

subsys/bluetooth/audio/csip_set_member.c

+	}
+
+	CHECKIF(sirk == NULL) {
+		LOG_DBG("NULL svc_inst");


Suggested change

LOG_DBG("NULL svc_inst");

LOG_DBG("NULL sirk");

cvinayak · 2024-02-03T06:46:03Z

subsys/bluetooth/audio/csip_set_member.c

+	}
+
+	CHECKIF(sirk == NULL) {
+		LOG_DBG("NULL svc_inst");


Suggested change

LOG_DBG("NULL svc_inst");

LOG_DBG("NULL sirk");

cvinayak · 2024-02-03T06:48:16Z

include/zephyr/bluetooth/audio/csip.h

+ * @brief Callback when the SIRK value of a set of a connected device changes.
+ *
+ * @param inst    The Coordinated Set Identification Service instance that was changed.
+ *                The new SIKR can be accessed via the @p inst.info.


Suggested change

* The new SIKR can be accessed via the @p inst.info.

* The new SIRK can be accessed via the @p inst.info.

pin-zephyr

LGTM

cvinayak · 2024-02-05T11:34:46Z

doc/connectivity/bluetooth/api/shell/cap.rst

+
+This command can modify the currently used SIRK. To get the new RSI to advertise on air,
+:code:`bt adv-data`` or :code:`bt advertise` must be called again to set the new advertising data.
+If :code:`CONFIG_BT_CSIP_SET_MEMBER_NOTIFIABLE`` is enabled, this will also notify connected


@Thalley you have not addressed comments from @pin-zephyr ?

cvinayak · 2024-02-05T11:35:59Z

tests/bsim/bluetooth/audio/src/csip_set_member_test.c

+	printk("Getting new SIRK\n");
+	err = bt_csip_set_member_get_sirk(svc_inst, tmp_sirk);
+	if (err != 0) {
+		FAIL("Failed to set SIRK: %d\n", err);


Not going to stress on typos in test code... (not reviewing further...)

Suggested change

FAIL("Failed to set SIRK: %d\n", err);

FAIL("Failed to get SIRK: %d\n", err);

Add support for dynamically change the SIRK in a CSIS. Signed-off-by: Emil Gydesen <[email protected]>

The print_sirk function that simply logs the SIRK has been removed, as the SIRK is now directly accessible to the application via the get_sirk function. Signed-off-by: Emil Gydesen <[email protected]>

The CSIP commands had a typo in the prefix of the shell functions. Signed-off-by: Emil Gydesen <[email protected]>

pin-zephyr

LGTM

aescolar · 2024-02-06T17:45:04Z

@Thalley Given that we are past RC1 for 3.6, should this target 3.7? (assuming so I change the milestone, but please correct it if you disagree)

Thalley · 2024-02-06T17:48:53Z

@Thalley Given that we are past RC1 for 3.6, should this target 3.7? (assuming so I change the milestone, but please correct it if you disagree)

3.7 is fine :)

Thalley force-pushed the csis_sirk_notifiable branch 3 times, most recently from f89292d to 1bcbc3d Compare January 15, 2024 10:13

Thalley force-pushed the csis_sirk_notifiable branch from 1bcbc3d to 193c8da Compare February 1, 2024 08:22

Thalley marked this pull request as ready for review February 1, 2024 09:33

Thalley requested a review from cvinayak February 1, 2024 09:34

zephyrbot added area: Bluetooth Audio area: Bluetooth labels Feb 1, 2024

zephyrbot requested review from asbjornsabo, Casper-Bonde-Bose, fredrikdanebjer, hermabe, jhedberg, kruithofa, larsgk, MariuszSkamra, sjanc and Vudentz February 1, 2024 09:34

zephyrbot assigned Thalley Feb 1, 2024

kruithofa previously approved these changes Feb 2, 2024

View reviewed changes

subsys/bluetooth/audio/shell/cap_acceptor.c Outdated Show resolved Hide resolved

subsys/bluetooth/audio/shell/cap_acceptor.c Show resolved Hide resolved

Thalley dismissed kruithofa’s stale review via 5a8f97c February 2, 2024 12:23

Thalley force-pushed the csis_sirk_notifiable branch from 193c8da to 5a8f97c Compare February 2, 2024 12:23

Thalley requested a review from kruithofa February 2, 2024 12:23

kruithofa previously approved these changes Feb 2, 2024

View reviewed changes

fredrikdanebjer reviewed Feb 2, 2024

View reviewed changes

Thalley requested a review from fredrikdanebjer February 2, 2024 14:52

fredrikdanebjer previously approved these changes Feb 2, 2024

View reviewed changes

Thalley dismissed stale reviews from fredrikdanebjer and kruithofa via 2c196eb February 2, 2024 16:10

Thalley force-pushed the csis_sirk_notifiable branch from 5a8f97c to 2c196eb Compare February 2, 2024 16:10

pin-zephyr previously approved these changes Feb 2, 2024

View reviewed changes

cvinayak added this to the v3.6.0 milestone Feb 3, 2024

cvinayak reviewed Feb 3, 2024

View reviewed changes

Thalley dismissed stale reviews from pin-zephyr and fredrikdanebjer via 83b2990 February 5, 2024 10:08

Thalley force-pushed the csis_sirk_notifiable branch from 9106c64 to 83b2990 Compare February 5, 2024 10:08

Thalley requested review from pin-zephyr, fredrikdanebjer and cvinayak February 5, 2024 10:08

kruithofa previously approved these changes Feb 5, 2024

View reviewed changes

pin-zephyr previously approved these changes Feb 5, 2024

View reviewed changes

cvinayak reviewed Feb 5, 2024

View reviewed changes

Thalley added 3 commits February 5, 2024 13:06

Bluetooth: CSIP: Add support for dynamic SIRKs

a85d1c3

Add support for dynamically change the SIRK in a CSIS. Signed-off-by: Emil Gydesen <[email protected]>

Bluetooth: CSIP: remove print_sirk

4586244

The print_sirk function that simply logs the SIRK has been removed, as the SIRK is now directly accessible to the application via the get_sirk function. Signed-off-by: Emil Gydesen <[email protected]>

Bluetooth: CSIP: Fix typo from cm_csip to cmd_csip

6df4c94

The CSIP commands had a typo in the prefix of the shell functions. Signed-off-by: Emil Gydesen <[email protected]>

Thalley dismissed stale reviews from pin-zephyr and kruithofa via 6df4c94 February 5, 2024 12:07

Thalley force-pushed the csis_sirk_notifiable branch from 83b2990 to 6df4c94 Compare February 5, 2024 12:07

Thalley requested review from pin-zephyr, kruithofa and cvinayak February 5, 2024 12:07

cvinayak approved these changes Feb 5, 2024

View reviewed changes

pin-zephyr approved these changes Feb 5, 2024

View reviewed changes

kruithofa approved these changes Feb 5, 2024

View reviewed changes

aescolar modified the milestones: v3.6.0, v3.7.0 Feb 6, 2024

nashif merged commit 28276ce into zephyrproject-rtos:main Feb 26, 2024
22 checks passed

Thalley deleted the csis_sirk_notifiable branch February 26, 2024 08:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LE Audio: Add support for dynamically setting the CSIP SIRK #67548

LE Audio: Add support for dynamically setting the CSIP SIRK #67548

Thalley commented Jan 12, 2024

kruithofa left a comment

fredrikdanebjer Feb 2, 2024

Thalley Feb 2, 2024

fredrikdanebjer Feb 2, 2024

Thalley Feb 2, 2024

fredrikdanebjer Feb 2, 2024

pin-zephyr left a comment •

edited

Loading

pin-zephyr Feb 2, 2024

cvinayak Feb 5, 2024

pin-zephyr Feb 2, 2024

pin-zephyr Feb 2, 2024

Thalley Feb 5, 2024

cvinayak Feb 3, 2024

cvinayak Feb 3, 2024

cvinayak Feb 3, 2024

pin-zephyr left a comment

cvinayak Feb 5, 2024

cvinayak Feb 5, 2024

pin-zephyr left a comment

aescolar commented Feb 6, 2024

Thalley commented Feb 6, 2024

	If :code:`CONFIG_BT_CSIP_SET_MEMBER_NOTIFIABLE`` is enabled, this will also notify connected
	If :code:`CONFIG_BT_CSIP_SET_MEMBER_NOTIFIABLE` is enabled, this will also notify connected

	* The new SIKR can be accessed via the @p inst.info.
	* The new SIRK can be accessed via the @p inst.info.

	FAIL("Failed to set SIRK: %d\n", err);
	FAIL("Failed to get SIRK: %d\n", err);

LE Audio: Add support for dynamically setting the CSIP SIRK #67548

LE Audio: Add support for dynamically setting the CSIP SIRK #67548

Conversation

Thalley commented Jan 12, 2024

kruithofa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pin-zephyr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pin-zephyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pin-zephyr left a comment

Choose a reason for hiding this comment

aescolar commented Feb 6, 2024

Thalley commented Feb 6, 2024

pin-zephyr left a comment •

edited

Loading