-
Notifications
You must be signed in to change notification settings - Fork 8k
Bluetooth: Controller: Introduce channel metrics events #95956
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
43a2ec7
to
72f8e8b
Compare
72f8e8b
to
0a4fb1a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces channel metrics events for the Bluetooth controller to report reception statistics on each ACL data channel in use. The feature enables monitoring of channel quality by tracking good and bad receptions per channel and generating events based on configurable thresholds.
- Adds channel metrics tracking infrastructure with per-channel statistics
- Implements periodic or threshold-based event generation for channel quality reporting
- Provides debug printing functionality for channel metrics visualization
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
subsys/bluetooth/controller/ll_sw/ull_conn.c | Adds periodic channel metrics event generation logic |
subsys/bluetooth/controller/ll_sw/ull_adv.c | Initializes channel tracking for peripheral connections |
subsys/bluetooth/controller/ll_sw/ull.c | Adds channel metrics event handling in RX path |
subsys/bluetooth/controller/ll_sw/nordic/lll/lll_peripheral.c | Tracks current/previous channel state for peripheral role |
subsys/bluetooth/controller/ll_sw/nordic/lll/lll_conn.c | Implements channel quality tracking and CRC error handling |
subsys/bluetooth/controller/ll_sw/nordic/lll/lll_central.c | Tracks current channel state for central role |
subsys/bluetooth/controller/ll_sw/nordic/lll/lll.c | Removes stray whitespace |
subsys/bluetooth/controller/ll_sw/lll_conn.h | Adds channel state fields to connection structures |
subsys/bluetooth/controller/ll_sw/lll_chan.h | Defines channel metrics API and inline stubs |
subsys/bluetooth/controller/ll_sw/lll_chan.c | Implements core channel metrics functionality |
subsys/bluetooth/controller/ll_sw/lll.h | Adds new NODE_RX_TYPE_CHAN_METRICS event type |
subsys/bluetooth/controller/hci/hci.c | Adds channel metrics event processing and debug output |
subsys/bluetooth/controller/Kconfig.ll_sw_split | Adds configuration options for channel metrics |
samples/bluetooth/peripheral_gatt_write/prj.conf | Enables channel metrics for testing |
samples/bluetooth/peripheral_gatt_write/dts/arm/nordic/override.dtsi | Sets IRQ priority for testing |
samples/bluetooth/central_gatt_write/prj.conf | Enables channel metrics for testing |
samples/bluetooth/central_gatt_write/dts/arm/nordic/override.dtsi | Sets IRQ priority for testing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
if (false) { | ||
#if defined(CONFIG_BT_CENTRAL) | ||
/* Central role and consecutive good channel reception */ | ||
} else if (lll->role == BT_HCI_ROLE_CENTRAL) { | ||
lll_chan_metrics_chan_good(lll->chan_curr); | ||
#endif /* CONFIG_BT_CENTRAL */ | ||
#if defined(CONFIG_BT_PERIPHERAL) | ||
} else if (lll->role == BT_HCI_ROLE_PERIPHERAL) { | ||
lll_chan_metrics_chan_good(lll->periph.chan_prev); | ||
lll->periph.chan_prev = lll->periph.chan_curr; | ||
#endif /* CONFIG_BT_PERIPHERAL */ | ||
} |
Copilot
AI
Sep 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition if (false)
creates unreachable code. This appears to be debug code that should be removed or replaced with a proper condition.
if (false) { | |
#if defined(CONFIG_BT_CENTRAL) | |
/* Central role and consecutive good channel reception */ | |
} else if (lll->role == BT_HCI_ROLE_CENTRAL) { | |
lll_chan_metrics_chan_good(lll->chan_curr); | |
#endif /* CONFIG_BT_CENTRAL */ | |
#if defined(CONFIG_BT_PERIPHERAL) | |
} else if (lll->role == BT_HCI_ROLE_PERIPHERAL) { | |
lll_chan_metrics_chan_good(lll->periph.chan_prev); | |
lll->periph.chan_prev = lll->periph.chan_curr; | |
#endif /* CONFIG_BT_PERIPHERAL */ | |
} | |
#if defined(CONFIG_BT_CENTRAL) | |
/* Central role and consecutive good channel reception */ | |
if (lll->role == BT_HCI_ROLE_CENTRAL) { | |
lll_chan_metrics_chan_good(lll->chan_curr); | |
} | |
#endif /* CONFIG_BT_CENTRAL */ | |
#if defined(CONFIG_BT_PERIPHERAL) | |
if (lll->role == BT_HCI_ROLE_PERIPHERAL) { | |
lll_chan_metrics_chan_good(lll->periph.chan_prev); | |
lll->periph.chan_prev = lll->periph.chan_curr; | |
} | |
#endif /* CONFIG_BT_PERIPHERAL */ |
Copilot uses AI. Check for mistakes.
0d8c484
to
2791586
Compare
@cvinayak do you want to add the Copyright/SPDX stuff to suppress the Compliance warnings? |
Ah... will fix them. I was so blind to those github actions warnings!... why are these not failing CI in the Compliance Checks actions? |
CONFIG_LOG=n | ||
|
||
# CONFIG_BT_CTLR_DATA_LENGTH_MAX=251 | ||
|
||
CONFIG_BT_CTLR_ADVANCED_FEATURES=y | ||
CONFIG_BT_CTLR_CHAN_METRICS_EVENT=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... need to move this to overlay-bt_ll_sw_split.conf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the file... this was for manual testing
CONFIG_LOG=n | ||
|
||
# CONFIG_BT_CTLR_DATA_LENGTH_MAX=251 | ||
|
||
CONFIG_BT_CTLR_ADVANCED_FEATURES=y | ||
CONFIG_BT_CTLR_CHAN_METRICS_EVENT=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... need to move this to overlay-bt_ll_sw_split.conf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the file... this was for manual testing
config BT_CTLR_CHAN_METRICS_EVENT | ||
bool "Channel Metrics event" | ||
help | ||
Generate events for channel usage metrics. | ||
|
||
config BT_CTLR_CHAN_METRICS_BAD_COUNT | ||
int "Channel bad count threshold" | ||
depends on BT_CTLR_CHAN_METRICS_EVENT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could consider doing so that BT_CTLR_CHAN_METRICS_BAD_COUNT
defaults to 0 and omit BT_CTLR_CHAN_METRICS_EVENT
, and then you can just use a single Kconfig option for this purpose. If you want to have a bool, you could also do
config BT_CTLR_CHAN_METRICS_EVENT
defbool BT_CTLR_CHAN_METRICS_BAD_COUNT > 0
config BT_CTLR_CHAN_METRICS_BAD_COUNT
int "Channel bad count threshold"
It is also OK as is if you prefer that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the current way because this can be extended in the future to measure (or have threshold for) other details like CRC error counts, or retransmission counts etc,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure :) Just FYI, you would still be able to extend with my suggestion assuming that BT_CTLR_CHAN_METRICS_BAD_COUNT
still needs to be minimum 1 for the feature to be enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, say, RSSI metrics per channel is added. Then, CONFIG_BT_CTLR_CHAN_METRICS_BAD_COUNT=0
and CONFIG_BT_CTLR_CHAN_METRICS_LOW_RSSI_COUNT=3
to generate event on 3 consecutive low RSSI measurement on the channel.
{ | ||
uint8_t req; | ||
|
||
req = chan_metrics.req + 1U; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could potentially overflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is intentional, the follow check ensured the assignment does not rollover.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow.
if chan_metrics.req
is 255, then req
becomes 0, and then chan_metrics.req
becomes 0 if req != chan_metrics.ack
.
If overflow is intentional and support (although I'm not fully following the logic here), then please add a comment stating that. We should avoid having implicit over- and underflows IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#if !defined(CONFIG_BT_CTLR_CHAN_METRICS_EVENT) | ||
LLL_CHAN_METRICS void lll_chan_metrics_init(void) | ||
{ | ||
} | ||
|
||
LLL_CHAN_METRICS void lll_chan_metrics_chan_bad(uint8_t chan_curr) | ||
{ | ||
} | ||
|
||
LLL_CHAN_METRICS void lll_chan_metrics_chan_good(uint8_t chan_curr) | ||
{ | ||
} | ||
|
||
LLL_CHAN_METRICS bool lll_chan_metrics_is_notify(void) | ||
{ | ||
return false; | ||
} | ||
|
||
LLL_CHAN_METRICS bool lll_chan_metrics_notify_clear(void) | ||
{ | ||
return false; | ||
} | ||
|
||
LLL_CHAN_METRICS void lll_chan_metrics_print(void) | ||
{ | ||
} | ||
#endif /* !CONFIG_BT_CTLR_CHAN_METRICS_EVENT */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than doing this, I would suggest to remove the guard and guard all calls to these with CONFIG_BT_CTLR_CHAN_METRICS_EVENT
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caller's of the interface do not need to have conditional compiles, these empty functions get eliminated by linker,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow - Calls to conditional features can/should be guarded by IS_ENABLED
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When CONFIG_BT_CTLR_CHAN_METRICS_EVENT=n
This header file will have, for example a definition:
LLL_CHAN_METRICS bool lll_chan_metrics_is_notify(void)
{
return false;
}
Where #define LLL_CHAN_METRICS static __attribute__((always_inline)) inline
Calls like here:
https://github.com/cvinayak/zephyr/blob/7dc31ca862c1bc306df716a17658188d9333018c/subsys/bluetooth/controller/ll_sw/ull_conn.c#L1319-L1333
i.e. if (lll_chan_metrics_is_notify()) {
evaluates to false
eliminating the entire code block.
there is no need to have if (IS_ENABLED(CONFIG_BT_CTLR_CHAN_METRICS_EVENT))
to guard the function calls and rest of the code block there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker :)
samples/bluetooth/central_gatt_write/dts/arm/nordic/override.dtsi
Outdated
Show resolved
Hide resolved
2791586
to
eac0b7e
Compare
samples/bluetooth/central_gatt_write/dts/arm/nordic/override.dtsi
Outdated
Show resolved
Hide resolved
CONFIG_LOG=n | ||
|
||
# CONFIG_BT_CTLR_DATA_LENGTH_MAX=251 | ||
|
||
CONFIG_BT_CTLR_ADVANCED_FEATURES=y | ||
CONFIG_BT_CTLR_CHAN_METRICS_EVENT=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the file... this was for manual testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the file... this was for manual testing
CONFIG_LOG=n | ||
|
||
# CONFIG_BT_CTLR_DATA_LENGTH_MAX=251 | ||
|
||
CONFIG_BT_CTLR_ADVANCED_FEATURES=y | ||
CONFIG_BT_CTLR_CHAN_METRICS_EVENT=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the file... this was for manual testing
config BT_CTLR_CHAN_METRICS_EVENT | ||
bool "Channel Metrics event" | ||
help | ||
Generate events for channel usage metrics. | ||
|
||
config BT_CTLR_CHAN_METRICS_BAD_COUNT | ||
int "Channel bad count threshold" | ||
depends on BT_CTLR_CHAN_METRICS_EVENT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the current way because this can be extended in the future to measure (or have threshold for) other details like CRC error counts, or retransmission counts etc,
uint16_t count; | ||
uint16_t prev; | ||
uint16_t good; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
count
is the total calls to collect metrics of which good
is the good channel transmission. count
- good
gives the count for channels that have missed transmission (poor throughput).
Introduce channel metrics events that report number of good reception on each ACL data channel in use. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
eac0b7e
to
7dc31ca
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
||
/* Half the count and keep the current consecutive bad versus good difference. | ||
* We do not want to rollover `UINT16_MAX`, hence we normalize the `count` and | ||
* `good`, but keep the absolute `diff` between `total` and `prev`. |
Copilot
AI
Oct 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment mentions 'absolute diff
between total
and prev
' but the variable is called count
, not total
. Update the comment to use consistent variable names.
* `good`, but keep the absolute `diff` between `total` and `prev`. | |
* `good`, but keep the absolute `diff` between `count` and `prev`. |
Copilot uses AI. Check for mistakes.
const uint8_t max = 100U; | ||
|
||
printk("%s:\n", __func__); | ||
printk("chan #: (actual / expected reception) percentage -\n"); |
Copilot
AI
Oct 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment 'expected reception' is misleading since count
represents total attempts (both good and bad), not expected receptions. Consider changing to 'total attempts' for clarity.
printk("chan #: (actual / expected reception) percentage -\n"); | |
printk("chan #: (actual / total attempts) percentage -\n"); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expected reception
represents best the expected reception count (for the attempts)
c = '-'; | ||
} | ||
|
||
printk("%02d: (%05u / %05u) %03u - ", i, chan->good, chan->count, cnt); |
Copilot
AI
Oct 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The magic numbers 02d
, 05u
, and 03u
in the printf format string should be defined as constants to improve maintainability and make the output format more consistent.
Copilot uses AI. Check for mistakes.
Introduce channel metrics events that report number of good reception on each ACL data channel in use.
Manual testing:
Example print of throughput interleaved by the channel metrics (playing youtube video on the laptop over wifi connection):
Modifications to get the above output (forcing the prints regularly instead of the implemented bad count threshold logic):