Skip to content

Commit

Permalink
pw_bluetooth_sapphire: Allow LE remote feature interrogation to fail
Browse files Browse the repository at this point in the history 
When performing an LE remote feature step of peer interrogation,
if the controller reports a status of "unsupported remote feature"
(0x1a), return a successful status from the LowEnergyInterrogator.
This allows us to successfully connect with peers that don't
support the "Peripheral-Initiated Feature Exchange" feature.

Bug: b/361651988
Test: fx test //src/connectivity/bluetooth/core/bt-host
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1108632
GitOrigin-RevId: 678081254cb33195119ff2ed7b2e44989d239ad1
Change-Id: Ie0ea22d61da914bc67122722f307f628cf33a91a
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/232773
Lint: Lint 🤖 <[email protected]>
Pigweed-Auto-Submit: Jason Graffius <[email protected]>
Reviewed-by: Ben Lawson <[email protected]>
Commit-Queue: Auto-Submit <[email protected]>
  • Loading branch information
@josh-conner
josh-conner authored and CQ Bot Account committed Aug 31, 2024
1 parent 107eecb commit faa7a78
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 5 deletions.
9 changes: 5 additions & 4 deletions pw_bluetooth_sapphire/host/gap/low_energy_connection.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -461,15 +461,16 @@ void LowEnergyConnection::RequestConnectionParameterUpdate(

BT_ASSERT(peer_.is_alive());
// Ensure interrogation has completed.
BT_ASSERT(peer_->le()->features().has_value());
BT_ASSERT(peer_->le()->feature_interrogation_complete());

// TODO(fxbug.dev/42126713): check local controller support for LL Connection
// Parameters Request procedure (mask is currently in Adapter le state,
// consider propagating down)
bool ll_connection_parameters_req_supported =
peer_->le()->features()->le_features &
static_cast<uint64_t>(
hci_spec::LESupportedFeature::kConnectionParametersRequestProcedure);
peer_->le()->features().has_value() &&
(peer_->le()->features()->le_features &
static_cast<uint64_t>(hci_spec::LESupportedFeature::
kConnectionParametersRequestProcedure));

bt_log(TRACE,
"gap-le",
Expand Down
13 changes: 12 additions & 1 deletion pw_bluetooth_sapphire/host/gap/low_energy_interrogator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,21 @@ void LowEnergyInterrogator::Start(ResultCallback callback) {
// fxbug.dev/42138706 for details.
QueueReadRemoteVersionInformation();

if (!peer_->le()->features().has_value()) {
if (!peer_->le()->feature_interrogation_complete()) {
QueueReadLERemoteFeatures();
}

cmd_runner_.RunCommands([this](hci::Result<> result) {
// Accommodate unsupported remote feature interrogation (see
// http://b/361651988). In this case we know the peer doesn't support SCA,
// so we can return immediately.
if (peer_->le()->feature_interrogation_complete() &&
result == ToResult(pw::bluetooth::emboss::StatusCode::
UNSUPPORTED_REMOTE_FEATURE)) {
Complete(fit::ok());
return;
}

if (result.is_error() || !peer_->le()->features().has_value() ||
!controller_supports_sca_) {
Complete(result);
Expand Down Expand Up @@ -149,6 +159,7 @@ void LowEnergyInterrogator::QueueReadLERemoteFeatures() {
// |cmd_runner_| guarantees that |cmd_cb| won't be invoked if |cmd_runner_| is
// destroyed, and |this| outlives |cmd_runner_|.
auto cmd_cb = [this](const hci::EmbossEventPacket& event) {
peer_->MutLe().SetFeatureInterrogationComplete();
if (hci_is_error(event, WARN, "gap-le", "LE read remote features failed")) {
return;
}
Expand Down
38 changes: 38 additions & 0 deletions pw_bluetooth_sapphire/host/gap/low_energy_interrogator_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ class LowEnergyInterrogatorTest : public TestingBase {
peer_ = peer_cache()->NewPeer(kTestDevAddr, /*connectable=*/true);
ASSERT_TRUE(peer_->le());
EXPECT_FALSE(peer_->version());
EXPECT_FALSE(peer_->le()->feature_interrogation_complete());
EXPECT_FALSE(peer_->le()->features());
EXPECT_FALSE(peer_->le()->sleep_clock_accuracy());

Expand Down Expand Up @@ -153,6 +154,7 @@ TEST_F(LowEnergyInterrogatorTest, SuccessfulInterrogation) {
EXPECT_EQ(fit::ok(), *status);

EXPECT_TRUE(peer()->version());
ASSERT_TRUE(peer()->le()->feature_interrogation_complete());
ASSERT_TRUE(peer()->le()->features());
EXPECT_EQ(kFeatures.le_features, peer()->le()->features()->le_features);
ASSERT_TRUE(peer()->le()->sleep_clock_accuracy());
Expand Down Expand Up @@ -182,13 +184,15 @@ TEST_F(LowEnergyInterrogatorTest,
&le_request_peer_sca_complete_packet);

peer()->MutLe().SetFeatures(kFeatures);
peer()->MutLe().SetFeatureInterrogationComplete();

std::optional<hci::Result<>> status;
interrogator()->Start(
[&status](hci::Result<> cb_status) { status = cb_status; });
RunUntilIdle();
ASSERT_TRUE(status.has_value());
EXPECT_EQ(fit::ok(), *status);
ASSERT_TRUE(peer()->le()->feature_interrogation_complete());
ASSERT_TRUE(peer()->le()->features());
EXPECT_EQ(kFeatures.le_features, peer()->le()->features()->le_features);
ASSERT_TRUE(peer()->le()->sleep_clock_accuracy());
Expand Down Expand Up @@ -257,6 +261,36 @@ TEST_F(LowEnergyInterrogatorTest, LEReadRemoteFeaturesErrorStatus) {
RunUntilIdle();
ASSERT_TRUE(status.has_value());
EXPECT_FALSE(status->is_ok());
EXPECT_TRUE(peer()->le()->feature_interrogation_complete());
EXPECT_FALSE(peer()->le()->features().has_value());

// When previous operations fail, we shouldn't try to read SCA.
EXPECT_FALSE(peer()->le()->sleep_clock_accuracy());
}

// Test proper operation when a peer doesn't support reading LE remote features
TEST_F(LowEnergyInterrogatorTest, LEReadRemoteFeaturesUnsupported) {
const auto remote_version_complete_packet =
testing::ReadRemoteVersionInfoCompletePacket(kConnectionHandle);
const auto le_read_remote_features_unsupported_status_packet =
testing::CommandStatusPacket(
hci_spec::kLEReadRemoteFeatures,
pw::bluetooth::emboss::StatusCode::UNSUPPORTED_REMOTE_FEATURE);
EXPECT_CMD_PACKET_OUT(test_device(),
testing::ReadRemoteVersionInfoPacket(kConnectionHandle),
&kReadRemoteVersionInfoRsp,
&remote_version_complete_packet);
EXPECT_CMD_PACKET_OUT(test_device(),
testing::LEReadRemoteFeaturesPacket(kConnectionHandle),
&le_read_remote_features_unsupported_status_packet);

std::optional<hci::Result<>> status;
interrogator()->Start(
[&status](hci::Result<> cb_status) { status = cb_status; });
RunUntilIdle();
ASSERT_TRUE(status.has_value());
EXPECT_TRUE(status->is_ok());
EXPECT_TRUE(peer()->le()->feature_interrogation_complete());
EXPECT_FALSE(peer()->le()->features().has_value());

// When previous operations fail, we shouldn't try to read SCA.
Expand Down Expand Up @@ -324,6 +358,7 @@ TEST_F(LowEnergyInterrogatorTest,
EXPECT_FALSE(result.has_value());
// The read remote features handler should not update the features of a
// canceled interrogation.
ASSERT_FALSE(peer()->le()->feature_interrogation_complete());
EXPECT_FALSE(peer()->le()->features().has_value());
EXPECT_FALSE(peer()->le()->sleep_clock_accuracy());
}
Expand Down Expand Up @@ -381,6 +416,7 @@ TEST_F(LowEnergyInterrogatorTest, ScaUpdateNotSupportedOnController) {
EXPECT_EQ(fit::ok(), *status);

EXPECT_TRUE(peer()->version());
ASSERT_TRUE(peer()->le()->feature_interrogation_complete());
ASSERT_TRUE(peer()->le()->features());
EXPECT_EQ(kFeatures.le_features, peer()->le()->features()->le_features);
ASSERT_FALSE(peer()->le()->sleep_clock_accuracy());
Expand All @@ -400,6 +436,7 @@ TEST_F(LowEnergyInterrogatorTest, ScaUpdateNotSupportedOnPeer) {
EXPECT_EQ(fit::ok(), *status);

EXPECT_TRUE(peer()->version());
ASSERT_TRUE(peer()->le()->feature_interrogation_complete());
ASSERT_TRUE(peer()->le()->features());
EXPECT_EQ(kFeatures.le_features, peer()->le()->features()->le_features);
ASSERT_FALSE(peer()->le()->sleep_clock_accuracy());
Expand All @@ -418,6 +455,7 @@ TEST_F(LowEnergyInterrogatorTest, DestroyInterrogatorInCompleteCallback) {
RunUntilIdle();
ASSERT_TRUE(status.has_value());
EXPECT_TRUE(status->is_ok());
ASSERT_TRUE(peer()->le()->feature_interrogation_complete());
ASSERT_TRUE(peer()->le()->features());
EXPECT_EQ(kFeatures.le_features, peer()->le()->features()->le_features);
ASSERT_TRUE(peer()->le()->sleep_clock_accuracy());
Expand Down
14 changes: 14 additions & 0 deletions pw_bluetooth_sapphire/public/pw_bluetooth_sapphire/internal/host/gap/peer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,10 @@ class Peer final {
return *bond_data_;
}

bool feature_interrogation_complete() const {
return feature_interrogation_complete_;
}

// Bit mask of LE features (Core Spec v5.2, Vol 6, Part B, Section 4.6).
std::optional<hci_spec::LESupportedFeatures> features() const {
return *features_;
Expand Down Expand Up @@ -295,6 +299,10 @@ class Peer final {
// is disconnected. Does not notify listeners.
void ClearBondData();

void SetFeatureInterrogationComplete() {
feature_interrogation_complete_ = true;
}

void SetFeatures(hci_spec::LESupportedFeatures features) {
features_.Set(features);
}
Expand Down Expand Up @@ -368,6 +376,12 @@ class Peer final {

AutoConnectBehavior auto_conn_behavior_ = AutoConnectBehavior::kAlways;

bool feature_interrogation_complete_ = false;

// features_ will be unset if feature interrogation has not been attempted
// (in which case feature_interrogation_complete_ will be false) or if
// feature interrogation has failed (in which case
// feature_interrogation_complete_ will be true).
StringInspectable<std::optional<hci_spec::LESupportedFeatures>> features_;

// TODO(armansito): Store GATT service UUIDs.
Expand Down

0 comments on commit faa7a78

Please sign in to comment.