-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Bluetooth: Controller: build assert if comand buffer is too small #19869
Draft
KyraLengfeld
wants to merge
2
commits into
nrfconnect:main
Choose a base branch
from
KyraLengfeld:cmd_buffer_workaround
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+202
−2
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
/* | ||
* Copyright (c) 2025 Nordic Semiconductor ASA | ||
* | ||
* SPDX-License-Identifier: LicenseRef-Nordic-5-Clause | ||
*/ | ||
|
||
#include "hci_internal_wrappers.h" | ||
#include <zephyr/sys/util.h> | ||
#include <zephyr/bluetooth/buf.h> | ||
|
||
#if defined(CONFIG_BT_HCI_ACL_FLOW_CONTROL) | ||
/* | ||
* The Host will generate up to acl_pkts number of Host Number of Completed Packets command plus a | ||
* number of normal HCI commands, as such we need to ensure the tx command buffer count is big | ||
* enough to not block incoming ACKs from the host. | ||
* | ||
* When Controller to Host data flow control is supported, ensure that BT_BUF_CMD_TX_COUNT is | ||
* greater than or equal to (BT_BUF_ACL_RX_COUNT + Ncmd), where Ncmd is the supported maximum | ||
* Num_HCI_Command_Packets. | ||
* | ||
* The SDC controller (currently) does not support Num_HCI_Command_Packets > 1, which means Ncmd | ||
* is always 1. | ||
*/ | ||
#if defined(CONFIG_BT_CONN) && defined(CONFIG_BT_HCI_HOST) | ||
BUILD_ASSERT(BT_BUF_ACL_RX_COUNT < CONFIG_BT_BUF_CMD_TX_COUNT, | ||
"Too low HCI command buffers compared to ACL Rx buffers."); | ||
#else /* controller-only build */ | ||
BUILD_ASSERT((CONFIG_BT_BUF_CMD_TX_COUNT - 1) > 0, | ||
"We need at least two HCI command buffers to avoid deadlocks."); | ||
#endif /* CONFIG_BT_CONN && CONFIG_BT_HCI_HOST */ | ||
|
||
/* Note: When host and controller are built separately, the RX Buffer Count communicated here from | ||
* the host might be exeeding the controller's TX command buffer count. This may result in the | ||
* controller sending up to host_total_num_acl_data_packets, it cannot receive the acknoledgements | ||
* via its TX command buffers for. | ||
* As we do not propagate the TX command buffer count information to the controller, the simplest | ||
* approach is to align the communicated host RX Buffer Count to it here. | ||
*/ | ||
int sdc_hci_cmd_cb_host_buffer_size_wrapper(const sdc_hci_cmd_cb_host_buffer_size_t *cmd_params) | ||
{ | ||
sdc_hci_cmd_cb_host_buffer_size_t ctrl_cmd_params = | ||
*(const sdc_hci_cmd_cb_host_buffer_size_t *)cmd_params; | ||
ctrl_cmd_params.host_total_num_acl_data_packets = MIN( | ||
ctrl_cmd_params.host_total_num_acl_data_packets, (CONFIG_BT_BUF_CMD_TX_COUNT - 1)); | ||
|
||
return sdc_hci_cmd_cb_host_buffer_size((const void *)&ctrl_cmd_params); | ||
} | ||
#endif /* CONFIG_BT_HCI_ACL_FLOW_CONTROL */ |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
/* | ||
* Copyright (c) 2020 Nordic Semiconductor ASA | ||
* | ||
* SPDX-License-Identifier: LicenseRef-Nordic-5-Clause | ||
*/ | ||
|
||
/** @file | ||
* @brief Internal HCI interface Wrappers | ||
*/ | ||
|
||
#include <stdint.h> | ||
#include <stdbool.h> | ||
#include <sdc_hci_cmd_controller_baseband.h> | ||
|
||
#ifndef HCI_INTERNAL_WRAPPERS_H__ | ||
#define HCI_INTERNAL_WRAPPERS_H__ | ||
|
||
int sdc_hci_cmd_cb_host_buffer_size_wrapper(const sdc_hci_cmd_cb_host_buffer_size_t *cmd_params); | ||
|
||
#endif |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
# | ||
# Copyright (c) 2025 Nordic Semiconductor ASA | ||
# | ||
# SPDX-License-Identifier: LicenseRef-Nordic-5-Clause | ||
# | ||
|
||
cmake_minimum_required(VERSION 3.20.0) | ||
|
||
find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE}) | ||
project(hci_cmd_cb_host_buffer_size_test) | ||
|
||
# Generate runner for the test | ||
test_runner_generate(src/hci_cmd_cb_host_buffer_size_test.c) | ||
|
||
# Add Unit Under Test source files | ||
target_sources(app PRIVATE ${ZEPHYR_NRF_MODULE_DIR}/subsys/bluetooth/controller/hci_internal_wrappers.c) | ||
|
||
# Add test source file | ||
target_sources(app PRIVATE src/hci_cmd_cb_host_buffer_size_test.c) | ||
|
||
# Create mocks | ||
cmock_handle(${ZEPHYR_NRFXLIB_MODULE_DIR}/softdevice_controller/include/sdc_hci_cmd_controller_baseband.h) | ||
|
||
# Include paths | ||
include_directories(${ZEPHYR_HAL_NORDIC_MODULE_DIR}/nrfx) | ||
include_directories(${ZEPHYR_NRF_MODULE_DIR}/subsys/bluetooth/controller) |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
# | ||
# Copyright (c) 2025 Nordic Semiconductor ASA | ||
# | ||
# SPDX-License-Identifier: LicenseRef-Nordic-5-Clause | ||
# | ||
CONFIG_BT_CENTRAL=y | ||
CONFIG_BT_HCI=y | ||
CONFIG_BT=y | ||
|
||
CONFIG_BT_HCI_ACL_FLOW_CONTROL=y | ||
CONFIG_BT_BUF_CMD_TX_COUNT=10 | ||
|
||
CONFIG_UNITY=y |
62 changes: 62 additions & 0 deletions
62
tests/subsys/bluetooth/controller/src/hci_cmd_cb_host_buffer_size_test.c
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
/* | ||
* Copyright (c) 2025 Nordic Semiconductor ASA | ||
* | ||
* SPDX-License-Identifier: LicenseRef-Nordic-5-Clause | ||
*/ | ||
#include <unity.h> | ||
#include <stdbool.h> | ||
#include <stdio.h> | ||
#include <errno.h> | ||
#include <zephyr/sys/util.h> | ||
|
||
#include "hci_internal_wrappers.h" | ||
#include "cmock_sdc_hci_cmd_controller_baseband.h" | ||
|
||
#define TEST_HOST_ACL_DATA_PCKT_LNGTH 55 | ||
#define TEST_HOST_SYNC_DATA_PCKT_LNGTH 55 | ||
#define TEST_HOST_NUM_SYNC_DATA_PCKTS 5555 | ||
|
||
/* The unity_main is not declared in any header file. It is only defined in the generated test | ||
* runner because of ncs' unity configuration. It is therefore declared here to avoid a compiler | ||
* warning. | ||
*/ | ||
extern int unity_main(void); | ||
|
||
typedef struct { | ||
uint16_t acl_input_pckts; | ||
uint16_t exp_acl_pckts; | ||
} test_vector_t; | ||
|
||
static const test_vector_t test_vectors[] = { | ||
{5, 5}, /* Input within range, no adjustment needed */ | ||
{15, (CONFIG_BT_BUF_CMD_TX_COUNT - 1)}, /* Input exceeds TX buffer count - 1 limit */ | ||
{0xFF, (CONFIG_BT_BUF_CMD_TX_COUNT - 1)}, /* Maximum input value, corner case */ | ||
}; | ||
|
||
void test_sdc_hci_cmd_cb_host_buffer_size_wrapper(void) | ||
{ | ||
sdc_hci_cmd_cb_host_buffer_size_t cmd_params; | ||
|
||
cmd_params.host_acl_data_packet_length = TEST_HOST_ACL_DATA_PCKT_LNGTH; | ||
cmd_params.host_sync_data_packet_length = TEST_HOST_SYNC_DATA_PCKT_LNGTH; | ||
cmd_params.host_total_num_sync_data_packets = TEST_HOST_NUM_SYNC_DATA_PCKTS; | ||
|
||
for (size_t i = 0; i < ARRAY_SIZE(test_vectors); i++) { | ||
const test_vector_t *v = &test_vectors[i]; | ||
|
||
sdc_hci_cmd_cb_host_buffer_size_t exp_cmd_params = cmd_params; | ||
|
||
exp_cmd_params.host_total_num_acl_data_packets = v->exp_acl_pckts; | ||
__cmock_sdc_hci_cmd_cb_host_buffer_size_ExpectAndReturn(&exp_cmd_params, 0); | ||
|
||
cmd_params.host_total_num_acl_data_packets = v->acl_input_pckts; | ||
sdc_hci_cmd_cb_host_buffer_size_wrapper(&cmd_params); | ||
} | ||
} | ||
|
||
int main(void) | ||
{ | ||
(void)unity_main(); | ||
|
||
return 0; | ||
} |
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
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,8 @@ | ||||||||
tests: | ||||||||
bluetooth.controller: | ||||||||
platform_allow: native_sim | ||||||||
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.
Suggested change
|
||||||||
integration_platforms: | ||||||||
- native_sim | ||||||||
tags: | ||||||||
- unittest | ||||||||
- ci_tests_subsys_bluetooth_controller |
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.
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 think this should be moved to the .conf files in
boards
and be set along with the rest of the BT options