-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -6,6 +6,7 @@ | |||||
|
||||||
#include <stdint.h> | ||||||
#include <stdbool.h> | ||||||
#include <string.h> | ||||||
|
||||||
#include "hal/ccm.h" | ||||||
#include "hal/radio.h" | ||||||
|
@@ -331,6 +332,155 @@ static uint8_t chan_d(uint8_t n) | |||||
} | ||||||
#endif /* CONFIG_BT_CTLR_ISO */ | ||||||
|
||||||
#if defined(CONFIG_BT_CTLR_CHAN_METRICS_EVENT) | ||||||
Thalley marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
#include "hal/cpu.h" | ||||||
|
||||||
#define CHM_SURVEY_CHAN_COUNT_MAX 37U | ||||||
|
||||||
static struct { | ||||||
struct chan_metrics_chan { | ||||||
uint16_t count; /* Total metrics count */ | ||||||
uint16_t prev; /* Previous total metrics count when good tx acknowledgment */ | ||||||
uint16_t good; /* Total good tx acknowledgments */ | ||||||
} chan[CHM_SURVEY_CHAN_COUNT_MAX]; | ||||||
|
||||||
/* Differential state, context-safe, to notify upper layer */ | ||||||
uint8_t req; | ||||||
uint8_t ack; | ||||||
} chan_metrics; | ||||||
|
||||||
void lll_chan_metrics_init(void) | ||||||
{ | ||||||
(void)memset((void *)&chan_metrics, 0U, sizeof(chan_metrics)); | ||||||
} | ||||||
|
||||||
bool lll_chan_metrics_is_notify(void) | ||||||
{ | ||||||
return chan_metrics.req != chan_metrics.ack; | ||||||
} | ||||||
|
||||||
bool lll_chan_metrics_notify_clear(void) | ||||||
{ | ||||||
if (lll_chan_metrics_is_notify()) { | ||||||
chan_metrics.ack = chan_metrics.req; | ||||||
|
||||||
return true; | ||||||
} | ||||||
|
||||||
return false; | ||||||
} | ||||||
|
||||||
static bool chan_metrics_notify_set(void) | ||||||
{ | ||||||
uint8_t req; | ||||||
|
||||||
req = chan_metrics.req + 1U; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I follow. if 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 commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
if (req != chan_metrics.ack) { | ||||||
chan_metrics.req = req; | ||||||
cpu_dmb(); /* data memory barrier */ | ||||||
|
||||||
return true; | ||||||
} | ||||||
|
||||||
return false; | ||||||
} | ||||||
|
||||||
static struct chan_metrics_chan *chan_metrics_count_inc(uint8_t chan_idx) | ||||||
{ | ||||||
struct chan_metrics_chan *chan; | ||||||
|
||||||
LL_ASSERT(chan_idx < CHM_SURVEY_CHAN_COUNT_MAX); | ||||||
|
||||||
/* Mitigate metrics overflow, half the collected good and count value */ | ||||||
chan = &chan_metrics.chan[chan_idx]; | ||||||
if (chan->count == UINT16_MAX) { | ||||||
uint16_t diff; | ||||||
|
||||||
/* Current consecutive bad versus good difference */ | ||||||
diff = chan->count - chan->prev; | ||||||
|
||||||
/* 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`. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment mentions 'absolute
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
*/ | ||||||
chan->count >>= 1; | ||||||
if (diff < chan->count) { | ||||||
chan->prev = chan->count - diff; | ||||||
} else { | ||||||
chan->prev = 0U; | ||||||
} | ||||||
|
||||||
/* Half the good count */ | ||||||
chan->good >>= 1; | ||||||
} | ||||||
|
||||||
/* Increment the channel metrics count */ | ||||||
chan->count++; | ||||||
|
||||||
return chan; | ||||||
} | ||||||
|
||||||
void lll_chan_metrics_chan_bad(uint8_t chan_idx) | ||||||
{ | ||||||
struct chan_metrics_chan *chan; | ||||||
uint16_t diff; | ||||||
|
||||||
/* Increment the per channel metrics count */ | ||||||
chan = chan_metrics_count_inc(chan_idx); | ||||||
|
||||||
/* Notify beyond a bad versus good difference */ | ||||||
diff = chan->count - chan->prev; | ||||||
if (diff > CONFIG_BT_CTLR_CHAN_METRICS_BAD_COUNT) { | ||||||
(void)chan_metrics_notify_set(); | ||||||
|
||||||
/* Reset consecutive bad versus good difference */ | ||||||
chan->prev = chan->count; | ||||||
} | ||||||
} | ||||||
|
||||||
void lll_chan_metrics_chan_good(uint8_t chan_idx) | ||||||
{ | ||||||
struct chan_metrics_chan *chan; | ||||||
|
||||||
/* Increment the per channel metrics count */ | ||||||
chan = chan_metrics_count_inc(chan_idx); | ||||||
|
||||||
/* Reset the consecutive bad versus good difference */ | ||||||
chan->prev = chan->count; | ||||||
|
||||||
/* Increment good count */ | ||||||
chan->good++; | ||||||
} | ||||||
|
||||||
void lll_chan_metrics_print(void) | ||||||
{ | ||||||
const uint8_t max = 100U; | ||||||
|
||||||
printk("%s:\n", __func__); | ||||||
printk("chan #: (actual / expected reception) percentage -\n"); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment 'expected reception' is misleading since
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
for (uint8_t i = 0; i < CHM_SURVEY_CHAN_COUNT_MAX; i++) { | ||||||
const struct chan_metrics_chan *chan; | ||||||
uint8_t cnt; | ||||||
char c; | ||||||
|
||||||
chan = &chan_metrics.chan[i]; | ||||||
if (chan->count != 0U) { | ||||||
cnt = chan->good * max / chan->count; | ||||||
c = '*'; | ||||||
} else { | ||||||
cnt = max; | ||||||
c = '-'; | ||||||
} | ||||||
|
||||||
printk("%02d: (%05u / %05u) %03u - ", i, chan->good, chan->count, cnt); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The magic numbers Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
for (uint8_t j = 0; j < cnt; j++) { | ||||||
printk("%c", c); | ||||||
} | ||||||
printk("\n"); | ||||||
Thalley marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
} | ||||||
} | ||||||
#endif /* CONFIG_BT_CTLR_CHAN_METRICS_EVENT */ | ||||||
|
||||||
#if defined(CONFIG_BT_CTLR_TEST) | ||||||
/* Refer to Bluetooth Specification v5.2 Vol 6, Part C, Section 3 LE Channel | ||||||
* Selection algorithm #2 sample data | ||||||
|
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 omitBT_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 doIt 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 enabledThere 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
andCONFIG_BT_CTLR_CHAN_METRICS_LOW_RSSI_COUNT=3
to generate event on 3 consecutive low RSSI measurement on the channel.