bluetooth: bap: Fix shift of BIS_Sync parameter of notification

[mkasenberg]

The PTS BASS/SR/CP test cases fails because of wrong value of the BIS_Sync parameter of the Add Source and Modify Source operations and of notification of Broadcast Audio Scan Control Point.

[Thalley]

Can you elaborate?

The BIS sync (and requested_bis_sync) are bitfields of BIS indexes and are stored in the same manner that should be sent over air, so it's unclear to me what the issue is what this is fixing

[mkasenberg]

The internal_state->requested_bis_sync[i] contains the BIS_Sync bitfield described in BASS spec in Table 3.5: Format of Add Source operation

received over the air in Add Source or Modify Source operation, where BIT(0) is allowed and represents BIS_index 1. But the bis_sync field

zephyr//include/zephyr/bluetooth/audio/bap.h

Lines 281 to 283 in 90fec53

	struct bt_bap_scan_delegator_subgroup {
	/** BIS synced bitfield */
	uint32_t bis_sync;

is filled like this:

zephyr/subsys/bluetooth/audio/bap_broadcast_sink.c

Lines 107 to 123 in 90fec53

	mod_src_param.num_subgroups = base->subgroup_count;
	for (uint8_t i = 0U; i < base->subgroup_count; i++) {
	struct bt_bap_scan_delegator_subgroup *subgroup_param = &mod_src_param.subgroups[i];
	const struct bt_bap_base_subgroup *subgroup = &base->subgroups[i];
	/* Update the BIS sync indexes for the subgroup based on the BASE*/
	for (size_t j = 0U; j < subgroup->bis_count; j++) {
	const struct bt_bap_base_bis_data *bis_data = &subgroup->bis_data[j];
	subgroup_param->bis_sync |= BIT(bis_data->index);
	}
	/* Update the bis_sync so that the bis_sync value only contains the indexes that we
	* are actually synced to
	*/
	subgroup_param->bis_sync &= sink->indexes_bitfield;
	}

so it seems to be left-shifted by 1 bit relative to internal_state->requested_bis_sync[i]. So to compare them with bits_subset_of(), one should be shifted. The BIS_Sync_State parameter of Broadcast Receive State Characteristic is defined as BIS_Sync, so a shift is needed here too:

(void)net_buf_simple_add_le32(&read_buf, subgroup->bis_sync >> 1);

[Thalley]

The internal_state->requested_bis_sync[i] contains the BIS_Sync bitfield described in BASS spec in Table 3.5: Format of Add Source operation

received over the air in Add Source or Modify Source operation, where BIT(0) is allowed and represents BIS_index 1. But the bis_sync field

zephyr//include/zephyr/bluetooth/audio/bap.h

Lines 281 to 283 in 90fec53

	struct bt_bap_scan_delegator_subgroup {
	/** BIS synced bitfield */
	uint32_t bis_sync;

is filled like this:

zephyr/subsys/bluetooth/audio/bap_broadcast_sink.c

Lines 107 to 123 in 90fec53

	mod_src_param.num_subgroups = base->subgroup_count;
	for (uint8_t i = 0U; i < base->subgroup_count; i++) {
	struct bt_bap_scan_delegator_subgroup *subgroup_param = &mod_src_param.subgroups[i];
	const struct bt_bap_base_subgroup *subgroup = &base->subgroups[i];
	/* Update the BIS sync indexes for the subgroup based on the BASE*/
	for (size_t j = 0U; j < subgroup->bis_count; j++) {
	const struct bt_bap_base_bis_data *bis_data = &subgroup->bis_data[j];
	subgroup_param->bis_sync |= BIT(bis_data->index);
	}
	/* Update the bis_sync so that the bis_sync value only contains the indexes that we
	* are actually synced to
	*/
	subgroup_param->bis_sync &= sink->indexes_bitfield;
	}

so it seems to be left-shifted by 1 bit relative to internal_state->requested_bis_sync[i]. So to compare them with bits_subset_of(), one should be shifted. The BIS_Sync_State parameter of Broadcast Receive State Characteristic is defined as BIS_Sync, so a shift is needed here too:

(void)net_buf_simple_add_le32(&read_buf, subgroup->bis_sync >> 1);

I'm sorry, I'm still not following how the above does any shifting.

The BIS index bitfields of the subgroups are populated as

subgroup_param->bis_sync |= BIT(bis_data->index);

and since bis_data->index cannot be 0, then BIT(0) is never set.

Can you provide any logs to describe the issue?

[mkasenberg]

Yes, please have a look at logs from BASS/SR/CP/BV-07-C test case:

• Bluetooth Protocol Viewer logs:
  BASS_SR_CP_BV_07_C_2023_11_03_14_42_42.zip
• Zephyr IUT logs:
  BASS_SR_CP_BV-07-C_iutctl.log

The PTS sent Modify Source operation with BIS_Sync bitfield set to 0x01.

So because the requested_bis_sync[0] takes its value directly from buffer:

zephyr/subsys/bluetooth/audio/bap_scan_delegator.c

Line 672 in 8ba8c18

internal_state->requested_bis_sync[i] = net_buf_simple_pull_le32(buf);

internal_state->requested_bis_sync[0] will be equal to 0x01 (the value that BIT(0) would return).

So since bis_data->index can have values only from range defined for BIS_index, 1-31, the subgroup_param->bis_sync is not equivalent of BIS_Sync parameter of Modify Source (or Add Source) operation.

And that is why this log in IUT logs appears:
[00:00:16.753,845] <dbg> bt_bap_scan_delegator: bt_bap_scan_delegator_mod_src: Subgroup[0] invalid bis_sync value 2 for 1
and the Modify Source operation fails.

[Thalley]

Yes, please have a look at logs from BASS/SR/CP/BV-07-C test case:

* Bluetooth Protocol Viewer logs:
  [BASS_SR_CP_BV_07_C_2023_11_03_14_42_42.zip](https://github.com/zephyrproject-rtos/zephyr/files/13250870/BASS_SR_CP_BV_07_C_2023_11_03_14_42_42.zip)

* Zephyr IUT logs:
  [BASS_SR_CP_BV-07-C_iutctl.log](https://github.com/zephyrproject-rtos/zephyr/files/13250922/BASS_SR_CP_BV-07-C_iutctl.log)

The PTS sent Modify Source operation with BIS_Sync bitfield set to 0x01.

So because the requested_bis_sync[0] takes its value directly from buffer:

zephyr/subsys/bluetooth/audio/bap_scan_delegator.c

Line 672 in 8ba8c18

internal_state->requested_bis_sync[i] = net_buf_simple_pull_le32(buf);

internal_state->requested_bis_sync[0] will be equal to 0x01 (the value that BIT(0) would return).

So since bis_data->index can have values only from range defined for BIS_index, 1-31, the subgroup_param->bis_sync is not equivalent of BIS_Sync parameter of Modify Source (or Add Source) operation.

And that is why this log in IUT logs appears: [00:00:16.753,845] <dbg> bt_bap_scan_delegator: bt_bap_scan_delegator_mod_src: Subgroup[0] invalid bis_sync value 2 for 1 and the Modify Source operation fails.

I see.

I think this part was missing for me:

4-octet bitfield. Bit 0-30 = BIS_index[1-31]

So BIS_index 0x01 uses BIT(0).

Which is both counterintuitive and conflicting with

0xxxxxxxxx: 0b1 = Synchronize to BIS_index[x]

As 0b00000000000000000000000000000001 indicates that we are syncing to BIS_index[0] as x is 0 in that case.

Or do you read it differently? That 0b00000000000000000000000000000001 indicates BIS_index[1]?

[mkasenberg]

I would treat those x-es as a wildcard values. There are some examples in BAP_v1.0.1.pdf like this one from 'Figure 6.13: Broadcast Assistant initiates Add Source operation'

which seem to confirm this usage.

But I must say I would sleep better too if the range of allowed values for BIS Indexes started from 0 instead of 1.

[Thalley]

I would treat those x-es as a wildcard values. There are some examples in BAP_v1.0.1.pdf like this one from 'Figure 6.13: Broadcast Assistant initiates Add Source operation'

which seem to confirm this usage.

But I must say I would sleep better too if the range of allowed values for BIS Indexes started from 0 instead of 1.

Thanks for that example - That seems to agree with PTS.

I think it makes sense to keep the BAP API to be similar to the ISO API (i.e. BIT(1) means index 1), and then as you've suggested here, we will shift the values when sending/receiving over air.

But I must say I would sleep better too if the range of allowed values for BIS Indexes started from 0 instead of 1.

In Zephyr, or in the spec? The BIS indexes range are defined by the core spec, and I think it won't be backwards compatible if modified to start from 0x00 instead of 0x01.

We could also, since the bitfield for indexes are not defined by the core, modify Zephyr to use BIT(0) for Index 1, BIT(1) for Index 2, etc. so that the Zephyr API follows the ranges of BASS.

[Thalley]

One comment, otherwise LGTM as an initial fix.

I've created #64838 to implement a more thorough solution.

[Thalley]

I think it would be better if we shifted the requested_bis_sync when storing it, so that the requested_bis_sync and param->subgroups[i].bis_sync; are stored the same way

That would also remove the need to shift in bt_bap_scan_delegator_set_bis_sync_state

[Thalley]

The API should still expect BIT(1) for BIS index 1, so if this is needed, then further fixes is required in the stack.

[mkasenberg]

I see, so a shift of the bis_sync is needed inside of bt_bap_broadcast_assistant_add_src and bt_bap_broadcast_assistant_mod_src, so the BIS_Sync value sent over the air could contain BIT(0) for BIS index 1.

[Thalley]

Indeed, at least until we do it for all functions as part of #64838