From 3adb98a5ff6d78e2ca74c48a5145f11713e0670c Mon Sep 17 00:00:00 2001 From: Janusz Sobczak Date: Fri, 26 May 2023 11:49:32 -0700 Subject: [PATCH] Introduce scanning sessions FastPairScanner::ScanningSession and ScannerBroker::ScanningSession separate the lifetime of FastPairScanner/ScannerBroker from the lifetime of scanning sessions. This simplifies the cleanup. PiperOrigin-RevId: 535675988 --- fastpair/keyed_service/fast_pair_mediator.cc | 5 +-- fastpair/keyed_service/fast_pair_mediator.h | 1 + .../fastpair/fake_fast_pair_scanner.h | 6 +++- .../scanning/fastpair/fast_pair_scanner.h | 8 ++++- .../fastpair/fast_pair_scanner_impl.cc | 26 ++++++++++++++-- .../fastpair/fast_pair_scanner_impl.h | 8 ++--- .../fastpair/fast_pair_scanner_impl_test.cc | 31 +++++++++++++++++-- fastpair/scanning/mock_scanner_broker.h | 4 +-- fastpair/scanning/scanner_broker.h | 9 ++++-- fastpair/scanning/scanner_broker_impl.cc | 30 +++++++++++++++--- fastpair/scanning/scanner_broker_impl.h | 5 +-- fastpair/scanning/scanner_broker_impl_test.cc | 4 ++- internal/platform/single_thread_executor.h | 9 ++++++ 13 files changed, 122 insertions(+), 24 deletions(-) diff --git a/fastpair/keyed_service/fast_pair_mediator.cc b/fastpair/keyed_service/fast_pair_mediator.cc index fee1b2c9..2f84dee1 100644 --- a/fastpair/keyed_service/fast_pair_mediator.cc +++ b/fastpair/keyed_service/fast_pair_mediator.cc @@ -40,10 +40,11 @@ void Mediator::OnDeviceLost(FastPairDevice& device) { void Mediator::StartScanning() { if (IsFastPairEnabled()) { - scanner_broker_->StartScanning(Protocol::kFastPairInitialPairing); + scanning_session_ = + scanner_broker_->StartScanning(Protocol::kFastPairInitialPairing); return; } - scanner_broker_->StopScanning(Protocol::kFastPairInitialPairing); + scanning_session_.reset(); } bool Mediator::IsFastPairEnabled() { diff --git a/fastpair/keyed_service/fast_pair_mediator.h b/fastpair/keyed_service/fast_pair_mediator.h index 499b6ea2..1b25b303 100644 --- a/fastpair/keyed_service/fast_pair_mediator.h +++ b/fastpair/keyed_service/fast_pair_mediator.h @@ -42,6 +42,7 @@ class Mediator final : public ScannerBroker::Observer { bool IsFastPairEnabled(); std::unique_ptr scanner_broker_; + std::unique_ptr scanning_session_; std::unique_ptr fast_pair_repository_; }; diff --git a/fastpair/scanning/fastpair/fake_fast_pair_scanner.h b/fastpair/scanning/fastpair/fake_fast_pair_scanner.h index ed9d9254..c6f5f666 100644 --- a/fastpair/scanning/fastpair/fake_fast_pair_scanner.h +++ b/fastpair/scanning/fastpair/fake_fast_pair_scanner.h @@ -15,6 +15,8 @@ #ifndef THIRD_PARTY_NEARBY_FASTPAIR_SCANNING_FASTPAIR_FAKE_FAST_PAIR_SCANNER_H_ #define THIRD_PARTY_NEARBY_FASTPAIR_SCANNING_FASTPAIR_FAKE_FAST_PAIR_SCANNER_H_ +#include + #include "fastpair/scanning/fastpair/fast_pair_scanner.h" #include "internal/base/observer_list.h" @@ -32,7 +34,9 @@ class FakeFastPairScanner final : public FastPairScanner { void RemoveObserver(Observer* observer) override; void NotifyDeviceFound(const BlePeripheral& peripheral); void NotifyDeviceLost(const BlePeripheral& peripheral); - void StartScanning() override {}; + std::unique_ptr StartScanning() override { + return std::make_unique(); + }; private: ObserverList observer_; diff --git a/fastpair/scanning/fastpair/fast_pair_scanner.h b/fastpair/scanning/fastpair/fast_pair_scanner.h index 3146684d..5205d518 100644 --- a/fastpair/scanning/fastpair/fast_pair_scanner.h +++ b/fastpair/scanning/fastpair/fast_pair_scanner.h @@ -36,10 +36,16 @@ class FastPairScanner { virtual void OnDeviceLost(const BlePeripheral& peripheral) = 0; }; + // Represents scanning session. Must be destroyed before FastPairScanner. + class ScanningSession { + public: + virtual ~ScanningSession() = default; + }; + virtual void AddObserver(Observer* observer) = 0; virtual void RemoveObserver(Observer* observer) = 0; - virtual void StartScanning() = 0; + virtual std::unique_ptr StartScanning() = 0; virtual ~FastPairScanner() = default; }; diff --git a/fastpair/scanning/fastpair/fast_pair_scanner_impl.cc b/fastpair/scanning/fastpair/fast_pair_scanner_impl.cc index c0e3021e..5778a01e 100644 --- a/fastpair/scanning/fastpair/fast_pair_scanner_impl.cc +++ b/fastpair/scanning/fastpair/fast_pair_scanner_impl.cc @@ -30,6 +30,17 @@ namespace { constexpr absl::Duration kFastPairLowPowerActiveSeconds = absl::Seconds(2); constexpr absl::Duration kFastPairLowPowerInactiveSeconds = absl::Seconds(3); constexpr char kFastPairServiceUuid[] = "0000FE2C-0000-1000-8000-00805F9B34FB"; + +class ScanningSessionImpl : public FastPairScanner::ScanningSession { + public: + explicit ScanningSessionImpl(FastPairScannerImpl* scanner) + : scanner_(scanner) {} + ~ScanningSessionImpl() override { scanner_->StopScanning(); } + + private: + FastPairScannerImpl* scanner_; +}; + } // namespace // FastPairScannerImpl @@ -45,11 +56,14 @@ void FastPairScannerImpl::RemoveObserver(FastPairScanner::Observer* observer) { observer_.RemoveObserver(observer); } -void FastPairScannerImpl::StartScanning() { +std::unique_ptr +FastPairScannerImpl::StartScanning() { NEARBY_LOGS(VERBOSE) << __func__; executor_->Execute("scanning", [this]() ABSL_EXCLUSIVE_LOCKS_REQUIRED( *executor_) { StartScanningInternal(); }); + return std::make_unique(this); } + void FastPairScannerImpl::StartScanningInternal() { if (mediums_.GetBluetoothRadio().Enable() && mediums_.GetBle().IsAvailable() && @@ -80,7 +94,7 @@ void FastPairScannerImpl::StartScanningInternal() { if (IsFastPairLowPowerEnabled()) { StartTimer(kFastPairLowPowerActiveSeconds, [this]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(*executor_) { - StopScanning(); + PauseScanning(); }); } } else { @@ -90,6 +104,14 @@ void FastPairScannerImpl::StartScanningInternal() { } void FastPairScannerImpl::StopScanning() { + executor_->Execute("stop-scan", + [this]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(*executor_) { + timer_.reset(); + mediums_.GetBle().StopScanning(kServiceId); + }); +} + +void FastPairScannerImpl::PauseScanning() { DCHECK(IsFastPairLowPowerEnabled()); mediums_.GetBle().StopScanning(kServiceId); StartTimer(kFastPairLowPowerInactiveSeconds, diff --git a/fastpair/scanning/fastpair/fast_pair_scanner_impl.h b/fastpair/scanning/fastpair/fast_pair_scanner_impl.h index 6599668e..474f60e1 100644 --- a/fastpair/scanning/fastpair/fast_pair_scanner_impl.h +++ b/fastpair/scanning/fastpair/fast_pair_scanner_impl.h @@ -49,11 +49,13 @@ class FastPairScannerImpl : public FastPairScanner { // Todo(b/267348348): Support Flags to control feature ramp bool IsFastPairLowPowerEnabled() const { return false; } - void StartScanning() override; + std::unique_ptr StartScanning() override; + void StopScanning(); private: void StartScanningInternal() ABSL_EXCLUSIVE_LOCKS_REQUIRED(*executor_); - void StopScanning() ABSL_EXCLUSIVE_LOCKS_REQUIRED(*executor_); + // Pauses, and then restarts, scanning for a few seconds to safe power. + void PauseScanning() ABSL_EXCLUSIVE_LOCKS_REQUIRED(*executor_); void StartTimer(absl::Duration delay, absl::AnyInvocable callback) ABSL_EXCLUSIVE_LOCKS_REQUIRED(*executor_); @@ -65,8 +67,6 @@ class FastPairScannerImpl : public FastPairScanner { // seen. absl::flat_hash_map> device_address_advertisement_data_map_; - - BluetoothAdapter bluetooth_adapter_; ObserverList observer_; }; diff --git a/fastpair/scanning/fastpair/fast_pair_scanner_impl_test.cc b/fastpair/scanning/fastpair/fast_pair_scanner_impl_test.cc index 91fd0607..80c9c254 100644 --- a/fastpair/scanning/fastpair/fast_pair_scanner_impl_test.cc +++ b/fastpair/scanning/fastpair/fast_pair_scanner_impl_test.cc @@ -32,6 +32,7 @@ namespace fastpair { namespace { constexpr absl::Duration kTaskWaitTimeout = absl::Milliseconds(1000); +constexpr absl::Duration kShortTimeout = absl::Milliseconds(100); constexpr absl::string_view kServiceID{"Fast Pair"}; constexpr absl::string_view kModelId{"718c17"}; constexpr absl::string_view kFastPairServiceUuid{ @@ -85,7 +86,7 @@ TEST_F(FastPairScannerImplTest, StartScanning) { service_id, advertisement_bytes, fast_pair_service_uuid); // Fast Pair scanner startScanning - scanner->StartScanning(); + auto scan_session = scanner->StartScanning(); // Notify device found EXPECT_TRUE(accept_latch.Await(kTaskWaitTimeout).result()); @@ -93,9 +94,35 @@ TEST_F(FastPairScannerImplTest, StartScanning) { mediums_2.GetBle().GetMedium().StopAdvertising(service_id); // Notify device lost EXPECT_TRUE(lost_latch.Await(kTaskWaitTimeout).result()); - + scan_session.reset(); env_.Stop(); } + +TEST_F(FastPairScannerImplTest, StopScanning) { + env_.Start(); + // Create Fast Pair Scanner and add its observer + Mediums mediums_1; + auto scanner = std::make_unique(mediums_1, &executor_); + CountDownLatch accept_latch(1); + CountDownLatch lost_latch(1); + FastPairScannerObserver observer(scanner.get(), &accept_latch, &lost_latch); + // Create Advertiser and startAdvertising + Mediums mediums_2; + std::string service_id(kServiceID); + ByteArray advertisement_bytes{absl::HexStringToBytes(kModelId)}; + std::string fast_pair_service_uuid(kFastPairServiceUuid); + mediums_2.GetBle().GetMedium().StartAdvertising( + service_id, advertisement_bytes, fast_pair_service_uuid); + + auto scan_session = scanner->StartScanning(); + scan_session.reset(); + + mediums_2.GetBle().GetMedium().StopAdvertising(service_id); + // Device lost event should not be delivered when scan session has terminated. + EXPECT_FALSE(lost_latch.Await(kShortTimeout).result()); + env_.Stop(); +} + } // namespace } // namespace fastpair } // namespace nearby diff --git a/fastpair/scanning/mock_scanner_broker.h b/fastpair/scanning/mock_scanner_broker.h index 29042c8d..5ce7252e 100644 --- a/fastpair/scanning/mock_scanner_broker.h +++ b/fastpair/scanning/mock_scanner_broker.h @@ -24,8 +24,8 @@ namespace fastpair { class MockScannerBroker : public ScannerBroker { public: - MOCK_METHOD(void, StartScanning, (Protocol), (override)); - MOCK_METHOD(void, StopScanning, (Protocol), (override)); + MOCK_METHOD(std::unique_ptr, StartScanning, (Protocol), + (override)); void AddObserver(Observer* observer) override { observers_.AddObserver(observer); diff --git a/fastpair/scanning/scanner_broker.h b/fastpair/scanning/scanner_broker.h index 3242e7b4..5340f029 100644 --- a/fastpair/scanning/scanner_broker.h +++ b/fastpair/scanning/scanner_broker.h @@ -15,6 +15,8 @@ #ifndef THIRD_PARTY_NEARBY_FASTPAIR_SCANNING_SCANNER_BROKER_H_ #define THIRD_PARTY_NEARBY_FASTPAIR_SCANNING_SCANNER_BROKER_H_ +#include + #include "fastpair/common/fast_pair_device.h" #include "fastpair/common/protocol.h" @@ -35,14 +37,17 @@ class ScannerBroker { virtual void OnDeviceFound(FastPairDevice& device) = 0; virtual void OnDeviceLost(FastPairDevice& device) = 0; }; + class ScanningSession { + public: + virtual ~ScanningSession() = default; + }; virtual ~ScannerBroker() = default; virtual void AddObserver(Observer* observer) = 0; virtual void RemoveObserver(Observer* observer) = 0; - virtual void StartScanning(Protocol protocol) = 0; - virtual void StopScanning(Protocol protocol) = 0; + virtual std::unique_ptr StartScanning(Protocol protocol) = 0; }; } // namespace fastpair diff --git a/fastpair/scanning/scanner_broker_impl.cc b/fastpair/scanning/scanner_broker_impl.cc index 9734784c..3650b3cd 100644 --- a/fastpair/scanning/scanner_broker_impl.cc +++ b/fastpair/scanning/scanner_broker_impl.cc @@ -15,6 +15,7 @@ #include "fastpair/scanning/scanner_broker_impl.h" #include +#include #include "absl/functional/bind_front.h" #include "fastpair/common/fast_pair_device.h" @@ -24,6 +25,22 @@ namespace nearby { namespace fastpair { + +namespace { + +class ScanningSessionImpl : public ScannerBroker::ScanningSession { + public: + ScanningSessionImpl(ScannerBrokerImpl* scanner, Protocol protocol) + : scanner_(scanner), protocol_(protocol) {} + ~ScanningSessionImpl() override { scanner_->StopScanning(protocol_); } + + private: + ScannerBrokerImpl* scanner_; + Protocol protocol_; +}; + +} // namespace + ScannerBrokerImpl::ScannerBrokerImpl( Mediums& mediums, SingleThreadExecutor* executor, FastPairDeviceRepository* device_repository) @@ -39,12 +56,14 @@ void ScannerBrokerImpl::RemoveObserver(Observer* observer) { observers_.RemoveObserver(observer); } -void ScannerBrokerImpl::StartScanning(Protocol protocol) { +std::unique_ptr +ScannerBrokerImpl::StartScanning(Protocol protocol) { NEARBY_LOGS(VERBOSE) << __func__ << ": protocol=" << protocol; executor_->Execute("start-scan", [this]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(*executor_) { StartFastPairScanning(); }); + return std::make_unique(this, protocol); } void ScannerBrokerImpl::StopScanning(Protocol protocol) { @@ -63,14 +82,15 @@ void ScannerBrokerImpl::StartFastPairScanning() { absl::bind_front(&ScannerBrokerImpl::NotifyDeviceFound, this), absl::bind_front(&ScannerBrokerImpl::NotifyDeviceLost, this), executor_, device_repository_); - scanner_->StartScanning(); + scanning_session_ = scanner_->StartScanning(); } void ScannerBrokerImpl::StopFastPairScanning() { - fast_pair_discoverable_scanner_.reset(); - scanner_.reset(); - observers_.Clear(); NEARBY_LOGS(VERBOSE) << __func__ << "Stopping Fast Pair Scanning."; + scanning_session_.reset(); + observers_.Clear(); + DestroyOnExecutor(std::move(fast_pair_discoverable_scanner_), executor_); + DestroyOnExecutor(std::move(scanner_), executor_); } void ScannerBrokerImpl::NotifyDeviceFound(FastPairDevice& device) { diff --git a/fastpair/scanning/scanner_broker_impl.h b/fastpair/scanning/scanner_broker_impl.h index a6663b20..6804df58 100644 --- a/fastpair/scanning/scanner_broker_impl.h +++ b/fastpair/scanning/scanner_broker_impl.h @@ -38,8 +38,8 @@ class ScannerBrokerImpl : public ScannerBroker { // ScannerBroker: void AddObserver(Observer* observer) override; void RemoveObserver(Observer* observer) override; - void StartScanning(Protocol protocol) override; - void StopScanning(Protocol protocol) override; + std::unique_ptr StartScanning(Protocol protocol) override; + void StopScanning(Protocol protocol); private: void StartFastPairScanning() ABSL_EXCLUSIVE_LOCKS_REQUIRED(*executor_); @@ -54,6 +54,7 @@ class ScannerBrokerImpl : public ScannerBroker { ABSL_GUARDED_BY(*executor_); ObserverList observers_; FastPairDeviceRepository* device_repository_; + std::unique_ptr scanning_session_; }; } // namespace fastpair diff --git a/fastpair/scanning/scanner_broker_impl_test.cc b/fastpair/scanning/scanner_broker_impl_test.cc index 612646df..418462f9 100644 --- a/fastpair/scanning/scanner_broker_impl_test.cc +++ b/fastpair/scanning/scanner_broker_impl_test.cc @@ -99,7 +99,8 @@ TEST_F(ScannerBrokerImplTest, CanStartScanning) { service_id, advertisement_bytes, fast_pair_service_uuid); // Fast Pair scanner startScanning - scanner_broker->StartScanning(Protocol::kFastPairInitialPairing); + auto scanning_session = + scanner_broker->StartScanning(Protocol::kFastPairInitialPairing); // Notify device found EXPECT_TRUE(accept_latch.Await(kTaskWaitTimeout).result()); @@ -109,6 +110,7 @@ TEST_F(ScannerBrokerImplTest, CanStartScanning) { // Notify device lost EXPECT_TRUE(lost_latch.Await(kTaskWaitTimeout).result()); + scanning_session.reset(); env_.Stop(); } } // namespace diff --git a/internal/platform/single_thread_executor.h b/internal/platform/single_thread_executor.h index eb53bbee..d71ddd86 100644 --- a/internal/platform/single_thread_executor.h +++ b/internal/platform/single_thread_executor.h @@ -34,6 +34,15 @@ class ABSL_LOCKABLE SingleThreadExecutor : public SubmittableExecutor { SingleThreadExecutor& operator=(SingleThreadExecutor&&) = default; }; +// Moves the object to `executor` and destroys it there. +// This pattern is useful when there are some tasks running on `executor` that +// hold a reference to `object`. The tasks will complete before `object` is +// destroyed. +template +void DestroyOnExecutor(T object, SingleThreadExecutor* executor) { + executor->Execute([object = std::move(object)] {}); +} + } // namespace nearby #endif // PLATFORM_PUBLIC_SINGLE_THREAD_EXECUTOR_H_