Bluetooth: BAP: Add a set of suggested intervals to use with BAP #81093

Thalley · 2024-11-07T18:05:22Z

Add a selection of interval values that are suitable for BAP,
which will allow better coexistence between ISO and ACL,
for both broadcast and unicast.

Samples and tests have been updated to use these new values.

The shell has also been updated to use the LE Audio (BAP) values
if audio is enabled, and the audio.conf file has disabled automatic
updating of the connection parameters as the peripheral, as we rarely
(if ever) want to do that.

Thalley · 2024-11-09T00:26:29Z

include/zephyr/bluetooth/audio/bap.h

+ * Suggest use is to use this as both the minimum and maximum interval for best results.
+ * The intervals are suited for both unicast and broadcast.
+ * @{


Chose to use a set of intervals in MS, rather than sets of extended advertising parameters, periodic advertising parameters and sets of connection parameters.

If someone thinks the 3 types of sets (similar to how we have BT_LE_CONN_PARAM_DEFAULT, BT_LE_ADV_CONN, BT_LE_PER_ADV_DEFAULT, etc.), please let me know so we can discuss it, but it will be 3 times as many macros.

Thalley · 2024-11-09T00:27:44Z

include/zephyr/bluetooth/audio/bap.h

+ */
+/**


Some of these are not used by any sample or test. Should we remove one of the "FAST" ones, or keep the unused ones?

Since these are suggestions to the application developer of good intervals to use we can keep the FAST ones, even when they are not used in a sample IMO

include/zephyr/bluetooth/audio/bap.h

kruithofa

Only minor nitpicking

samples/bluetooth/cap_acceptor/src/main.c

kruithofa · 2024-11-19T12:56:52Z

samples/bluetooth/cap_initiator/src/cap_initiator_broadcast.c

@@ -69,19 +69,34 @@ static void broadcast_stream_sent_cb(struct bt_bap_stream *stream)

 static int setup_extended_adv(struct bt_le_ext_adv **adv)
 {
+	/* Zephyr Controller works best while Extended Advertising interval to be a multiple


Suggested change

/* Zephyr Controller works best while Extended Advertising interval to be a multiple

/* Zephyr Controller works best while Extended Advertising interval is a multiple

kruithofa · 2024-11-19T12:57:26Z

samples/bluetooth/pbp_public_broadcast_source/src/main.c

@@ -146,10 +146,27 @@ static struct bt_bap_stream_ops broadcast_stream_ops = {

 static int setup_extended_adv(struct bt_le_ext_adv **adv)
 {
+	/* Zephyr Controller works best while Extended Advertising interval to be a multiple


Suggested change

/* Zephyr Controller works best while Extended Advertising interval to be a multiple

/* Zephyr Controller works best while Extended Advertising interval is a multiple

kruithofa · 2024-11-19T12:57:46Z

samples/bluetooth/tmap_bms/src/cap_initiator.c

@@ -133,10 +133,27 @@ static const struct bt_data ad[] = {

 static int setup_extended_adv(struct bt_le_ext_adv **adv)
 {
+	/* Zephyr Controller works best while Extended Advertising interval to be a multiple


Suggested change

/* Zephyr Controller works best while Extended Advertising interval to be a multiple

/* Zephyr Controller works best while Extended Advertising interval is a multiple

kruithofa · 2024-11-19T12:59:42Z

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

@@ -149,6 +155,42 @@ BT_CONN_CB_DEFINE(conn_callbacks) = {
 	.security_changed = security_changed_cb,
 };

+void setup_broadcast_adv(struct bt_le_ext_adv **adv)
+{
+	/* Zephyr Controller works best while Extended Advertising interval to be a multiple


Suggested change

/* Zephyr Controller works best while Extended Advertising interval to be a multiple

/* Zephyr Controller works best while Extended Advertising interval is a multiple

rugeGerritsen

Looked through bap.h.
My concerns are mostly related to that the selected intervals should match what the BAP specification defines as suggested intervals.

rugeGerritsen · 2024-11-19T14:46:33Z

include/zephyr/bluetooth/audio/bap.h

@@ -50,6 +50,63 @@ extern "C" {
 /** An invalid Broadcast ID */
 #define BT_BAP_INVALID_BROADCAST_ID 0xFFFFFFFFU

+/**


Is there a reason for selecting different sets of intervals for 7.5 ms and 10 ms SDU intervals? To keep things simple, I would suggest using a common factor of these. That is, use intervals 30, 60, or 90 ms.

Edit. The BAP specification in itself suggests to use connection intervals of 7.5 - 30 ms, or 50 - 70 ms, see https://www.bluetooth.com/specifications/specs/bap-1-0-2/. I would suggest to stick to that.

rugeGerritsen · 2024-11-19T14:47:31Z

include/zephyr/bluetooth/audio/bap.h

The BAP specification suggests to use an advertising interval of 20-30 ms or 150 ms. See https://www.bluetooth.com/specifications/specs/bap-1-0-2/. Is there a reason for not sticking to that?

rugeGerritsen · 2024-11-19T14:51:22Z

include/zephyr/bluetooth/audio/bap.h

+ *
+ * This works well with ISO intervals of 10, 20 and 30 milliseconds for broadcast and unicast
+ */
+#define BT_BAP_COEX_INT_MS_10_FAST_1   60U


"Coex" is often used when talking about coexistence with wifi and other protocols. Therefore, to avoid confusion I would suggest to leave that out.

rugeGerritsen · 2024-11-19T14:51:41Z

subsys/bluetooth/audio/bap_broadcast_assistant.c

@@ -784,6 +784,7 @@ static void bap_broadcast_assistant_write_cp_cb(struct bt_conn *conn, uint8_t er
 	net_buf_simple_reset(&att_buf);

 	atomic_clear_bit(inst->flags, BAP_BA_FLAG_BUSY);
+	LOG_ERR("");


I guess you indent to remove this one? :)

Thalley · 2024-11-22T13:21:12Z

My concerns are mostly related to that the selected intervals should match what the BAP specification defines as suggested intervals.

You make a very good point. I had forgotten about the values defined in BAP, and it's counterintuitive that we defined different values here.

I will revisit the changes in this PR, and see if I can make a good solution with the BAP defined values.

Thalley · 2024-11-22T22:07:57Z

I've refactored this PR to defined some more complete parameters, based on the values defined the BAP spec.

For broadcast there are no values defined by BAP, and in those cases I've just defined some myself.

I opted to use BT_BAP_CONN_PARAM_RELAXED instead of BT_BAP_CONN_PARAM_SHORT_75 or BT_BAP_CONN_PARAM_SHORT_10, as I think that gives a better user experience with fewer disconnects and ISO challenges, over a slightly faster discovery discovery.

Let me know if you disagree

cvinayak · 2024-11-23T12:19:24Z

d_01: @00:00:24.895630 ASSERTION FAIL [cig_offset_us > 0] @ WEST_TOPDIR/zephyr/subsys/bluetooth/controller/ll_sw/ull_conn_iso.c:1020
d_01: @00:00:24.895630 @ WEST_TOPDIR/zephyr/subsys/bluetooth/controller/ll_sw/ull_conn_iso.c:1020
d_01: @00:00:24.895630 [00:00:24.895,629] os: z_fatal_error: >>> ZEPHYR FATAL ERROR 3: Kernel oops on CPU 0
d_01: @00:00:24.895630 [00:00:24.895,629] os: z_fatal_error: Current thread: 0x855f180 (idle)
d_01: @00:00:24.895630 [00:00:24.895,629] os: k_sys_fatal_error_handler: Halting system

This due to missing implementation to handle the overlapping ACL (or other) events at the CIS establishment instant.

	/* Make sure we have time to service first subevent. TODO: Improve
	 * by skipping <n> interval(s) and incrementing event_count.
	 */
	LL_ASSERT(cig_offset_us > 0);

May I know the ACL connection and ISO intervals in this failing test?

cvinayak · 2024-11-25T04:02:25Z

d_01: @00:00:24.895630 ASSERTION FAIL [cig_offset_us > 0] @ WEST_TOPDIR/zephyr/subsys/bluetooth/controller/ll_sw/ull_conn_iso.c:1020
d_01: @00:00:24.895630 @ WEST_TOPDIR/zephyr/subsys/bluetooth/controller/ll_sw/ull_conn_iso.c:1020
d_01: @00:00:24.895630 [00:00:24.895,629] os: z_fatal_error: >>> ZEPHYR FATAL ERROR 3: Kernel oops on CPU 0
d_01: @00:00:24.895630 [00:00:24.895,629] os: z_fatal_error: Current thread: 0x855f180 (idle)
d_01: @00:00:24.895630 [00:00:24.895,629] os: k_sys_fatal_error_handler: Halting system

This due to missing implementation to handle the overlapping ACL (or other) events at the CIS establishment instant.
	/* Make sure we have time to service first subevent. TODO: Improve
	 * by skipping <n> interval(s) and incrementing event_count.
	 */
	LL_ASSERT(cig_offset_us > 0);
May I know the ACL connection and ISO intervals in this failing test?

Fix for the symptom (as fix to handle the symptom needs more work, TODO is still valid): #81823

Add a selection of interval values that are suitable for BAP, which will allow better coexistence between ISO and ACL, for both broadcast and unicast. Some of these are defined by the BAP spec, and some are defined by Zephyr, since they do have a suggested value from BAP. Samples and tests have been updated to use these new values. The shell has also been updated to use the LE Audio (BAP) values if audio is enabled, and the audio.conf file has disabled automatic updating of the connection parameters as the peripheral, as we rarely (if ever) want to do that. Due to the connection interval change, CI hit an issue with test_bass_broadcast_code in test_main_client_sync, where the reading of the long receive state did not finish before we attempted to do another procedure, so the function was updated to have a retry. Signed-off-by: Emil Gydesen <[email protected]>

Thalley changed the title ~~Audio interval macros~~ Bluetooth: BAP: Add a set of suggested intervals to use with BAP Nov 7, 2024

Thalley mentioned this pull request Nov 7, 2024

Bluetooth: Host: Add conversion macros from ms to various units #81068

Merged

Thalley added this to the v4.1.0 milestone Nov 7, 2024

Thalley force-pushed the audio_interval_macros branch 3 times, most recently from ad61f81 to b94bd7b Compare November 7, 2024 20:37

Thalley commented Nov 9, 2024

View reviewed changes

Thalley force-pushed the audio_interval_macros branch from b94bd7b to f4f3960 Compare November 9, 2024 00:28

Thalley force-pushed the audio_interval_macros branch from f4f3960 to 60198f5 Compare November 18, 2024 10:14

Thalley marked this pull request as ready for review November 18, 2024 16:05

zephyrbot added area: Samples Samples area: Bluetooth area: Bluetooth Host area: Bluetooth Audio labels Nov 18, 2024

zephyrbot requested review from asbjornsabo, babrsn, Casper-Bonde-Bose, fredrikdanebjer, jhedberg, jthm-ot, kartben, kruithofa, larsgk, MariuszSkamra, nashif, niym-ot, pin-zephyr and sjanc November 18, 2024 16:06

zephyrbot assigned kartben Nov 18, 2024

kartben assigned Thalley and unassigned kartben Nov 18, 2024

Thalley requested a review from cvinayak November 18, 2024 17:31

kruithofa reviewed Nov 19, 2024

View reviewed changes

include/zephyr/bluetooth/audio/bap.h Outdated Show resolved Hide resolved

kruithofa reviewed Nov 19, 2024

View reviewed changes

include/zephyr/bluetooth/audio/bap.h Outdated Show resolved Hide resolved

kruithofa requested changes Nov 19, 2024

View reviewed changes

Thalley requested a review from rugeGerritsen November 19, 2024 14:32

rugeGerritsen requested changes Nov 19, 2024

View reviewed changes

Thalley force-pushed the audio_interval_macros branch 2 times, most recently from 2537d93 to 73076a5 Compare November 22, 2024 22:03

Thalley requested review from rugeGerritsen and kruithofa November 22, 2024 22:08

Thalley force-pushed the audio_interval_macros branch 4 times, most recently from 7ad7bf2 to c0f2bd3 Compare November 22, 2024 23:17

cvinayak mentioned this pull request Nov 25, 2024

Bluetooth: Controller: Fix minimum offset for subsequent CISes #81823

Merged

Thalley force-pushed the audio_interval_macros branch 2 times, most recently from 43eb7b1 to 51ed62f Compare November 27, 2024 18:45

Thalley force-pushed the audio_interval_macros branch from 51ed62f to 66677da Compare November 29, 2024 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: BAP: Add a set of suggested intervals to use with BAP #81093

Bluetooth: BAP: Add a set of suggested intervals to use with BAP #81093

Thalley commented Nov 7, 2024 •

edited

Loading

Thalley Nov 9, 2024

Thalley Nov 9, 2024

kruithofa Nov 19, 2024

kruithofa left a comment

kruithofa Nov 19, 2024

kruithofa Nov 19, 2024

kruithofa Nov 19, 2024

kruithofa Nov 19, 2024

rugeGerritsen left a comment

rugeGerritsen Nov 19, 2024

rugeGerritsen Nov 19, 2024

rugeGerritsen Nov 19, 2024

rugeGerritsen Nov 19, 2024

Thalley commented Nov 22, 2024

Thalley commented Nov 22, 2024

cvinayak commented Nov 23, 2024

cvinayak commented Nov 25, 2024

	/* Zephyr Controller works best while Extended Advertising interval to be a multiple
	/* Zephyr Controller works best while Extended Advertising interval is a multiple

Bluetooth: BAP: Add a set of suggested intervals to use with BAP #81093

Are you sure you want to change the base?

Bluetooth: BAP: Add a set of suggested intervals to use with BAP #81093

Conversation

Thalley commented Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

rugeGerritsen 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

Thalley commented Nov 22, 2024

Thalley commented Nov 22, 2024

cvinayak commented Nov 23, 2024

cvinayak commented Nov 25, 2024

Thalley commented Nov 7, 2024 •

edited

Loading