Skip to content

LE Audio: Receiver start fix #51030

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 20, 2023
Merged

LE Audio: Receiver start fix #51030

merged 1 commit into from
Feb 20, 2023

Conversation

@Thalley
Copy link
Contributor

@Thalley Thalley commented Oct 6, 2022 •
edited
Loading

Modifies how and when the ASE on the unicast server side transitions to the streaming state.

Depends on #50822
fixes #50777

Reverts some changes done by #51027

@Thalley Thalley changed the title Receiver start fix LE Audio: Receiver start fix Oct 6, 2022
@Thalley Thalley force-pushed the receiver_start_fix branch 4 times, most recently from 0872f23 to afeb74a Compare October 7, 2022 11:21
@Thalley Thalley marked this pull request as ready for review October 7, 2022 11:21
@Thalley
Copy link
Contributor Author

Thalley commented Oct 7, 2022

Ping @szymon-czapracki

@Thalley Thalley requested a review from cvinayak October 7, 2022 11:22
@Thalley Thalley force-pushed the receiver_start_fix branch from afeb74a to 58e967a Compare October 7, 2022 11:36
@Thalley Thalley requested a review from kruithofa October 10, 2022 09:40
@Thalley Thalley added the bug The issue is a bug, or the PR is fixing a bug label Nov 7, 2022
@Thalley Thalley force-pushed the receiver_start_fix branch 3 times, most recently from 17e016b to d97c224 Compare November 7, 2022 11:57
@Thalley Thalley removed the bug The issue is a bug, or the PR is fixing a bug label Nov 7, 2022
@MariuszSkamra
Copy link
Contributor

MariuszSkamra commented Nov 14, 2022 •
edited
Loading

@szymon-czapracki could you please take a look at this one as it seems to be related to the issue you've discovered.
It's worth to run this fix against the PTS TC's that were failing for you

@Thalley Thalley mentioned this pull request Nov 17, 2022
Merged
@Thalley
Copy link
Contributor Author

Thalley commented Nov 25, 2022

Rebased on #51897

@Thalley Thalley mentioned this pull request Jan 6, 2023
Closed
MariuszSkamra
MariuszSkamra reviewed Jan 10, 2023
View reviewed changes
@Thalley Thalley requested a review from MariuszSkamra January 17, 2023 12:16
@Thalley Thalley force-pushed the receiver_start_fix branch from 8c9ec68 to 8d588e0 Compare January 17, 2023 12:16
@Thalley Thalley force-pushed the receiver_start_fix branch 2 times, most recently from 45e798c to 38c3101 Compare January 26, 2023 15:23
@Thalley Thalley requested a review from alwa-nordic as a code owner January 26, 2023 15:23
@Thalley Thalley mentioned this pull request Jan 26, 2023
Merged
@Thalley Thalley force-pushed the receiver_start_fix branch from 38c3101 to 4ada35c Compare January 26, 2023 15:28
MariuszSkamra
MariuszSkamra reviewed Feb 3, 2023
View reviewed changes
samples/bluetooth/unicast_audio_server/src/main.c Outdated
Comment on lines +552 to +559
Copy link
Contributor

@MariuszSkamra MariuszSkamra Feb 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why bt_audio_stream_start is called for SINK only? The API says:

 *  For the unicast server, this will put the stream into the streaming state if
 *  the CIS is connected (initialized by the unicast client). If the CIS is not
 *  connected yet, the stream will go into the streaming state as soon as the
 *  CIS is connected.

In other words I would expect bt_audio_stream_start will put the stream in streaming state, where both streams SINK and SOURCE need to be handled the same way from API point of view.

It's an ASCS specification detail that in case of SINK and SOURCE this method will be handled differently internally:
SINK:

  • Wait for CIS to be connected if not yet
  • Notify ASE state: Streaming
  • Call stream->started()

SOURCE:

  • Wait for CIS to be connected if not yet
  • Wait for ASE control operation: Receiver Start Ready
  • Notify ASE state: Streaming
  • Call stream->started()

Copy link
Contributor Author

@Thalley Thalley Feb 3, 2023

Choose a reason for hiding this comment

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

We could have similar behavior for sink and source ASEs for bt_audio_stream_start, but my thought here would be that bt_audio_stream_start is a wrapper for the receiver start ready operation, which can only be done on sink ASEs on the server, and similarly only for source ASEs on the client (as per the ASCS spec).

I agree that the documentation of the function is incorrect in this regard, and should be updated.

Copy link
Contributor Author

@Thalley Thalley Feb 3, 2023

Choose a reason for hiding this comment

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

Updated the documentation a bit :)

@Thalley
Bluetooth: Audio: Only allow receiver to send start command
0124000 
Only the receiver of the audio is allowed to send the
receiver start ready command.

Furthermore, this removes the automated transition to the
streaming state on the server, as the server now shall
call bt_audio_stream_start to put the stream into the
streaming state.

Signed-off-by: Emil Gydesen <[email protected]>
@Thalley Thalley force-pushed the receiver_start_fix branch from 4ada35c to 0124000 Compare February 3, 2023 13:20
@Thalley Thalley requested a review from MariuszSkamra February 3, 2023 13:23
fredrikdanebjer
fredrikdanebjer reviewed Feb 17, 2023
View reviewed changes
@fabiobaltieri fabiobaltieri merged commit e71460c into zephyrproject-rtos:main Feb 20, 2023
@Thalley Thalley deleted the receiver_start_fix branch March 8, 2023 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MariuszSkamra MariuszSkamra MariuszSkamra approved these changes

@fredrikdanebjer fredrikdanebjer fredrikdanebjer approved these changes

@jhedberg jhedberg Awaiting requested review from jhedberg

@Vudentz Vudentz Awaiting requested review from Vudentz

@sjanc sjanc Awaiting requested review from sjanc

@asbjornsabo asbjornsabo Awaiting requested review from asbjornsabo

@Casper-Bonde-Bose Casper-Bonde-Bose Awaiting requested review from Casper-Bonde-Bose

@cvinayak cvinayak Awaiting requested review from cvinayak

@szymon-czapracki szymon-czapracki Awaiting requested review from szymon-czapracki

@alwa-nordic alwa-nordic Awaiting requested review from alwa-nordic

@hermabe hermabe Awaiting requested review from hermabe

@jori-nordic jori-nordic Awaiting requested review from jori-nordic

+1 more reviewer

@kruithofa kruithofa kruithofa approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Thalley Thalley

Labels

area: Bluetooth Audio area: Bluetooth

Projects

Bluetooth LE Audio
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

LE Audio: Receiver start ready command shall only be sent by the receiver

6 participants

@Thalley @MariuszSkamra @fredrikdanebjer @kruithofa @fabiobaltieri @zephyrbot