Bluetooth: Host: Fix connection establishment upon RPA timeout #72674
Conversation
| /* Create connection canceled by timeout */ | ||
| } | ||
| } else { | ||
| int busy_status = k_work_delayable_busy_get(&conn->deferred_work); |
There was a problem hiding this comment.
The documentation suggests to not use k_work_delayable_busy_get(). Should we use another approach here instead?
There was a problem hiding this comment.
Can you refer to the documentation that states that here?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I need to look further into this!
There was a problem hiding this comment.
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
rugeGerritsen
force-pushed
the
fix_conn_create_with_rpa_timeout
branch
from
ac4e558 to
01ba584
Compare
zephyrbot
added
platform: nRF BSIM
zephyrbot
requested review from
aescolar,
alwa-nordic,
hermabe,
jhedberg,
jori-nordic,
sjanc,
Thalley and
theob-pro
|
Note: If we decide to do #71751, we will not need these complex code paths |
|
Unfortunately we will still have complexity regarding the resolving and accept lists. All roles have to be stopped for edits, @alwa-nordic is banging his head on this problem right now. He's implementing a module to take care of this properly. |
tests/bsim/bluetooth/host/privacy/central/test_scripts/run_test_short_conn_timeout.sh
Outdated
Show resolved
Hide resolved
tests/bsim/bluetooth/host/privacy/central/test_scripts/run_test_short_conn_timeout.sh
Outdated
Show resolved
Hide resolved
| err = bt_le_ext_adv_create(¶ms, &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); |
There was a problem hiding this comment.
Some clever people have worked on a testlib for BSIM, and I think some of this could be replaced by bt_testlib_adv_conn?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
That makes sense. Maybe the testlib can/should be expanded to support more options
tests/bsim/bluetooth/host/privacy/central/test_scripts/run_test_short_conn_timeout.sh
Show resolved
Hide resolved
rugeGerritsen
force-pushed
the
fix_conn_create_with_rpa_timeout
branch
2 times, most recently
from
6abdff3 to
e11a142
Compare
Before this commit, the following bugs were present: - When `CONFIG_BT_FILTER_ACCEPT_LIST` was set, connection establishment was cancelled upon RPA timeout. This required the application to restart the initiator every RPA timeout. - When `CONFIG_BT_FILTER_ACCEPT_LIST` was not set, the RPA was not updated while the initiator was running. This commit unifies the RPA timeout handling for both these cases. Upon RPA timeout the initiator is cancelled and restarted when the controller raises the LE Connection Complete event. The workqueue state is checked when restarting the initiator to prevent it being restarted when the timeout is hit. Corresponding test cases have been added to ensure that this feature works. Signed-off-by: Rubin Gerritsen <[email protected]>
rugeGerritsen
force-pushed
the
fix_conn_create_with_rpa_timeout
branch
from
e11a142 to
f373444
Compare
zephyrproject-rtos#72674 fixed a bug where this configuration did not work. Now that this configuration is tested, we should mark it as supported. The timeout check that was present in the code before was useless and was not working because the check was run before a default timeout of 0 was converted to a timeout. Signed-off-by: Rubin Gerritsen <[email protected]>
#72674 fixed a bug where this configuration did not work. Now that this configuration is tested, we should mark it as supported. The timeout check that was present in the code before was useless and was not working because the check was run before a default timeout of 0 was converted to a timeout. Signed-off-by: Rubin Gerritsen <[email protected]>
zephyrproject-rtos/zephyr#72674 fixed a bug where this configuration did not work. Now that this configuration is tested, we should mark it as supported. The timeout check that was present in the code before was useless and was not working because the check was run before a default timeout of 0 was converted to a timeout. (cherry picked from commit 9cf6839) Original-Signed-off-by: Rubin Gerritsen <[email protected]> GitOrigin-RevId: 9cf6839 Change-Id: Ice020d5825447400df12ccbd9be29fc9af00a7a1 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/5625521 Commit-Queue: Jeremy Bettis <[email protected]> Reviewed-by: Jeremy Bettis <[email protected]> Tested-by: ChromeOS Prod (Robot) <[email protected]> Tested-by: Jeremy Bettis <[email protected]>
… RPA timeout zephyrproject-rtos/zephyr#72674 fixed a bug where this configuration did not work. Now that this configuration is tested, we should mark it as supported. The timeout check that was present in the code before was useless and was not working because the check was run before a default timeout of 0 was converted to a timeout. Signed-off-by: Rubin Gerritsen <[email protected]> (cherry picked from commit 9cf6839)
… RPA timeout zephyrproject-rtos/zephyr#72674 fixed a bug where this configuration did not work. Now that this configuration is tested, we should mark it as supported. The timeout check that was present in the code before was useless and was not working because the check was run before a default timeout of 0 was converted to a timeout. Signed-off-by: Rubin Gerritsen <[email protected]> (cherry picked from commit 9cf6839)
Before this commit, the following bugs were present:
CONFIG_BT_FILTER_ACCEPT_LISTwas set, connection establishment was cancelled upon RPA timeout. This required the application to restart the initiator every RPA timeout.CONFIG_BT_FILTER_ACCEPT_LISTwas not set, the RPA was not updated while the initiator was running.This commit unifies the RPA timeout handling for both these cases. Upon RPA timeout the initiator is cancelled and restarted when the controller raises the LE Connection Complete event. The workqueue state is checked when restarting the initiator to prevent it being restarted when the timeout is hit.
Corresponding test cases have been added to ensure that this feature works.