samples: Bluetooth: ISO: Update conn and adv intervals #81227

Thalley · 2024-11-11T14:20:07Z

Update the connection and advertising intervals of the
broadcast and connected ISO samples (including benchmark
samples) to work better with the selected SDU intervals
and the resulting ISO intervals.

rugeGerritsen

It would be nice if the commit message could state that the workarounds applied are specific to the Zephyr Bluetooth Controller and may not be needed for others. It would also be nice to document with meant with "works better with". Maybe something like: "These changes ensure that the controller selects conflict-free scheduling patterns to avoid packet loss".

Also, there is a typo in the commit header Bluetoot -> Bluetooth :)

Added some other minor comments

rugeGerritsen · 2024-11-19T08:14:14Z

samples/bluetooth/iso_connected_benchmark/src/main.c

Some controllers are able to select a "good" connection interval if the connection interval range passed to the controller is sufficiently large. Can the zephyr controller do that?

That would result in a simpler implementation

At this point the controller would be unaware of any future ISO connections, and can thus not decide on a good interval that works with the future and unknown ISO interval, right?

This depends a bit upon if the application uses the test command or not. If not using the test command, the controller can select an ISO interval which fits with the other activities.

the controller can select an ISO interval which fits with the other activities.

To some extend; for unframed ISO the ISO interval shall be a multiple of the SDU inteval.

But are you suggesting that I revert this change, and leave it up to the controller?
Alternatively we can also ask the user for the conn parameters, as we do for the ISO parameters

Some controllers are able to select a "good" connection interval if the connection interval range passed to the controller is sufficiently large. Can the zephyr controller do that?

As the CIG has not been established yet (or never requested under non ISO applications), Zephyr Controller uses the minimum requested connection interval.

Applications can perform a connection update, if required, post CIS established; again, Zephyr Controller will only use the minimum connection interval but use a window offset to place the ACL in a non-overlapping offset wrt the CIS.

As applications could give incompatible min-max connection interval that can have overlapping intervals with CIS, the complexity in the Controller to have a resolution is not justified.

Could a generic solution instead be:

Create CIG

Connect ACL

Connect CIS

This sample is today doing

Connect ACL

Create CIG

Connect CIS

Would it help creating the CIG before establishing the ACL to help the controller pick reasonable ACL intervals?
Or is it better to create the ACL first so that the controller can pick reasonable ISO intervals, assuming that the test params are not used?

Ideally the host should just sending a reasonable interval range and have the controller do the intervals, as that is (or should be) controller agnostic

Could a generic solution instead be:

Create CIG
Connect ACL
Connect CIS

Modified the sample to do this instead. That should give the controller as much information as possible to select the best ISO and ACL intervals, and should be a controller agnostic solution.

rugeGerritsen · 2024-11-19T08:17:21Z

samples/bluetooth/iso_broadcast/src/main.c

Can we add a comment:

Suggested change

*/

* Other controllers are able to generate conflict-free scheduling patterns without setting the advertising interval

* in this way.

*/

Sure :) This one is very specific to ZLL, and not 100% sure if host samples should assume that ZLL is being used. For single-CPU builds we can check it, but for multi-CPU boards like the nRF5340 we cannot tell (unless we can read out some controller information at runtime).

Having the documentation for Zephyr Controller specific (limitation) is correct, and not to comment on other controller that may or may not support. (if they support handling overlaps, the specific interval are not problem anyway).

@cvinayak since we have samples that have host features not supported by the controller, there is already a general understanding/consensus that BT samples are not specific for any controller, not even the Zephyr controller. I'm unsure whether we should mention that specifically here. Coul also just be vague and say "some controllers"?

+1 for mentioning that other controllers may not need this. We have a similar case for documenting scan window when CONFIG_BT_SCAN_AND_INITIATE_IN_PARALLEL is used.

Replaced the specific mentioning of Zephyr with Some controllers work best. Is that OK?

I was initially thinking it was fine to mention that the zephyr controller needs this but others don't. But I don't have a strong preference here

Given that these samples are not ZLL specific, I'd rather keep it vague but still explain that it may be required.

rugeGerritsen · 2024-11-19T08:18:49Z

samples/bluetooth/iso_broadcast_benchmark/src/broadcaster.c

Can we change this warning to LOG_INF? Other controllers may not have this limitation.

Perhaps you can elaborate? My understanding is that having ISO and ACL/ADV intervals be a multiple of one or the other is generally better, as that could prevent from any future overlap.

Are other controllers able to handle this better, and how?

Because of the randomness of the advertising event, a controller may "move" the advertising events in such a way that they do not overlap with other ISO-activities. This is possible because the randomness is in the range of 10 ms

@cvinayak is this something we could do with ZLL?

This is possible because the randomness is in the range of 10 ms

But if it is random, how can the controller move things? Wouldn't that make it non-random?

"Random enough" is the key here. It is possible to move the advertising event slightly and still preserve some randomness

@Thalley This logging is redundant as #80733 ensures BIG events are not skipped.

So we do not "need" to have the ISO interval as a multiple of the ADV interval?

Correction, I got confused, this is related to SDU interval not in multiple of 0.625 ms. If unframed ISO SDU in use, yes the log is a good suggestion to the user which will ensure all periodic advertising PDU do not overlap and hence faster periodic sync established.

I think it makes sense to not log this for framed. Will add a check for framing.

For framed, can/should the host do anything?

When framed is used, the controller should be able to select a "nice" ISO interval similar to #81227 (comment)

Updated to only check for unframed and changed to LOG_INF

rugeGerritsen · 2024-11-22T13:24:27Z

samples/bluetooth/iso_connected_benchmark/prj.conf

Remove the debug logging?

Update the connection and advertising intervals of the broadcast and connected ISO samples (including benchmark samples) to work better with the selected SDU intervals and the resulting ISO intervals. For the ISO connected benchmark the order of CIG and ACl has changed, so that we create the CIG before connecting the ACL for the purpose of providing as much information as possible to the controller. Signed-off-by: Emil Gydesen <[email protected]>

Thalley mentioned this pull request Nov 11, 2024

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

Merged

Thalley force-pushed the iso_samples_intervals branch 3 times, most recently from 370d61d to a154502 Compare November 18, 2024 19:29

Thalley self-assigned this Nov 18, 2024

Thalley marked this pull request as ready for review November 18, 2024 23:58

zephyrbot added area: Samples Samples area: Bluetooth area: Bluetooth ISO Bluetooth LE Isochronous Channels labels Nov 18, 2024

zephyrbot requested review from jhedberg, kartben, kruithofa, nashif and rugeGerritsen November 18, 2024 23:59

rugeGerritsen requested changes Nov 19, 2024

View reviewed changes

Thalley requested a review from cvinayak November 19, 2024 10:08

Thalley force-pushed the iso_samples_intervals branch 3 times, most recently from 2477dc7 to 9177709 Compare November 22, 2024 13:18

Thalley requested a review from rugeGerritsen November 22, 2024 13:18

Thalley force-pushed the iso_samples_intervals branch from 9177709 to 1aadbac Compare November 22, 2024 13:22

rugeGerritsen reviewed Nov 22, 2024

View reviewed changes

samples/bluetooth/iso_connected_benchmark/prj.conf Outdated

Copy link

Contributor

rugeGerritsen Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the debug logging?

Thalley force-pushed the iso_samples_intervals branch 2 times, most recently from ff03670 to eaafda2 Compare November 22, 2024 13:56

rugeGerritsen approved these changes Nov 22, 2024

View reviewed changes

Thalley force-pushed the iso_samples_intervals branch from eaafda2 to e5e7395 Compare November 22, 2024 20:42

cvinayak changed the title ~~samples: Bluetoot: ISO: Update conn and adv intervals~~ samples: Bluetooth: ISO: Update conn and adv intervals Nov 24, 2024

cvinayak approved these changes Nov 24, 2024

View reviewed changes

kartben merged commit 420cafc into zephyrproject-rtos:main Nov 25, 2024
21 checks passed

Thalley deleted the iso_samples_intervals branch November 25, 2024 09:17

-	 */
+	* Other controllers are able to generate conflict-free scheduling patterns without setting the advertising interval
+	* in this way.
+	*/

samples: Bluetooth: ISO: Update conn and adv intervals #81227

samples: Bluetooth: ISO: Update conn and adv intervals #81227

Uh oh!

Conversation

Thalley commented Nov 11, 2024

Uh oh!

rugeGerritsen left a comment

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!

Thalley Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Thalley Nov 19, 2024 •

edited

Loading