samples: Bluetooth: observer: Extended Scanning on BBC Micro Bit board #95191
Conversation
cvinayak
force-pushed
the
github_observer_bbc_microbit
branch
from
c48bb97 to
1f04d78
Compare
zephyrbot
added
area: Bluetooth Host
zephyrbot
requested review from
HaavardRei,
JarmouniA,
PavelVPV,
Thalley,
alwa-nordic,
carlescufi,
hermabe,
jhedberg,
kartben,
nashif,
ppryga,
rugeGerritsen,
sjanc,
thoh-ot and
wopu-ot
cvinayak
force-pushed
the
github_observer_bbc_microbit
branch
2 times, most recently
from
449ff5e to
05a4f31
Compare
| .window = BT_GAP_SCAN_FAST_WINDOW, | ||
| .options = BT_LE_SCAN_OPT_NONE, | ||
| .interval = BT_GAP_MS_TO_SCAN_INTERVAL(10U), | ||
| .window = BT_GAP_MS_TO_SCAN_INTERVAL(10U), |
There was a problem hiding this comment.
I suppose this should be BT_GAP_MS_TO_SCAN_WINDOW()
cvinayak
force-pushed
the
github_observer_bbc_microbit
branch
2 times, most recently
from
f1f3c77 to
b524daf
Compare
There was a problem hiding this comment.
BBC micro bit v1, in itself is deprecated, do not see much value of this PR for users.
I use nRF51, nrf52, nrf53 and nrf54 as sanity test on target as they are all of different CPU speeds, and help uncover any context safety issue where ISRs have different latencies and changes when they step/pre-empt each other.
Unless this will be interesting for hobbyists to optimize implementation and make extended scanning (not assert) work with reduced CPU use; I leave to the reviewers opinion, I can close this PR otherwise.
From a more objective perspective, as long as the board definition is part of the upstream tree, I think it makes sense to take support for it seriously. From a subjective perspective, I have many of these boards still lying around, so it's nice to be able to play with them and use them for testing :) |
Very well, the BBC micro bit board can be used to scan extended advertising (assertions can be worked around, can be used as Auracast finder etc.). I have appended the assertion as known issue here: #82399 |
| tags: bluetooth | ||
| extra_args: | ||
| - CONF_FILE="prj_extended.conf" | ||
| - EXTRA_CONF_FILE=overlay_bbc_microbit-bt_ll_sw_split.conf |
There was a problem hiding this comment.
It's kind of odd to have a board specific overlay, instead of having a file in the boards directory, although I kind of understand why, given the limitations of the build system.
It's also somewhat odd that overlay_bbc_microbit-bt_ll_sw_split.conf specifically is used (only with) prj_extended.conf, but the former has # CONFIG_BT_EXT_ADV=y.
Since overlay_bbc_microbit-bt_ll_sw_split.conf is an overlay, does it need values to include values that are otherwise set by prj.conf or prj_extended.conf?
There was a problem hiding this comment.
It's kind of odd to have a board specific overlay
I am moving away from using board specific overlay for samples (many samples seems to not have board specific overlays) due to possibility of supporting different Controller implementations downstream (though nRF51 will not be supported by Nordic downstream). Also, there are too many boards in Zephyr, and board specific overlay will not scale.
Since overlay_bbc_microbit-bt_ll_sw_split.conf is an overlay, does it need values to include values that are otherwise set by prj.conf or prj_extended.conf?
in the case of BBC Micro Bit overlay the Scan Data Length is reduced to 192 (from 1650) bytes and rx buffer counts lowered due to RAM restrictions.
There was a problem hiding this comment.
I am moving away from using board specific overlay for samples (many samples seems to not have board specific overlays) due to possibility of supporting different Controller implementations downstream (though nRF51 will not be supported by Nordic downstream). Also, there are too many boards in Zephyr, and board specific overlay will not scale.
I think I'm missing something here - Aren't you adding a board specific overlay in this PR?
in the case of BBC Micro Bit overlay the Scan Data Length is reduced to 192 (from 1650) bytes and rx buffer counts lowered due to RAM restrictions.
This overlay does way more than that though :o
There was a problem hiding this comment.
I think I'm missing something here - Aren't you adding a board specific overlay in this PR?
board + Controller (choice) implementation overlay
This overlay does way more than that though :o
Yes, overlays to reduce code and RAM size, and use Controller Kconfig variants to workaround some CPU usage overheads/latencies (at the expense of reduced IRQ priorities available for application peripherals). Default Kconfigs for nRF51 cover BT Spec v4.2 features only.
| # Enable thread analysis | ||
| CONFIG_THREAD_ANALYZER=y | ||
| CONFIG_THREAD_ANALYZER_AUTO=y | ||
| CONFIG_THREAD_ANALYZER_AUTO_INTERVAL=5 | ||
| CONFIG_THREAD_NAME=y |
There was a problem hiding this comment.
Should this really be enabled by default? IIRC the bbc_microbit is fairly resource contrained, so it's odd to enable this for that board specifically.
There was a problem hiding this comment.
It was a chicken-or-egg problem when setting the stack size; with large values the firmware does not build or with low value it does not run. Hence, had to have the thread analysis enabled by default to ensure future (code or RAM usage changes require) tuning, is possible. If the firmware is running expected with thread analysis, then it can be disabled to save on code and RAM usage.
There was a problem hiding this comment.
But why do we only specifically enable the thread analyzer for this specific board? And by default? Seems like a debugging configuration
There was a problem hiding this comment.
I will move this to debug.conf and include in the sample.yaml for CI builds. Please request changes.
There was a problem hiding this comment.
I won't block the PR on this, and if @jhedberg and @HaavardRei both are fine with this, then it's OK :)
There was a problem hiding this comment.
It does strike me as a bit odd to include a debugging config for one specific board/CI build.
At the least could you add a line to README.rst describing the debug.conf file? It would also be nice to have a future reference besides this PR for the motivation of adding the thread analyzer specifically for this board (commit message?).
There was a problem hiding this comment.
Seems like they've now been moved to a new debug.conf
cvinayak
force-pushed
the
github_observer_bbc_microbit
branch
from
b524daf to
ddfa54e
Compare
Relax the radio packet pointer assignment deadline assertion until access address being transmitted. The PDU buffer is probably only needed just after access address is being transmitted or received by the radio. This will give some more breathing room for slow CPUs like in nRF51x SoCs. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Updated observer sample to use 10 ms continuous scanning parameters. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
cvinayak
force-pushed
the
github_observer_bbc_microbit
branch
from
ddfa54e to
4681e08
Compare
Add configuration overlay file to support observer sample
with Extended Scanning on BBC Micro Bit board.
Due to slow CPU, there will be assertions, and this commit
(for now) validates build-only. And the sample may run for
a duration until it asserts.
Asserts:
- ASSERTION FAIL [start_us == (aux_start_us + 1U)]
@ WEST_TOPDIR/zephyr/subsys/bluetooth/controller/ll_sw/
nordic/lll/lll_scan_aux.c:359
This will happen for small aux offset value, definitely
for the 300 us because CPU usage latency to setup such
auxiliary PDU reception on nRF51 is high due to slow CPU.
- ASSERTION FAIL [0]
@ WEST_TOPDIR/zephyr/subsys/bluetooth/controller/ll_sw/
nordic/lll/lll_scan_aux.c:592
prepare_cb: Actual EVENT_OVERHEAD_START_US = 579
This will happen due to CPU usage latencies scheduling
the radio events, due to slow CPU.
Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
cvinayak
force-pushed
the
github_observer_bbc_microbit
branch
from
4681e08 to
20354b2
Compare
|
7 participants
Add configuration overlay file to support observer sample with Extended Scanning on BBC Micro Bit board.
Due to slow CPU, there will be assertions, and this commit (for now) validates build-only. And the sample may run for a duration until it asserts.
Asserts:
ASSERTION FAIL [start_us == (aux_start_us + 1U)] @ WEST_TOPDIR/zephyr/subsys/bluetooth/controller/ll_sw/ nordic/lll/lll_scan_aux.c:359 This will happen for small aux offset value, definitely for the 300 us because CPU usage latency to setup such auxiliary PDU reception on nRF51 is high due to slow CPU.
ASSERTION FAIL [0] @ WEST_TOPDIR/zephyr/subsys/bluetooth/controller/ll_sw/ nordic/lll/lll_scan_aux.c:592 prepare_cb: Actual EVENT_OVERHEAD_START_US = 579 This will happen due to CPU usage latencies scheduling the radio events, due to slow CPU.
Sample console (receiving legacy, extended 1M and 2M extended advertising reports :-) ):
Example assertion:
Build commandline:
Flashing:
Code and RAM Sizes: