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

Commit 212357f2 authored by Rahul Arya's avatar Rahul Arya
Browse files

Fix missing advertising set callbacks 

I didn't realize that OnAdvertisingSetEnable is also called when you
*disable* the advertiser.

Unit tests also didn't catch it because nothing tested what happens when
you enable, then disable.

Test: unit
Bug: 263435509
Change-Id: I56f3149e622515e314292044352600a12d2dd568
parent 9b1e5f58

system/gd/hci/le_advertising_manager.cc

+42 −42
Original line number Diff line number Diff line
@@ -61,7 +61,6 @@ struct Advertiser { 
  int8_t tx_power;

  uint16_t duration;

  uint8_t max_extended_advertising_events;

  bool pending_start = false;  // whether we have started but are still in the queue

  bool started = false;

  bool connectable = false;

  bool directed = false;
@@ -348,7 +347,6 @@ struct LeAdvertisingManager::impl : public bluetooth::hci::LeAddressManagerCallb 
          enable_advertiser(id, true, 0, 0);

        } else {

          enabled_sets_[id].advertising_handle_ = id;

          advertising_sets_[id].pending_start = true;

        }

      } break;

      case (AdvertisingApiType::ANDROID_HCI): {
@@ -374,7 +372,6 @@ struct LeAdvertisingManager::impl : public bluetooth::hci::LeAddressManagerCallb 
          enable_advertiser(id, true, 0, 0);

        } else {

          enabled_sets_[id].advertising_handle_ = id;

          advertising_sets_[id].pending_start = true;

        }

      } break;

      case (AdvertisingApiType::EXTENDED): {
@@ -486,9 +483,6 @@ struct LeAdvertisingManager::impl : public bluetooth::hci::LeAddressManagerCallb 
    if (!paused) {

      enable_advertiser(id, true, duration, max_ext_adv_events);

    } else {

      // invoke callbacks upon OnResume()

      advertising_sets_[id].pending_start = true;



      EnabledSet curr_set;

      curr_set.advertising_handle_ = id;

      curr_set.duration_ = duration;
@@ -933,11 +927,6 @@ struct LeAdvertisingManager::impl : public bluetooth::hci::LeAddressManagerCallb 
      return;

    }



    if (enable) {

      // so that callbacks get invoked on command complete

      advertising_sets_[advertiser_id].pending_start = true;

    }



    switch (advertising_api_type_) {

      case (AdvertisingApiType::LEGACY): {

        le_advertising_interface_->EnqueueCommand(
@@ -946,22 +935,29 @@ struct LeAdvertisingManager::impl : public bluetooth::hci::LeAddressManagerCallb 
                this,

                &impl::on_set_advertising_enable_complete<LeSetAdvertisingEnableCompleteView>,

                enable,

                enabled_sets));

                enabled_sets,

                true /* trigger callbacks */));

      } break;

      case (AdvertisingApiType::ANDROID_HCI): {

        le_advertising_interface_->EnqueueCommand(

            hci::LeMultiAdvtSetEnableBuilder::Create(enable_value, advertiser_id),

            module_handler_->BindOnceOn(

                this, &impl::on_set_advertising_enable_complete<LeMultiAdvtCompleteView>, enable, enabled_sets));

                this,

                &impl::on_set_advertising_enable_complete<LeMultiAdvtCompleteView>,

                enable,

                enabled_sets,

                true /* trigger callbacks */));

      } break;

      case (AdvertisingApiType::EXTENDED): {

        le_advertising_interface_->EnqueueCommand(

            hci::LeSetExtendedAdvertisingEnableBuilder::Create(enable_value, enabled_sets),

            module_handler_->BindOnceOn(

                this,

                &impl::on_set_extended_advertising_enable_complete<LeSetExtendedAdvertisingEnableCompleteView>,

                &impl::on_set_extended_advertising_enable_complete<

                    LeSetExtendedAdvertisingEnableCompleteView>,

                enable,

                enabled_sets));

                enabled_sets,

                true /* trigger callbacks */));

      } break;

    }
@@ -1128,13 +1124,17 @@ struct LeAdvertisingManager::impl : public bluetooth::hci::LeAddressManagerCallb 
        case (AdvertisingApiType::LEGACY): {

          le_advertising_interface_->EnqueueCommand(

              hci::LeSetAdvertisingEnableBuilder::Create(Enable::ENABLED),

              common::init_flags::trigger_advertising_callbacks_on_first_resume_after_pause_is_enabled()

              common::init_flags::

                      trigger_advertising_callbacks_on_first_resume_after_pause_is_enabled()

                  ? module_handler_->BindOnceOn(

                        this,

                        &impl::on_set_advertising_enable_complete<LeSetAdvertisingEnableCompleteView>,

                        &impl::on_set_advertising_enable_complete<

                            LeSetAdvertisingEnableCompleteView>,

                        true,

                        enabled_sets)

                  : module_handler_->BindOnce(impl::check_status<LeSetAdvertisingEnableCompleteView>));

                        enabled_sets,

                        false /* trigger_callbacks */)

                  : module_handler_->BindOnce(

                        impl::check_status<LeSetAdvertisingEnableCompleteView>));

        } break;

        case (AdvertisingApiType::ANDROID_HCI): {

          for (size_t i = 0; i < enabled_sets_.size(); i++) {
@@ -1142,12 +1142,14 @@ struct LeAdvertisingManager::impl : public bluetooth::hci::LeAddressManagerCallb 
            if (id != kInvalidHandle) {

              le_advertising_interface_->EnqueueCommand(

                  hci::LeMultiAdvtSetEnableBuilder::Create(Enable::ENABLED, id),

                  common::init_flags::trigger_advertising_callbacks_on_first_resume_after_pause_is_enabled()

                  common::init_flags::

                          trigger_advertising_callbacks_on_first_resume_after_pause_is_enabled()

                      ? module_handler_->BindOnceOn(

                            this,

                            &impl::on_set_advertising_enable_complete<LeMultiAdvtCompleteView>,

                            true,

                            enabled_sets)

                            enabled_sets,

                            false /* trigger_callbacks */)

                      : module_handler_->BindOnce(impl::check_status<LeMultiAdvtCompleteView>));

            }

          }
@@ -1156,14 +1158,17 @@ struct LeAdvertisingManager::impl : public bluetooth::hci::LeAddressManagerCallb 
          if (enabled_sets.size() != 0) {

            le_advertising_interface_->EnqueueCommand(

                hci::LeSetExtendedAdvertisingEnableBuilder::Create(Enable::ENABLED, enabled_sets),

                common::init_flags::trigger_advertising_callbacks_on_first_resume_after_pause_is_enabled()

                common::init_flags::

                        trigger_advertising_callbacks_on_first_resume_after_pause_is_enabled()

                    ? module_handler_->BindOnceOn(

                          this,

                          &impl::on_set_extended_advertising_enable_complete<

                              LeSetExtendedAdvertisingEnableCompleteView>,

                          true,

                          enabled_sets)

                    : module_handler_->BindOnce(impl::check_status<LeSetExtendedAdvertisingEnableCompleteView>));

                          enabled_sets,

                          false /* trigger_callbacks */)

                    : module_handler_->BindOnce(

                          impl::check_status<LeSetExtendedAdvertisingEnableCompleteView>));

          }

        } break;

      }
@@ -1230,7 +1235,11 @@ struct LeAdvertisingManager::impl : public bluetooth::hci::LeAddressManagerCallb 
  }



  template <class View>

  void on_set_advertising_enable_complete(bool enable, std::vector<EnabledSet> enabled_sets, CommandCompleteView view) {

  void on_set_advertising_enable_complete(

      bool enable,

      std::vector<EnabledSet> enabled_sets,

      bool trigger_callbacks,

      CommandCompleteView view) {

    ASSERT(view.IsValid());

    auto complete_view = View::Create(view);

    ASSERT(complete_view.IsValid());
@@ -1259,15 +1268,9 @@ struct LeAdvertisingManager::impl : public bluetooth::hci::LeAddressManagerCallb 
      }



      if (started) {

        // This event can be triggered from OnPause / OnResume. If so, only invoke callbacks if we were initially paused

        // (pending_start) after an API invocation (i.e. StartAdvertising -> currently paused -> OnResume -> enabled

        // [callback], but StartAdvertising -> enabled [callback] -> resumed -> OnPause -> OnResume [NO callback]

        if (!advertising_sets_[enabled_set.advertising_handle_].pending_start) {

          continue;

        }

        if (trigger_callbacks) {

          advertising_callbacks_->OnAdvertisingEnabled(id, enable, advertising_status);

        // since we have started, we are no longer pending

        advertising_sets_[enabled_set.advertising_handle_].pending_start = false;

        }

      } else {

        advertising_sets_[enabled_set.advertising_handle_].started = true;

        advertising_callbacks_->OnAdvertisingSetStarted(reg_id, id, le_physical_channel_tx_power_, advertising_status);
@@ -1277,7 +1280,10 @@ struct LeAdvertisingManager::impl : public bluetooth::hci::LeAddressManagerCallb 


  template <class View>

  void on_set_extended_advertising_enable_complete(

      bool enable, std::vector<EnabledSet> enabled_sets, CommandCompleteView view) {

      bool enable,

      std::vector<EnabledSet> enabled_sets,

      bool trigger_callbacks,

      CommandCompleteView view) {

    ASSERT(view.IsValid());

    auto complete_view = LeSetExtendedAdvertisingEnableCompleteView::Create(view);

    ASSERT(complete_view.IsValid());
@@ -1309,15 +1315,9 @@ struct LeAdvertisingManager::impl : public bluetooth::hci::LeAddressManagerCallb 
      }



      if (started) {

        // This event can be triggered from OnPause / OnResume. If so, only invoke callbacks if we were initially paused

        // (pending_start) after an API invocation (i.e. StartAdvertising -> currently paused -> OnResume -> enabled

        // [callback], but StartAdvertising -> enabled [callback] -> resumed -> OnPause -> OnResume [NO callback]

        if (!advertising_sets_[enabled_set.advertising_handle_].pending_start) {

          continue;

        }

        if (trigger_callbacks) {

          advertising_callbacks_->OnAdvertisingEnabled(id, enable, advertising_status);

        // since we have started, we are no longer pending

        advertising_sets_[enabled_set.advertising_handle_].pending_start = false;

        }

      } else {

        advertising_sets_[enabled_set.advertising_handle_].started = true;

        advertising_callbacks_->OnAdvertisingSetStarted(reg_id, id, tx_power, advertising_status);

system/gd/hci/le_advertising_manager_test.cc

+87 −6
Original line number Diff line number Diff line
@@ -36,8 +36,13 @@ namespace bluetooth { 
namespace hci {

namespace {



using namespace std::literals;



using packet::RawBuilder;



using testing::_;

using testing::InSequence;



class TestController : public Controller {

 public:

  bool IsSupported(OpCode op_code) const override {
@@ -228,11 +233,7 @@ class LeAdvertisingManagerTest : public ::testing::Test { 
  void on_set_terminated(ErrorCode error_code, uint8_t, uint8_t) {}



  void sync_client_handler() {

    std::promise<void> promise;

    auto future = promise.get_future();

    client_handler_->Post(common::BindOnce(&std::promise<void>::set_value, common::Unretained(&promise)));

    auto future_status = future.wait_for(std::chrono::seconds(1));

    ASSERT_EQ(future_status, std::future_status::ready);

    ASSERT(thread_.GetReactor()->WaitForIdle(2s));

  }



  class MockAdvertisingCallback : public AdvertisingCallback {
@@ -1003,6 +1004,47 @@ TEST_F(LeExtendedAdvertisingAPITest, disable_enable_advertiser_test) { 
  test_hci_layer_->IncomingEvent(LeSetExtendedAdvertisingEnableCompleteBuilder::Create(uint8_t{1}, ErrorCode::SUCCESS));

}



TEST_F(LeExtendedAdvertisingAPITest, disable_after_enable) {

  // we expect Started -> Enable(false) -> Enable(true) -> Enable(false)



  // setup already arranges everything and starts the advertiser



  // expect

  InSequence s;

  EXPECT_CALL(mock_advertising_callback_, OnAdvertisingEnabled(_, false, _));

  EXPECT_CALL(mock_advertising_callback_, OnAdvertisingEnabled(_, true, _));

  EXPECT_CALL(mock_advertising_callback_, OnAdvertisingEnabled(_, false, _));

  EXPECT_CALL(mock_advertising_callback_, OnAdvertisingEnabled(_, true, _));



  // act



  // disable

  le_advertising_manager_->EnableAdvertiser(advertiser_id_, false, 0x00, 0x00);

  test_hci_layer_->GetCommand();

  test_hci_layer_->IncomingEvent(

      LeSetExtendedAdvertisingEnableCompleteBuilder::Create(uint8_t{1}, ErrorCode::SUCCESS));



  // enable

  le_advertising_manager_->EnableAdvertiser(advertiser_id_, true, 0x00, 0x00);

  test_hci_layer_->GetCommand();

  test_hci_layer_->IncomingEvent(

      LeSetExtendedAdvertisingEnableCompleteBuilder::Create(uint8_t{1}, ErrorCode::SUCCESS));



  // disable

  le_advertising_manager_->EnableAdvertiser(advertiser_id_, false, 0x00, 0x00);

  test_hci_layer_->GetCommand();

  test_hci_layer_->IncomingEvent(

      LeSetExtendedAdvertisingEnableCompleteBuilder::Create(uint8_t{1}, ErrorCode::SUCCESS));



  // enable

  le_advertising_manager_->EnableAdvertiser(advertiser_id_, true, 0x00, 0x00);

  test_hci_layer_->GetCommand();

  test_hci_layer_->IncomingEvent(

      LeSetExtendedAdvertisingEnableCompleteBuilder::Create(uint8_t{1}, ErrorCode::SUCCESS));



  sync_client_handler();

}



TEST_F(LeExtendedAdvertisingAPITest, set_periodic_parameter) {

  PeriodicAdvertisingParameters advertising_config{};

  advertising_config.max_interval = 0x1000;
@@ -1132,7 +1174,6 @@ TEST_F(LeExtendedAdvertisingAPITest, trigger_advertiser_callbacks_if_started_whi 
      [](std::promise<uint8_t> promise, uint8_t id, uint8_t _status) { promise.set_value(id); },

      std::move(id_promise)));

  sync_client_handler();

  LOG_INFO("start");

  auto set_id = id_future.get();



  auto status_promise = std::promise<ErrorCode>{};
@@ -1175,6 +1216,46 @@ TEST_F(LeExtendedAdvertisingAPITest, trigger_advertiser_callbacks_if_started_whi 


  // assert

  EXPECT_EQ(status_future.get(), ErrorCode::SUCCESS);



  sync_client_handler();

}



TEST_F(LeExtendedAdvertisingAPITest, no_callbacks_on_pause) {

  // arrange

  auto test_le_address_manager = (TestLeAddressManager*)test_acl_manager_->GetLeAddressManager();



  // expect

  EXPECT_CALL(mock_advertising_callback_, OnAdvertisingEnabled(_, _, _)).Times(0);



  // act

  LOG_INFO("pause");

  test_le_address_manager->client_->OnPause();

  test_hci_layer_->GetCommand();

  test_hci_layer_->IncomingEvent(

      LeSetExtendedAdvertisingEnableCompleteBuilder::Create(1, ErrorCode::SUCCESS));



  sync_client_handler();

}



TEST_F(LeExtendedAdvertisingAPITest, no_callbacks_on_resume) {

  // arrange

  auto test_le_address_manager = (TestLeAddressManager*)test_acl_manager_->GetLeAddressManager();

  test_le_address_manager->client_->OnPause();

  test_hci_layer_->GetCommand();

  test_hci_layer_->IncomingEvent(

      LeSetExtendedAdvertisingEnableCompleteBuilder::Create(1, ErrorCode::SUCCESS));

  sync_client_handler();



  // expect

  EXPECT_CALL(mock_advertising_callback_, OnAdvertisingEnabled(_, _, _)).Times(0);



  // act

  test_le_address_manager->client_->OnResume();

  test_hci_layer_->GetCommand();

  test_hci_layer_->IncomingEvent(

      LeSetExtendedAdvertisingEnableCompleteBuilder::Create(1, ErrorCode::SUCCESS));



  sync_client_handler();

}



}  // namespace