Bluetooth: Controller: Adds path loss monitoring functionality #92450
Conversation
|
Hello @tyler-huffman-demant, and thank you very much for your first pull request to the Zephyr project! |
github-actions
bot
added
area: Bluetooth
area: Bluetooth Controller
area: Bluetooth Host
github-actions
bot
requested review from
alwa-nordic,
carlescufi,
cvinayak,
erbr-ot,
hermabe,
jhedberg,
rugeGerritsen,
sjanc,
Thalley,
theob-pro,
thoh-ot,
Tronil and
wopu-ot
|
Seems like the title is wrong? There are no host-side changes in this PR? |
tyler-huffman-demant
changed the title
tyler-huffman-demant
force-pushed
the
path_loss_changes
branch
2 times, most recently
from
ba37e43 to
c781c30
Compare
tyler-huffman-demant
force-pushed
the
path_loss_changes
branch
2 times, most recently
from
d9180d8 to
4ed34f8
Compare
| struct bt_hci_cp_le_set_path_loss_reporting_parameters *cmd = (void *)buf->data; | ||
| struct bt_hci_rp_le_set_path_loss_reporting_parameters *rp; | ||
|
|
||
| uint16_t handle = sys_le16_to_cpu(cmd->handle); |
There was a problem hiding this comment.
Not particularly related to this PR, as it's basically done nowhere in the controller, but cmd->handle is only 12 bits. If we receive a a handle of 0xFFFF we should return BT_HCI_ERR_INVALID_PARAM, right?
There was a problem hiding this comment.
Very good point. Maybe it's worth making a Github issue and fixing this file up? 👍
There was a problem hiding this comment.
Sure, could be a nice first-time issue as it's just fixing a missing check several places :) Chances are that it's being handled somewhere else that I'm not aware of @cvinayak ?
| struct bt_hci_evt_le_path_loss_threshold *sep; | ||
| struct node_rx_path_loss *pl_pdu; | ||
|
|
||
| pl_pdu = (void *)pdu_data; |
There was a problem hiding this comment.
I don't see how you can meaningfully can cast a struct pdu_data to a struct node_rx_path_loss. Those 2 structs seem quite disjoint
There was a problem hiding this comment.
AFAIK struct pdu_data is the PDU's payload, so long as the raw PDU is correctly following the Bluetooth spec, then it shouldn't be disjoint.
But more importantly, I just modeled it after these functions:
zephyr/subsys/bluetooth/controller/hci/hci.c
Line 8584 in fdd02c5
zephyr/subsys/bluetooth/controller/hci/hci.c
Line 8606 in fdd02c5
zephyr/subsys/bluetooth/controller/hci/hci.c
Line 8631 in fdd02c5
There was a problem hiding this comment.
Hmm, yeah, this is outside of my knowledge.
I was just looking an comparing the 2 structs, and they don't really share any common fields, so that's why casting one to the other seems weird. Someone who knows this better than I should comment here :)
There was a problem hiding this comment.
Do you have any comments @cvinayak ?
|
|
||
| struct path_loss_params { | ||
| uint16_t min_time_spent; | ||
| uint8_t enabled; |
There was a problem hiding this comment.
Some values like enabled and conn_handle can use fewer bits than the full size of their types. Consider if we can optimize the size here a bit
There was a problem hiding this comment.
In practice, I think they can be optimized (like enabled), though it seems the Bluetooth spec only specifies between 1 and 2 octets without specifying meaningful bits, so I'd prefer to keep it as is.
|
|
||
| conn = ll_connected_get(handle); | ||
|
|
||
| if (!conn) { |
There was a problem hiding this comment.
| if (!conn) { | |
| if (conn == NULL) { |
There was a problem hiding this comment.
I would prefer to leave this as is, since this file has many cases of being done this way.
|
|
||
| conn = ll_connected_get(handle); | ||
|
|
||
| if (!conn) { |
There was a problem hiding this comment.
| if (!conn) { | |
| if (conn == NULL) { |
There was a problem hiding this comment.
I would prefer to leave this as is, since this file has many cases of being done this way.
There was a problem hiding this comment.
Pull Request Overview
Adds support for LE Path Loss Monitoring by defining new data structures, APIs, HCI commands, and event handling across the controller layers.
- Introduces
node_rx_path_lossand related structs in ULL/LLL for reporting path loss. - Implements
ll_conn_set_path_loss_parametersandll_conn_set_path_loss_reportingwith HCI command handlers. - Updates Kconfig, HCI opcode definitions, and RX demux to include
NODE_RX_TYPE_PATH_LOSS.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| subsys/bluetooth/controller/ll_sw/ull_conn_types.h | New RX path loss event struct |
| subsys/bluetooth/controller/ll_sw/ull_conn_internal.h | Added (mismatched) prototypes for ULL path loss APIs |
| subsys/bluetooth/controller/ll_sw/ull_conn.c | Implements LL path loss parameter setters |
| subsys/bluetooth/controller/ll_sw/ull.c | Demux support for NODE_RX_TYPE_PATH_LOSS |
| subsys/bluetooth/controller/ll_sw/lll_conn.h | Adds path_loss_params, path_loss_state and zone field |
| subsys/bluetooth/controller/ll_sw/lll.h | Inserts new RX type in enum node_rx_type |
| subsys/bluetooth/controller/include/ll.h | Declares HCI-side functions ll_conn_set_path_loss_* |
| subsys/bluetooth/controller/hci/hci.c | HCI command handlers and encoder for path loss reporting |
| subsys/bluetooth/controller/Kconfig | Removes unnecessary dependency for power control |
| include/zephyr/bluetooth/hci_types.h | Defines new HCI opcodes and command/response structs |
Comments suppressed due to low confidence (5)
subsys/bluetooth/controller/ll_sw/ull_conn_types.h:306
- [nitpick] Field
current_path_lossconflicts withcurr_path_lossinstruct path_loss_event. Use a consistent name (e.g.,curr_path_loss) across both structs to avoid confusion.
uint8_t current_path_loss;
subsys/bluetooth/controller/ll_sw/ull_conn.c:2913
- New path loss parameter and reporting APIs lack automated unit or integration tests. Consider adding tests for boundary conditions of thresholds, hysteresis, and invalid inputs.
#if defined(CONFIG_BT_CTLR_LE_PATH_LOSS_MONITORING)
subsys/bluetooth/controller/ll_sw/ull_conn_internal.h:145
- Prototype
ull_path_loss_set_parameters(void)doesn’t match the implementedll_conn_set_path_loss_parameters(uint16_t, uint8_t, ...). Remove or correct this declaration to prevent unresolved symbols or mismatches.
void ull_path_loss_set_parameters(void);
subsys/bluetooth/controller/hci/hci.c:8711
- The variable
pdu_datais undefined in this scope; you likely meant to cast thepduparameter. Change topl_pdu = (void *)pdu;to compile correctly.
pl_pdu = (void *)pdu_data;
subsys/bluetooth/controller/hci/hci.c:2837
- After setting
rp->status = BT_HCI_ERR_INVALID_PARAMfor invalidenable, this line still overwrites it. Add anelseor earlyreturnto prevent sending a success status when parameters are invalid.
rp->status = ll_conn_set_path_loss_reporting(handle, cmd->enable);
4ed34f8 to
9f7f0ff
Compare
This functionality is implemented in the HCI-facing side of the controller and the ULL. LLL functionality is not implemented in this commit. Furthermore, this removes a dependency for LE Power Control, as the features are related, but not necessarily required. Signed-off-by: Tyler Joseph Huffman <tyhu@demant.com>
tyler-huffman-demant
force-pushed
the
path_loss_changes
branch
from
9f7f0ff to
fa947a1
Compare
|
This functionality is implemented in the HCI-facing side of the controller and the ULL. LLL functionality is not implemented in this commit.
Furthermore, this removes a dependency for LE Power Control, as the features are related, but not necessarily required.