Samples: Bluetooth: Fix PAST support for broadcast sink #69040
Conversation
Fix missing support for PAST in the broadcast audio sink sample, where it previously failed to request PAST from the broadcast assistant by not setting the PA state to BT_BAP_PA_STATE_INFO_REQ. Signed-off-by: Emil Gydesen <[email protected]>
Thalley
requested review from
larsgk,
cvinayak,
MariuszSkamra,
fredrikdanebjer,
kruithofa,
Casper-Bonde-Bose and
pin-zephyr
zephyrbot
requested review from
asbjornsabo,
hermabe,
jhedberg,
kartben,
nashif,
sjanc and
Vudentz
| /* start scan */ | ||
| err = 0; | ||
|
|
||
| err = bt_bap_scan_delegator_set_pa_state(recv_state->src_id, |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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() ?
There was a problem hiding this comment.
Hmm, that is a fair point.
So basically we have the option to do
- Subscribe to past
- Set state (if this fails, no state change was done, and we can return error)
or
- Set state (if this fails, no state change was done, and we can return error)
- Subscribe to past
- 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
There was a problem hiding this comment.
I see, it is ok for me to choose option 1 :-)
| /* start scan */ | ||
| err = 0; | ||
|
|
||
| err = bt_bap_scan_delegator_set_pa_state(recv_state->src_id, |
There was a problem hiding this comment.
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() ?
Fix missing support for PAST in the broadcast audio sink sample, where it previously failed to request PAST from the broadcast assistant by not setting the PA state to BT_BAP_PA_STATE_INFO_REQ.
fixes #68212