Bluetooth: Controller: Separate Tx/Rx frame spacing #79113

cvinayak · 2024-09-27T13:20:56Z

Separate Tx/Rx frame spacing.

Blocked by defines in #79584

cvinayak · 2024-09-27T16:49:10Z

@Thalley any idea what happened in the bsim nrf5340 testing of unicast client in this PR?

Thalley · 2024-09-27T21:06:07Z

@Thalley any idea what happened in the bsim nrf5340 testing of unicast client in this PR?

Looks like one device gets a segmentation fault which triggers a CIS disconnect. The client, attempting to send, does not get the callback that the stream is stopped until after a few TX attempts it seems

cvinayak · 2024-09-28T05:39:34Z

@Thalley any idea what happened in the bsim nrf5340 testing of unicast client in this PR?

Looks like one device gets a segmentation fault which triggers a CIS disconnect. The client, attempting to send, does not get the callback that the stream is stopped until after a few TX attempts it seems

ok... it does not say which, app core or net core crashes... will try running at desk to determine the cause.

cvinayak · 2024-09-28T19:42:24Z

@Thalley any idea what happened in the bsim nrf5340 testing of unicast client in this PR?

Looks like one device gets a segmentation fault which triggers a CIS disconnect. The client, attempting to send, does not get the callback that the stream is stopped until after a few TX attempts it seems

ok... it does not say which, app core or net core crashes... will try running at desk to determine the cause.

The segmentation fault is due to having net_buf_alloc() call (which is no more has the blocking behavior) in the audio_timer_timeout() system work queue work in the samples/bluetooth/bap_unicast_server/src/main.c .

@Thalley The audio samples will need to be ported to gracefully handle net_buf_alloc() returning NULL if being called inside system work queue...

Thalley · 2024-09-30T07:20:29Z

@Thalley any idea what happened in the bsim nrf5340 testing of unicast client in this PR?

Looks like one device gets a segmentation fault which triggers a CIS disconnect. The client, attempting to send, does not get the callback that the stream is stopped until after a few TX attempts it seems

ok... it does not say which, app core or net core crashes... will try running at desk to determine the cause.

The segmentation fault is due to having net_buf_alloc() call (which is no more has the blocking behavior) in the audio_timer_timeout() system work queue work in the samples/bluetooth/bap_unicast_server/src/main.c .

@Thalley The audio samples will need to be ported to gracefully handle net_buf_alloc() returning NULL if being called inside system work queue...

Huh, weird, thought I had fixed all the K_FOREVER in the samples's workqueue callbacks. Will fix today

Thalley · 2024-09-30T09:14:30Z

@cvinayak #79187 should fix the remaining samples.

subsys/bluetooth/controller/Kconfig.ll_sw_split

subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio.c

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_conn.c

subsys/bluetooth/controller/ll_sw/ull_conn.c

Thalley · 2024-10-09T09:12:26Z

subsys/bluetooth/controller/ll_sw/ull_internal.h

Is 0 right here? Shouldn't it be 1?

0 is valid value and here it represents a 500 us unit. A minimum meaningful 1 byte data PDU (120us), hence 394 (120 + 120 + 154 tIFS) could in theory be a connection interval, hence not ruling out 500 us unit.

Hmm, 0 means 500us sounds weird to me :D If ms isn't possible to use, then perhaps these macros should be in us instead? It sounds weird that an interval of 0 is not in fact 0.

Thalley · 2024-10-09T09:15:12Z

tests/bsim/bluetooth/ll/conn/src/test_connect2.c

Hmm, this looks kind of odd. I guess it makes sense to send a notification every 1ms when testing with 1ms conn interval, but why are we sending every 1 second if not? What if the interval is 10ms, shouldn't we send every 10ms then? What if the connection interval is 4 seconds?

Shouldn't we just ignore the Kconfig values and just always sleep for the current connection interval?

The original (probably the very first) bsim test simulated a peripheral HR service where the notification were 1 second apart. Do not want to change the existing test behavior in this PR.

Only the verification that the number of notifications received in the 3 seconds of simulation has been updated.

Do not want to change the existing test behavior in this PR.

We can create a PR modifying the behavior to use the connection interval first, and then you wouldn't need this change at all ;)

subsys/bluetooth/controller/Kconfig.ll_sw_split

Minor updates and correction to code comments. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Add defines for radio timer capture and compare indices used in the nRF Radio HAL implementation. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Add radio timer ISR usage support. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Introduce deferred ACL Tx packet transmission setup by radio, so that an enqueued ACL Data packet can be transmitted with the shortest latency. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Added test to verify deferred ACL Tx transmission setup, such that GATT discovery completes with least latency and GATT notifications can be received earliest after connection establishment. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Separate Tx/Rx frame spacing. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

1 ms connection interval support. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Test 1ms connection interval support. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Thalley

As usual, my reviews for the controller does not verify the correctness of the implementation as I don't know all the details, but CI is passing and the changes look OK to me

Thalley · 2024-10-21T09:39:02Z

subsys/bluetooth/controller/ll_sw/pdu.h

 /* Event interframe timings */
 #define EVENT_IFS_US            150
+/* Event interframe timings, default */
+#define EVENT_IFS_DEFAULT_US    EVENT_IFS_US
 /* Standard allows 2 us timing uncertainty inside the event */
 #define EVENT_IFS_MAX_US        (EVENT_IFS_US + EVENT_CLOCK_JITTER_US)


What's the purpose of the duplicated #define?

If we intend to keep it, should that be used for EVENT_IFS_MAX_US since that refers to a "standard"?

Will these 2 ever be different?

Looks like there is inconsistent use of EVENT_IFS_US, EVENT_IFS_MAX_US, + 4, - 4andEVENT_CLOCK_JITTER_US` .... this needs clean, but out side the scope of this PR.

cvinayak marked this pull request as ready for review September 28, 2024 19:42

zephyrbot added platform: nRF BSIM Nordic Semiconductors, nRF BabbleSim area: Bluetooth area: Bluetooth Controller labels Sep 28, 2024

zephyrbot requested review from aescolar, carlescufi, erbr-ot, kruithofa, mtpr-ot, thoh-ot and wopu-ot September 28, 2024 19:43

zephyrbot assigned aescolar Sep 28, 2024

cvinayak assigned cvinayak and unassigned aescolar Sep 28, 2024

cvinayak force-pushed the github_frame_spacing branch 3 times, most recently from d19fabd to f49c8c9 Compare October 9, 2024 03:46

Thalley requested changes Oct 9, 2024

View reviewed changes

cvinayak force-pushed the github_frame_spacing branch 2 times, most recently from b594237 to 8c6b473 Compare October 11, 2024 08:06

cvinayak force-pushed the github_frame_spacing branch 2 times, most recently from 6dad110 to e7aebe2 Compare October 18, 2024 12:18

cvinayak added 2 commits October 18, 2024 16:25

Bluetooth: Controller: Minor updates to code comments

6c4e5b5

Minor updates and correction to code comments. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Bluetooth: Controller: Defines for radio timer capture/compare indices

0f2473b

Add defines for radio timer capture and compare indices used in the nRF Radio HAL implementation. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

cvinayak added 6 commits October 18, 2024 16:25

Bluetooth: Controller: Add radio timer ISR usage support

848d71e

Add radio timer ISR usage support. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Bluetooth: Controller: Introduce deferred ACL Tx packet transmission

69332fa

Introduce deferred ACL Tx packet transmission setup by radio, so that an enqueued ACL Data packet can be transmitted with the shortest latency. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Bluetooth: Controller: Separate Tx/Rx frame spacing

f732fa2

Separate Tx/Rx frame spacing. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Bluetooth: Controller: 1 ms connection

52d1ba2

1 ms connection interval support. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

tests: bsim: Bluetooth: Test 1ms connection interval support

a279993

Test 1ms connection interval support. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

cvinayak force-pushed the github_frame_spacing branch from e7aebe2 to a279993 Compare October 18, 2024 14:31

Thalley approved these changes Oct 21, 2024

View reviewed changes

cvinayak added this to the v4.0.0 milestone Oct 24, 2024

carlescufi approved these changes Oct 24, 2024

View reviewed changes

fabiobaltieri merged commit af0aeb3 into zephyrproject-rtos:main Oct 24, 2024
26 checks passed

Bluetooth: Controller: Separate Tx/Rx frame spacing #79113

Bluetooth: Controller: Separate Tx/Rx frame spacing #79113

Uh oh!

Conversation

cvinayak commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cvinayak commented Sep 27, 2024

Uh oh!

Thalley commented Sep 27, 2024

Uh oh!

cvinayak commented Sep 28, 2024

Uh oh!

cvinayak commented Sep 28, 2024

Uh oh!

Thalley commented Sep 30, 2024

Uh oh!

Thalley commented Sep 30, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Thalley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cvinayak commented Sep 27, 2024 •

edited

Loading