Skip to content
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: Host: Fix connection establishment upon RPA timeout #72674

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
36 changes: 20 additions & 16 deletions subsys/bluetooth/host/hci_core.c
Original file line number Diff line number Diff line change
Expand Up @@ -1159,6 +1159,7 @@ static void conn_auto_initiate(struct bt_conn *conn)

static void le_conn_complete_cancel(uint8_t err)
{
int ret;
struct bt_conn *conn;

/* Handle create connection cancel.
Expand All @@ -1172,27 +1173,30 @@ static void le_conn_complete_cancel(uint8_t err)
return;
}

conn->err = err;

/* Handle cancellation of outgoing connection attempt. */
if (!IS_ENABLED(CONFIG_BT_FILTER_ACCEPT_LIST)) {
/* We notify before checking autoconnect flag
* as application may choose to change it from
* callback.
*/
bt_conn_set_state(conn, BT_CONN_DISCONNECTED);
/* Check if device is marked for autoconnect. */
if (atomic_test_bit(conn->flags, BT_CONN_AUTO_CONNECT)) {
if (atomic_test_bit(conn->flags, BT_CONN_AUTO_CONNECT)) {
if (!IS_ENABLED(CONFIG_BT_FILTER_ACCEPT_LIST)) {
/* Restart passive scanner for device */
bt_conn_set_state(conn, BT_CONN_SCAN_BEFORE_INITIATING);
} else {
/* Restart FAL initiator after RPA timeout. */
ret = bt_le_create_conn(conn);
if (ret) {
LOG_ERR("Failed to restart initiator");
}
}
} else {
if (atomic_test_bit(conn->flags, BT_CONN_AUTO_CONNECT)) {
/* Restart FAL initiator after RPA timeout. */
bt_le_create_conn(conn);
} else {
/* Create connection canceled by timeout */
int busy_status = k_work_delayable_busy_get(&conn->deferred_work);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The documentation suggests to not use k_work_delayable_busy_get(). Should we use another approach here instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you refer to the documentation that states that here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking about https://docs.zephyrproject.org/latest/kernel/services/threads/workqueue.html#don-t-optimize-prematurely. It feels a bit dangerous to check the state of a work queue item

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could an alternative be to use https://docs.zephyrproject.org/latest/kernel/services/threads/workqueue.html#c.k_work_cancel_delayable_sync and use the return value to determine the next step?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need to look further into this!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think there is another way right now - we cannot simply cancel it as we need to know how much time is remaining. As long as the Bluetooth threads are cooperative this will work fine


if (!(busy_status & (K_WORK_QUEUED | K_WORK_DELAYED))) {
/* Connection initiation timeout triggered. */
conn->err = err;
bt_conn_set_state(conn, BT_CONN_DISCONNECTED);
} else {
/* Restart initiator after RPA timeout. */
ret = bt_le_create_conn(conn);
if (ret) {
LOG_ERR("Failed to restart initiator");
}
}
}

Expand Down
1 change: 0 additions & 1 deletion subsys/bluetooth/host/id.c
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,6 @@ static void le_update_private_addr(void)
}
#endif
if (IS_ENABLED(CONFIG_BT_CENTRAL) &&
IS_ENABLED(CONFIG_BT_FILTER_ACCEPT_LIST) &&
atomic_test_bit(bt_dev.flags, BT_DEV_INITIATING)) {
/* Canceled initiating procedure will be restarted by
* connection complete event.
Expand Down
6 changes: 6 additions & 0 deletions tests/bsim/bluetooth/host/privacy/central/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ cmake_minimum_required(VERSION 3.20.0)
find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
project(bsim_test_rpa_central)

add_subdirectory(${ZEPHYR_BASE}/tests/bluetooth/common/testlib testlib)
target_link_libraries(app PRIVATE testlib)

add_subdirectory(${ZEPHYR_BASE}/tests/bsim/babblekit babblekit)
target_link_libraries(app PRIVATE babblekit)

target_sources(app PRIVATE
src/bs_bt_utils.c
src/tester.c
Expand Down
5 changes: 5 additions & 0 deletions tests/bsim/bluetooth/host/privacy/central/prj.conf
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,8 @@ CONFIG_BT_RPA_TIMEOUT=10
CONFIG_BT_CTLR_ADV_SET=2
CONFIG_BT_CTLR_SCAN_REQ_NOTIFY=y
CONFIG_BT_SCAN_WITH_IDENTITY=y

# Enable the possibility to set the RPA such that the RPA
# refreshes before the connection attempt has completed
CONFIG_BT_RPA_TIMEOUT_DYNAMIC=y
CONFIG_BT_CREATE_CONN_TIMEOUT=10
70 changes: 70 additions & 0 deletions tests/bsim/bluetooth/host/privacy/central/src/dut.c
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
#include <zephyr/bluetooth/conn.h>
#include <zephyr/bluetooth/bluetooth.h>

#include <babblekit/testcase.h>
#include <testlib/conn.h>
#include <testlib/scan.h>

void start_scanning(void)
{
int err;
Expand Down Expand Up @@ -46,3 +50,69 @@ void dut_procedure(void)

PASS("PASS\n");
}

void dut_procedure_connect_short_rpa_timeout(void)
{
backchannel_init(1);

const uint16_t rpa_timeout_s = 1;

int err;
bt_addr_le_t peer = {};
struct bt_conn *conn = NULL;

/* Enable bluetooth */
err = bt_enable(NULL);
TEST_ASSERT(!err, "Failed to enable bluetooth (err %d)");

/* Central to use a short RPA timeout */
err = bt_le_set_rpa_timeout(rpa_timeout_s);
TEST_ASSERT(!err, "Failed to set RPA timeout (err %d)", err);

err = bt_testlib_scan_find_name(&peer, CONFIG_BT_DEVICE_NAME);
TEST_ASSERT(!err, "Failed to start scan (err %d)", err);

/* Indicate to the peer device that we have found the advertiser. */
backchannel_sync_send();

/* Create a connection using that address */
err = bt_testlib_connect(&peer, &conn);
TEST_ASSERT(!err, "Failed to initiate connection (err %d)", err);

PASS("PASS\n");
}

void dut_procedure_connect_timeout(void)
{
const uint16_t rpa_timeout_s = 1;

int err;
bt_addr_le_t peer = {.type = BT_ADDR_LE_RANDOM, .a = {.val = {1, 2, 3, 4, 5, 6}}};
struct bt_conn *conn = NULL;

/* Enable bluetooth */
err = bt_enable(NULL);
TEST_ASSERT(!err, "Failed to enable bluetooth (err %d)", err);

/* Central to use a short RPA timeout */
err = bt_le_set_rpa_timeout(rpa_timeout_s);
TEST_ASSERT(!err, "Failed to set RPA timeout (err %d)", err);

int64_t old_time = k_uptime_get();

/* Create a connection using that address */
err = bt_testlib_connect(&peer, &conn);
TEST_ASSERT(err == BT_HCI_ERR_UNKNOWN_CONN_ID,
"Expected connection establishment to time out (err %d)", err);

int64_t new_time = k_uptime_get();
int64_t time_diff_ms = new_time - old_time;
int64_t expected_conn_timeout_ms = CONFIG_BT_CREATE_CONN_TIMEOUT * 1000;
int64_t diff_to_expected_ms = abs(time_diff_ms - expected_conn_timeout_ms);

printk("Connection creation timed out after %d ms\n", time_diff_ms);
TEST_ASSERT(diff_to_expected_ms < 0.1 * expected_conn_timeout_ms,
"Connection timeout not within 10 \%% of expected timeout");

PASS("PASS\n");
}
30 changes: 30 additions & 0 deletions tests/bsim/bluetooth/host/privacy/central/src/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,51 @@
#include "bstests.h"

void tester_procedure(void);
void tester_procedure_periph_delayed_start_of_conn_adv(void);
void dut_procedure(void);
void dut_procedure_connect_short_rpa_timeout(void);
void dut_procedure_connect_timeout(void);

static const struct bst_test_instance test_to_add[] = {
{
.test_id = "central",
.test_descr = "Central performs active scanning using RPA",
.test_post_init_f = test_init,
.test_tick_f = test_tick,
.test_main_f = dut_procedure,
},
{
.test_id = "central_connect_short_rpa_timeout",
.test_descr = "Central connects to a peripheral using a short RPA timeout",
.test_post_init_f = test_init,
.test_tick_f = test_tick,
.test_main_f = dut_procedure_connect_short_rpa_timeout,
},
{
.test_id = "central_connect_fails_with_short_rpa_timeout",
.test_descr = "Central connects to a peripheral using a short RPA timeout"
" but expects connection establishment to time out.",
.test_post_init_f = test_init,
.test_tick_f = test_tick,
.test_main_f = dut_procedure_connect_timeout,
},
{
.test_id = "peripheral",
.test_descr = "Performs scannable advertising, validates that the scanner"
" RPA address refreshes",
.test_post_init_f = test_init,
.test_tick_f = test_tick,
.test_main_f = tester_procedure,
},
{
.test_id = "periph_delayed_start_of_conn_adv",
.test_descr = "Performs connectable advertising. "
"The advertiser is stopped for 10 seconds when instructed by the DUT"
" to allow it to run the initiator for longer than its RPA timeout.",
.test_post_init_f = test_init,
.test_tick_f = test_tick,
.test_main_f = tester_procedure_periph_delayed_start_of_conn_adv,
},
BSTEST_END_MARKER,
};

Expand Down
60 changes: 60 additions & 0 deletions tests/bsim/bluetooth/host/privacy/central/src/tester.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@
#include <zephyr/bluetooth/conn.h>
#include <zephyr/toolchain.h>

#include <babblekit/testcase.h>

DEFINE_FLAG(flag_new_address);
DEFINE_FLAG(flag_connected);

void scanned_cb(struct bt_le_ext_adv *adv, struct bt_le_ext_adv_scanned_info *info)
{
Expand Down Expand Up @@ -64,8 +67,14 @@ void scanned_cb(struct bt_le_ext_adv *adv, struct bt_le_ext_adv_scanned_info *in
old_addr = new_addr;
}

static void connected_cb(struct bt_le_ext_adv *adv, struct bt_le_ext_adv_connected_info *info)
{
SET_FLAG(flag_connected);
}

static struct bt_le_ext_adv_cb adv_callbacks = {
.scanned = scanned_cb,
.connected = connected_cb
};

void start_advertising(void)
Expand Down Expand Up @@ -125,3 +134,54 @@ void tester_procedure(void)

PASS("PASS\n");
}

void tester_procedure_periph_delayed_start_of_conn_adv(void)
{
backchannel_init(0);

int err;
struct bt_le_adv_param params =
BT_LE_ADV_PARAM_INIT(BT_LE_ADV_OPT_CONNECTABLE | BT_LE_ADV_OPT_USE_IDENTITY,
BT_GAP_ADV_FAST_INT_MIN_2,
BT_GAP_ADV_FAST_INT_MAX_2, NULL);
struct bt_data ad;
struct bt_le_ext_adv *adv;

/* Enable bluetooth */
err = bt_enable(NULL);
TEST_ASSERT(!err, "Failed to enable bluetooth (err %d)");

/* Advertiser to use a long RPA timeout */
err = bt_le_set_rpa_timeout(100);
TEST_ASSERT(!err, "Failed to set RPA timeout (err %d)", err);

err = bt_le_ext_adv_create(&params, &adv_callbacks, &adv);
TEST_ASSERT(!err, "Failed to create advertising set (err %d)", err);

ad.type = BT_DATA_NAME_COMPLETE;
ad.data_len = strlen(CONFIG_BT_DEVICE_NAME);
ad.data = (const uint8_t *)CONFIG_BT_DEVICE_NAME;

err = bt_le_ext_adv_set_data(adv, &ad, 1, NULL, 0);
TEST_ASSERT(!err, "Failed to set advertising data (err %d)", err);

err = bt_le_ext_adv_start(adv, BT_LE_EXT_ADV_START_DEFAULT);
TEST_ASSERT(!err, "Failed to start advertiser (err %d)", err);
Comment on lines +158 to +169
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some clever people have worked on a testlib for BSIM, and I think some of this could be replaced by bt_testlib_adv_conn?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I looked into that initially. It is nice to have such a library. Here I explicitly wanted to use BT_LE_ADV_OPT_USE_IDENTITY so that the advertiser would use the same address when restarting

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense. Maybe the testlib can/should be expanded to support more options


backchannel_sync_wait();

err = bt_le_ext_adv_stop(adv);
TEST_ASSERT(!err, "Failed to stop advertiser (err %d)", err);

/* Wait a few RPA cycles before restaring the advertiser to force RPA timeout
* on the DUT.
*/
k_sleep(K_SECONDS(7));
Thalley marked this conversation as resolved.
Show resolved Hide resolved

err = bt_le_ext_adv_start(adv, BT_LE_EXT_ADV_START_DEFAULT);
TEST_ASSERT(!err, "Failed to restart advertiser (err %d)", err);

WAIT_FOR_FLAG(flag_connected);

PASS("PASS\n");
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
#!/usr/bin/env bash
# Copyright 2024 Nordic Semiconductor ASA
# SPDX-License-Identifier: Apache-2.0
set -eu

source ${ZEPHYR_BASE}/tests/bsim/sh_common.source

EXECUTE_TIMEOUT=100
verbosity_level=2

# Test that connection establishment times out when RPA
# timeout is shorter than the connection establishment timeout
simulation_id="test_central_connect_fails_with_short_rpa_timeout"

central_exe="${BSIM_OUT_PATH}/bin/bs_${BOARD_TS}_$(guess_test_long_name)_prj_conf"
Thalley marked this conversation as resolved.
Show resolved Hide resolved

cd ${BSIM_OUT_PATH}/bin

Execute "$central_exe" \
-v=${verbosity_level} -s=${simulation_id} -d=0 \
-testid=central_connect_fails_with_short_rpa_timeout -RealEncryption=1

Execute ./bs_2G4_phy_v1 -v=${verbosity_level} -s=${simulation_id} \
-D=1 -sim_length=60e6 $@

wait_for_background_jobs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#!/usr/bin/env bash
# Copyright 2024 Nordic Semiconductor ASA
# SPDX-License-Identifier: Apache-2.0
set -eu

source ${ZEPHYR_BASE}/tests/bsim/sh_common.source

EXECUTE_TIMEOUT=100
verbosity_level=2

# Test connection establishment when the RPA times out before
# we expect the connection to be established
simulation_id="test_central_connect_short_rpa_timeout"

central_exe="${BSIM_OUT_PATH}/bin/bs_${BOARD_TS}_$(guess_test_long_name)_prj_conf"
peripheral_exe="${central_exe}"

cd ${BSIM_OUT_PATH}/bin

Execute "$central_exe" \
-v=${verbosity_level} -s=${simulation_id} -d=0 \
-testid=central_connect_short_rpa_timeout -RealEncryption=1

Execute "$peripheral_exe" \
-v=${verbosity_level} -s=${simulation_id} -d=1 \
-testid=periph_delayed_start_of_conn_adv -RealEncryption=1

Execute ./bs_2G4_phy_v1 -v=${verbosity_level} -s=${simulation_id} \
-D=2 -sim_length=60e6 $@

wait_for_background_jobs
Loading