Skip to content

Bluetooth: Audio: BASS server shall set BIS_Sync_State to be 0x0 afte… #89890

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+8 −1
Open

Bluetooth: Audio: BASS server shall set BIS_Sync_State to be 0x0 afte… #89890

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
9 changes: 8 additions & 1 deletion subsys/bluetooth/audio/bap_broadcast_sink.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,18 @@ static void update_recv_state_big_cleared(const struct bt_bap_broadcast_sink *si
reason == BT_HCI_ERR_TERM_DUE_TO_MIC_FAIL) {
/* Sync failed due to bad broadcast code */
mod_src_param.encrypt_state = BT_BAP_BIG_ENC_STATE_BAD_CODE;
} else if (reason == BT_HCI_ERR_CONN_TIMEOUT ||
reason == BT_HCI_ERR_LOCALHOST_TERM_CONN ||
reason == BT_HCI_ERR_REMOTE_USER_TERM_CONN) {
/* BIS is not synced anymore */
mod_src_param.encrypt_state = BT_BAP_BIG_ENC_STATE_NO_ENC;
} else {
mod_src_param.encrypt_state = recv_state->encrypt_state;
}
Comment on lines +173 to 180
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't the else here just be mod_src_param.encrypt_state = BT_BAP_BIG_ENC_STATE_NO_ENC;?

If update_recv_state_big_cleared is called and it isn't failing due to BT_HCI_ERR_TERM_DUE_TO_MIC_FAIL when we are attempting to decrypt, I guess the encrypt state should just always be set to BT_BAP_BIG_ENC_STATE_NO_ENC


if (reason != BT_HCI_ERR_LOCALHOST_TERM_CONN) {
if (reason != BT_HCI_ERR_CONN_TIMEOUT &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can BT_HCI_ERR_CONN_TIMEOUT happen when attempting to sync to the BIG? Or only after a successfull sync?

I'm wondering if the logic should be:
If mod_src_param.subgroups[i].bis_sync is set for any subgroup, then we consider it synced and set it to 0.
If mod_src_param.subgroups[i].bis_sync is 0, then we consider it non-synced and set it to BT_BAP_BIS_SYNC_FAILED.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as I understand, BT_HCI_ERR_CONN_TIMEOUT(e.g link loss) can happen at any time, I am not so sure if the logic you mentioned can cause some potential issue :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

My concern is that it's not particular explicit in the core spec what termination reason is sent when. The point of my comment was to achieve the same solution but in a way that won't be affected by controllers that may have different behavior

For example, if the BIG Sync fails, then some controller may perhaps send BT_HCI_ERR_CONN_TIMEOUT as the reason, in which case BT_BAP_BIS_SYNC_FAILED should be set

reason != BT_HCI_ERR_LOCALHOST_TERM_CONN &&
reason != BT_HCI_ERR_REMOTE_USER_TERM_CONN) {
for (uint8_t i = 0U; i < recv_state->num_subgroups; i++) {
mod_src_param.subgroups[i].bis_sync = BT_BAP_BIS_SYNC_FAILED;
}
Expand Down