Bluetooth: CAP: Commander Reception start procedure

[kruithofa]

Add the CAP commander reception start procedure which starts reception on one or more CAP acceptors

fixes #64358

[Thalley]

Suggested change

	* bt_cap_initiator_unucast_audio_cancel()
	* bt_cap_commander_cancel()

[Thalley]

@kruithofa you may also have missed this one

[Thalley]

Some additional comments, and some comments still remaining from previous review

[Thalley]

A few minor comments (and some old comments that are still missing responses

[Thalley]

Suggested change

	if (start_param->num_subgroups > CONFIG_BT_BAP_BASS_MAX_SUBGROUPS) {
	CHECKIF(start_param->num_subgroups > CONFIG_BT_BAP_BASS_MAX_SUBGROUPS) {

[Thalley]

We should also consider validating that the bis_sync of each subgroup are distinct, e.g. subgroup 1 and 2 cannot both have index 0x02.

This can be done by something like

uint32_t total_bis_sync = 0U;

for (...) {
    if (total_bis_sync & start_param->subgroups[j].bis_sync != 0) {
        /* invalid parameters */
    }

    total_bis_sync |= start_param->subgroups[j].bis_sync;
}

[Thalley]

Since metadata is always LTV encoded, we could (should?) check if it's valid. You can use bt_audio_valid_ltv for that

[Thalley]

Suggest to move this to https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/bluetooth/audio/mocks/src/bap_stream.c

[kruithofa]

That creates conflicts in other unittests. Instead I'll compile in the complete bap_stream.c file (even though that adds a lot that we do not need)

[Thalley]

That creates conflicts in other unittests.

Oh :O What conflicts? Adding an implementation of a single function seems unlikely to cause such issues.

[kruithofa]

There are some other unittests which include the original bap_stream.c (from subsys/bluetooth/audio), which causes multiple definitions of a function.

[Thalley]

Suggested change

	#if defined(CONFIG_BT_BAP_BROADCAST_ASSISTANT)
	struct cap_broadcast_reception_start {
	bt_addr_le_t addr;
	uint8_t adv_sid;
	uint32_t broadcast_id;
	uint16_t pa_interval;
	uint8_t num_subgroups;
	struct bt_bap_bass_subgroup subgroups[CONFIG_BT_BAP_BASS_MAX_SUBGROUPS];
	};
	#endif /* CONFIG_BT_BAP_BROADCAST_ASSISTANT */
	#if defined(CONFIG_BT_BAP_BROADCAST_ASSISTANT)
	struct cap_broadcast_reception_start {
	bt_addr_le_t addr;
	uint8_t adv_sid;
	uint32_t broadcast_id;
	uint16_t pa_interval;
	uint8_t num_subgroups;
	struct bt_bap_bass_subgroup subgroups[CONFIG_BT_BAP_BASS_MAX_SUBGROUPS];
	};
	#endif /* CONFIG_BT_BAP_BROADCAST_ASSISTANT */

Didn't consider that we'd have to move this out of the union to pass it on as an argument, but of course that's required.

It's OK as is, but you can also revert this if you like

[kruithofa]

I think I like it this way, and it can be argued that the union is more readable if we move all structs outside of it

[pin-zephyr]

LGTM