diff --git a/CODEOWNERS b/CODEOWNERS index 2adbab8697a9..f2a168ab8b29 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -865,6 +865,7 @@ /tests/subsys/app_event_manager/ @nrfconnect/ncs-si-muffin @nrfconnect/ncs-si-bluebagel /tests/subsys/audio/audio_module_template/ @nrfconnect/ncs-audio /tests/subsys/audio_module/ @nrfconnect/ncs-audio +/tests/subsys/bluetooth/controller/ @nrfconnect/ncs-dragoon /tests/subsys/bluetooth/gatt_dm/ @nrfconnect/ncs-si-muffin /tests/subsys/bluetooth/enocean/ @nrfconnect/ncs-paladin /tests/subsys/bluetooth/fast_pair/ @nrfconnect/ncs-si-bluebagel diff --git a/boards/nordic/thingy91/thingy91_nrf52840_defconfig b/boards/nordic/thingy91/thingy91_nrf52840_defconfig index a21adb47660d..e123dfe24767 100644 --- a/boards/nordic/thingy91/thingy91_nrf52840_defconfig +++ b/boards/nordic/thingy91/thingy91_nrf52840_defconfig @@ -14,3 +14,6 @@ CONFIG_CONSOLE=y CONFIG_USB_DEVICE_STACK=y CONFIG_BOOTLOADER_MCUBOOT=y + +# BT +CONFIG_BT_BUF_CMD_TX_COUNT=3 diff --git a/doc/nrf/releases_and_maturity/known_issues.rst b/doc/nrf/releases_and_maturity/known_issues.rst index 7157c99320b9..1657738bec05 100644 --- a/doc/nrf/releases_and_maturity/known_issues.rst +++ b/doc/nrf/releases_and_maturity/known_issues.rst @@ -191,6 +191,16 @@ Bluetooth LE .. rst-class:: v2-9-0-nRF54H20-rc1 v2-9-0 v2-8-0 v2-7-0 v2-6-3 v2-6-2 v2-6-1 v2-6-0 +DRGN-24352: Missing disconnection events when HCI Controller to host flow control is enabled + The :zephyr:code-sample:`bluetooth_hci_ipc` sample and :ref:`ipc_radio` application do not support stream flow control and drop bytes. + With the deferred disconnection complete generation added in the |NCS| v2.6.0, there are cases where the :kconfig:option:`CONFIG_BT_HCI_ACL_FLOW_CONTROL` Kconfig option has value ``y``, and the value of :kconfig:option:`CONFIG_BT_BUF_CMD_TX_COUNT` is smaller than (:kconfig:option:`CONFIG_BT_BUF_ACL_RX_COUNT` + 1). + This might result in dropped HCI command packets. + The visible symptom is a missing disconnect event and subsequent failure in connection, if a peripheral device falls out of range or is powered off. + + **Workaround:** Make sure that the value of the :kconfig:option:`CONFIG_BT_BUF_ACL_RX_COUNT` Kconfig option is smaller than the one for :kconfig:option:`CONFIG_BT_BUF_CMD_TX_COUNT` in your project. + +.. rst-class:: v2-9-0-nRF54H20-rc1 v2-9-0 v2-8-0 v2-7-0 v2-6-3 v2-6-2 v2-6-1 v2-6-0 + NCSDK-31095: Issues with the :kconfig:option:`CONFIG_SEGGER_SYSVIEW` Kconfig option Using this Kconfig option causes the data parameter in the macros :c:macro:`k_fifo_put`, :c:macro:`k_fifo_alloc_put`, :c:macro:`k_lifo_put`, and :c:macro:`k_lifo_alloc_put` to be evaluated multiple times. This can cause problems if the data parameter is a function call incrementing a reference counter. diff --git a/scripts/ci/tags.yaml b/scripts/ci/tags.yaml index 5459e0616d4e..9ac60f57f10c 100644 --- a/scripts/ci/tags.yaml +++ b/scripts/ci/tags.yaml @@ -940,6 +940,11 @@ ci_tests_subsys_audio_module: - nrf/subsys/audio/ - nrf/tests/subsys/audio/ +ci_tests_subsys_bluetooth_controller: + files: + - nrf/subsys/bluetooth/controller + - nrf/tests/subsys/bluetooth/controller + ci_tests_subsys_mpsl: files: - nrf/subsys/mpsl/ diff --git a/subsys/bluetooth/controller/CMakeLists.txt b/subsys/bluetooth/controller/CMakeLists.txt index 6f8d09b6a906..b4b9f6670f41 100644 --- a/subsys/bluetooth/controller/CMakeLists.txt +++ b/subsys/bluetooth/controller/CMakeLists.txt @@ -9,6 +9,7 @@ zephyr_library() zephyr_library_sources( hci_driver.c hci_internal.c + hci_internal_wrappers.c ) zephyr_library_sources_ifdef( diff --git a/subsys/bluetooth/controller/hci_internal.c b/subsys/bluetooth/controller/hci_internal.c index 8dbdcb26f92f..c78253a0c78b 100644 --- a/subsys/bluetooth/controller/hci_internal.c +++ b/subsys/bluetooth/controller/hci_internal.c @@ -16,6 +16,7 @@ #include #include "hci_internal.h" +#include "hci_internal_wrappers.h" #include "ecdh.h" #define CMD_COMPLETE_MIN_SIZE (BT_HCI_EVT_HDR_SIZE \ @@ -903,7 +904,7 @@ static uint8_t controller_and_baseband_cmd_put(uint8_t const * const cmd, case SDC_HCI_OPCODE_CMD_CB_SET_CONTROLLER_TO_HOST_FLOW_CONTROL: return sdc_hci_cmd_cb_set_controller_to_host_flow_control((void *)cmd_params); case SDC_HCI_OPCODE_CMD_CB_HOST_BUFFER_SIZE: - return sdc_hci_cmd_cb_host_buffer_size((void *)cmd_params); + return sdc_hci_cmd_cb_host_buffer_size_wrapper((void *)cmd_params); case SDC_HCI_OPCODE_CMD_CB_HOST_NUMBER_OF_COMPLETED_PACKETS: return sdc_hci_cmd_cb_host_number_of_completed_packets((void *)cmd_params); #endif diff --git a/subsys/bluetooth/controller/hci_internal_wrappers.c b/subsys/bluetooth/controller/hci_internal_wrappers.c new file mode 100644 index 000000000000..74847e921c9d --- /dev/null +++ b/subsys/bluetooth/controller/hci_internal_wrappers.c @@ -0,0 +1,78 @@ +/* + * Copyright (c) 2025 Nordic Semiconductor ASA + * + * SPDX-License-Identifier: LicenseRef-Nordic-5-Clause + */ + +#include "hci_internal_wrappers.h" +#include +#include + +#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 `CONFIG_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. + * + * `CONFIG_BT_BUF_CMD_TX_COUNT` is used differently depending on whether `CONFIG_BT_HCI_HOST` or + * `BT_HCI_RAW` is defined. And as ACL packet pools are only used in connections, + * we need to limit the build assert as such. See the comments above the asserts for more + * information on the specific scenario. + */ +#if defined(CONFIG_BT_HCI_HOST) && (BT_BUF_ACL_RX_COUNT > 0) +/* + * When `CONFIG_BT_HCI_HOST`, `CONFIG_BT_BUF_CMD_TX_COUNT` controls the capacity of + * `bt_hci_cmd_create`. Which is used for all BT HCI Commands, and can be called concurrently from + * many different application contexts, initiating various processes. + * + * Currently `bt_hci_cmd_create` is generally `K_FOREVER`, as such this build assert only guarantees + * to avoid deadlocks due to missing CMD buffers, if the host is only allocating the next command + * once the previous is completed. + */ +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 */ +/* + * On a controller-only build (`BT_HCI_RAW`) `CONFIG_BT_BUF_CMD_TX_COUNT` controls the capacity of + * `bt_buf_get_tx(BT_BUF_CMD)`. Which is only used to receive commands from the Host at the rate the + * command flow control dictates. Considering one buffer is needed to the Num_HCI_Command_Packet, to + * do flow control, at least one more buffer is needed. + * + */ +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 */ + +/* + * This wrapper addresses a limitation in some Zephyr HCI samples such as Zephyr hci_ipc, namely the + * dropping of Host Number of Completed Packets (HNCP) messages when buffers are full. Dropping + * these messages causes a resource leak. + * + * The buffers are full when CONFIG_BT_BUF_CMD_TX_COUNT is exhausted. This wrapper implements a + * workaround that ensures the buffers are never exhausted, by limiting the + * Host_Total_Num_ACL_Data_Packets value. Limiting this value naturally limits the number of HNCP to + * one the sample buffers can handle. + * + * Considering the sample uses the same buffer pool for normal commands, which take up one buffer at + * most (look up `HCI_Command_Complete` for more details), this wrapper artifically limits the + * Host_Total_Num_ACL_Data_Packets to at most one less than the CONFIG_BT_BUF_CMD_TX_COUNT pool + * capacity. + * . + */ +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 = *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(&ctrl_cmd_params); +} +#endif /* CONFIG_BT_HCI_ACL_FLOW_CONTROL */ diff --git a/subsys/bluetooth/controller/hci_internal_wrappers.h b/subsys/bluetooth/controller/hci_internal_wrappers.h new file mode 100644 index 000000000000..7cd28b2efd7b --- /dev/null +++ b/subsys/bluetooth/controller/hci_internal_wrappers.h @@ -0,0 +1,22 @@ +/* + * Copyright (c) 2025 Nordic Semiconductor ASA + * + * SPDX-License-Identifier: LicenseRef-Nordic-5-Clause + */ + +/** @file + * @brief Internal HCI interface Wrappers + */ + +#include +#include +#include + +#ifndef HCI_INTERNAL_WRAPPERS_H__ +#define HCI_INTERNAL_WRAPPERS_H__ + +#if defined(CONFIG_BT_HCI_ACL_FLOW_CONTROL) +int sdc_hci_cmd_cb_host_buffer_size_wrapper(const sdc_hci_cmd_cb_host_buffer_size_t *cmd_params); +#endif + +#endif diff --git a/tests/subsys/bluetooth/controller/CMakeLists.txt b/tests/subsys/bluetooth/controller/CMakeLists.txt new file mode 100644 index 000000000000..68f2a9255e72 --- /dev/null +++ b/tests/subsys/bluetooth/controller/CMakeLists.txt @@ -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) diff --git a/tests/subsys/bluetooth/controller/prj.conf b/tests/subsys/bluetooth/controller/prj.conf new file mode 100644 index 000000000000..8c317c5c4c56 --- /dev/null +++ b/tests/subsys/bluetooth/controller/prj.conf @@ -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 diff --git a/tests/subsys/bluetooth/controller/src/hci_cmd_cb_host_buffer_size_test.c b/tests/subsys/bluetooth/controller/src/hci_cmd_cb_host_buffer_size_test.c new file mode 100644 index 000000000000..4f3353989546 --- /dev/null +++ b/tests/subsys/bluetooth/controller/src/hci_cmd_cb_host_buffer_size_test.c @@ -0,0 +1,62 @@ +/* + * Copyright (c) 2025 Nordic Semiconductor ASA + * + * SPDX-License-Identifier: LicenseRef-Nordic-5-Clause + */ +#include +#include +#include +#include +#include + +#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; +} diff --git a/tests/subsys/bluetooth/controller/testcase.yaml b/tests/subsys/bluetooth/controller/testcase.yaml new file mode 100644 index 000000000000..72ad74b6f9f1 --- /dev/null +++ b/tests/subsys/bluetooth/controller/testcase.yaml @@ -0,0 +1,9 @@ +tests: + bluetooth.controller: + platform_allow: + - native_sim + integration_platforms: + - native_sim + tags: + - unittest + - ci_tests_subsys_bluetooth_controller