Bluetooth: Controller: Fix incorrect CIS payload count at CIS Estab #87538

cvinayak · 2025-03-24T04:50:14Z

Fix incorrect payload count at CIS Establish due to existing
CIG event overlapping the ACL event at the instant when the
CIS gets the active flag set.

The overlapping CIG event picked up the new CIS that had its
active flag set in the current CIG event instead of at the
actual CIS offset which is in the next CIG event.

Fix incorrect CIS offset in use if instant is picked from
the peer sent CIS RSP PDU. Instead, keep the instant that
was sent in the CIS REQ PDU as the instant to send in the
CIS IND PDU.

This fixes CIS failed to be established when dissimilar
ACL and ISO intervals are in use.

Fix Peripheral CIS sorted by CIG implemenation to use CIS
offset stored in LLL context which is the correct offset
from the CIG anchor point. CIS offset in the ULL context
is the offset from the ACL anchor point at the time of
the CIS establishment.

Fix CIS event_count_prepare use missed as part of fixes
related to commit be91cfe ("Bluetooth: Controller: Fix
incorrect event_count when CIG overlaps").

Fixes #83584.

Thalley · 2025-03-25T17:55:27Z

subsys/bluetooth/controller/ll_sw/lll_conn_iso.h

@@ -58,6 +58,7 @@ struct lll_conn_iso_stream {
 	uint8_t datapath_ready_rx:1;/* 1 if datapath for RX is ready */

 #if !defined(CONFIG_BT_CTLR_JIT_SCHEDULING)
+	uint8_t established:1;      /* 1 if CIS LLL is established */


What is the difference between this and the active field?

When are they different values? Can a CIS be established, but not active, or vice versa?

active flag is set by the ACL connection, and the new established flag will be set by the CIS connection "only" at the start of the CIG event. This way an active flag being set between CISes of a CIG be not picked up.

Not sure I fully understand :)

A CIS is allocated/created with the Set CIG Parameters commands. Does this set any any of these bits? (This is before any ACL has been paired with it).

The Create CIS command effectively binds a CIS to an ACL. I guess this is the one that sets the active field?

When the CIS is successfully established, is that when the established field is set?

If the above is correct, then it also always goes that active is true when established is true? Or is there a case, e.g. doing disconnection of the CIS, where active == false and established == true?

The reason I'm asking about this, is that you have some places where you do e.g. next_cis_lll->active && next_cis_lll->established, which implies that both may need to be true, instead of just established

Could we expand the documentation of these fields a bit, because their current documentation does not really provide anything as they are basically just repeating the name of the field, and not when it is set/unset or what it represents.

The Create CIS command effectively binds a CIS to an ACL. I guess this is the one that sets the active field?

No, active is set at the ACL connection event counter instant, thereafter at the CIS offset from the ACL anchor point the CIS is established in the CIG radio event.

When the CIS is successfully established, is that when the established field is set?

As stated above.

If the above is correct, then it also always goes that active is true when established is true?

When established is set, yes, active is already set too.

is there a case, e.g. doing disconnection of the CIS, where active == false and established == true?

No.

Ok. the next_cis_lll->active && next_cis_lll->established can be replaced with just next_cis_lll->established check.

cvinayak · 2025-03-26T08:32:54Z

subsys/bluetooth/controller/ll_sw/ull_llcp_pdu.c

+	/* FIXME: We do not pick the instant from the response; if we do, then
+	 *        we need to calculate again a new offset because the ACL and
+	 *        ISO intervals can be dissimilar and hence will be a different
+	 *        CIS offset when sending the CIS_IND PDU at this instant.
+	 *
+
 	ctx->data.cis_create.conn_event_count =
 		sys_le16_to_cpu(pdu->llctrl.cis_rsp.conn_event_count);
+	 */


@erbr-ot This change for the central to not pick the peripheral sent instant value is failing unit test; because ull_central_iso_cis_offset_get() is mocked to do nothing, leading to conn_event_count not being assigned the value to be verified.

Your review and ideas will be helpful here.

cvinayak · 2025-03-26T08:37:11Z

subsys/bluetooth/controller/ll_sw/lll_conn_iso.h

@@ -58,6 +58,7 @@ struct lll_conn_iso_stream {
 	uint8_t datapath_ready_rx:1;/* 1 if datapath for RX is ready */

 #if !defined(CONFIG_BT_CTLR_JIT_SCHEDULING)
+	uint8_t established:1;      /* 1 if CIS LLL is established */


active flag is set by the ACL connection, and the new established flag will be set by the CIS connection "only" at the start of the CIG event. This way an active flag being set between CISes of a CIG be not picked up.

Fix CIS event_count_prepare use missed as part of fixes related to commit be91cfe ("Bluetooth: Controller: Fix incorrect event_count when CIG overlaps"). Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>

Fix Peripheral CIS sorted by CIG implemenation to use CIS offset stored in LLL context which is the correct offset from the CIG anchor point. CIS offset in the ULL context is the offset from the ACL anchor point at the time of the CIS establishment. Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>

Fix incorrect payload count at CIS Establish due to existing CIG event overlapping the ACL event at the instant when the CIS gets the active flag set. The overlapping CIG event picked up the new CIS that had its active flag set in the current CIG event instead of at the actual CIS offset which is in the next CIG event. Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>

Fix incorrect CIS offset in use if instant is picked from the peer sent CIS RSP PDU. Instead, keep the instant that was sent in the CIS REQ PDU as the instant to send in the CIS IND PDU. This fixes CIS failed to be established when dissimilar ACL and ISO intervals are in use. Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>

Fix CIS offset calculation due to use of decremented ACL event counter, where as the CIS offset is inquired in the next ACL event. Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>

Enable back previously failed CIS tests due to MIC failures. Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>

Thalley · 2025-03-26T16:38:12Z

#AutoPTS run zephyr nrf53 BAP/UCL/STR/BV-523-C BAP/UCL/STR/BV-535-C BAP/USR/STR/BV-360-C BAP/USR/STR/BV-380-C

codecoup-tester · 2025-03-26T16:39:33Z

Scheduled PR #87538 (comment), board: nrf53, estimated start time: 17:40:32, test case count: 4, estimated duration: 0:15:52

Test cases to be run

BAP/UCL/STR/BV-535-C
BAP/UCL/STR/BV-523-C
BAP/USR/STR/BV-360-C
BAP/USR/STR/BV-380-C

codecoup-tester · 2025-03-26T17:04:32Z

AutoPTS Bot results:
No failed test found.

Successful tests (4)

BAP BAP/UCL/STR/BV-523-C PASS
BAP BAP/UCL/STR/BV-535-C PASS
BAP BAP/USR/STR/BV-360-C PASS
BAP BAP/USR/STR/BV-380-C PASS

cvinayak · 2025-03-27T15:29:11Z

@Tronil If there is no strong objections to the changes in this PR (i.e. conflicts or regressions possibly in your downstream), please provide your review/approval accordingly.

Tronil · 2025-03-28T09:18:06Z

subsys/bluetooth/controller/ll_sw/lll_conn_iso.h

+	 * which set the `active` flag and then when CIG event prepare has picked up the set
+	 * `active` flag.
+	 */
+	uint8_t established:1;


If I understand the usage of this flag correctly it is basically used as an "was active when prepare was called". Naming it established is misleading since that has a very specific meaning in the BT spec; Wouldn't active_prepare work better (that seems like the usual naming convention)?

agree.. how about just cis->prepared ?

Makes sense 👍

…Estab Fix incorrect payload count at CIS Establish due to existing CIG event overlapping the ACL event at the instant when the CIS gets the active flag set. The overlapping CIG event picked up the new CIS that had its active flag set in the current CIG event instead of at the actual CIS offset which is in the next CIG event. Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>

cvinayak marked this pull request as ready for review March 24, 2025 08:51

zephyrbot added area: Bluetooth area: Bluetooth Controller labels Mar 24, 2025

zephyrbot requested review from carlescufi, erbr-ot, mtpr-ot, thoh-ot and wopu-ot March 24, 2025 08:52

zephyrbot assigned cvinayak Mar 24, 2025

cvinayak force-pushed the github_cis_sorted_get_fix branch from 2059381 to 03a912e Compare March 25, 2025 14:15

zephyrbot added area: Bluetooth Audio platform: nRF BSIM Nordic Semiconductors, nRF BabbleSim labels Mar 25, 2025

zephyrbot requested review from asbjornsabo, Casper-Bonde-Bose, fredrikdanebjer, jhedberg, larsgk, MariuszSkamra, niym-ot, pin-zephyr, sjanc and Thalley March 25, 2025 17:12

Thalley reviewed Mar 25, 2025

View reviewed changes

cvinayak commented Mar 26, 2025

View reviewed changes

cvinayak added 6 commits March 26, 2025 11:29

Bluetooth: Controller: Fix CIS event_count_prepare use

fe17be2

Fix CIS event_count_prepare use missed as part of fixes related to commit be91cfe ("Bluetooth: Controller: Fix incorrect event_count when CIG overlaps"). Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>

Bluetooth: Controller: Fix CIS offset for dissimilar ACL & CIG interval

b889072

Fix CIS offset calculation due to use of decremented ACL event counter, where as the CIS offset is inquired in the next ACL event. Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>

tests: bsim: Bluetooth: Enable MIC failure CIS tests

caa0308

Enable back previously failed CIS tests due to MIC failures. Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>

cvinayak force-pushed the github_cis_sorted_get_fix branch from 9a2e7e1 to caa0308 Compare March 26, 2025 13:49

Thalley previously approved these changes Mar 26, 2025

View reviewed changes

cvinayak changed the title ~~Bluetooth: Controller: Fix Peripheral CIS sorted by CIG~~ Bluetooth: Controller: Fix incorrect CIS payload count at CIS Estab Mar 26, 2025

cvinayak mentioned this pull request Mar 27, 2025

Bluetooth: Controller: Fix assert when flushing multiple CISes #87280

Open

Tronil reviewed Mar 28, 2025

View reviewed changes

cvinayak dismissed Thalley’s stale review via 40ddb15 March 28, 2025 12:24

cvinayak requested review from Tronil and Thalley March 28, 2025 12:25

Thalley approved these changes Mar 28, 2025

View reviewed changes

Tronil approved these changes Mar 28, 2025

View reviewed changes

carlescufi approved these changes Mar 28, 2025

View reviewed changes

kartben merged commit dd1c394 into zephyrproject-rtos:main Mar 29, 2025
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: Controller: Fix incorrect CIS payload count at CIS Estab #87538

Bluetooth: Controller: Fix incorrect CIS payload count at CIS Estab #87538

cvinayak commented Mar 24, 2025 •

edited

Loading

Thalley Mar 25, 2025

cvinayak Mar 26, 2025

Thalley Mar 26, 2025

cvinayak Mar 26, 2025

cvinayak Mar 26, 2025

cvinayak Mar 26, 2025

Thalley commented Mar 26, 2025

codecoup-tester commented Mar 26, 2025

codecoup-tester commented Mar 26, 2025

cvinayak commented Mar 27, 2025

Tronil Mar 28, 2025

cvinayak Mar 28, 2025

Tronil Mar 28, 2025

Bluetooth: Controller: Fix incorrect CIS payload count at CIS Estab #87538

Bluetooth: Controller: Fix incorrect CIS payload count at CIS Estab #87538

Conversation

cvinayak commented Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thalley commented Mar 26, 2025

codecoup-tester commented Mar 26, 2025

codecoup-tester commented Mar 26, 2025

cvinayak commented Mar 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvinayak commented Mar 24, 2025 •

edited

Loading