Bluetooth: Controller: Fix LLCP event_counter value used during prepare

[cvinayak]

Fix LLCP to use event_counter value used during prepare to compare instant to be the value as when in prepare and not incorrectly use a stale value (when two radio events overlap).

[Copilot]

Pull Request Overview

This PR fixes the LLCP instant comparison to use the event counter value captured during the prepare phase, preventing stale counter usage when radio events overlap.

• Introduce ull_conn_event_counter_at_prepare() and replace direct calls to ull_conn_event_counter() in all LLCP instant checks.
• Adjust channel map update instant to include slave latency.
• Add declaration and implementation of the prepare-phase event counter helper.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
subsys/bluetooth/controller/ll_sw/ull_llcp_phy.c	Replaced instant checks to use the new prepare-phase counter helper.
subsys/bluetooth/controller/ll_sw/ull_llcp_conn_upd.c	Swapped out raw event counter calls for prepare-phase variant in connection update.
subsys/bluetooth/controller/ll_sw/ull_llcp_chmu.c	Added slave latency to channel map instant; switched runtime checks to use prepare-phase counter.
subsys/bluetooth/controller/ll_sw/ull_llcp_cc.c	Updated CIS create instant checks to utilize the prepare-phase counter.
subsys/bluetooth/controller/ll_sw/ull_conn_internal.h	Declared ull_conn_event_counter_at_prepare().
subsys/bluetooth/controller/ll_sw/ull_conn.c	Implemented ull_conn_event_counter_at_prepare() and replaced a counter usage in parameter update.

Comments suppressed due to low confidence (2)

subsys/bluetooth/controller/ll_sw/ull_conn_internal.h:87

• Add a brief comment describing that this returns the event counter value as captured during the prepare phase (event_counter + latency_prepare + lazy).

uint16_t ull_conn_event_counter_at_prepare(struct ll_conn *conn);

subsys/bluetooth/controller/ll_sw/ull_conn.c:2180

• No unit tests appear to cover ull_conn_event_counter_at_prepare. Adding tests for various latency_prepare and llcp.prep.lazy scenarios will help validate its behavior.

uint16_t ull_conn_event_counter_at_prepare(struct ll_conn *conn)

[Copilot]

This manual calculation may not match the helper's logic (which adds latency_prepare and lazy). Consider using ull_conn_event_counter_at_prepare(conn) + CHMU_INSTANT_DELTA to keep consistency across instant calculations.

Suggested change

	ctx->data.chmu.instant = ull_conn_event_counter(conn) + conn->lll.latency +
	CHMU_INSTANT_DELTA;
	ctx->data.chmu.instant = ull_conn_event_counter_at_prepare(conn) + CHMU_INSTANT_DELTA;

[Thalley]

Minor suggestion, but otherwise LGTM

[thoh-ot]

Need time to look at this.

[cvinayak]

Need time to look at this.

The issue was discovered running the central_gatt_write and peripheral_gatt_write samples with conn update procedures that was added in #92572. The sample can be run in bsim to reproduce supervision timeout due to incorrect instant being checked when consecutive connection events (prepare requesting preemption) overlap under high throughput scenario.

[thoh-ot]

This PR removes fixes that were made as part of #52822 to stabilize CIS therefore I'm not really fond of it.

ull_conn_event_counter_at_prepare() is just the prepare path (TX path) of ull_conn_event_counter(), so something must be wrong with using ull_ref_get() for the post-prepare path (RX path) of ull_conn_event_counter().

I'm also a bit curious to why the LLCP unit-tests are passing with this change, either they aren't being run or they don't catch this

[cvinayak]

This PR removes fixes that were made as part of #52822 to stabilize CIS therefore I'm not really fond of it.

/* Check if this connection event is where we need to start the CIS */

Above is the added fixes/improvement comment, but which can execute anyway inside a connection event anywhere upto 150 us before the next connection event. During the overlap of preemption timeout duration, the ull_ref_get() value used in rp_cc_check_instant() call done insde the event (not a prepare) is in that corner case.

I'm also a bit curious to why the LLCP unit-tests are passing with this change, either they aren't being run or they don't catch this.

I do not think there is a test to cover the fixes/improvements added in #52822 where the call to rp_cc_check_instant() before next event and after prepare unto preempt is tested. The change in #52822 seems like a workaround compared to other control procedures that check instant in prepare and not during PDU reception.

[thoh-ot]

This PR removes fixes that were made as part of #52822 to stabilize CIS therefore I'm not really fond of it.

/* Check if this connection event is where we need to start the CIS */

Above is the added fixes/improvement comment, but which can execute anyway inside a connection event anywhere upto 150 us before the next connection event. During the overlap of preemption timeout duration, the ull_ref_get() value used in rp_cc_check_instant() call done insde the event (not a prepare) is in that corner case.

I'm also a bit curious to why the LLCP unit-tests are passing with this change, either they aren't being run or they don't catch this.

I do not think there is a test to cover the fixes/improvements added in #52822 where the call to rp_cc_check_instant() before next event and after prepare unto preempt is tested. The change in #52822 seems like a workaround compared to other control procedures that check instant in prepare and not during PDU reception.

The original commit has a bit more explaining: 4f5b711

It's to handle the case where the CIS begins in the next event.

[cvinayak]

It's to handle the case where the CIS begins in the next event.

Correction, i.e it is an improvement to handle cases where CIS begins in the current ACL event (and before next ACL event) where the CIS_IND was received. Without the improvement, the CIS will still be established in the next ACL event after calculating any instant latency and appropriately compensating for elapsed CIS intervals and CIS offsets (hence no CI tests failed with/without this improvement)

I have added back the improvement with renamed function names to bring clarity to when the instant is checked, state machine run at prepare versus on reception of CIS_IND PDU.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Thalley]

Is there any way to test this fix? CI passes both before and after

[thoh-ot]

I'm wondering if the usefulness of ull_conn_event_counter() is gone, now we know that the ull_ref_get() logic is flawed...

None the less, I'll approve these changes.

[cvinayak]

I'm wondering if the usefulness of ull_conn_event_counter() is gone, now we know that the ull_ref_get() logic is flawed...

None the less, I'll approve these changes.

ull_ref_get() was intended to be used to count references when same state/role events overlap, like ACL connections occupying entire event length, directed advertising and continuous scanning so that state/role can be gracefully disabled when reference count reaches zero; definitely not to identify being inside an event, though it works. Yes, ull_conn_event_counter() is not useful in providing the event_counter, it is better to use the right event_counter value inlined based on where in the timeline the event_counter is needed.

That said, current use of ull_conn_event_counter() to assign instant value in initiating PDUs being in Central role is acceptable for now.

[cvinayak]

Is there any way to test this fix? CI passes both before and after

Testable in unit test (otherwise flakky to test with a modified tester Controller implementation under bsim).

This PR is not fixing anything fatal, I can get back to adding unit test after some quiet period!