From 8710fd71258f53b845d1e0b3941dc3ebe452b889 Mon Sep 17 00:00:00 2001 From: Rahul Tank Date: Fri, 14 Aug 2026 10:51:12 +0530 Subject: [PATCH] fix(nimble): Fix for pytest throughput failure In example, central clears CCCD while peripheral is still sending notify. This causes ATT write response to starve resulting in disconnect. --- .../bluetooth/nimble/pytest_nimble_test.py | 10 ++ .../gatt/blecent_throughput/main/gattc.h | 1 + .../gatt/blecent_throughput/main/main.c | 150 ++++++++++++++++-- .../gatt/blecent_throughput/main/scli.c | 56 +++++++ .../gatt/bleprph_throughput/main/gatt_svr.c | 5 + .../gatt/bleprph_throughput/main/gatts_sens.h | 7 +- .../gatt/bleprph_throughput/main/main.c | 44 ++++- 7 files changed, 252 insertions(+), 21 deletions(-) diff --git a/examples/bluetooth/nimble/pytest_nimble_test.py b/examples/bluetooth/nimble/pytest_nimble_test.py index f32225dfe38..63e8549abdd 100644 --- a/examples/bluetooth/nimble/pytest_nimble_test.py +++ b/examples/bluetooth/nimble/pytest_nimble_test.py @@ -1470,6 +1470,16 @@ def _run_nimble_gatt_throughput_func(dut: tuple[IdfDut, IdfDut], throughput_mode central.write('Insert No') central.expect_exact('User entered: Insert No', timeout=10) + # Boards of the same pipeline, chip and example share the CI device name, so + # pin the central to the MAC of the board it was paired with for this run. + central.expect_exact('Enter peer address in this format: `peer xx:xx:xx:xx:xx:xx`', timeout=10) + central.write(f'peer {peripheral_addr}') + central.expect_exact(f'User entered: peer {peripheral_addr}', timeout=10) + central.expect_exact( + f'blecent_throughput: Peer address filter set to {peripheral_addr.lower()}', + timeout=10, + ) + found_addr = ( central.expect( r'blecent_throughput: Found device: addr: ' + MAC_PATTERN + rf', name: {name}', diff --git a/examples/bluetooth/nimble/throughput_app/gatt/blecent_throughput/main/gattc.h b/examples/bluetooth/nimble/throughput_app/gatt/blecent_throughput/main/gattc.h index b90f4a998cb..eb6e6890aaa 100644 --- a/examples/bluetooth/nimble/throughput_app/gatt/blecent_throughput/main/gattc.h +++ b/examples/bluetooth/nimble/throughput_app/gatt/blecent_throughput/main/gattc.h @@ -117,6 +117,7 @@ void ble_register_cli(void); int scli_receive_key(int key[6]); int cli_receive_key(int key[6]); int scli_receive_yesno(bool *key); +int scli_receive_peer_addr(uint8_t peer_addr[PEER_ADDR_VAL_SIZE]); void scli_reset_queue(void); diff --git a/examples/bluetooth/nimble/throughput_app/gatt/blecent_throughput/main/main.c b/examples/bluetooth/nimble/throughput_app/gatt/blecent_throughput/main/main.c index 13dcecf653a..5ad6d65ec07 100644 --- a/examples/bluetooth/nimble/throughput_app/gatt/blecent_throughput/main/main.c +++ b/examples/bluetooth/nimble/throughput_app/gatt/blecent_throughput/main/main.c @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +#include #include "esp_log.h" #include "nvs_flash.h" /* BLE */ @@ -53,6 +54,14 @@ #define WRITE_THROUGHPUT_PAYLOAD 495 /* 502 bytes ACL -> 2x 251 LL packets exactly (495 + 3 Write Cmd + 4 L2CAP) */ #define LL_PACKET_TIME 2120 #define LL_PACKET_LENGTH 251 + +/* One byte command asking the peripheral to stop the notify test */ +#define THRPT_CMD_STOP_NOTIFY 0xF0 +#define NOTIFY_DRAIN_DELAY_MS 500 /* Let queued notifications drain */ +#define UNSUB_TIMEOUT_MS 5000 /* Well below the 30 s ATT timeout */ +#define UNSUB_MAX_ATTEMPTS 3 +#define STOP_CMD_MAX_ATTEMPTS 5 +#define UNSUB_STATUS_PENDING INT_MIN static const char *tag = "blecent_throughput"; static int blecent_gap_event(struct ble_gap_event *event, void *arg); @@ -90,6 +99,18 @@ static uint16_t handle; static volatile int current_phy_updated; #endif +/* Result of the in-flight CCCD write, set from the GATT write callback */ +static volatile int unsub_status = UNSUB_STATUS_PENDING; + +#if CONFIG_EXAMPLE_CI_ID && CONFIG_EXAMPLE_CI_PIPELINE_ID +/* Address of the board this run is paired with, supplied over the console. + * The CI device name is shared by every board of the same pipeline, chip and + * example, so boards left advertising by an earlier run would otherwise be + * valid scan results. */ +static uint8_t ci_peer_addr[PEER_ADDR_VAL_SIZE]; +static bool ci_peer_addr_valid; +#endif + /* State for callback-chained read throughput test */ static volatile bool read_test_active = false; static uint16_t read_val_handle_g; @@ -430,6 +451,94 @@ int update_phy(uint16_t conn_handle, uint8_t phy_mode) } #endif +static int +blecent_unsubscribe_cb(uint16_t conn_handle, const struct ble_gatt_error *error, + struct ble_gatt_attr *attr, void *arg) +{ + unsub_status = (error == NULL) ? 0 : error->status; + return 0; +} + +/* Ask the peripheral to stop notifying using an unacknowledged write, then let + * the queued notifications drain. Clearing the CCCD while the peripheral is + * still flooding starves the ATT response and the host tears the link down + * after the 30 s GATT procedure timeout. */ +static void +blecent_stop_notify_traffic(uint16_t conn_handle, const struct peer *peer) +{ + const struct peer_chr *cmd_chr; + uint8_t cmd = THRPT_CMD_STOP_NOTIFY; + int attempt; + int rc; + + cmd_chr = peer_chr_find_uuid(peer, + THRPT_UUID_DECLARE(THRPT_SVC), + THRPT_UUID_DECLARE(THRPT_CHR_READ_WRITE)); + if (cmd_chr == NULL) { + ESP_LOGW(tag, "Peer lacks the control characteristic (0x0006); " + "cannot request notify stop"); + return; + } + + /* The notify traffic keeps the mbuf pool busy, so the command itself can + * fail with BLE_HS_ENOMEM; retry until a buffer frees up. */ + for (attempt = 1; attempt <= STOP_CMD_MAX_ATTEMPTS; attempt++) { + rc = ble_gattc_write_no_rsp_flat(conn_handle, cmd_chr->chr.val_handle, + &cmd, sizeof cmd); + if (rc == 0) { + break; + } + + ESP_LOGW(tag, "Notify stop request attempt %d failed; rc=%d", + attempt, rc); + vTaskDelay(100 / portTICK_PERIOD_MS); + } + + vTaskDelay(NOTIFY_DRAIN_DELAY_MS / portTICK_PERIOD_MS); +} + +static void +blecent_unsubscribe(uint16_t conn_handle, uint16_t cccd_handle) +{ + uint8_t unsub_value[2] = {0, 0}; + struct ble_gap_conn_desc desc; + int attempt; + int waited; + int rc; + + for (attempt = 1; attempt <= UNSUB_MAX_ATTEMPTS; attempt++) { + unsub_status = UNSUB_STATUS_PENDING; + + rc = ble_gattc_write_flat(conn_handle, cccd_handle, + unsub_value, sizeof unsub_value, + blecent_unsubscribe_cb, NULL); + if (rc != 0) { + ESP_LOGW(tag, "Unsubscribe attempt %d not started; rc=%d", + attempt, rc); + } else { + for (waited = 0; unsub_status == UNSUB_STATUS_PENDING && + waited < UNSUB_TIMEOUT_MS; waited += 100) { + vTaskDelay(100 / portTICK_PERIOD_MS); + } + + if (unsub_status == 0) { + ESP_LOGI(tag, "Unsubscribed from notifications"); + return; + } + + ESP_LOGW(tag, "Unsubscribe attempt %d failed; status=%d", + attempt, unsub_status); + } + + if (ble_gap_conn_find(conn_handle, &desc) != 0) { + ESP_LOGW(tag, "Connection gone; giving up on unsubscribe"); + return; + } + } + + ESP_LOGE(tag, "Failed to unsubscribe from notifications"); +} + static void throughput_task(void *arg) { struct peer *peer = (struct peer *)arg; @@ -665,19 +774,11 @@ read_cleanup: } vTaskDelay((TickType_t)test_data[1] * 1000 / portTICK_PERIOD_MS); - /* Unsubscribe so the next notify test triggers a fresh - * BLE_GAP_EVENT_SUBSCRIBE on the peripheral (cur_notify 0→1) */ - { - uint8_t unsub_value[2] = {0, 0}; - rc = ble_gattc_write_flat(conn_handle, dsc->dsc.handle, - unsub_value, sizeof unsub_value, - NULL, NULL); - if (rc != 0) { - ESP_LOGW(tag, "Unsubscribe failed; rc=%d (non-fatal)", rc); - } else { - ESP_LOGI(tag, "Unsubscribed from notifications"); - } - } + /* Stop the notify traffic first, then unsubscribe so the next + * notify test triggers a fresh BLE_GAP_EVENT_SUBSCRIBE on the + * peripheral (cur_notify 0→1) */ + blecent_stop_notify_traffic(conn_handle, peer); + blecent_unsubscribe(conn_handle, dsc->dsc.handle); break; default: @@ -808,6 +909,11 @@ ext_blecent_should_connect(const struct ble_gap_ext_disc_desc *disc) } #if CONFIG_EXAMPLE_CI_ID && CONFIG_EXAMPLE_CI_PIPELINE_ID + if (ci_peer_addr_valid && + memcmp(ci_peer_addr, disc->addr.val, sizeof(disc->addr.val)) != 0) { + return 0; + } + /* Match on the CI device name only, so that boards belonging to other * pipelines or examples in the same RF environment are ignored. */ do { @@ -907,6 +1013,11 @@ blecent_should_connect(const struct ble_gap_disc_desc *disc) } #if CONFIG_EXAMPLE_CI_ID && CONFIG_EXAMPLE_CI_PIPELINE_ID + if (ci_peer_addr_valid && + memcmp(ci_peer_addr, disc->addr.val, sizeof(disc->addr.val)) != 0) { + return 0; + } + /* Match on the CI device name only, so that boards belonging to other * pipelines or examples in the same RF environment are ignored. */ if (fields.name != NULL && @@ -1253,6 +1364,19 @@ blecent_on_sync(void) scli_reset_queue(); } } + +#if CONFIG_EXAMPLE_CI_ID && CONFIG_EXAMPLE_CI_PIPELINE_ID + ESP_LOGI(tag, "Enter peer address in this format: `peer xx:xx:xx:xx:xx:xx` "); + if (scli_receive_peer_addr(ci_peer_addr)) { + ci_peer_addr_valid = true; + ESP_LOGI(tag, "Peer address filter set to %s", addr_str(ci_peer_addr)); + } else { + ESP_LOGI(tag, "No peer address provided; connecting to any board with" + " the CI device name"); + } + scli_reset_queue(); +#endif + /* Begin scanning for a peripheral to connect to. */ blecent_scan(); } diff --git a/examples/bluetooth/nimble/throughput_app/gatt/blecent_throughput/main/scli.c b/examples/bluetooth/nimble/throughput_app/gatt/blecent_throughput/main/scli.c index f163708473a..fcd9658ef30 100644 --- a/examples/bluetooth/nimble/throughput_app/gatt/blecent_throughput/main/scli.c +++ b/examples/bluetooth/nimble/throughput_app/gatt/blecent_throughput/main/scli.c @@ -21,6 +21,7 @@ #define BLE_RX_TIMEOUT (30000 / portTICK_PERIOD_MS) #define BLE_RX_PARAM (10000 / portTICK_PERIOD_MS) #define YES_NO_PARAM (5000 / portTICK_PERIOD_MS) +#define PEER_ADDR_PARAM (5000 / portTICK_PERIOD_MS) static QueueHandle_t cli_handle; @@ -30,6 +31,9 @@ static QueueHandle_t cli_handle; #define CLI_MSG_TYPE_MTU 2 #define CLI_MSG_TYPE_THROUGHPUT 3 #define CLI_MSG_TYPE_YESNO 4 +#define CLI_MSG_TYPE_PEER_ADDR 5 + +#define CLI_PEER_ADDR_LEN 6 struct cli_msg { int type; @@ -38,6 +42,7 @@ struct cli_msg { int mtu; int key[3]; bool yes; + uint8_t peer_addr[CLI_PEER_ADDR_LEN]; } data; }; @@ -174,6 +179,52 @@ static int yesno_handler(int argc, char *argv[]) return 0; } +static int peer_addr_handler(int argc, char *argv[]) +{ + struct cli_msg msg = { + .type = CLI_MSG_TYPE_PEER_ADDR, + }; + + if (argc != 2) { + return -1; + } + + /* val[0] holds the least significant byte, so parse in reverse order. */ + if (sscanf(argv[1], "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx", + &msg.data.peer_addr[5], &msg.data.peer_addr[4], + &msg.data.peer_addr[3], &msg.data.peer_addr[2], + &msg.data.peer_addr[1], &msg.data.peer_addr[0]) != CLI_PEER_ADDR_LEN) { + ESP_LOGE("CLI", "Failed to parse peer address"); + return -1; + } + + ESP_LOGI("User entered", "%s %s", argv[0], argv[1]); + if (cli_handle == NULL) { + ESP_LOGE("CLI", "Queue not initialized"); + return -1; + } + xQueueSend(cli_handle, &msg, 500 / portTICK_PERIOD_MS); + return 0; +} + +int scli_receive_peer_addr(uint8_t peer_addr[6]) +{ + struct cli_msg msg; + + if (cli_handle == NULL) { + return 0; + } + if (xQueueReceive(cli_handle, &msg, PEER_ADDR_PARAM) != pdTRUE) { + return 0; + } + if (msg.type != CLI_MSG_TYPE_PEER_ADDR) { + return 0; + } + + memcpy(peer_addr, msg.data.peer_addr, CLI_PEER_ADDR_LEN); + return 1; +} + int scli_receive_yesno(bool *console_key) { struct cli_msg msg; @@ -266,6 +317,11 @@ static esp_console_cmd_t cmds[] = { .help = "Enter Insert Yes for YES or Insert No for NO", .func = &yesno_handler, }, + { + .command = "peer", + .help = "Connect only to this peer address: peer xx:xx:xx:xx:xx:xx", + .func = &peer_addr_handler, + }, }; void ble_register_cli(void) diff --git a/examples/bluetooth/nimble/throughput_app/gatt/bleprph_throughput/main/gatt_svr.c b/examples/bluetooth/nimble/throughput_app/gatt/bleprph_throughput/main/gatt_svr.c index b5535b81e05..8f79a7f017f 100644 --- a/examples/bluetooth/nimble/throughput_app/gatt/bleprph_throughput/main/gatt_svr.c +++ b/examples/bluetooth/nimble/throughput_app/gatt/bleprph_throughput/main/gatt_svr.c @@ -148,6 +148,11 @@ gatt_svr_read_write_long_test(uint16_t conn_handle, uint16_t attr_handle, uint8_t requested_phy; os_mbuf_copydata(ctxt->om, 0, 1, &requested_phy); rc = 0; + if (requested_phy == THRPT_CMD_STOP_NOTIFY) { + ESP_LOGI(tag, "Central requested notify test stop via GATT command"); + bleprph_notify_stop_req(); + return 0; + } if (requested_phy == 0) { /* 0 = PHY_1M */ rc = ble_gap_set_prefered_le_phy(conn_handle, BLE_HCI_LE_PHY_1M_PREF_MASK, BLE_HCI_LE_PHY_1M_PREF_MASK, 0); ESP_LOGI(tag, "Central requested 1M PHY via GATT command"); diff --git a/examples/bluetooth/nimble/throughput_app/gatt/bleprph_throughput/main/gatts_sens.h b/examples/bluetooth/nimble/throughput_app/gatt/bleprph_throughput/main/gatts_sens.h index 1313c12156c..81d3d3128c0 100644 --- a/examples/bluetooth/nimble/throughput_app/gatt/bleprph_throughput/main/gatts_sens.h +++ b/examples/bluetooth/nimble/throughput_app/gatt/bleprph_throughput/main/gatts_sens.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2021 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -16,11 +16,16 @@ extern "C" { extern uint16_t notify_handle; +/* One byte command written by the central to THRPT_CHR_READ_WRITE. Values + * 0-3 select the PHY, this one asks the notify test to stop sending. */ +#define THRPT_CMD_STOP_NOTIFY 0xF0 + struct ble_hs_cfg; struct ble_gatt_register_ctxt; void gatt_svr_register_cb(struct ble_gatt_register_ctxt *ctxt, void *arg); int gatt_svr_init(void); +void bleprph_notify_stop_req(void); #ifdef __cplusplus } diff --git a/examples/bluetooth/nimble/throughput_app/gatt/bleprph_throughput/main/main.c b/examples/bluetooth/nimble/throughput_app/gatt/bleprph_throughput/main/main.c index a68fee989c1..413d545ca85 100644 --- a/examples/bluetooth/nimble/throughput_app/gatt/bleprph_throughput/main/main.c +++ b/examples/bluetooth/nimble/throughput_app/gatt/bleprph_throughput/main/main.c @@ -43,9 +43,12 @@ static char device_name[32] = "nimble_prph"; #define LL_PACKET_LENGTH 251 #define MTU_DEF 512 +#define NOTIFY_DISABLE_WAIT_MS 5000 /* Max wait for the central to clear the CCCD */ + static const char *tag = "bleprph_throughput"; static TaskHandle_t notify_task_handle = NULL; -static bool notify_state; +static volatile bool notify_state; +static volatile bool notify_stop_req; static int notify_test_time = 60; static uint16_t conn_handle; /* Dummy variable */ @@ -274,6 +277,18 @@ gatts_advertise(void) } #endif +/* Called from the GATT write handler when the central asks the notify test to + * stop. Stopping the traffic before the CCCD is cleared keeps the link idle + * enough for the central's ATT write to complete. */ +void +bleprph_notify_stop_req(void) +{ + notify_stop_req = true; + if (notify_task_handle) { + xTaskNotifyGive(notify_task_handle); + } +} + /* This function sends notifications to the client */ static void notify_task(void *arg) @@ -305,11 +320,12 @@ notify_task(void *arg) break; } - while (notify_state && notify_time < (notify_test_time * 1000)) { + while (notify_state && !notify_stop_req && + notify_time < (notify_test_time * 1000)) { ulTaskNotifyTake(pdFALSE, portMAX_DELAY); - /* Stop immediately if central unsubscribed */ - if (!notify_state) { + /* Stop immediately if central unsubscribed or asked us to stop */ + if (!notify_state || notify_stop_req) { break; } @@ -357,6 +373,17 @@ notify_task(void *arg) notify_count += 1; } + /* Traffic has stopped; give the central time to clear the CCCD so + * that the results are printed after "Notifications disabled". */ + if (notify_stop_req) { + int waited = 0; + while (notify_state && waited < NOTIFY_DISABLE_WAIT_MS) { + vTaskDelay(100 / portTICK_PERIOD_MS); + waited += 100; + } + notify_stop_req = false; + } + /* Use actual elapsed time for accurate throughput calculation */ { int actual_secs = (int)(notify_time / 1000); @@ -431,6 +458,7 @@ gatts_gap_event(struct ble_gap_event *event, void *arg) /* Stop notification task loop cleanly */ notify_state = false; + notify_stop_req = false; if (notify_task_handle) { xTaskNotifyGive(notify_task_handle); } @@ -464,9 +492,11 @@ gatts_gap_event(struct ble_gap_event *event, void *arg) if (event->subscribe.attr_handle == notify_handle) { notify_state = event->subscribe.cur_notify; if (notify_state) { - /* Always reset test time on new subscription. - * The central controls the actual duration by unsubscribing. - * Use a large default so the peripheral never stops on its own. */ + /* Always reset test time on new subscription. The central + * controls the actual duration: it sends THRPT_CMD_STOP_NOTIFY + * when done, so this is only an upper bound that keeps the + * peripheral from notifying forever if that command is lost. */ + notify_stop_req = false; notify_test_time = 3600; ESP_LOGI(tag, "Notifications enabled, test time = %d sec", notify_test_time); /* Prime the notification pipeline to allow multiple in-flight