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 samples: Keep broadcast assistant connected #72409

Merged
merged 1 commit into from
May 17, 2024

Conversation

jthm-ot
Copy link
Contributor

@jthm-ot jthm-ot commented May 7, 2024

When Broadcast Sink is connected to Broadcast Assistant then keep connection when Broadcast Source is removed.

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.

A few things that needs a fix, a few things that needs a bit of formatting, but overall looks good.

@@ -107,6 +108,7 @@ static struct broadcast_sink_stream {
} streams[CONFIG_BT_BAP_BROADCAST_SNK_STREAM_COUNT];

static struct bt_bap_stream *streams_p[ARRAY_SIZE(streams)];
static bool stream_started;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should consider using the count of sem_stream_started instead of a new bool. You can do k_sem_count_get(sem_stream_started) to check if there are any streams started

You can also keep this, but then perhaps rename it to big_synced and make it volatile or guard it with a mutex since you access it from multiple threads

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wanted to wait for stream started. But k_sem_take(&sem_stream_started, SEM_TIMEOUT) sets it to zero and k_sem_count_get(sem_stream_started) no longer indicates whether streams are started

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will skip the wait for sem_stream_started and keep the old 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.

Now waiting for stream connected

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wanted to wait for stream started. But k_sem_take(&sem_stream_started, SEM_TIMEOUT) sets it to zero and k_sem_count_get(sem_stream_started) no longer indicates whether streams are started

Indeed. Using semaphores for signaling is kind of a weird use of them :)

I've previously (several times) consider using https://docs.zephyrproject.org/latest/kernel/services/polling.html instead, but always felt that it was overkill

samples/bluetooth/broadcast_audio_sink/src/main.c Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_audio_sink/src/main.c Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_audio_sink/src/main.c Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_audio_sink/src/main.c Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_audio_sink/src/main.c Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_audio_sink/src/main.c Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_audio_sink/src/main.c Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_audio_sink/src/main.c Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_audio_sink/src/main.c Outdated Show resolved Hide resolved
@larsgk larsgk requested a review from Thalley May 8, 2024 19:12
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.

Changes looks OK, but some comments/questions left that I'd like to get resolved first

samples/bluetooth/broadcast_audio_sink/src/main.c Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_audio_sink/src/main.c Outdated Show resolved Hide resolved
Comment on lines 1325 to 1348
if (broadcast_assistant_conn == NULL) {
k_sem_reset(&sem_connected);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

setup implies something you do once at the beginning.

Maybe we need to split this into 2 functions: 1 for the broadcast stuff, and 1 for the connection stuff?

Thalley
Thalley previously approved these changes May 15, 2024
samples/bluetooth/broadcast_audio_sink/src/main.c Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_audio_sink/src/main.c Outdated Show resolved Hide resolved
@jthm-ot jthm-ot force-pushed the work_broadcast_sink_2 branch 2 times, most recently from 92893bd to 294a951 Compare May 16, 2024 12:58
When Broadcast Sink is connected to Broadcast Assistant then keep
connection when Broadcast Source is removed.

Signed-off-by: Jens Rehhoff Thomsen <jthm@demant.com>
@jhedberg jhedberg assigned Thalley and unassigned jhedberg May 17, 2024
@carlescufi carlescufi merged commit a2ebe49 into zephyrproject-rtos:main May 17, 2024
19 checks passed
larsgk added a commit to larsgk/zephyr that referenced this pull request Jun 11, 2024
Jens has contributed the following PRs

zephyrproject-rtos#68678
zephyrproject-rtos#72409
zephyrproject-rtos#72915
zephyrproject-rtos#73564
zephyrproject-rtos#73656

and is attending the weekly LE Audio Zephyr meetings.

Signed-off-by: Lars Knudsen <larsgk@gmail.com>
larsgk added a commit to larsgk/zephyr that referenced this pull request Jun 11, 2024
Jens has contributed the following PRs

zephyrproject-rtos#68678
zephyrproject-rtos#72409
zephyrproject-rtos#72915
zephyrproject-rtos#73564
zephyrproject-rtos#73656

and is attending the weekly LE Audio Zephyr meetings.

Signed-off-by: Lars Knudsen <LAKD@Demant.com>
jhedberg pushed a commit that referenced this pull request Jun 12, 2024
Jens has contributed the following PRs

#68678
#72409
#72915
#73564
#73656

and is attending the weekly LE Audio Zephyr meetings.

Signed-off-by: Lars Knudsen <LAKD@Demant.com>
coreboot-org-bot pushed a commit to coreboot/zephyr-cros that referenced this pull request Jun 13, 2024
Jens has contributed the following PRs

zephyrproject-rtos/zephyr#68678
zephyrproject-rtos/zephyr#72409
zephyrproject-rtos/zephyr#72915
zephyrproject-rtos/zephyr#73564
zephyrproject-rtos/zephyr#73656

and is attending the weekly LE Audio Zephyr meetings.

(cherry picked from commit 0923750)

Original-Signed-off-by: Lars Knudsen <LAKD@Demant.com>
GitOrigin-RevId: 0923750
Change-Id: Ic57e788e2dc4d5ae0dbbb0687bb23657c6c9f8f5
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/5629141
Tested-by: Jeremy Bettis <jbettis@chromium.org>
Reviewed-by: Jeremy Bettis <jbettis@chromium.org>
Tested-by: ChromeOS Prod (Robot) <chromeos-ci-prod@chromeos-bot.iam.gserviceaccount.com>
Commit-Queue: Jeremy Bettis <jbettis@chromium.org>
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.

None yet

6 participants