zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.59k stars 6.48k forks source link

Bluetooth: Controller: ISO: Does not support differen SDU intervals/latencies in each direction #74544

Closed Thalley closed 1 month ago

Thalley commented 3 months ago

Describe the bug When create a CIG the application can choose to have difference SDU intervals and maximum transport latency in each direction, e.g. 10ms SDU interval from central to peripheral and 7.5ms SDU interval from peripheral to central.

However doing this seems to trigger an assert.

To Reproduce Steps to reproduce the behavior:

  1. Checkout https://github.com/zephyrproject-rtos/zephyr/pull/74495
  2. Build and run https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/bsim/bluetooth/audio/test_scripts/bap_unicast_client_async_group.sh
  3. See error

Expected behavior Expect the controller to either work with different values for each direction, or at the very least it should return an error. It should not assert.

Impact Annoyance as this is a arguably a corner case for most use cases.

Logs and console output

======== BAP Unicast Client Aync Group parameters test =========

d_00: @00:00:00.000000  *** Booting Zephyr OS build v3.7.0-rc1-30-g1c84b5d85358 ***
d_00: @00:00:00.000000  [00:00:00.000,000] <dbg> bt_tbs: tbs_inst_init: inst 0x8259f60 index 0x00 provider_name Unknown
d_00: @00:00:00.000000  [00:00:00.000,000] <dbg> bt_tbs: tbs_inst_init: inst 0x825a4a0 index 0xff provider_name Generic TBS
d_00: @00:00:00.000000  [00:00:00.000,000] <dbg> bt_ots: bt_gatt_ots_l2cap_init: Initialized OTS L2CAP
d_00: @00:00:00.005768  [00:00:00.005,767] <inf> bt_hci_core: HW Platform: Nordic Semiconductor (0x0002)
d_00: @00:00:00.005768  [00:00:00.005,767] <inf> bt_hci_core: HW Variant: nRF52x (0x0002)
d_00: @00:00:00.005768  [00:00:00.005,767] <inf> bt_hci_core: Firmware: Standard Bluetooth controller (0x00) Version 3.7 Build 0
d_00: @00:00:00.005768  [00:00:00.005,767] <wrn> bt_id: vs_read_static_addr: No static addresses stored in controller
d_00: @00:00:00.006488  [00:00:00.006,469] <dbg> bt_smp: bt_smp_init: LE SC enabled
d_00: @00:00:00.006488  [00:00:00.006,469] <inf> bt_hci_core: Identity: CC:AB:C3:77:B1:7A (random)
d_00: @00:00:00.006488  [00:00:00.006,469] <inf> bt_hci_core: HCI: version 5.4 (0x0d) revision 0x0000, manufacturer 0x05f1
d_00: @00:00:00.006488  [00:00:00.006,469] <inf> bt_hci_core: LMP: version 5.4 (0x0d) subver 0xffff
d_00: @00:00:00.006488  [00:00:00.006,469] <dbg> bt_bap_unicast_client: bt_bap_unicast_client_new_audio_iso: New bap_iso 0x825d220
d_00: @00:00:00.006488  [00:00:00.006,469] <dbg> bt_bap_unicast_client: unicast_group_add_stream: group 0x825d460 stream 0xf732a25c qos 0xf732a23c iso 0x825d220 dir 2
d_00: @00:00:00.006488  [00:00:00.006,469] <dbg> bt_bap_iso: bt_bap_iso_bind_stream: bap_iso 0x825d220 stream 0xf732a25c dir source
d_00: @00:00:00.006488  [00:00:00.006,469] <dbg> bt_bap_iso: bt_bap_iso_get_ep: iso 0x825d220 dir sink
d_00: @00:00:00.006488  [00:00:00.006,469] <dbg> bt_bap_unicast_client: unicast_group_add_stream: group 0x825d460 stream 0xf732a284 qos 0xf732a24c iso 0x825d220 dir 1
d_00: @00:00:00.006488  [00:00:00.006,469] <dbg> bt_bap_iso: bt_bap_iso_bind_stream: bap_iso 0x825d220 stream 0xf732a284 dir sink
d_00: @00:00:00.006488  [00:00:00.006,469] <dbg> bt_bap_iso: bt_bap_iso_get_ep: iso 0x825d220 dir source
d_00: @00:00:00.006488  [00:00:00.006,469] <dbg> bt_bap_unicast_client: bt_audio_cig_create: group 0x825d460
d_00: @00:00:00.006488  [00:00:00.006,469] <dbg> bt_iso: bt_iso_chan_add: iso 0x82582c0 chan 0x825d220
d_00: @00:00:00.006488  [00:00:00.006,469] <dbg> bt_iso: hci_le_set_cig_params: id 0, latency C to P 100, latency P to C 75, interval C to P 10000, interval P to C 7500, sca 0, packing 0, framing 0, num_cis 1
d_00: @00:00:00.006488  [00:00:00.006,469] <dbg> bt_iso: hci_le_set_cig_params: [0]: id 0, c_phy 2, c_sdu 40, c_rtn 2, p_phy 2, p_sdu 30, p_rtn 2
d_00: @00:00:00.006488  ASSERTION FAIL [iso_interval % sdu_interval == 0] @ WEST_TOPDIR/zephyr/subsys/bluetooth/controller/ll_sw/ull_central_iso.c:1225
d_00: @00:00:00.006488  @ WEST_TOPDIR/zephyr/subsys/bluetooth/controller/ll_sw/ull_central_iso.c:1225
d_00: @00:00:00.006488  [00:00:00.006,469] <err> os: z_fatal_error: >>> ZEPHYR FATAL ERROR 3: Kernel oops on CPU 0
d_00: @00:00:00.006488  [00:00:00.006,469] <err> os: z_fatal_error: Current thread: 0x8267b80 (sysworkq)
d_00: @00:00:00.006488  [00:00:00.006,469] <err> os: k_sys_fatal_error_handler: Halting system
d_00: @00:00:00.006488 ERROR: Exiting due to fatal error

Environment (please complete the following information):

Additional context image

wopu-ot commented 3 months ago

As the comment right above the assert indicates, the ISO_Interval must be equal to or an integer multiple of the SDU_Interval for unframed streams according to the specification. The correct fix for the issue is therefore to return an error instead of triggering an assert.

Thalley commented 3 months ago

As the comment right above the assert indicates, the ISO_Interval must be equal to or an integer multiple of the SDU_Interval for unframed streams according to the specification. The correct fix for the issue is therefore to return an error instead of triggering an assert.

Wouldn't it be possible to actually support this use case? Or is it a simple case where the parameters wouldn't work in this case?

I guess if using both 10ms and 7.5ms for a single CIS, then the ISO interval would need to be 30ms which wouldn't work with the supplied maximum transport latency, right?

Would the above parameters work if using framed or if it was 2 CIS instead of 1?

wopu-ot commented 3 months ago

You are right about the 30ms ISO interval being in conflict with the transport latency. However, I don't know if the controller would figure out that option in the first place. Two CISes should not change anything, because the CISes in a CIG have the same ISO interval. Framed should work.

Thalley commented 3 months ago

Did another test setting the maximum transport latency to 100 in both directions, but still kept 7.5ms SDU interval in P to C and 10ms in C to P with the same assert.

The same parameters work with other controllers such as the Nordic SDC