Skip to content

Conversation

@MariuszSkamra
Copy link
Contributor

This fixes possible ASE state race condition. The notification is sent immediately once the ASE state changed that eliminates a situation where the state was changed by user action (API function call) when the state was not yet notified to the remote Unicast Client.

Fixes: BAP/USR/SCC/BV-158-C
Signed-off-by: Mariusz Skamra [email protected]

@MariuszSkamra
Copy link
Contributor Author

MariuszSkamra commented Nov 14, 2022

I tried this one against PTS BAP/USR/SCC/BV-158-C and AOSP media, call

Copy link
Contributor

@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.

A few minor comments, which needs to ether be fixed or at least commented on. The fix for the state change to streaming should probably be done as part of #51030

Comment on lines +218 to +244
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this part was just moved, and it seems like the removal of this is missing from #51030: But ideally Sink ASEs should never autonousmly go into the streaming state - This should only happen as part of calling bt_audio_stream_start on the server side for Sink ASEs, as the Unicast Server application may want to delay going into the streaming state to setup anything related to processing audio.

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 I'm moving the code only, I would rather leave the logic unchanged for now. This can be fixed in another PR

Comment on lines +560 to +601
Copy link
Contributor

Choose a reason for hiding this comment

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

See my previous comment about autonomously going into the streaming state

@Thalley Thalley added the bug The issue is a bug, or the PR is fixing a bug label Dec 20, 2022
@MariuszSkamra MariuszSkamra force-pushed the fix_ase_state_ntf branch 2 times, most recently from 7c4e3d5 to c6daa4f Compare January 4, 2023 12:37
@MariuszSkamra MariuszSkamra requested a review from Thalley January 4, 2023 13:25
Thalley
Thalley previously approved these changes Jan 4, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm I'm not that much familiar with this code, but this & suggests that ase is container of ep, so this function can be called with ep from some other "pool"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I did not put much thought while rebasing the code. Looks like I can simply use CONTAINER_OF to get the ASE object

This fixes possible ASE state race condition. The notification is sent
immediately once the ASE state changed that eliminates a situation where
the state was changed by user action (API function call) when the state
was not yet notified to the remote Unicast Client.

Fixes: BAP/USR/SCC/BV-158-C
Signed-off-by: Mariusz Skamra <[email protected]>
@carlescufi carlescufi merged commit 8cd6c55 into zephyrproject-rtos:main Jan 5, 2023
@MariuszSkamra MariuszSkamra deleted the fix_ase_state_ntf branch January 5, 2023 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Bluetooth Audio area: Bluetooth bug The issue is a bug, or the PR is fixing a bug

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants