summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
author Jakub Pawlowski <jpawlowski@google.com> 2024-10-22 22:15:02 +0200
committer Jakub Pawłowski <jpawlowski@google.com> 2024-12-18 10:13:40 -0800
commita10d311700c1a6f0dceb7f3b7257d4ee54bc751f (patch)
treeaea120a4034898581004404124f2e009dfe1d30d
parent8ccbfbd716c8478084dc6d80d724693ba0ab281c (diff)
connection_manager: one method for direct connect establishment
Currently in stack we have two methods for establishing direct conenct: * create_le_connection * direct_connect_add create_le_connection is making direct call to GD to schedule direct connection. It relies on timer in GD to stop the attempt after 30 seconds. It doesn't track which app requested direct connect, so it can't properly cancel direct connect if request was made from multiple clients, and just one cancels it. direct_connect_add keeps track of clients that made a request, but it works only if it's the only function used to establish direct connect. If create_le_connection, or GD is called directly, the counting breaks. The logic for scheduling/canceling direct connect using direct_connect_add was already tested in unit tests, but when create_le_connection is in use through the stack, those tests don't reflect the stack behavior. In create_le_connection, some extra address checks and lookups were added to prevent stack from attempting direct connect to targetted announcements. These are copied to direct_connect_add. Test: atest net_test_conn_multiplexing Bug: 372202918 Flag: EXEMPT, covered under unittests Change-Id: I76b2a4d5012fe473aa01a3df89e7b4794689a1d6
-rw-r--r--system/stack/connection_manager/connection_manager.cc64
-rw-r--r--system/stack/connection_manager/connection_manager.h32
-rw-r--r--system/stack/fuzzers/l2cap_fuzzer.cc4
-rw-r--r--system/stack/gatt/gatt_api.cc2
-rw-r--r--system/stack/gatt/gatt_main.cc2
-rw-r--r--system/stack/l2cap/l2c_ble.cc2
-rw-r--r--system/stack/test/connection_manager_test.cc5
-rw-r--r--system/test/mock/mock_stack_connection_manager.cc9
8 files changed, 65 insertions, 55 deletions
diff --git a/system/stack/connection_manager/connection_manager.cc b/system/stack/connection_manager/connection_manager.cc
index 4cd7df3f28..cc12e8a047 100644
--- a/system/stack/connection_manager/connection_manager.cc
+++ b/system/stack/connection_manager/connection_manager.cc
@@ -455,57 +455,59 @@ static void find_in_device_record(const RawAddress& bd_addr, tBLE_BD_ADDR* addre
return;
}
-bool create_le_connection(uint8_t /* id */, const RawAddress& bd_addr, tBLE_ADDR_TYPE addr_type) {
+bool direct_connect_add(uint8_t app_id, const RawAddress& address, tBLE_ADDR_TYPE addr_type) {
tBLE_BD_ADDR address_with_type{
.type = addr_type,
- .bda = bd_addr,
+ .bda = address,
};
- find_in_device_record(bd_addr, &address_with_type);
-
- log::debug("Creating le direct connection to:{} type:{} (initial type: {})", address_with_type,
- AddressTypeText(address_with_type.type), AddressTypeText(addr_type));
+ find_in_device_record(address, &address_with_type);
if (address_with_type.type == BLE_ADDR_ANONYMOUS) {
- log::warn(
- "Creating le direct connection to:{}, address type 'anonymous' is "
- "invalid",
- address_with_type);
+ log::warn("Can't use anonymous address for connection: {}", address_with_type);
return false;
}
- bluetooth::shim::ACL_AcceptLeConnectionFrom(address_with_type,
- /* is_direct */ true);
- return true;
-}
+ log::debug("app_id=0x{:x}, address={} (initial type: {})", static_cast<int>(app_id),
+ address_with_type, AddressTypeText(addr_type));
-/** Add a device to the direct connection list. Returns true if device
- * added to the list, false otherwise */
-bool direct_connect_add(uint8_t app_id, const RawAddress& address) {
- log::debug("app_id={}, address={}", static_cast<int>(app_id), address);
bool in_acceptlist = false;
auto it = bgconn_dev.find(address);
if (it != bgconn_dev.end()) {
+ const tAPPS_CONNECTING& info = it->second;
// app already trying to connect to this particular device
- if (it->second.doing_direct_conn.count(app_id)) {
- log::info("direct connect attempt from app_id=0x{:x} already in progress", app_id);
+ if (info.doing_direct_conn.count(app_id)) {
+ log::info("attempt from app_id=0x{:x} to {} already in progress", app_id, address_with_type);
return false;
}
+ // This is to match existing GD connection manager behavior - if multiple apps try direct
+ // connect at same time, only 1st request is fully processed
+ if (!info.doing_direct_conn.empty()) {
+ log::info("app_id=0x{:x}: attempt from other app already in progress, will merge {}", app_id,
+ address_with_type);
+ return true;
+ }
+
// are we already in the acceptlist ?
- if (it->second.is_in_accept_list) {
- log::warn("Background connection attempt already in progress app_id={:x}", app_id);
+ if (info.is_in_accept_list) {
+ log::warn("background connect attempt already in progress app_id=0x{:x} {}", app_id,
+ address_with_type);
in_acceptlist = true;
}
}
if (!in_acceptlist) {
- if (!bluetooth::shim::ACL_AcceptLeConnectionFrom(BTM_Sec_GetAddressWithType(address), true)) {
+ if (!bluetooth::shim::ACL_AcceptLeConnectionFrom(address_with_type, true /* is_direct */)) {
// if we can't add to acceptlist, turn parameters back to slow.
- log::warn("Unable to add le device to acceptlist");
+ log::warn("unable to add le device to acceptlist {}", address_with_type);
return false;
}
bgconn_dev[address].is_in_accept_list = true;
+ } else {
+ // if already in accept list, we should just bump parameters up for direct
+ // connection. There is no API for that yet, so use API that's adding to accept list.
+ bluetooth::shim::ACL_AcceptLeConnectionFrom(address_with_type, true /* is_direct */);
}
// Setup a timer
@@ -514,7 +516,6 @@ bool direct_connect_add(uint8_t app_id, const RawAddress& address) {
base::BindOnce(&wl_direct_connect_timeout_cb, app_id, address));
bgconn_dev[address].doing_direct_conn.emplace(app_id, unique_alarm_ptr(timeout, &alarm_free));
-
return true;
}
@@ -526,13 +527,13 @@ bool direct_connect_remove(uint8_t app_id, const RawAddress& address, bool conne
log::debug("app_id={}, address={}", static_cast<int>(app_id), address);
auto it = bgconn_dev.find(address);
if (it == bgconn_dev.end()) {
- log::warn("Unable to find background connection to remove peer:{}", address);
+ log::warn("unable to find entry to remove: {}", address);
return false;
}
auto app_it = it->second.doing_direct_conn.find(app_id);
if (app_it == it->second.doing_direct_conn.end()) {
- log::warn("Unable to find direct connection to remove peer:{}", address);
+ log::warn("unable to find direct connection to remove: {}", address);
return false;
}
@@ -544,13 +545,12 @@ bool direct_connect_remove(uint8_t app_id, const RawAddress& address, bool conne
if (is_anyone_interested_to_use_accept_list(it)) {
if (connection_timeout) {
- /* In such case we need to add device back to allow list because,
- * when connection timeout out, the lower layer removes device from
- * the allow list.
+ /* In such case we need to add device back to allow list because, when connection timeout
+ * out, the lower layer removes device from the allow list.
*/
if (!bluetooth::shim::ACL_AcceptLeConnectionFrom(BTM_Sec_GetAddressWithType(address),
- false)) {
- log::warn("Failed to re-add device {} to accept list after connection timeout", address);
+ false /* is_direct */)) {
+ log::warn("Failed to re-add {} to accept list after connection timeout", address);
}
}
return true;
diff --git a/system/stack/connection_manager/connection_manager.h b/system/stack/connection_manager/connection_manager.h
index 4d86eae0e3..0f029fbe60 100644
--- a/system/stack/connection_manager/connection_manager.h
+++ b/system/stack/connection_manager/connection_manager.h
@@ -37,10 +37,22 @@ namespace connection_manager {
using tAPP_ID = uint8_t;
-/* for background connection */
+/* Mark device as using targeted announcements.
+ *
+ * @return true if device added to the list, false otherwise */
bool background_connect_targeted_announcement_add(tAPP_ID app_id, const RawAddress& address);
+
+/* Add a background connect request.
+ *
+ * @return true if device added to the list, false otherwise */
bool background_connect_add(tAPP_ID app_id, const RawAddress& address);
+
+/* Remove a background connection request.
+ *
+ * @return true if the request is removed, false otherwise.
+ */
bool background_connect_remove(tAPP_ID app_id, const RawAddress& address);
+
bool remove_unconditional(const RawAddress& address);
void reset(bool after_reset);
@@ -50,15 +62,15 @@ void on_connection_complete(const RawAddress& address);
std::set<tAPP_ID> get_apps_connecting_to(const RawAddress& remote_bda);
-/* create_le_connection is adding device directly to AclManager, and relying on it's "direct
- * connect" implementation.
- * direct_connect_add method is doing multiplexing of apps request, and
- * sending the request to AclManager, but it lacks some extra checks and lookups. Currently these
- * methods are exclusive, if you try to use both you will get some bad behavior. These should be
- * merged into one. */
-bool create_le_connection(uint8_t /* id */, const RawAddress& bd_addr,
- tBLE_ADDR_TYPE addr_type = BLE_ADDR_PUBLIC);
-bool direct_connect_add(tAPP_ID app_id, const RawAddress& address);
+/* Add a direct connect request.
+ *
+ * @return true if device added to the list, false otherwise */
+bool direct_connect_add(tAPP_ID app_id, const RawAddress& address,
+ tBLE_ADDR_TYPE addr_type = BLE_ADDR_PUBLIC);
+/* Remove a direct connection request.
+ *
+ * @return true if the request is removed, false otherwise.
+ */
bool direct_connect_remove(tAPP_ID app_id, const RawAddress& address,
bool connection_timeout = false);
diff --git a/system/stack/fuzzers/l2cap_fuzzer.cc b/system/stack/fuzzers/l2cap_fuzzer.cc
index 1963983aef..c60d59f68a 100644
--- a/system/stack/fuzzers/l2cap_fuzzer.cc
+++ b/system/stack/fuzzers/l2cap_fuzzer.cc
@@ -97,8 +97,8 @@ void SnoopLogger::SetL2capChannelOpen(uint16_t, uint16_t, uint16_t, uint16_t, bo
} // namespace bluetooth
namespace connection_manager {
-bool create_le_connection(uint8_t /* id */, const RawAddress& /* bd_addr */,
- tBLE_ADDR_TYPE /* addr_type */) {
+bool direct_connect_add(uint8_t /* id */, const RawAddress& /* bd_addr */,
+ tBLE_ADDR_TYPE /* addr_type */) {
return true;
}
} // namespace connection_manager
diff --git a/system/stack/gatt/gatt_api.cc b/system/stack/gatt/gatt_api.cc
index fa42e91aaa..19ea3a3090 100644
--- a/system/stack/gatt/gatt_api.cc
+++ b/system/stack/gatt/gatt_api.cc
@@ -1494,7 +1494,7 @@ bool GATT_Connect(tGATT_IF gatt_if, const RawAddress& bd_addr, tBLE_ADDR_TYPE ad
log::warn("{} already added to gatt_if {} direct conn list", bd_addr, gatt_if);
}
- ret = connection_manager::create_le_connection(gatt_if, bd_addr, addr_type);
+ ret = connection_manager::direct_connect_add(gatt_if, bd_addr, addr_type);
}
} else {
diff --git a/system/stack/gatt/gatt_main.cc b/system/stack/gatt/gatt_main.cc
index e530d9fd5e..0d8d51d846 100644
--- a/system/stack/gatt/gatt_main.cc
+++ b/system/stack/gatt/gatt_main.cc
@@ -235,7 +235,7 @@ static bool gatt_connect(const RawAddress& rem_bda, tBLE_ADDR_TYPE addr_type, tG
}
p_tcb->att_lcid = L2CAP_ATT_CID;
- return connection_manager::create_le_connection(gatt_if, rem_bda, addr_type);
+ return connection_manager::direct_connect_add(gatt_if, rem_bda, addr_type);
}
/*******************************************************************************
diff --git a/system/stack/l2cap/l2c_ble.cc b/system/stack/l2cap/l2c_ble.cc
index 389fb798dd..9a76d92be7 100644
--- a/system/stack/l2cap/l2c_ble.cc
+++ b/system/stack/l2cap/l2c_ble.cc
@@ -944,7 +944,7 @@ void l2cble_process_sig_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) {
/** This function is to initiate a direct connection. Returns true if connection
* initiated, false otherwise. */
bool l2cble_create_conn(tL2C_LCB* p_lcb) {
- if (!connection_manager::create_le_connection(CONN_MGR_ID_L2CAP, p_lcb->remote_bd_addr)) {
+ if (!connection_manager::direct_connect_add(CONN_MGR_ID_L2CAP, p_lcb->remote_bd_addr)) {
return false;
}
diff --git a/system/stack/test/connection_manager_test.cc b/system/stack/test/connection_manager_test.cc
index 80d25763d3..e875548f16 100644
--- a/system/stack/test/connection_manager_test.cc
+++ b/system/stack/test/connection_manager_test.cc
@@ -262,9 +262,10 @@ TEST_F(BleConnectionManager, test_app_unregister) {
*/
EXPECT_CALL(*localAcceptlistMock, AcceptlistAdd(address1, true)).WillOnce(Return(true));
- EXPECT_CALL(*localAcceptlistMock, AcceptlistAdd(address2, false)).WillOnce(Return(true));
EXPECT_TRUE(direct_connect_add(CLIENT1, address1));
+ EXPECT_CALL(*localAcceptlistMock, AcceptlistAdd(address2, false)).WillOnce(Return(true));
EXPECT_TRUE(background_connect_add(CLIENT1, address2));
+ EXPECT_CALL(*localAcceptlistMock, AcceptlistAdd(address2, true)).WillOnce(Return(true));
EXPECT_TRUE(direct_connect_add(CLIENT2, address2));
Mock::VerifyAndClearExpectations(localAcceptlistMock.get());
@@ -274,6 +275,8 @@ TEST_F(BleConnectionManager, test_app_unregister) {
EXPECT_CALL(*localAcceptlistMock, AcceptlistRemove(address2)).Times(1);
on_app_deregistered(CLIENT2);
+
+ Mock::VerifyAndClearExpectations(localAcceptlistMock.get());
}
/** Verify adding device to both direct connection and background connection. */
diff --git a/system/test/mock/mock_stack_connection_manager.cc b/system/test/mock/mock_stack_connection_manager.cc
index 97cd160854..5f4d045e63 100644
--- a/system/test/mock/mock_stack_connection_manager.cc
+++ b/system/test/mock/mock_stack_connection_manager.cc
@@ -44,13 +44,8 @@ bool connection_manager::background_connect_remove(uint8_t /* app_id */,
return false;
}
-bool connection_manager::create_le_connection(uint8_t /* id */, const RawAddress& /* bd_addr */,
- tBLE_ADDR_TYPE /* addr_type */) {
- inc_func_call_count(__func__);
- return false;
-}
-
-bool connection_manager::direct_connect_add(uint8_t /* app_id */, const RawAddress& /* address */) {
+bool connection_manager::direct_connect_add(uint8_t /* app_id */, const RawAddress& /* address */,
+ tBLE_ADDR_TYPE /* addr_type */) {
inc_func_call_count(__func__);
return false;
}