Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

samples: Add Broadcast Assistant sample #68228

Merged

Conversation

larsgk
Copy link
Contributor

@larsgk larsgk commented Jan 29, 2024

First version of Broadcast Assistant sample
with hard coded selection of sink and source.

@larsgk larsgk force-pushed the add_broadcast_assistant_sample branch 2 times, most recently from cba5780 to c2d6280 Compare January 29, 2024 18:52
@larsgk larsgk marked this pull request as ready for review January 29, 2024 18:52
@larsgk larsgk requested a review from Thalley January 29, 2024 18:53
@larsgk larsgk force-pushed the add_broadcast_assistant_sample branch from c2d6280 to c72862a Compare January 29, 2024 18:57
Copy link
Member

@jhedberg jhedberg left a comment

Choose a reason for hiding this comment

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

Looks generally really nice and clean! A few whitespace related coding style comments inline.

samples/bluetooth/broadcast_assistant/src/main.c Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_assistant/src/main.c Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_assistant/src/main.c Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_assistant/src/main.c Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_assistant/src/main.c Outdated Show resolved Hide resolved
@larsgk larsgk force-pushed the add_broadcast_assistant_sample branch from c72862a to 11900a1 Compare January 29, 2024 19:38
@larsgk
Copy link
Contributor Author

larsgk commented Jan 29, 2024

Looks generally really nice and clean! A few whitespace related coding style comments inline.

Thanks for reviewing! - I think I missed setting the right tab size in the editor. (also just pushed a small fix for nrf52840 - when using with the Samsung Galaxy Buds2 Pro, there is so much advertising data it seems ;) - but tested with both nrf5340 Audio DK/Zephyr hci_ipc + nrf52840 DK now)

@larsgk larsgk force-pushed the add_broadcast_assistant_sample branch 2 times, most recently from 38af85b to b2a0769 Compare January 29, 2024 19:55
samples/bluetooth/broadcast_assistant/Kconfig Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_assistant/Kconfig Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_assistant/Kconfig Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_assistant/README.rst Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_assistant/README.rst Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_assistant/README.rst Outdated Show resolved Hide resolved
@larsgk
Copy link
Contributor Author

larsgk commented Jan 29, 2024

I'll just add some printk logs here of this application running on an nRF5340 Audio DK against a Broadcast Audio Source (nRF52840 Dongle) and the Samsung Galaxy Buds2 Pro (broadcast audio plays just after that last line in the log):

*** Booting Zephyr OS build zephyr-v3.5.0-4883-g38af85b622b7 ***
[00:00:00.406,188] <inf> bt_hci_core: HW Platform: Nordic Semiconductor (0x0002)
[00:00:00.406,250] <inf> bt_hci_core: HW Variant: nRF53x (0x0003)
[00:00:00.406,250] <inf> bt_hci_core: Firmware: Standard Bluetooth controller (0x00) Version 3.5 Build 99
[00:00:00.407,958] <inf> bt_hci_core: Identity: E1:62:6A:FE:50:01 (random)
[00:00:00.407,989] <inf> bt_hci_core: HCI: version 5.4 (0x0d) revision 0x0000, manufacturer 0x05f1
[00:00:00.408,020] <inf> bt_hci_core: LMP: version 5.4 (0x0d) subver 0xffff
Bluetooth initialized
Scanning for Broadcast Sink successfully started
Broadcast Sink Found:
  BT Name:        Galaxy Buds2 Pro
Match found for 'Galaxy'
Connecting to Broadcast Sink: Galaxy Buds2 Pro
Connected: 43:22:0B:70:20:57 (random)
Security level changed: 2
Scanning for Broadcast Source successfully started
Broadcast Source Found:
  BT Name:        LAKD Broadcast Music
  Broadcast Name: LAKD Broadcast Music
  Broadcast ID:   0xf147b6

Match found for 'LAKD Broadcast'
Selecting Broadcast ID: 0xf147b6


Only change to config is:

CONFIG_SELECT_SINK_NAME="Galaxy"
CONFIG_SELECT_SOURCE_NAME="LAKD Broadcast"

@larsgk larsgk force-pushed the add_broadcast_assistant_sample branch from b2a0769 to 261d6d2 Compare January 29, 2024 20:31
Copy link
Collaborator

@Thalley Thalley left a comment

Choose a reason for hiding this comment

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

Nicely done for an initial sample. A few suggestions

samples/bluetooth/broadcast_assistant/README.rst Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_assistant/README.rst Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_assistant/README.rst Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_assistant/README.rst Outdated Show resolved Hide resolved
Comment on lines 44 to 51
Building for an nrf52840dk
--------------------------

.. zephyr-app-commands::
:zephyr-app: samples/bluetooth/broadcast_assistant/
:board: nrf52840dk_nrf52840
:goals: build
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have a look at https:/zephyrproject-rtos/zephyr/blob/main/samples/bluetooth/broadcast_audio_sink/README.rst?plain=1#L29-L80 and see if it makes sense to add similar (or the same) text here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as we don't need the extra support for actual ISO streams and because the sample is not power hungry enough to require an nRF5340, I decided it was not needed and also decided to use the nRF52840 as build example. If someone wants to run it on an nRF5340 device, the readme has a link to the bluetooth samples overview with the info on how to build hci_ipc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nonetheless, we still probably want to build this for the nRF5340 ADK in CI, given that it's audio related, right? And the above sample also adds instructions on how to build it for that.

Choose a reason for hiding this comment

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

Suggested change
Building for an nrf52840dk
--------------------------
.. zephyr-app-commands::
:zephyr-app: samples/bluetooth/broadcast_assistant/
:board: nrf52840dk_nrf52840
:goals: build
Building for an nrf52840dk
--------------------------
.. zephyr-app-commands::
:zephyr-app: samples/bluetooth/broadcast_assistant/
:board: nrf52840dk_nrf52840
:goals: build
Building for an nrf5340dk
-------------------------
You can build both the application core image and an appropriate controller image for the network
core with:
.. zephyr-app-commands::
:zephyr-app: samples/bluetooth/broadcast_assistant/
:board: nrf5340dk_nrf5340_cpuapp
:goals: build
:west-args: --sysbuild
If you prefer to only build the application core image, you can do so by doing instead:
.. zephyr-app-commands::
:zephyr-app: samples/bluetooth/broadcast_assistant/
:board: nrf5340dk_nrf5340_cpuapp
:goals: build
In that case you can pair this application core image with the
:ref:`hci_ipc sample <bluetooth-hci-ipc-sample>`
:zephyr_file:`samples/bluetooth/hci_ipc/nrf5340_cpunet_iso-bt_ll_sw_split.conf` configuration.
Building for a simulated nrf5340bsim
------------------------------------
Similarly to how you would for real HW, you can do:
.. zephyr-app-commands::
:zephyr-app: samples/bluetooth/broadcast_assistant/
:board: nrf5340bsim_nrf5340_cpuapp
:goals: build
:west-args: --sysbuild
Note this will produce a Linux executable in `./build/zephyr/zephyr.exe`.
For more information, check :ref:`this board documentation <nrf5340bsim>`.

Is the "samples/bluetooth/broadcast_audio_sink/overlay-bt_ll_sw_split.conf" file need in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add instructions for how to build for nRF5340 Audio DK, but I'd prefer to add sysbuild & multiple targets in sample.yml in a subsequent PR, if that would be ok?

samples/bluetooth/broadcast_assistant/src/main.c Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_assistant/src/main.c Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_assistant/src/main.c Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_assistant/src/main.c Outdated Show resolved Hide resolved
param.pa_interval = selected_pa_interval;
param.broadcast_id = selected_broadcast_id;
param.pa_sync = true;
param.num_subgroups = 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hardcoded value :)

Not sure what is valid to do here; e.g. if the source has 2 subgroups, can we just provide one?

BASS states

The Num_Subgroups field is used to expose the number of subgroups present in the Broadcast Audio Source Endpoint (BASE) structure, defined in Section 3.7.2.2 in [3], used to describe a BIG.

So I think we have to PA sync to the broadcast source to get the BASE to provide that value to the server, but the specs are actually a bit unclear on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I will try with a few variants of sources today. Maybe it just takes the first one found/indexed if set to 1? I'll try with both the Samsung Galaxy Buds2 Pro and a board with this: #65014

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't currently have an easy way of testing with multiple subgroups. The current broadcast_audio_sample has one. Would this be a potential future improvement. Also, it would require doing the PA sync and more.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed. The question is whether this is valid behavior (a sample should not have known invalid behavior).

The way I read it, we need to sync to the PA for the BASE to get the subgroup count etc. (or even if is syncable). The current iteration of this sample is built on quite a few assumptions :D

Similarly a broadcast assistant should also verify that the sink can support the BASE before, but as discussed offline this isn't possible yet (#68283)

@hermabe hermabe removed their request for review January 30, 2024 09:23
@larsgk larsgk force-pushed the add_broadcast_assistant_sample branch 2 times, most recently from b955fb6 to 50c9182 Compare January 30, 2024 10:02
@larsgk larsgk requested a review from Thalley January 30, 2024 10:03
Thalley
Thalley previously approved these changes Jan 31, 2024
Thalley
Thalley previously approved these changes Jan 31, 2024
@larsgk
Copy link
Contributor Author

larsgk commented Jan 31, 2024

@kartben and @jhedberg - can you accept the PR (can I dismiss your change requests)?

jhedberg
jhedberg previously approved these changes Jan 31, 2024
Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

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

A couple comments - adding a proper description to the sample would be my main request for change, the rest is not necessarily blocking but feels like it could be improved.
Thank you!

:name: Bluetooth: Broadcast Audio Assistant
:relevant-api: bt_bap

Bluetooth: Broadcast Audio Assistant
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is meant to be a short text description of the sample. It is recommended to word this as if you were completing the sentence "This code sample shows how to ..."). Not particularly fancy but maybe:

Suggested change
Bluetooth: Broadcast Audio Assistant
Use LE Broadcast Audio Assistant functionality

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll go with that - can't come up with something more fancy atm. Also, I should add that we plan to extend the functionality during the next month to make it a more usable.

Comment on lines +12 to +15
The sample will automatically try to connect to a device in the BAP Scan Delegator
role (advertising support for the Broadcast Audio Scan Service (BASS)).
It will then search for a broadcast source and (if found) add the broadcast ID to
the BAP Scan Delegator.
Copy link
Collaborator

@kartben kartben Jan 31, 2024

Choose a reason for hiding this comment

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

Just a thought but this might sound a bit complex for someone not really familiar with or interested in the underlying concepts and yet willing to try this "audio broadcast" thing. Since it looks like you've been using with sample in a very practical way with e.g. earbuds, does it make sense to add another paragraph that describes how a user may practically give this a try? "A typical way of running this sample would be to ... ... blah blah earbuds ..." :)
Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense, thanks

First version of Broadcast Assistant sample
with hard coded selection of sink and source.

Signed-off-by: Lars Knudsen <[email protected]>
@larsgk larsgk dismissed stale reviews from jhedberg and Thalley via 707e114 January 31, 2024 22:17
@larsgk larsgk force-pushed the add_broadcast_assistant_sample branch from 584119e to 707e114 Compare January 31, 2024 22:17
@larsgk
Copy link
Contributor Author

larsgk commented Jan 31, 2024

@kartben, I just added your suggestions to the README file (only change in the last push)

Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

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

Doc looks good, thank you!
Nit, maybe for a future update: I'm tend to be a sucker for including hyperlinks anywhere it makes sense, for better navigability/discoverability of the docs, and it would be nice to have actual links to the sink and source samples.

@larsgk
Copy link
Contributor Author

larsgk commented Jan 31, 2024

Doc looks good, thank you! Nit, maybe for a future update: I'm tend to be a sucker for including hyperlinks anywhere it makes sense, for better navigability/discoverability of the docs, and it would be nice to have actual links to the sink and source samples.

I can promise you that all broadcast audio samples will get some love and attention in the next weeks, where we will make sure to look at the docs too :)

@carlescufi carlescufi merged commit b77df76 into zephyrproject-rtos:main Feb 1, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

9 participants