Bluetooth: Controller: Add support for scanning complete periodic adv trains #37943

andrzej-kaczmarek · 2021-08-26T00:25:18Z

This adds support to scan complete periodic adv trains including mixed scheduling from ULL/LLL if necessary.

subsys/bluetooth/controller/ll_sw/ull.c

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_sync_internal.h

subsys/bluetooth/controller/ll_sw/lll.h

samples/bluetooth/direction_finding_connectionless_rx/boards/nrf52833dk_nrf52833.conf

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

cvinayak · 2021-08-26T05:56:40Z

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

Rename is_aux_sched to aux_lll_sched in LLL context for consistency.

Suggested change

if (ftr->aux_lll_sched) {

lll->aux_lll_sched = ftr->aux_lll_sched;

if (ftr->aux_lll_sched) {

ppryga-nordic

I'm wondering about one situation. What in case the scanner is synchronized to periodic advertisement, there is a chain of e.g. 6 PDUs. 4th AUX_CHAIN_IND PDU is received with wrong CRC. node_rx is set to type: NODE_RX_TYPE_EXT_AUX_RELEASE but no report is generated to inform host about incomplete data and no more IQ reports.

See 7.7.65.21 LE Connectionless IQ Report event:

This event is also used by the Controller to report that it has insufficient
resources to report IQ samples for all received Constant Tone Extensions and
has failed to sample at least once. In this case Packet_Status shall be set to
0xFF and Sample_Count to 0x00.

subsys/bluetooth/controller/ll_sw/lll_sync.h

ppryga-nordic · 2021-08-26T07:20:15Z

subsys/bluetooth/controller/ll_sw/lll_sync.h

This function declaration should go into nordic/lll/lll_sync_interna.h. There is no such file, so please create a new one.

The function may be changed to static. It is used in lll_sync.c only.

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

ppryga-nordic · 2021-08-26T08:31:43Z

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

err here overlaps with err declared at the beginning of the function.

what about this?

subsys/bluetooth/controller/hci/hci.c

samples/bluetooth/direction_finding_connectionless_rx/boards/nrf52833dk_nrf52833.conf

subsys/bluetooth/controller/ll_sw/ull.c

cvinayak · 2021-08-26T10:39:20Z

I'm wondering about one situation. What in case the scanner is synchronized to periodic advertisement, there is a chain of e.g. 6 PDUs. 4th AUX_CHAIN_IND PDU is received with wrong CRC. node_rx is set to type: NODE_RX_TYPE_EXT_AUX_RELEASE but no report is generated to inform host about incomplete data and no more IQ reports.

See 7.7.65.21 LE Connectionless IQ Report event:

This event is also used by the Controller to report that it has insufficient
resources to report IQ samples for all received Constant Tone Extensions and
has failed to sample at least once. In this case Packet_Status shall be set to
0xFF and Sample_Count to 0x00.

Yes, I noticed this when making changes towards NODE_RX_TYPE_RELEASE handling in ull.c. We have to address the incomplete data status in separate PR as a bug fix.

ppryga-nordic · 2021-08-26T12:22:54Z

The PR has to be merged after: #37930

ppryga-nordic · 2021-08-26T12:40:45Z

I've just spot one more thing.
There is not enough node_rx buffers for LLL to ULL when per. adv. scan and direction finding are enabled.
Default value of BT_CTLR_ADV_EXT_RX_CNT is set to 0 if CONFIG_BT_MAX_CONN is not set (selected by CONFIG_BT_PERIPHERAL or CONFIG_BT_CENTRAL) or 1 in other case.

Max number of node_rx required to single per. adv. event (when DF is also enabled) is 16x2 (per. adv. reports, cte IQ reports).
Including shortest possible duration between chained PDUs to be T_MAFS, single event may last ~8.5ms (16 * ~530us).

I suggest to change the BT_CTLR_ADV_EXT_RX_CNT to 32. @cvinayak what is your opinion?

This is solved by commit: a1b8d14

cvinayak · 2021-08-26T12:44:21Z

I suggest to change the BT_CTLR_ADV_EXT_RX_CNT to 32. @cvinayak what is your opinion?

Set the required number conditionally based on features enabled (Do not want increased RAM when Periodic and/or DF is not used).

andrzej-kaczmarek · 2021-08-26T14:41:06Z

@cvinayak @ppryga I pushed additional commit that addresses main issue i.e. moving sync specific code from lll_scan_aux to lll_sync. There's just one extra branch in prepare_cb that depends on scan/sync context since the same ticker is used by both, but it can be compiled out if periodic advertising sync is not enabled as it just calls code from lll_sync if needed. I'll start addressing remaining comments now.

cvinayak

Did a quick pass, looks good. Will do some testing and review early tomorrow, and will get back.

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

ppryga-nordic

I went over last commit very briefly. I'll continue that in the morning.

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

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

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

ppryga-nordic · 2021-08-27T07:26:31Z

No more comments from my site.

andrzej-kaczmarek · 2021-08-27T10:11:00Z

@ppryga @cvinayak All comments are fixed, I hope I did not miss anything. I also added few #ifdefs in ull_scan_aux.c to compile out code for sync paths if periodic advertising is not enabled. PR is now also rebased on latest main.

ppryga-nordic

There are some not addressed comments.

ppryga-nordic · 2021-08-27T11:05:58Z

subsys/bluetooth/controller/ll_sw/lll_sync.h

The function may be changed to static. It is used in lll_sync.c only.

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

subsys/bluetooth/controller/hci/hci.c

ppryga-nordic · 2021-08-27T11:37:30Z

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

what about this?

cvinayak

No more stressing change requests from my side. I could not get to testing, but will do when I can get to it in my todo. Any functional issues can be address when discovered as bug fixes.

cvinayak · 2021-08-27T12:18:13Z

subsys/bluetooth/controller/ll_sw/ull.c

Thinking aloud, no change required. Application ecosystems with known max data length may want the count to be configurable to reduce RAM.

ppryga-nordic

Tested with use of direction_finding_connectionless_rx. Changes approved.

Periodic advertising reports can have 255 bytes of payload so need to use that value by default if periodic advertising is enabled. Signed-off-by: Andrzej Kaczmarek <[email protected]>

We should decide on flush immediately vs. from disabled_cb based on ull_hdr reference count instead of last rxd node - if ull_hdr has non-zero ref, then done event is still pending and we should flush from there. Signed-off-by: Andrzej Kaczmarek <[email protected]>

aux->rx_last cannot be NULL since it's always set after acquire to a valid node so the flow in flush() can be simplified. rx parameter is only used to update PDU chain, then it's overwritten, so we can instead update PDU chain in caller since there's only one place when this should happen. Signed-off-by: Andrzej Kaczmarek <[email protected]>

This adds complete support for scanning for periodic advertising trains. AUX_SYNC_IND is always scheduled from ULL as usual, then code for aux scanning is reused to allow for AUX_CHAIN_IND scanning scheduled from both ULL and LLL, depending on AuxPtr. Signed-off-by: Andrzej Kaczmarek <[email protected]>

We use 1st node enqueued in aux context to retrieve lll scan/sync struct, but that only works if we buffer PDUs in aux context. It's better to store parent lll struct as explicit member in aux context as this also works if we skip buffering. Signed-off-by: Andrzej Kaczmarek <[email protected]>

Periodic advertising reports can be reated directly from single PDU as they do not require any information from superior PDU, so we can dispatch them immediately instead of buffering in aux context and flushing at the end of chain. This also resolves proper order of Periodic advertising and IQ reports. Signed-off-by: Andrzej Kaczmarek <[email protected]>

AdvA, TargetA, ADI and SyncInfo are RFU in periodic advertising PDUs so we should ignore them when present in PDU. Signed-off-by: Andrzej Kaczmarek <[email protected]>

Periodic advertising PDUs are now dispatched immediately one by one (i.e. without list of PDUs as when flushed from aux context) so we do not need to iterate such a list. Signed-off-by: Andrzej Kaczmarek <[email protected]>

This updates DF to properly receive CTE in per adv chains scanning. Signed-off-by: Andrzej Kaczmarek <[email protected]>

Periodic advertising train scanner implemented in lll_scan_aux adds lots of branches that cannot be compiled out with periodic advertising sync disabled. This commit moves sync parts of the code from lll_scan_aux to lll_sync. Signed-off-by: Andrzej Kaczmarek <[email protected]>

This seems not to be needed anymore, it's only written. Signed-off-by: Andrzej Kaczmarek <[email protected]>

We need more RX nodes when scanning either extended/periodic advertising trains and CTE samples so pudate those values based on observer and DF features enabled. The number of nodes for non-DF allows to scan complete chain for each aux scan set (assuming max data length and optimal fragmentation by advertiser), for DF it allows to scan max possible PDUs and CTE samples. Signed-off-by: Andrzej Kaczmarek <[email protected]>

This was changes when sync chain handling was done in lll_scan_aux, we can now revert to original code. Signed-off-by: Andrzej Kaczmarek <[email protected]>

Add todos for few useful improvements that can be done later. Signed-off-by: Andrzej Kaczmarek <[email protected]>

'err' is already defined in parent scope, we can use. Just need to set it back to 0 before returning from function. Signed-off-by: Andrzej Kaczmarek <[email protected]>

andrzej-kaczmarek · 2021-08-27T13:46:51Z

rebased due to conflict after #37934 was merged

andrzej-kaczmarek requested review from cvinayak and ppryga-nordic August 26, 2021 00:25

andrzej-kaczmarek requested review from Vudentz, carlescufi, jhedberg, joerchan, kruithofa and thoh-ot as code owners August 26, 2021 00:25

github-actions bot added area: Bluetooth area: Bluetooth Controller area: Samples Samples labels Aug 26, 2021

andrzej-kaczmarek force-pushed the bt-per-adv branch from 1cf6837 to 8583311 Compare August 26, 2021 00:49

cvinayak requested changes Aug 26, 2021

View reviewed changes

cvinayak added this to the v2.7.0 milestone Aug 26, 2021

ppryga-nordic suggested changes Aug 26, 2021

View reviewed changes

zephyrbot requested review from Thalley and asbjornsabo August 26, 2021 14:15

zephyrbot assigned cvinayak Aug 26, 2021

andrzej-kaczmarek force-pushed the bt-per-adv branch from b002c3e to 22feb53 Compare August 26, 2021 14:37

cvinayak reviewed Aug 26, 2021

View reviewed changes

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_sync.c Outdated Show resolved Hide resolved

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_sync.c Outdated Show resolved Hide resolved

ppryga-nordic suggested changes Aug 26, 2021

View reviewed changes

cvinayak reviewed Aug 27, 2021

View reviewed changes

andrzej-kaczmarek force-pushed the bt-per-adv branch from 22feb53 to a1b8d14 Compare August 27, 2021 10:09

andrzej-kaczmarek force-pushed the bt-per-adv branch from a1b8d14 to 1c0ed5a Compare August 27, 2021 11:07

ppryga-nordic reviewed Aug 27, 2021

View reviewed changes

cvinayak approved these changes Aug 27, 2021

View reviewed changes

ppryga-nordic approved these changes Aug 27, 2021

View reviewed changes

andrzej-kaczmarek added 15 commits August 27, 2021 15:43

Bluetooth: Controller: Set default HCI event size with per adv

5d40986

Periodic advertising reports can have 255 bytes of payload so need to use that value by default if periodic advertising is enabled. Signed-off-by: Andrzej Kaczmarek <[email protected]>

Bluetooth: Controller: Fix parsing sync report PDU

dfa6344

AdvA, TargetA, ADI and SyncInfo are RFU in periodic advertising PDUs so we should ignore them when present in PDU. Signed-off-by: Andrzej Kaczmarek <[email protected]>

Bluetooth: Controller: Rework support for periodic adv reports

001d902

Periodic advertising PDUs are now dispatched immediately one by one (i.e. without list of PDUs as when flushed from aux context) so we do not need to iterate such a list. Signed-off-by: Andrzej Kaczmarek <[email protected]>

Bluetooth: Controller: Fix DF for per adv chains scanning

8f695da

This updates DF to properly receive CTE in per adv chains scanning. Signed-off-by: Andrzej Kaczmarek <[email protected]>

Bluetooth: Controller: Remove unused flag

6763c9b

This seems not to be needed anymore, it's only written. Signed-off-by: Andrzej Kaczmarek <[email protected]>

Bluetooth: Controller: Make lll_create_iq_report static again

70805cb

This was changes when sync chain handling was done in lll_scan_aux, we can now revert to original code. Signed-off-by: Andrzej Kaczmarek <[email protected]>

Bluetooth: Controller: Add todos for per adv chain scan

452a259

Add todos for few useful improvements that can be done later. Signed-off-by: Andrzej Kaczmarek <[email protected]>

Bluetooth: Controller: Remove redundant local variable

844712f

'err' is already defined in parent scope, we can use. Just need to set it back to 0 before returning from function. Signed-off-by: Andrzej Kaczmarek <[email protected]>

andrzej-kaczmarek force-pushed the bt-per-adv branch from 044e0bb to 844712f Compare August 27, 2021 13:46

nashif merged commit f2f99c0 into zephyrproject-rtos:main Aug 27, 2021

	if (ftr->aux_lll_sched) {
	lll->aux_lll_sched = ftr->aux_lll_sched;
	if (ftr->aux_lll_sched) {

Bluetooth: Controller: Add support for scanning complete periodic adv trains #37943

Bluetooth: Controller: Add support for scanning complete periodic adv trains #37943

Uh oh!

Conversation

andrzej-kaczmarek commented Aug 26, 2021

Uh oh!

Uh oh!

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!

ppryga-nordic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cvinayak commented Aug 26, 2021

Uh oh!

ppryga-nordic commented Aug 26, 2021

Uh oh!

ppryga-nordic commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cvinayak commented Aug 26, 2021

Uh oh!

andrzej-kaczmarek commented Aug 26, 2021

Uh oh!

cvinayak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ppryga-nordic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ppryga-nordic commented Aug 27, 2021

Uh oh!

andrzej-kaczmarek commented Aug 27, 2021

Uh oh!

ppryga-nordic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cvinayak left a comment

Choose a reason for hiding this comment

ppryga-nordic commented Aug 26, 2021 •

edited

Loading