Bluetooth: controller: nRF5: Back-to-Back Radio Tx interface #33118

cvinayak · 2021-03-08T06:45:23Z

Add Radio interface to perform back-to-back transmit of PDU
with a configurable inter frame spacing.

Signed-off-by: Vinayak Kariappa Chettimada [email protected]

andrzej-kaczmarek

lgtm

tested with #31875

kruithofa · 2021-03-15T14:29:22Z

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

To improve readability: can we use define's instead of hardcoded 1/0

kruithofa · 2021-03-15T14:29:29Z

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

To improve readability: can we use define's instead of hardcoded 1/0

kruithofa · 2021-03-15T14:29:52Z

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

To improve readability: can we use define's instead of hardcoded 1/0

kruithofa · 2021-03-15T14:34:29Z

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

Is the comment still correct? If (dir_curr !=0) then no such assumption is made, but flags_curr is used, right?

andrzej-kaczmarek

minor comments, verified again with #31875 and works fine

andrzej-kaczmarek · 2021-04-07T15:48:01Z

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

SW_SWITCH_DIR_* used for both TX and RX would be less confusing

These are aliases to hardcoded 0 or 1, used to convey the parameter name supplied to inlined sw_switch file static function. They are not intended to be used as parameter values to other functions.

Do you mean to rename to

Suggested change

#define SW_SWITCH_PREV_RX 0

#define SW_SWITCH_PREV_DIR_RX 0

#define SW_SWITCH_NEXT_DIR_RX 0

#define SW_SWITCH_PREV_DIR_TX 1

#define SW_SWITCH_NEXT_DIR_TX 1

andrzej-kaczmarek · 2021-04-07T15:49:42Z

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

better use PHY_LEGACY or even better PHY_1M instead since this is actual value passed from LLL

Same here, macros are used as readable string (instead of using defines) which would convey the parameters purpose. Otherwise, the call at line 607 will look like:

sw_switch(SW_SWITCH_DIR_TX, SW_SWITCH_DIR_RX, PHY_1M, SW_SWITCH_PREV_FLAGS_DONTCARE, phy_rx, SW_SWITCH_NEXT_FLAGS_DONTCARE);

andrzej-kaczmarek · 2021-04-07T15:52:42Z

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

could be defined in pdu.h and eventually also used in LLL since flags are magic values there as well

Again the macros are replacement to hardcoded values passed to sw_switch function which is vendor specific interface, not sure if other vendors will use 0 as the dont care values.

Change other magic values is outside the scope of this PR.

cvinayak

This PR depends on #31336 as that PR has resolved the conflicts of having a separate _DPPI.h header file introduced in #30732.

I have resolved conflicts in this PR over #31336 in my another branch, hence it will be less work if #31336 is merged first.

cvinayak · 2021-04-08T05:49:20Z

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

These are aliases to hardcoded 0 or 1, used to convey the parameter name supplied to inlined sw_switch file static function. They are not intended to be used as parameter values to other functions.

Do you mean to rename to

Suggested change

#define SW_SWITCH_PREV_RX 0

#define SW_SWITCH_PREV_DIR_RX 0

#define SW_SWITCH_NEXT_DIR_RX 0

#define SW_SWITCH_PREV_DIR_TX 1

#define SW_SWITCH_NEXT_DIR_TX 1

cvinayak · 2021-04-08T06:03:18Z

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

Again the macros are replacement to hardcoded values passed to sw_switch function which is vendor specific interface, not sure if other vendors will use 0 as the dont care values.

Change other magic values is outside the scope of this PR.

cvinayak · 2021-04-08T06:05:04Z

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

Same here, macros are used as readable string (instead of using defines) which would convey the parameters purpose. Otherwise, the call at line 607 will look like:

sw_switch(SW_SWITCH_DIR_TX, SW_SWITCH_DIR_RX, PHY_1M, SW_SWITCH_PREV_FLAGS_DONTCARE, phy_rx, SW_SWITCH_NEXT_FLAGS_DONTCARE);

carlescufi · 2021-04-19T13:17:52Z

@andrzej-kaczmarek can you please take another look?

cvinayak · 2021-04-19T14:59:46Z

This needs a rebase and also would like #31336 to go first to avoid more work resolving conflicts

Enclose macro parameters in paranthesis to avoid ambiguous macro expansions. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Minor indentation change and replaced if-then-else-if clause with toggle implementation. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

nRF53 implementation of sw_switch always requires Radio End event, hence optimize out redundant code due to explicit use of radio_tmr_end_capture. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Add Radio interface to perform back-to-back transmit of PDU with a configurable inter frame spacing. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

cvinayak requested a review from andrzej-kaczmarek March 8, 2021 06:45

github-actions bot added area: Bluetooth area: Bluetooth Controller labels Mar 8, 2021

cvinayak mentioned this pull request Mar 8, 2021

Bluetooth: controller: Add initial support for chaining in periodic advertising #31875

Merged

andrzej-kaczmarek approved these changes Mar 9, 2021

View reviewed changes

cvinayak marked this pull request as ready for review March 12, 2021 09:36

cvinayak requested review from carlescufi, kruithofa and thoh-ot as code owners March 12, 2021 09:36

kruithofa suggested changes Mar 15, 2021

View reviewed changes

cvinayak force-pushed the github_b2b_tx branch from 5b362d6 to bf39390 Compare March 19, 2021 09:21

cvinayak requested review from andrzej-kaczmarek and kruithofa March 19, 2021 09:34

cvinayak self-assigned this Apr 6, 2021

cvinayak added the Enhancement Changes/Updates/Additions to existing features label Apr 6, 2021

andrzej-kaczmarek reviewed Apr 7, 2021

View reviewed changes

cvinayak commented Apr 8, 2021

View reviewed changes

cvinayak added 2 commits April 21, 2021 18:42

Bluetooth: controller: Enclose macro parameters in paranthesis

0471175

Enclose macro parameters in paranthesis to avoid ambiguous macro expansions. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Bluetooth: controller: Minor indentation and HAL code change

81b912a

Minor indentation change and replaced if-then-else-if clause with toggle implementation. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

cvinayak force-pushed the github_b2b_tx branch from bf39390 to ada3f61 Compare April 21, 2021 13:16

cvinayak added 2 commits April 21, 2021 18:52

Bluetooth: controller: nRF53 sw_switch always required Radio End

d5c621e

nRF53 implementation of sw_switch always requires Radio End event, hence optimize out redundant code due to explicit use of radio_tmr_end_capture. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Bluetooth: controller: nRF5: Back-to-Back Radio Tx interface

f597fa4

Add Radio interface to perform back-to-back transmit of PDU with a configurable inter frame spacing. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

cvinayak force-pushed the github_b2b_tx branch from ada3f61 to f597fa4 Compare April 21, 2021 13:23

kruithofa approved these changes Apr 21, 2021

View reviewed changes

andrzej-kaczmarek approved these changes Apr 21, 2021

View reviewed changes

carlescufi merged commit a379196 into zephyrproject-rtos:master Apr 22, 2021

-#define SW_SWITCH_PREV_RX             0
+#define SW_SWITCH_PREV_DIR_RX             0
+#define SW_SWITCH_NEXT_DIR_RX             0
+#define SW_SWITCH_PREV_DIR_TX             1
+#define SW_SWITCH_NEXT_DIR_TX             1

Bluetooth: controller: nRF5: Back-to-Back Radio Tx interface #33118

Bluetooth: controller: nRF5: Back-to-Back Radio Tx interface #33118

Uh oh!

Conversation

cvinayak commented Mar 8, 2021

Uh oh!

andrzej-kaczmarek 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrzej-kaczmarek 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!

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!

cvinayak 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!

Choose a reason for hiding this comment

Uh oh!

carlescufi commented Apr 19, 2021

Uh oh!

cvinayak commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cvinayak commented Apr 19, 2021 •

edited

Loading