nimble/controller: Add Channel Sounding setup phase #1765
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mkasenberg
force-pushed
the
cs-setup-phase
branch
4 times, most recently
from
fcafb89
to
6a71e3d
Compare
sjanc
reviewed
May 8, 2024
nimble/controller/src/ble_ll_cs.c
Outdated
| cssm = connsm->cssm; | ||
|
|
||
| /* Check if a disabled role is used in CS configs */ | ||
| for (int i = 0; i < ARRAY_SIZE(cssm->config); i++) { |
There was a problem hiding this comment.
nitpick: we typically declare variable at top of the function, also could be unsigned to avoid signed-unsigned comparison
|
|
||
| /* Check if a disabled role is used in CS configs */ | ||
| for (int i = 0; i < ARRAY_SIZE(cssm->config); i++) { | ||
| struct ble_ll_cs_config *conf = &cssm->config[i]; |
nimble/controller/src/ble_ll_cs.c
Outdated
| } | ||
|
|
||
| cssm->roles_enabled = cmd->role_enable; | ||
| cssm->cs_sync_antenna_selection = cmd->cs_sync_antenna_selection; |
There was a problem hiding this comment.
not sure if we are very consistent with this but maybe sm could be modified only after all validation is done? ie to avoid modifying it and stil returning error status
nimble/controller/src/ble_ll_cs.c
Outdated
| int | ||
| ble_ll_cs_rx_fae_req(struct ble_ll_conn_sm *connsm, uint8_t *rspbuf) | ||
| { | ||
| memcpy(rspbuf, connsm->cssm->local_fae_table, 72); |
There was a problem hiding this comment.
this is quite a lot, is it guaranteed that there is enough linear space in underlying mbuf?
There was a problem hiding this comment.
Good point, there wasn't ;)
|
|
||
| if (!rc) { | ||
| swap_buf(out_data, ecb.cipher_text, BLE_ENC_BLOCK_SIZE); | ||
| rc = 0; |
nimble/controller/src/ble_ll_cs.c
Outdated
| { | ||
| uint8_t config_id = connsm->cssm->config_req_id; | ||
| uint8_t action = connsm->cssm->config_req_action; | ||
| const struct ble_ll_cs_config *conf = &connsm->cssm->config[config_id]; |
There was a problem hiding this comment.
is config_id already validated here?
| return BLE_ERR_UNK_CONN_ID; | ||
| } | ||
|
|
||
| conf = &connsm->cssm->config[cmd->config_id]; |
There was a problem hiding this comment.
config_id should be validated before being used for indexing
sjanc
approved these changes
May 24, 2024
mkasenberg
force-pushed
the
cs-setup-phase
branch
from
a1d3776
to
66e8783
Compare
andrzej-kaczmarek
requested changes
Jun 11, 2024
|
|
||
| pkg.cflags.TEST: | ||
| - -Irepos/mbedtls/include | ||
| - -Irepos/include/mbedtls |
There was a problem hiding this comment.
includes should be added automatically from pkg dep
nimble/controller/src/ble_ll_cs.c
Outdated
| uint8_t *pv = connsm->cssm->drbg_ctx.pv; | ||
|
|
||
| if (!connsm->flags.encrypted) { | ||
| return rej_ext_ind_make(BLE_LL_CTRL_CS_SEC_REQ, |
There was a problem hiding this comment.
ble_ll_ctrl_rej_ext_ind_make + return
nimble/controller/src/ble_ll_cs.c
Outdated
| struct ble_ll_cs_drbg_ctx *drbg_ctx = &connsm->cssm->drbg_ctx; | ||
|
|
||
| if (!IS_PENDING_CTRL_PROC(connsm, BLE_LL_CTRL_PROC_CS_SEC_START)) { | ||
| /* Should never happen */ |
There was a problem hiding this comment.
in bluetooth it will happen ;)
just change the comment to "ignore" or smth
nimble/controller/src/ble_ll_cs.c
Outdated
| void | ||
| ble_ll_cs_rx_capabilities_rsp(struct ble_ll_conn_sm *connsm, uint8_t *dptr) | ||
| { | ||
| ble_ll_cs_update_rem_capabilities(connsm, dptr); |
There was a problem hiding this comment.
this should verify data in rsp and return error if needed
A dedicated CS DRBG is used to coordinate the randomization of several transaction types during a CS procedure.
The BT specification provides sample data that can be used to test the correctness of a CS DRBG implementation.
ble_cs.c contains initial implementation that caused warnings when running command: newt test @apache-mynewt-nimble/nimble
Latest spec draft contains Optional_TX_SNR_Capability field as a CS capability. Companion signal parameter was removed.
Initialize the CS state machine with the connection state machine.
Add lengths and opcodes of Channel Sounding LL CTRL PDUs.
Implement the LE CS Read Local Supported Capabilities command. Enable basic CS capabilities in local controller.
Implements: - LE CS Read Remote Supported Capabilities command, - LE CS Write Cached Remote Supported Capabilities command.
The HCI_LE_CS_Set_Default_Settings command is used by a Host to set default CS settings in the local Controller for the connection.
Implements: - LE CS Read Remote FAE Table command - LE CS Write Cached Remote FAE Table command
Implements: - LE CS Create Config command - LE CS Remove Config command
Implements LE CS Security Enable command.
mkasenberg
force-pushed
the
cs-setup-phase
branch
from
66e8783
to
95655da
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.