Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 9d3fc40c authored by Łukasz Rymanowski's avatar Łukasz Rymanowski
Browse files

le_advertiser: Fix reduntant HCI Command 

Make sure to disable periodic advertising only when it is needed to
avoid Comand Disallowed.

< HCI Command: LE Set Extended A.. (0x08|0x0039) plen 6  #18224 14:40:29.928161
        Extended advertising: Disabled (0x00)
        Number of sets: 1 (0x01)
        Entry 0
          Handle: 0x00
          Duration: 0 ms (0x00)
          Max ext adv events: 0
> HCI Event: Command Complete (0x0e) plen 4              #18225 14:40:29.931694
      LE Set Extended Advertising Enable (0x08|0x0039) ncmd 1
        Status: Success (0x00)
< HCI Command: LE Set Periodic A.. (0x08|0x0040) plen 2  #18226 14:40:29.931988
        Periodic advertising: Disabled (0x00)
        Handle: 0
> HCI Event: Command Complete (0x0e) plen 4              #18227 14:40:29.938424
      LE Set Periodic Advertising Enable (0x08|0x0040) ncmd 1
        Status: Command Disallowed (0x0c)

Bug: 339689853
Test: atest bluetooth_test_gd_unit
Test: Manual test with nRF connect and local Broadcaster
Flag: Exempt, trivial fix, regression checked with unit tests
Change-Id: I24539459f2e7ed9cb451dea8b92c3637f1f3ba5e
parent 98876e4f

system/gd/hci/le_advertising_manager.cc

+8 −2
Original line number Diff line number Diff line
@@ -84,6 +84,7 @@ struct Advertiser { 
  bool discoverable = false;

  bool directed = false;

  bool in_use = false;

  bool is_periodic = false;

  std::unique_ptr<os::Alarm> address_rotation_alarm;

};
@@ -677,7 +678,8 @@ struct LeAdvertisingManager::impl : public bluetooth::hci::LeAddressManagerCallb 
  }



  void stop_advertising(AdvertiserId advertiser_id) {

    if (advertising_sets_.find(advertiser_id) == advertising_sets_.end()) {

    auto advertising_iter = advertising_sets_.find(advertiser_id);

    if (advertising_iter == advertising_sets_.end()) {

      log::info("Unknown advertising set {}", advertiser_id);

      return;

    }
@@ -704,8 +706,11 @@ struct LeAdvertisingManager::impl : public bluetooth::hci::LeAddressManagerCallb 
            hci::LeSetExtendedAdvertisingEnableBuilder::Create(Enable::DISABLED, enabled_vector),

            module_handler_->BindOnce(check_complete<LeSetExtendedAdvertisingEnableCompleteView>));



        bool is_periodic = advertising_iter->second.is_periodic;

        log::debug("advertiser_id: {} is_periodic: {}", advertiser_id, is_periodic);



        // Only set periodic advertising if supported.

        if (controller_->SupportsBlePeriodicAdvertising()) {

        if (is_periodic && controller_->SupportsBlePeriodicAdvertising()) {

          le_advertising_interface_->EnqueueCommand(

              hci::LeSetPeriodicAdvertisingEnableBuilder::Create(false, false, advertiser_id),

              module_handler_->BindOnce(
@@ -799,6 +804,7 @@ struct LeAdvertisingManager::impl : public bluetooth::hci::LeAddressManagerCallb 
    advertising_sets_[advertiser_id].discoverable = config.discoverable;

    advertising_sets_[advertiser_id].tx_power = config.tx_power;

    advertising_sets_[advertiser_id].directed = config.directed;

    advertising_sets_[advertiser_id].is_periodic = config.periodic_advertising_parameters.enable;



    // based on logic in new_advertiser_address

    auto own_address_type = static_cast<OwnAddressType>(

system/gd/hci/le_advertising_manager_test.cc

+60 −3
Original line number Diff line number Diff line
@@ -720,6 +720,66 @@ TEST_F(LeExtendedAdvertisingManagerTest, create_advertiser_test) { 
  }

  sync_client_handler();



  // Remove the advertiser

  ASSERT_NE(LeAdvertisingManager::kInvalidId, id);

  le_advertising_manager_->RemoveAdvertiser(id);

  ASSERT_EQ(OpCode::LE_SET_EXTENDED_ADVERTISING_ENABLE, test_hci_layer_->GetCommand().GetOpCode());

  ASSERT_EQ(OpCode::LE_REMOVE_ADVERTISING_SET, test_hci_layer_->GetCommand().GetOpCode());

}



TEST_F(LeExtendedAdvertisingManagerTest, create_periodic_advertiser_test) {

  AdvertisingConfig advertising_config{};

  advertising_config.advertising_type = AdvertisingType::ADV_IND;

  advertising_config.requested_advertiser_address_type = AdvertiserAddressType::PUBLIC;

  std::vector<GapData> gap_data{};

  GapData data_item{};

  data_item.data_type_ = GapDataType::FLAGS;

  data_item.data_ = {0x34};

  gap_data.push_back(data_item);

  data_item.data_type_ = GapDataType::COMPLETE_LOCAL_NAME;

  data_item.data_ = {'r', 'a', 'n', 'd', 'o', 'm', ' ', 'd', 'e', 'v', 'i', 'c', 'e'};

  gap_data.push_back(data_item);

  advertising_config.advertisement = gap_data;

  advertising_config.scan_response = gap_data;

  advertising_config.channel_map = 1;

  advertising_config.sid = 0x01;

  advertising_config.periodic_advertising_parameters.enable = true;



  AdvertiserId id;

  EXPECT_CALL(

      mock_advertising_callback_,

      OnAdvertisingSetStarted(0, _, -23, AdvertisingCallback::AdvertisingStatus::SUCCESS))

      .WillOnce(SaveArg<1>(&id));



  le_advertising_manager_->ExtendedCreateAdvertiser(

      kAdvertiserClientIdJni,

      0x00,

      advertising_config,

      scan_callback,

      set_terminated_callback,

      0,

      0,

      client_handler_);



  std::vector<OpCode> adv_opcodes = {

      OpCode::LE_SET_EXTENDED_ADVERTISING_PARAMETERS,

      OpCode::LE_SET_EXTENDED_SCAN_RESPONSE_DATA,

      OpCode::LE_SET_EXTENDED_ADVERTISING_DATA,

      OpCode::LE_SET_EXTENDED_ADVERTISING_ENABLE,

  };

  std::vector<uint8_t> success_vector{static_cast<uint8_t>(ErrorCode::SUCCESS)};

  for (size_t i = 0; i < adv_opcodes.size(); i++) {

    ASSERT_EQ(adv_opcodes[i], test_hci_layer_->GetCommand().GetOpCode());

    if (adv_opcodes[i] == OpCode::LE_SET_EXTENDED_ADVERTISING_PARAMETERS) {

      test_hci_layer_->IncomingEvent(LeSetExtendedAdvertisingParametersCompleteBuilder::Create(

          uint8_t{1}, ErrorCode::SUCCESS, static_cast<uint8_t>(-23)));

    } else {

      test_hci_layer_->IncomingEvent(CommandCompleteBuilder::Create(

          uint8_t{1}, adv_opcodes[i], std::make_unique<RawBuilder>(success_vector)));

    }

  }

  sync_client_handler();



  // Remove the advertiser

  ASSERT_NE(LeAdvertisingManager::kInvalidId, id);

  le_advertising_manager_->RemoveAdvertiser(id);
@@ -786,7 +846,6 @@ TEST_F_WITH_FLAGS( 
  ASSERT_NE(LeAdvertisingManager::kInvalidId, id);

  le_advertising_manager_->RemoveAdvertiser(id);

  ASSERT_EQ(OpCode::LE_SET_EXTENDED_ADVERTISING_ENABLE, test_hci_layer_->GetCommand().GetOpCode());

  ASSERT_EQ(OpCode::LE_SET_PERIODIC_ADVERTISING_ENABLE, test_hci_layer_->GetCommand().GetOpCode());

  ASSERT_EQ(OpCode::LE_REMOVE_ADVERTISING_SET, test_hci_layer_->GetCommand().GetOpCode());

}
@@ -856,7 +915,6 @@ TEST_F_WITH_FLAGS( 
  ASSERT_NE(LeAdvertisingManager::kInvalidId, id);

  le_advertising_manager_->RemoveAdvertiser(id);

  ASSERT_EQ(OpCode::LE_SET_EXTENDED_ADVERTISING_ENABLE, test_hci_layer_->GetCommand().GetOpCode());

  ASSERT_EQ(OpCode::LE_SET_PERIODIC_ADVERTISING_ENABLE, test_hci_layer_->GetCommand().GetOpCode());

  ASSERT_EQ(OpCode::LE_REMOVE_ADVERTISING_SET, test_hci_layer_->GetCommand().GetOpCode());

}
@@ -960,7 +1018,6 @@ TEST_F(LeExtendedAdvertisingManagerTest, ignore_on_pause_on_resume_after_unregis 
  ASSERT_NE(LeAdvertisingManager::kInvalidId, id);

  le_advertising_manager_->RemoveAdvertiser(id);

  ASSERT_EQ(OpCode::LE_SET_EXTENDED_ADVERTISING_ENABLE, test_hci_layer_->GetCommand().GetOpCode());

  ASSERT_EQ(OpCode::LE_SET_PERIODIC_ADVERTISING_ENABLE, test_hci_layer_->GetCommand().GetOpCode());

  ASSERT_EQ(OpCode::LE_REMOVE_ADVERTISING_SET, test_hci_layer_->GetCommand().GetOpCode());

  sync_client_handler();