Skip to content

Commit

Permalink
pw_bluetooth_sapphire: Do not scan during LE connection procedure
Browse files Browse the repository at this point in the history 
Remove scan during LE connection procedure. The controller already does
a scan as part of connecting.

Update the local address before creating an LE connection. This was a
side effect of the scanning procedure previously, so this change just
preserves this side effect. This behavior is already tested by
adapter_test.cc.

Bug: 42172291
Test: Manually tested an LE connection from Nelson to phone with
$ fx shell
$ bt-le-central -n Test -c
and verified from the logs that the connection succeeded and no
scanning took place.

Change-Id: I42fca6d07c6159c50b101c001192d327596d7123
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/256052
Docs-Not-Needed: Marie Janssen <[email protected]>
Reviewed-by: Marie Janssen <[email protected]>
Lint: Lint 🤖 <[email protected]>
Commit-Queue: Ben Lawson <[email protected]>
  • Loading branch information
@BenjaminLawson
BenjaminLawson authored and CQ Bot Account committed Dec 20, 2024
1 parent 2c90975 commit 8b1d112
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 179 deletions.
6 changes: 4 additions & 2 deletions pw_bluetooth_sapphire/host/gap/adapter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -866,9 +866,11 @@ TEST_F(AdapterTest, LocalAddressForConnections) {
EXPECT_EQ(pw::bluetooth::emboss::LEOwnAddressType::PUBLIC,
test_device()->le_connect_params()->own_address_type);

// Create a new connection. The second attempt should use a random address.
// re-enabled.
// Disconnect
conn_ref = nullptr;

// Create a new connection. The LowEnergyConnectionManager should try to
// update the local address before creating the connection.
adapter()->le()->Connect(
peer->identifier(), connect_cb, LowEnergyConnectionOptions());
RunUntilIdle();
Expand Down
6 changes: 4 additions & 2 deletions pw_bluetooth_sapphire/host/gap/low_energy_connection_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,8 @@ void LowEnergyConnectionManager::RegisterRemoteInitiatedLink(
l2cap_,
gatt_,
adapter_state_,
dispatcher_);
dispatcher_,
local_address_delegate_);
auto [conn_iter, _] = remote_connectors_.emplace(
peer_id, RequestAndConnector{std::move(request), std::move(connector)});
// Wait until the connector is in the map to start in case the result callback
Expand Down Expand Up @@ -575,7 +576,8 @@ void LowEnergyConnectionManager::TryCreateNextConnection() {
l2cap_,
gatt_,
adapter_state_,
dispatcher_);
dispatcher_,
local_address_delegate_);
connector->AttachInspect(inspect_node_,
kInspectOutboundConnectorNodeName);

Expand Down
66 changes: 1 addition & 65 deletions pw_bluetooth_sapphire/host/gap/low_energy_connection_manager_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3349,70 +3349,6 @@ TEST_F(LowEnergyConnectionManagerTest, AutoConnectSkipsScanning) {
EXPECT_EQ(scan_cb_count, 0u);
}

TEST_F(LowEnergyConnectionManagerTest, ConnectSinglePeerStartDiscoveryFailed) {
auto* peer = peer_cache()->NewPeer(kAddress0, /*connectable=*/true);
EXPECT_TRUE(peer->temporary());

auto fake_peer = std::make_unique<FakePeer>(kAddress0, dispatcher());
test_device()->AddPeer(std::move(fake_peer));

size_t connect_cb_count = 0;
auto callback = [&connect_cb_count](auto result) {
EXPECT_TRUE(result.is_error());
connect_cb_count++;
};

// Cause discovery to fail.
test_device()->SetDefaultCommandStatus(
hci_spec::kLESetScanEnable,
pw::bluetooth::emboss::StatusCode::COMMAND_DISALLOWED);

EXPECT_TRUE(connected_peers().empty());
conn_mgr()->Connect(peer->identifier(), callback, kConnectionOptions);
ASSERT_TRUE(peer->le());
EXPECT_EQ(Peer::ConnectionState::kInitializing,
peer->le()->connection_state());

RunUntilIdle();
EXPECT_EQ(connect_cb_count, 1u);
EXPECT_FALSE(peer->temporary());
EXPECT_EQ(Peer::ConnectionState::kNotConnected,
peer->le()->connection_state());
}

TEST_F(LowEnergyConnectionManagerTest,
ConnectSinglePeerDiscoveryFailedDuringScan) {
auto* peer = peer_cache()->NewPeer(kAddress0, /*connectable=*/true);
EXPECT_TRUE(peer->temporary());

// Don't add peer to FakeController to prevent scan from completing.

size_t connect_cb_count = 0;
auto callback = [&connect_cb_count](auto result) {
EXPECT_TRUE(result.is_error());
connect_cb_count++;
};

EXPECT_TRUE(connected_peers().empty());
conn_mgr()->Connect(peer->identifier(), callback, kConnectionOptions);
ASSERT_TRUE(peer->le());
EXPECT_EQ(Peer::ConnectionState::kInitializing,
peer->le()->connection_state());
RunUntilIdle();
EXPECT_EQ(connect_cb_count, 0u);

// Cause discovery to fail when attempting to restart scan after scan period
// ends.
test_device()->SetDefaultCommandStatus(
hci_spec::kLESetScanEnable,
pw::bluetooth::emboss::StatusCode::COMMAND_DISALLOWED);
RunFor(kLEGeneralDiscoveryScanMin);
EXPECT_EQ(connect_cb_count, 1u);
EXPECT_FALSE(peer->temporary());
EXPECT_EQ(Peer::ConnectionState::kNotConnected,
peer->le()->connection_state());
}

TEST_F(LowEnergyConnectionManagerTest,
PeerDisconnectBeforeInterrogationCompletes) {
auto* peer = peer_cache()->NewPeer(kAddress0, /*connectable=*/true);
Expand Down Expand Up @@ -3924,7 +3860,7 @@ TEST_F(LowEnergyConnectionManagerTest, Inspect) {
StringIs("peer_id", peer->identifier().ToString()),
IntIs("connection_attempt", 0),
BoolIs("is_outbound", true),
StringIs("state", "StartingScanning"))))));
StringIs("state", "Connecting"))))));

auto empty_connections_matcher =
AllOf(NodeMatches(NameMatches("connections")),
Expand Down
123 changes: 21 additions & 102 deletions pw_bluetooth_sapphire/host/gap/low_energy_connector.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ LowEnergyConnector::LowEnergyConnector(
l2cap::ChannelManager* l2cap,
gatt::GATT::WeakPtr gatt,
const AdapterState& adapter_state,
pw::async::Dispatcher& dispatcher)
pw::async::Dispatcher& dispatcher,
hci::LocalAddressDelegate* local_address_delegate)
: dispatcher_(dispatcher),
peer_id_(peer_id),
peer_cache_(peer_cache),
Expand All @@ -61,7 +62,8 @@ LowEnergyConnector::LowEnergyConnector(
adapter_state_(adapter_state),
options_(options),
hci_(std::move(hci)),
le_connection_manager_(std::move(conn_mgr)) {
le_connection_manager_(std::move(conn_mgr)),
local_address_delegate_(local_address_delegate) {
PW_CHECK(peer_cache_);
PW_CHECK(l2cap_);
PW_CHECK(gatt_.is_alive());
Expand Down Expand Up @@ -116,11 +118,7 @@ void LowEnergyConnector::StartOutbound(
result_cb_ = std::move(cb);
set_is_outbound(true);

if (options_.auto_connect) {
RequestCreateConnection();
} else {
StartScanningForPeer();
}
EnsureLocalAddress();
}

void LowEnergyConnector::StartInbound(
Expand Down Expand Up @@ -156,13 +154,7 @@ void LowEnergyConnector::Cancel() {
// There is nothing to do if cancel is called before the procedure has
// started. There is no result callback to call yet.
break;
case State::kStartingScanning:
discovery_session_.reset();
NotifyFailure(ToResult(HostError::kCanceled));
break;
case State::kScanning:
discovery_session_.reset();
scan_timeout_task_.reset();
case State::kEnsuringLocalAddress:
NotifyFailure(ToResult(HostError::kCanceled));
break;
case State::kConnecting:
Expand Down Expand Up @@ -204,10 +196,8 @@ const char* LowEnergyConnector::StateToString(State state) {
switch (state) {
case State::kDefault:
return "Default";
case State::kStartingScanning:
return "StartingScanning";
case State::kScanning:
return "Scanning";
case State::kEnsuringLocalAddress:
return "EnsuringLocalAddress";
case State::kConnecting:
return "Connecting";
case State::kInterrogating:
Expand All @@ -223,94 +213,23 @@ const char* LowEnergyConnector::StateToString(State state) {
}
}

void LowEnergyConnector::StartScanningForPeer() {
if (!discovery_manager_.is_alive()) {
return;
}
auto self = weak_self_.GetWeakPtr();

state_.Set(State::kStartingScanning);

discovery_manager_->StartDiscovery(/*active=*/false, [self](auto session) {
if (self.is_alive()) {
self->OnScanStart(std::move(session));
}
});
}

void LowEnergyConnector::OnScanStart(LowEnergyDiscoverySessionPtr session) {
if (*state_ == State::kFailed) {
return;
}
PW_CHECK(*state_ == State::kStartingScanning);

// Failed to start scan, abort connection procedure.
if (!session) {
bt_log(INFO, "gap-le", "failed to start scan (peer: %s)", bt_str(peer_id_));
NotifyFailure(ToResult(HostError::kFailed));
return;
}

bt_log(INFO,
"gap-le",
"started scanning for pending connection (peer: %s)",
bt_str(peer_id_));
state_.Set(State::kScanning);

auto self = weak_self_.GetWeakPtr();
scan_timeout_task_.emplace(
dispatcher_, [this](pw::async::Context& /*ctx*/, pw::Status status) {
if (!status.ok()) {
void LowEnergyConnector::EnsureLocalAddress() {
PW_CHECK(*state_ == State::kDefault);
state_.Set(State::kEnsuringLocalAddress);
local_address_delegate_->EnsureLocalAddress(
/*address_type=*/std::nullopt, [self = weak_self_.GetWeakPtr()](auto) {
if (!self.is_alive() || *self->state_ == State::kFailed) {
return;
}
PW_CHECK(*state_ == State::kScanning);
bt_log(INFO,
"gap-le",
"scan for pending connection timed out (peer: %s)",
bt_str(peer_id_));
NotifyFailure(ToResult(HostError::kTimedOut));
self->RequestCreateConnection();
});
// The scan timeout may include time during which scanning is paused.
scan_timeout_task_->PostAfter(kLEGeneralCepScanTimeout);

discovery_session_ = std::move(session);
discovery_session_->filter()->set_connectable(true);

// The error callback must be set before the result callback in case the
// result callback is called synchronously.
discovery_session_->set_error_callback([self] {
PW_CHECK(self->state_.value() == State::kScanning);
bt_log(INFO,
"gap-le",
"discovery error while scanning for peer (peer: %s)",
bt_str(self->peer_id_));
self->scan_timeout_task_.reset();
self->NotifyFailure(ToResult(HostError::kFailed));
});

discovery_session_->SetResultCallback([self](auto& peer) {
PW_CHECK(self->state_.value() == State::kScanning);

if (peer.identifier() != self->peer_id_) {
return;
}

bt_log(INFO,
"gap-le",
"discovered peer for pending connection (peer: %s)",
bt_str(self->peer_id_));

self->scan_timeout_task_.reset();
self->discovery_session_->Stop();

self->RequestCreateConnection();
});
}

void LowEnergyConnector::RequestCreateConnection() {
// Scanning may be skipped. When the peer disconnects during/after
// interrogation, a retry may be initiated by calling this method.
PW_CHECK(*state_ == State::kDefault || *state_ == State::kScanning ||
// When the peer disconnects during/after interrogation, a retry may be
// initiated by calling this method.
PW_CHECK(*state_ == State::kDefault ||
*state_ == State::kEnsuringLocalAddress ||
*state_ == State::kPauseBeforeConnectionRetry);

// Pause discovery until connection complete.
Expand Down Expand Up @@ -458,8 +377,8 @@ void LowEnergyConnector::OnInterrogationComplete(hci::Result<> status) {

void LowEnergyConnector::OnPeerDisconnect(
pw::bluetooth::emboss::StatusCode status_code) {
// The peer can't disconnect while scanning or connecting, and we unregister
// from disconnects after kFailed & kComplete.
// The peer can't disconnect while connecting, and we unregister from
// disconnects after kFailed & kComplete.
PW_CHECK(
*state_ == State::kInterrogating ||
*state_ == State::kAwaitingConnectionFailedToBeEstablishedDisconnect,
Expand Down
16 changes: 8 additions & 8 deletions ...h_sapphire/host/gap/public/pw_bluetooth_sapphire/internal/host/gap/low_energy_connector.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ class LowEnergyConnector final {
l2cap::ChannelManager* l2cap,
gatt::GATT::WeakPtr gatt,
const AdapterState& adapter_state,
pw::async::Dispatcher& dispatcher);
pw::async::Dispatcher& dispatcher,
hci::LocalAddressDelegate* local_address_delegate);

// Instances should only be destroyed after the result callback is called
// (except for stack tear down). Due to the asynchronous nature of cancelling
Expand Down Expand Up @@ -82,8 +83,7 @@ class LowEnergyConnector final {
private:
enum class State {
kDefault,
kStartingScanning, // Outbound only
kScanning, // Outbound only
kEnsuringLocalAddress, // Outbound only
kConnecting, // Outbound only
kInterrogating, // Outbound & inbound
kAwaitingConnectionFailedToBeEstablishedDisconnect, // Outbound & inbound
Expand All @@ -94,9 +94,9 @@ class LowEnergyConnector final {

static const char* StateToString(State);

// Initiate scanning for peer before connecting to ensure it is advertising.
void StartScanningForPeer();
void OnScanStart(LowEnergyDiscoverySessionPtr session);
// Ensure the local address is updated (e.g. if privacy mode was just
// changed).
void EnsureLocalAddress();

// Initiate HCI connection procedure.
void RequestCreateConnection();
Expand Down Expand Up @@ -183,8 +183,6 @@ class LowEnergyConnector final {
// Task called after the scan attempt times out.
std::optional<SmartTask> scan_timeout_task_;

std::unique_ptr<LowEnergyDiscoverySession> discovery_session_;

// Sends HCI commands that request version and feature support information
// from peer controllers. Initialized only during interrogation.
std::optional<LowEnergyInterrogator> interrogator_;
Expand All @@ -198,6 +196,8 @@ class LowEnergyConnector final {
// Only used to construct a LowEnergyConnection.
WeakSelf<LowEnergyConnectionManager>::WeakPtr le_connection_manager_;

hci::LocalAddressDelegate* local_address_delegate_;

struct InspectProperties {
inspect::StringProperty peer_id;
inspect::BoolProperty is_outbound;
Expand Down

0 comments on commit 8b1d112

Please sign in to comment.