Skip to content

Bluetooth: Controller: Fix conn timeout due to delayed conn upd ind #92572

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+70 −17
Open

Bluetooth: Controller: Fix conn timeout due to delayed conn upd ind #92572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f2e11a6
Bluetooth: Controller: Remove duplicate define CONN_DATA_BUFFERS
cvinayak Jul 4, 2025
3b15397
tests: bsim: Bluetooth: Add param update to throughput test
cvinayak Jul 4, 2025
8c72e60
tests: bsim: Bluetooth: Fail throughput test with increased Tx count
cvinayak Jul 4, 2025
a3347a3
Bluetooth: Controller: Fix conn timeout due to delayed conn upd ind
cvinayak Jul 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions samples/bluetooth/central_gatt_write/src/gatt_write_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
static uint32_t phy_update_countdown;
static uint8_t phy_param_idx;

/* Count down number of metrics intervals before performing a param update */
#define PARAM_UPDATE_COUNTDOWN PHY_UPDATE_COUNTDOWN
Comment on lines +20 to +21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this feels like even more test code that isn't related to GATT :( Would prefer that we test things like this in BSIM, instead of polluting a GATT sample with PHY tests (personal opinion)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not be related to GATT but a typical user application requiring a GATT Write without Response will perform both PHY update and Connection Update to tune the throughput. In that sense, is a sample that demonstrate GATT Write without Response, PHY Update and Connection Update (there are no samples for these procedures currently in the upstream repo!).

static uint32_t param_update_countdown;

static void phy_update_iterate(struct bt_conn *conn)
{
const struct bt_conn_le_phy_param phy_param[] = {
Expand Down Expand Up @@ -178,6 +182,34 @@ static void write_cmd_cb(struct bt_conn *conn, void *user_data)
phy_update_iterate(conn);
}

const struct bt_le_conn_param update_params[2] = {{
.interval_min = 0x0029,
.interval_max = 0x0029,
.latency = 0,
.timeout = 31,
}, {
.interval_min = 0x0028,
.interval_max = 0x0028,
.latency = 0,
.timeout = 30,
Comment on lines +186 to +194
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using the BT_GAP_MS_TO_CONN_INTERVAL or BT_GAP_US_TO_CONN_INTERVAL and BT_GAP_MS_TO_CONN_TIMEOUT or BT_GAP_US_TO_CONN_TIMEOUT for more human-readable values

},
};
static uint8_t param_update_count;
int err;

if ((param_update_countdown--) != 0U) {
return;
}

param_update_countdown = PARAM_UPDATE_COUNTDOWN;

err = bt_conn_le_param_update(conn, &update_params[param_update_count & 0x1]);
Comment on lines +205 to +206
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to add something like

Suggested change
err = bt_conn_le_param_update(conn, &update_params[param_update_count & 0x1]);
/* Switch between the 2 update_params based on param_update_count */
err = bt_conn_le_param_update(conn, &update_params[param_update_count & 0x1]);

Given that this is a sample

if (err != 0) {
printk("Parameter update failed (err %d)\n", err);
}

param_update_count++;

} else {
uint16_t len;

Expand Down Expand Up @@ -256,6 +288,14 @@ static void connected(struct bt_conn *conn, uint8_t conn_err)
phy_update_countdown = PHY_UPDATE_COUNTDOWN;
phy_param_idx = 0U;
}

/* Every 1 second the acknowledged total GATT Write without Response data size is used for
* the throughput calculation.
* PHY update is performed in reference to this calculation interval, and connection update
* is offset by 1 of this interval so that connection update is initiated one such interval
* after PHY update was requested.
*/
param_update_countdown = PARAM_UPDATE_COUNTDOWN + 1U;
}

static void disconnected(struct bt_conn *conn, uint8_t reason)
Expand Down
35 changes: 21 additions & 14 deletions subsys/bluetooth/controller/ll_sw/ull_conn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,23 +127,18 @@ static uint8_t force_md_cnt_calc(struct lll_conn *lll_conn, uint32_t tx_rate);
(LL_LENGTH_OCTETS_TX_MAX + \
BT_CTLR_USER_TX_BUFFER_OVERHEAD))

#define CONN_DATA_BUFFERS CONFIG_BT_BUF_ACL_TX_COUNT

static MFIFO_DEFINE(conn_tx, sizeof(struct lll_tx), CONN_DATA_BUFFERS);
static MFIFO_DEFINE(conn_tx, sizeof(struct lll_tx), CONFIG_BT_BUF_ACL_TX_COUNT);
static MFIFO_DEFINE(conn_ack, sizeof(struct lll_tx),
(CONN_DATA_BUFFERS +
LLCP_TX_CTRL_BUF_COUNT));
(CONFIG_BT_BUF_ACL_TX_COUNT + LLCP_TX_CTRL_BUF_COUNT));

static struct {
void *free;
uint8_t pool[CONN_TX_BUF_SIZE * CONN_DATA_BUFFERS];
uint8_t pool[CONN_TX_BUF_SIZE * CONFIG_BT_BUF_ACL_TX_COUNT];
} mem_conn_tx;

static struct {
void *free;
uint8_t pool[sizeof(memq_link_t) *
(CONN_DATA_BUFFERS +
LLCP_TX_CTRL_BUF_COUNT)];
uint8_t pool[sizeof(memq_link_t) * (CONFIG_BT_BUF_ACL_TX_COUNT + LLCP_TX_CTRL_BUF_COUNT)];
} mem_link_tx;

#if defined(CONFIG_BT_CTLR_DATA_LENGTH)
Expand Down Expand Up @@ -1483,6 +1478,21 @@ void ull_conn_tx_lll_enqueue(struct ll_conn *conn, uint8_t count)
}
}

uint16_t ull_conn_tx_enqueue_count_get(struct ll_conn *conn)
{
uint16_t count = 0;
memq_link_t *curr;

curr = memq_peek(conn->lll.memq_tx.head, conn->lll.memq_tx.tail, NULL);
while (curr != NULL) {
count++;

curr = memq_peek(curr->next, conn->lll.memq_tx.tail, NULL);
}

return count;
Comment on lines 1478 to +1493
Copy link
Preview

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function iterates through the entire TX queue on each call (O(n) complexity); consider maintaining a counter that tracks the queue length to avoid walking the list every time.

Suggested change
}
}
uint16_t ull_conn_tx_enqueue_count_get(struct ll_conn *conn)
{
uint16_t count = 0;
memq_link_t *curr;
curr = memq_peek(conn->lll.memq_tx.head, conn->lll.memq_tx.tail, NULL);
while (curr != NULL) {
count++;
curr = memq_peek(curr->next, conn->lll.memq_tx.tail, NULL);
}
return count;
conn->tx_queue_count++;
}
}
uint16_t ull_conn_tx_enqueue_count_get(struct ll_conn *conn)
{
return conn->tx_queue_count;

Copilot uses AI. Check for mistakes.

}

void ull_conn_link_tx_release(void *link)
{
mem_release(link, &mem_link_tx.free);
Expand Down Expand Up @@ -1693,14 +1703,11 @@ static int init_reset(void)
}

/* Initialize tx pool. */
mem_init(mem_conn_tx.pool, CONN_TX_BUF_SIZE, CONN_DATA_BUFFERS,
&mem_conn_tx.free);
mem_init(mem_conn_tx.pool, CONN_TX_BUF_SIZE, CONFIG_BT_BUF_ACL_TX_COUNT, &mem_conn_tx.free);

/* Initialize tx link pool. */
mem_init(mem_link_tx.pool, sizeof(memq_link_t),
(CONN_DATA_BUFFERS +
LLCP_TX_CTRL_BUF_COUNT),
&mem_link_tx.free);
(CONFIG_BT_BUF_ACL_TX_COUNT + LLCP_TX_CTRL_BUF_COUNT), &mem_link_tx.free);

/* Initialize control procedure system. */
ull_cp_init();
Expand Down
1 change: 1 addition & 0 deletions subsys/bluetooth/controller/ll_sw/ull_conn_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ int ull_conn_llcp(struct ll_conn *conn, uint32_t ticks_at_expire,
void ull_conn_done(struct node_rx_event_done *done);
void ull_conn_tx_demux(uint8_t count);
void ull_conn_tx_lll_enqueue(struct ll_conn *conn, uint8_t count);
uint16_t ull_conn_tx_enqueue_count_get(struct ll_conn *conn);
void ull_conn_link_tx_release(void *link);
uint8_t ull_conn_ack_last_idx_get(void);
memq_link_t *ull_conn_ack_peek(uint8_t *ack_last, uint16_t *handle,
Expand Down
6 changes: 5 additions & 1 deletion subsys/bluetooth/controller/ll_sw/ull_llcp_conn_upd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,9 @@ static void lp_cu_send_conn_update_ind_finalize(struct ll_conn *conn, struct pro
static void lp_cu_send_conn_update_ind(struct ll_conn *conn, struct proc_ctx *ctx, uint8_t evt,
void *param)
{
if (llcp_lr_ispaused(conn) || !llcp_tx_alloc_peek(conn, ctx)) {
if (llcp_lr_ispaused(conn) || !llcp_tx_alloc_peek(conn, ctx) ||
ull_conn_tx_enqueue_count_get(conn) != 0U) {
llcp_tx_pause_data(conn, LLCP_TX_QUEUE_PAUSE_DATA_CONN_UPD);
ctx->state = LP_CU_STATE_WAIT_TX_CONN_UPDATE_IND;
} else {
/* ensure alloc of TX node, before possibly waiting for NTF node */
Expand All @@ -448,6 +450,7 @@ static void lp_cu_send_conn_update_ind(struct ll_conn *conn, struct proc_ctx *ct
ctx->state = LP_CU_STATE_WAIT_NTF_AVAIL;
} else {
lp_cu_send_conn_update_ind_finalize(conn, ctx, evt, param);
llcp_tx_resume_data(conn, LLCP_TX_QUEUE_PAUSE_DATA_CONN_UPD);
Copy link
Preview

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The resume call is duplicated in multiple branches; consider centralizing resume logic (e.g., in lp_cu_send_conn_update_ind_finalize) to reduce code duplication and ease maintenance.

Suggested change
llcp_tx_resume_data(conn, LLCP_TX_QUEUE_PAUSE_DATA_CONN_UPD);

Copilot uses AI. Check for mistakes.

}
}
}
Expand All @@ -459,6 +462,7 @@ static void lp_cu_st_wait_ntf_avail(struct ll_conn *conn, struct proc_ctx *ctx,
case LP_CU_EVT_RUN:
if (llcp_ntf_alloc_is_available()) {
lp_cu_send_conn_update_ind_finalize(conn, ctx, evt, param);
llcp_tx_resume_data(conn, LLCP_TX_QUEUE_PAUSE_DATA_CONN_UPD);
}
break;
default:
Expand Down
1 change: 1 addition & 0 deletions subsys/bluetooth/controller/ll_sw/ull_llcp_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ enum llcp_tx_q_pause_data_mask {
LLCP_TX_QUEUE_PAUSE_DATA_PHY_UPDATE = 0x02,
LLCP_TX_QUEUE_PAUSE_DATA_DATA_LENGTH = 0x04,
LLCP_TX_QUEUE_PAUSE_DATA_TERMINATE = 0x08,
LLCP_TX_QUEUE_PAUSE_DATA_CONN_UPD = 0x10,
};

#if ((CONFIG_BT_CTLR_LLCP_COMMON_TX_CTRL_BUF_NUM <\
Expand Down
4 changes: 2 additions & 2 deletions tests/bsim/bluetooth/ll/throughput/prj.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,15 @@ CONFIG_BT_BUF_CMD_TX_SIZE=255

# BT HCI Event Buffers
# Greater than BT_BUF_ACL_TX_COUNT, to be able to receive Number of Completed Packets events
CONFIG_BT_BUF_EVT_RX_COUNT=4
CONFIG_BT_BUF_EVT_RX_COUNT=7
CONFIG_BT_BUF_EVT_RX_SIZE=255

# BT HCI Discardable Event Buffers
CONFIG_BT_BUF_EVT_DISCARDABLE_COUNT=3
CONFIG_BT_BUF_EVT_DISCARDABLE_SIZE=255

# BT HCI ACL TX Data Buffers
CONFIG_BT_BUF_ACL_TX_COUNT=3
CONFIG_BT_BUF_ACL_TX_COUNT=6
CONFIG_BT_BUF_ACL_TX_SIZE=251

# BT HCI ACL RX Data Buffers
Expand Down
Loading