[bt][fidl] Prefer BR/EDR for connections to dual-mode peers
fuchsia.bluetooth.host.Host.Connect now prefers the BR/EDR transport if
the peer supports it. This changes the old behavior that limited
BR/EDR connections to just BR/EDR-only peers.
Bug: fxb/1242, fxb/40004
Test: bt-host-unittests - FIDL_HostServerTest
Change-Id: I62aea9afa8925a0072fd2d2d010da601a91cf7c4
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/334837
Commit-Queue: Arman Uguray <armansito@google.com>
Reviewed-by: Lucas Jenkins <lucasjenkins@google.com>
Testability-Review: Lucas Jenkins <lucasjenkins@google.com>
diff --git a/pw_bluetooth_sapphire/host/fidl/host_server.cc b/pw_bluetooth_sapphire/host/fidl/host_server.cc
index dd2a108..4460ffa 100644
--- a/pw_bluetooth_sapphire/host/fidl/host_server.cc
+++ b/pw_bluetooth_sapphire/host/fidl/host_server.cc
@@ -523,10 +523,10 @@
return;
}
- // TODO(BT-649): Dual-mode currently not supported; if the peer supports
- // LowEnergy we assume LE. If a dual-mode peer, we should attempt to connect
+ // TODO(fxb/1242): Dual-mode currently not supported; if the peer supports
+ // BR/EDR we prefer BR/EDR. If a dual-mode peer, we should attempt to connect
// both protocols.
- if (!peer->le()) {
+ if (peer->bredr()) {
ConnectBrEdr(id, std::move(callback));
return;
}
diff --git a/pw_bluetooth_sapphire/host/fidl/host_server_unittest.cc b/pw_bluetooth_sapphire/host/fidl/host_server_unittest.cc
index 90935b7..4aa0a57 100644
--- a/pw_bluetooth_sapphire/host/fidl/host_server_unittest.cc
+++ b/pw_bluetooth_sapphire/host/fidl/host_server_unittest.cc
@@ -126,24 +126,35 @@
return pairing_delegate;
}
- std::tuple<bt::gap::Peer*, FakeChannel*> ConnectFakePeer(bool connect_le = true) {
- auto device_addr = connect_le ? kLeTestAddr : kBredrTestAddr;
- auto* peer = adapter()->peer_cache()->NewPeer(device_addr, true);
- EXPECT_TRUE(peer->temporary());
+ bt::gap::Peer* AddFakePeer(const bt::DeviceAddress& address) {
+ bt::gap::Peer* peer = adapter()->peer_cache()->NewPeer(address, /*connectable=*/true);
+ ZX_ASSERT(peer);
+ ZX_ASSERT(peer->temporary());
+
+ test_device()->AddPeer(std::make_unique<FakePeer>(address));
+
+ return peer;
+ }
+
+ using ConnectResult = fit::result<void, fsys::Error>;
+ std::optional<ConnectResult> ConnectFakePeer(bt::PeerId id) {
+ std::optional<ConnectResult> result;
+ host_client()->Connect(fbt::PeerId{id.value()},
+ [&](ConnectResult _result) { result = _result; });
+ RunLoopUntilIdle();
+ return result;
+ }
+
+ std::tuple<bt::gap::Peer*, FakeChannel*> CreateAndConnectFakePeer(bool connect_le = true) {
+ auto address = connect_le ? kLeTestAddr : kBredrTestAddr;
+ bt::gap::Peer* peer = AddFakePeer(address);
+
// This is to capture the channel created during the Connection process
FakeChannel* fake_chan = nullptr;
data_domain()->set_channel_callback(
[&fake_chan](fbl::RefPtr<FakeChannel> new_fake_chan) { fake_chan = new_fake_chan.get(); });
- auto fake_peer = std::make_unique<FakePeer>(device_addr);
- test_device()->AddPeer(std::move(fake_peer));
-
- std::optional<fit::result<void, fsys::Error>> connect_result;
- host_client()->Connect(fbt::PeerId{peer->identifier().value()}, [&](auto result) {
- ASSERT_FALSE(result.is_err());
- connect_result = std::move(result);
- });
- RunLoopUntilIdle();
+ auto connect_result = ConnectFakePeer(peer->identifier());
if (!connect_result || connect_result->is_error()) {
peer = nullptr;
@@ -176,7 +187,7 @@
if (!fake_peer_ || !fake_chan_) {
ASSERT_FALSE(fake_peer_);
ASSERT_FALSE(fake_chan_);
- std::tie(fake_peer_, fake_chan_) = ConnectFakePeer(is_le);
+ std::tie(fake_peer_, fake_chan_) = CreateAndConnectFakePeer(is_le);
ASSERT_TRUE(fake_peer_);
ASSERT_TRUE(fake_chan_);
ASSERT_EQ(bt::gap::Peer::ConnectionState::kConnected, fake_peer_->le()->connection_state());
@@ -565,7 +576,7 @@
}
TEST_F(FIDL_HostServerTest, InitiateBrEdrPairingLePeerFails) {
- auto [peer, fake_chan] = ConnectFakePeer();
+ auto [peer, fake_chan] = CreateAndConnectFakePeer();
ASSERT_TRUE(peer);
ASSERT_TRUE(fake_chan);
ASSERT_EQ(bt::gap::Peer::ConnectionState::kConnected, peer->le()->connection_state());
@@ -593,7 +604,8 @@
}
TEST_F(FIDL_HostServerTest, WatchPeersRepliesOnFirstCallWithExistingPeers) {
- __UNUSED auto* peer = adapter()->peer_cache()->NewPeer(kLeTestAddr, /*connectable=*/true);
+ __UNUSED bt::gap::Peer* peer =
+ adapter()->peer_cache()->NewPeer(kLeTestAddr, /*connectable=*/true);
ResetHostServer();
// By default the peer cache contains no entries when HostServer is first constructed. The first
@@ -620,7 +632,7 @@
ASSERT_FALSE(removed.has_value());
// Adding a new peer should resolve the hanging get.
- auto* peer = adapter()->peer_cache()->NewPeer(kLeTestAddr, /*connectable=*/true);
+ bt::gap::Peer* peer = adapter()->peer_cache()->NewPeer(kLeTestAddr, /*connectable=*/true);
ASSERT_TRUE(updated.has_value());
ASSERT_TRUE(removed.has_value());
EXPECT_EQ(1u, updated->size());
@@ -651,7 +663,7 @@
// Add then remove a peer. The watcher should only report the removal.
bt::PeerId id;
{
- auto* peer = adapter()->peer_cache()->NewPeer(kLeTestAddr, /*connectable=*/true);
+ bt::gap::Peer* peer = adapter()->peer_cache()->NewPeer(kLeTestAddr, /*connectable=*/true);
id = peer->identifier();
// |peer| becomes a dangling pointer after the call to RemoveDisconnectedPeer. We scoped the
@@ -683,5 +695,59 @@
adapter()->le_connection_manager()->security_mode());
}
+TEST_F(FIDL_HostServerTest, ConnectLowEnergy) {
+ bt::gap::Peer* peer = AddFakePeer(kLeTestAddr);
+ EXPECT_EQ(bt::gap::TechnologyType::kLowEnergy, peer->technology());
+
+ auto result = ConnectFakePeer(peer->identifier());
+ ASSERT_TRUE(result);
+ ASSERT_FALSE(result->is_error());
+
+ EXPECT_FALSE(peer->bredr());
+ ASSERT_TRUE(peer->le());
+ EXPECT_TRUE(peer->le()->connected());
+
+ // bt-host should only attempt to connect the LE transport.
+ EXPECT_EQ(1, test_device()->le_create_connection_command_count());
+ EXPECT_EQ(0, test_device()->acl_create_connection_command_count());
+}
+
+TEST_F(FIDL_HostServerTest, ConnectBredr) {
+ bt::gap::Peer* peer = AddFakePeer(kBredrTestAddr);
+ EXPECT_EQ(bt::gap::TechnologyType::kClassic, peer->technology());
+
+ auto result = ConnectFakePeer(peer->identifier());
+ ASSERT_TRUE(result);
+ ASSERT_FALSE(result->is_error());
+
+ EXPECT_FALSE(peer->le());
+ ASSERT_TRUE(peer->bredr());
+ EXPECT_TRUE(peer->bredr()->connected());
+
+ // bt-host should only attempt to connect the BR/EDR transport.
+ EXPECT_EQ(0, test_device()->le_create_connection_command_count());
+ EXPECT_EQ(1, test_device()->acl_create_connection_command_count());
+}
+
+TEST_F(FIDL_HostServerTest, ConnectDualMode) {
+ // Initialize the peer with data for both transport types.
+ bt::gap::Peer* peer = AddFakePeer(kBredrTestAddr);
+ peer->MutLe();
+ ASSERT_TRUE(peer->le());
+ peer->MutBrEdr();
+ ASSERT_TRUE(peer->bredr());
+ EXPECT_EQ(bt::gap::TechnologyType::kDualMode, peer->technology());
+
+ auto result = ConnectFakePeer(peer->identifier());
+ ASSERT_TRUE(result);
+ ASSERT_FALSE(result->is_error());
+
+ // bt-host should only attempt to connect the BR/EDR transport.
+ EXPECT_TRUE(peer->bredr()->connected());
+ EXPECT_FALSE(peer->le()->connected());
+ EXPECT_EQ(0, test_device()->le_create_connection_command_count());
+ EXPECT_EQ(1, test_device()->acl_create_connection_command_count());
+}
+
} // namespace
} // namespace bthost
diff --git a/pw_bluetooth_sapphire/host/testing/fake_controller.cc b/pw_bluetooth_sapphire/host/testing/fake_controller.cc
index cf89de7..201e306 100644
--- a/pw_bluetooth_sapphire/host/testing/fake_controller.cc
+++ b/pw_bluetooth_sapphire/host/testing/fake_controller.cc
@@ -563,6 +563,8 @@
void FakeController::OnCreateConnectionCommandReceived(
const hci::CreateConnectionCommandParams& params) {
+ acl_create_connection_command_count_++;
+
// Cannot issue this command while a request is already pending.
if (bredr_connect_pending_) {
RespondWithCommandStatus(hci::kCreateConnection, hci::StatusCode::kCommandDisallowed);
@@ -660,6 +662,8 @@
void FakeController::OnLECreateConnectionCommandReceived(
const hci::LECreateConnectionCommandParams& params) {
+ le_create_connection_command_count_++;
+
// Cannot issue this command while a request is already pending.
if (le_connect_pending_) {
RespondWithCommandStatus(hci::kLECreateConnection, hci::StatusCode::kCommandDisallowed);
diff --git a/pw_bluetooth_sapphire/host/testing/fake_controller.h b/pw_bluetooth_sapphire/host/testing/fake_controller.h
index 75cd8f4..cfa219f 100644
--- a/pw_bluetooth_sapphire/host/testing/fake_controller.h
+++ b/pw_bluetooth_sapphire/host/testing/fake_controller.h
@@ -165,6 +165,10 @@
// is unknown.
FakePeer* FindPeer(const DeviceAddress& address);
+ // Counters for HCI commands received.
+ int le_create_connection_command_count() const { return le_create_connection_command_count_; }
+ int acl_create_connection_command_count() const { return acl_create_connection_command_count_; }
+
// Sets a callback to be invoked when the the base controller parameters change due to a HCI
// command. These parameters are:
//
@@ -464,6 +468,10 @@
// The set of fake peers that are visible.
std::unordered_map<DeviceAddress, std::unique_ptr<FakePeer>> peers_;
+ // Callbacks and counters that are intended for unit tests.
+ int le_create_connection_command_count_ = 0;
+ int acl_create_connection_command_count_ = 0;
+
fit::closure controller_parameters_cb_;
ScanStateCallback scan_state_cb_;
fit::closure advertising_state_cb_;
