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.

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

Samples: Bluetooth: Fix PAST support for broadcast sink #69040

Merged
merged 1 commit into from
Feb 16, 2024
Merged

Samples: Bluetooth: Fix PAST support for broadcast sink #69040

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 26 additions & 7 deletions samples/bluetooth/broadcast_audio_sink/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,7 @@ static int pa_sync_past(struct bt_conn *conn, uint16_t pa_interval)
if (err != 0) {
printk("Could not do PAST subscribe: %d\n", err);
} else {
printk("Syncing with PAST: %d\n", err);
printk("Syncing with PAST\n");
(void)k_work_reschedule(&pa_timer, K_MSEC(param.timeout * 10));
}

Expand All @@ -652,7 +652,6 @@ static int pa_sync_req_cb(struct bt_conn *conn,
const struct bt_bap_scan_delegator_recv_state *recv_state,
bool past_avail, uint16_t pa_interval)
{
int err;

printk("Received request to sync to PA (PAST %savailble): %u\n", past_avail ? "" : "not ",
recv_state->pa_sync_state);
Expand All @@ -667,16 +666,29 @@ static int pa_sync_req_cb(struct bt_conn *conn,
}

if (IS_ENABLED(CONFIG_BT_PER_ADV_SYNC_TRANSFER_RECEIVER) && past_avail) {
int err;

err = pa_sync_past(conn, pa_interval);
if (err != 0) {
printk("Failed to subscribe to PAST: %d\n", err);

return err;
}

k_sem_give(&sem_past_request);
} else {
/* start scan */
err = 0;

err = bt_bap_scan_delegator_set_pa_state(recv_state->src_id,
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we set the PA state to BT_BAP_PA_STATE_INFO_REQ to request PAST prior to calling pa_sync_past() in the line 671 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should we set the PA state to BT_BAP_PA_STATE_INFO_REQ to request PAST prior to calling pa_sync_past() in the line 671 ?

Hmm, good question, but I would argue no. If we (for whatever reason) fail to subscribe to PAST (which is what pa_sync_past does), then we don't need to update the state. I think we makes sense to update the state, when we reach a point that we are waiting for the remote device

Copy link
Collaborator

Choose a reason for hiding this comment

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

how about if we are not allowed to set the state to be BT_BAP_PA_STATE_INFO_REQ for some reason (e.g Could not find recv_state by src_id ....), do we still need to subscribe to PAST by pa_sync_past() ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, that is a fair point.

So basically we have the option to do

  1. Subscribe to past
  2. Set state (if this fails, no state change was done, and we can return error)

or

  1. Set state (if this fails, no state change was done, and we can return error)
  2. Subscribe to past
  3. If 2. fails, we need to restore state (and what if this fails?)

I would be OK with changing it to the latter, but it is a more complex solution, where as subscription to PAST is not really hurtful (and cannot be undone), even if we don't receive PAST from the peer.

Let me know if you want me to change it, but my immediate opinion would be to keep as is to keep it simple

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, it is ok for me to choose option 1 :-)

BT_BAP_PA_STATE_INFO_REQ);
if (err != 0) {
printk("Failed to set PA state to BT_BAP_PA_STATE_INFO_REQ: %d\n", err);

return err;
}
}

k_sem_give(&sem_pa_request);

return err;
return 0;
}

static int pa_sync_term_req_cb(struct bt_conn *conn,
Expand Down Expand Up @@ -918,10 +930,17 @@ static struct bt_le_scan_cb bap_scan_cb = {
static void bap_pa_sync_synced_cb(struct bt_le_per_adv_sync *sync,
struct bt_le_per_adv_sync_synced_info *info)
{
if (sync == pa_sync) {
if (sync == pa_sync ||
(req_recv_state != NULL && bt_addr_le_eq(info->addr, &req_recv_state->addr) &&
info->sid == req_recv_state->adv_sid)) {
printk("PA sync %p synced for broadcast sink with broadcast ID 0x%06X\n", sync,
broadcaster_broadcast_id);

if (pa_sync == NULL) {
pa_sync = sync;
}

k_work_cancel_delayable(&pa_timer);
k_sem_give(&sem_pa_synced);
}
}
Expand Down
Loading