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

Bluetooth: BAP: Unicast client Split start and connect #73032

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

Thalley
Copy link
Collaborator

@Thalley Thalley commented May 20, 2024

Removes the CIS connection establishment from bt_bap_stream_start and move the behavior to a new funciton bt_bap_stream_connect.

This has 2 advantages:

  1. The behavior of bt_bap_stream_start is much more clear and more aligned with the spec's behavior for the receiver start ready opcode. 2) It is possible to connect streams in both the enabling and the QoS configured state with bt_bap_stream_connect as per the spec. This allows us to pass additional PTS test cases.

To implement this new behavior, samples and tests have been updated. The CAP Initiator implementation has also been updated, and slightly refactored, to accomodate for the change in BAP, but the CAP initiator implementation should work the same for application, except that it's now possible to do unicast start on ASEs in any order (#72138).

fixes #72138
fixes #70089

@Thalley Thalley force-pushed the bap_stream_connect branch 14 times, most recently from 454ded8 to 214e0c1 Compare May 24, 2024 14:50
@Thalley Thalley marked this pull request as ready for review May 24, 2024 18:42
@zephyrbot zephyrbot added area: Bluetooth area: Bluetooth Audio Release Notes To be mentioned in the release notes platform: nRF BSIM Nordic Semiconductors, nRF BabbleSim area: Samples Samples labels May 24, 2024
kruithofa
kruithofa previously approved these changes May 29, 2024
@@ -32,6 +32,7 @@ Commands
config : <direction: sink, source> <index> [loc <loc_bits>]
[preset <preset_name>]
stream_qos : interval [framing] [latency] [pd] [sdu] [phy] [rtn]
stream_connect : Connect the CIS of the stream
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be good to show this call in a small example (with complete flow)

Copy link
Contributor

@larsgk larsgk left a comment

Choose a reason for hiding this comment

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

It looks ok, but I would like to just try it out on a device

@@ -961,7 +1397,8 @@ static bool valid_unicast_audio_update_param(const struct bt_cap_unicast_audio_u
}

if (!can_update_metadata(bap_stream)) {
LOG_DBG("param->stream_params[%zu].stream is not in right state to be "
LOG_DBG("param->stream_params[%zu].stream is not in right state to "
"be "
Copy link
Contributor

Choose a reason for hiding this comment

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

extra line needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope - Some weird clang-format stuff I guess :D

uart:~$ bap qos
uart:~$ bap enable
uart:~$ bap connect
uart:~$ bap start
Copy link
Contributor

Choose a reason for hiding this comment

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

Trying this against a Samsung earbud, the audio flows fine, but it already seems to start streaming after calling 'connect'

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Your comment is for the Connect and establish a source stream when you are doing it on a sink stream. See the above steps for sink streams

Copy link
Contributor

Choose a reason for hiding this comment

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

yes... I see that now (was following the steps in the previous push that had config sink... all good... and approved :D

@Thalley
Copy link
Collaborator Author

Thalley commented May 31, 2024

Rebased to solve merge conflicts

larsgk
larsgk previously approved these changes May 31, 2024
uart:~$ bap qos
uart:~$ bap enable
uart:~$ bap connect
uart:~$ bap start
Copy link
Contributor

Choose a reason for hiding this comment

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

yes... I see that now (was following the steps in the previous push that had config sink... all good... and approved :D

@hermabe hermabe removed their request for review May 31, 2024 06:51
kruithofa
kruithofa previously approved these changes Jun 3, 2024
Removes the CIS connection establishment from bt_bap_stream_start
and move the behavior to a new funciton bt_bap_stream_connect.

This has 2 advantages:
1) The behavior of bt_bap_stream_start is much more clear and more aligned
with the spec's behavior for the receiver start ready opcode.
2) It is possible to connect streams in both the enabling
and the QoS configured state with bt_bap_stream_connect as
per the spec. This allows us to pass additional PTS test cases.

To implement this new behavior, samples and tests have been updated.

The CAP Initiator implementation has also been updated
to accomodate for the change in BAP, but the CAP
initiator implementation should work the same for application, except
that it's now possible to do unicast start on ASEs in any order
(zephyrproject-rtos#72138).

Signed-off-by: Emil Gydesen <[email protected]>
@Thalley Thalley dismissed stale reviews from kruithofa and larsgk via b37bcf8 June 3, 2024 07:32
@Thalley
Copy link
Collaborator Author

Thalley commented Jun 3, 2024

Rebased to solve merge conflicts

@Thalley Thalley requested review from kruithofa and larsgk June 3, 2024 07:32
@aescolar aescolar merged commit d2fbeff into zephyrproject-rtos:main Jun 3, 2024
24 checks passed
@Thalley Thalley deleted the bap_stream_connect branch June 3, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth Audio area: Bluetooth area: Samples Samples platform: nRF BSIM Nordic Semiconductors, nRF BabbleSim Release Notes To be mentioned in the release notes
Projects
Status: Done
5 participants