From 255bdb61394117ad6764c09e667929dcebf877b8 Mon Sep 17 00:00:00 2001 From: Janusz Sobczak Date: Wed, 30 Nov 2022 14:19:31 -0800 Subject: [PATCH] Add StopScan method. Add StopScan and refactor StartScan to return a sesssion id. Move handling of StartScan, StopScan and NotifyFoundBle to the same thread to avoid race conditions. Implement start advertising in service controller. PiperOrigin-RevId: 492016697 --- presence/data_types.h | 33 +--- .../implementation/mock_service_controller.h | 2 +- presence/implementation/scan_manager.cc | 155 +++++++++--------- presence/implementation/scan_manager.h | 53 +++--- presence/implementation/scan_manager_test.cc | 117 +++++-------- presence/implementation/service_controller.h | 5 +- .../implementation/service_controller_impl.cc | 5 +- .../implementation/service_controller_impl.h | 9 +- presence/presence_client.cc | 15 +- presence/presence_client.h | 32 ++-- presence/presence_client_test.cc | 13 +- presence/presence_service.cc | 5 +- presence/presence_service.h | 6 +- presence/presence_service_test.cc | 19 +-- 14 files changed, 220 insertions(+), 249 deletions(-) diff --git a/presence/data_types.h b/presence/data_types.h index e90c17e2..e8557b0b 100644 --- a/presence/data_types.h +++ b/presence/data_types.h @@ -26,37 +26,8 @@ namespace nearby { namespace presence { -// Holds the callback of stop scan for client to invoke later. -// TODO(b/254895067) Rework status for absl::Status -class ScanSession { - public: - ScanSession() - : stop_scan_callback_( - []() { return Status{Status::Value::kNotImplemented}; }) {} - explicit ScanSession(absl::AnyInvocable stop_scan_callback) - : stop_scan_callback_(std::move(stop_scan_callback)) {} - - ~ScanSession() { StopScan(); } - - Status StopScan() { - if (stop_called_) { - NEARBY_LOGS(WARNING) << "StopScan already called."; - return Status{Status::Value::kError}; - } - stop_called_ = true; - if (stop_scan_callback_) { - return std::move(stop_scan_callback_)(); - } - return Status{Status::Value::kError}; - } - - private: - // Nearby library would provide the implementation of this callback in - // runtime. Assigning with a default value NotImplemented to surface potential - // issue where library failed to provide the implementation. - absl::AnyInvocable stop_scan_callback_; - bool stop_called_ = false; -}; +// Unique Scan Session Identifier. +using ScanSessionId = uint64_t; // Callers would provide the implementation of these callbacks. If callers // don't need these signal updates, they can skip with the provided default diff --git a/presence/implementation/mock_service_controller.h b/presence/implementation/mock_service_controller.h index 0c26cfd6..84960f12 100644 --- a/presence/implementation/mock_service_controller.h +++ b/presence/implementation/mock_service_controller.h @@ -31,7 +31,7 @@ class MockServiceController : public ServiceController { MockServiceController() = default; ~MockServiceController() override = default; - MOCK_METHOD(std::unique_ptr, StartScan, + MOCK_METHOD(absl::StatusOr, StartScan, (ScanRequest scan_request, ScanCallback callback), (override)); MOCK_METHOD(absl::StatusOr, StartBroadcast, (BroadcastRequest broadcast_request, BroadcastCallback callback), diff --git a/presence/implementation/scan_manager.cc b/presence/implementation/scan_manager.cc index f24c52ab..c1cc86ae 100644 --- a/presence/implementation/scan_manager.cc +++ b/presence/implementation/scan_manager.cc @@ -22,8 +22,8 @@ #include #include "absl/random/random.h" -#include "absl/random/uniform_int_distribution.h" #include "absl/types/variant.h" +#include "internal/platform/future.h" #include "internal/platform/implementation/ble_v2.h" #include "internal/platform/uuid.h" #include "presence/data_types.h" @@ -47,86 +47,93 @@ using ScanningCallback = namespace nearby { namespace presence { -std::unique_ptr ScanManager::StartScan(ScanRequest scan_request, - ScanCallback cb) { - absl::BitGen gen; - uint64_t id = absl::uniform_int_distribution(0, UINT64_MAX)(gen); - ScanningCallback callback = ScanningCallback{ - .start_scanning_result = - [start_scan_client = - std::move(cb.start_scan_cb)](BleOperationStatus ble_status) { - Status status; - if (ble_status == BleOperationStatus::kSucceeded) { - status = Status{.value = Status::Value::kSuccess}; - } else { - status = Status{.value = Status::Value::kError}; - } - start_scan_client(status); - }, - // TODO(b/256686710): Track known devices - .advertisement_found_cb = - [this](BlePeripheral& peripheral, BleAdvertisementData data) { - NotifyFoundBle(data, peripheral); - }}; - std::unique_ptr scanning_session = - mediums_->GetBle().StartScanning(scan_request, std::move(callback)); - if (scanning_session == nullptr) { - return nullptr; - } - // We will not be needing the start_scan_cb anymore, so cb is ok to use here. - AddScanCallback(id, MapElement{ - .request = scan_request, - .callback = cb, - .decoder = AdvertisementDecoder(credential_manager_, - scan_request), - }); - return std::make_unique( - /*stop_scan_callback=*/[scanning_session_internal = - std::move(scanning_session), - this, id]() { - { - absl::MutexLock lock(&mutex_); - int erased = absl::erase_if( - scanning_callbacks_, - [id](const auto& entry) { return id == entry.first; }); - if (erased == 0) return Status{.value = Status::Value::kError}; - // Unlock mutex since we don't need to access the list anymore. +ScanSessionId ScanManager::StartScan(ScanRequest scan_request, + ScanCallback cb) { + ScanSessionId id = absl::Uniform(bit_gen_); + RunOnServiceControllerThread( + "start-scan", + [this, id, scan_request, scan_callback = std::move(cb)]() + ABSL_EXCLUSIVE_LOCKS_REQUIRED(*executor_) { + ScanningCallback callback = ScanningCallback{ + .start_scanning_result = + [start_scan_client = + std::move(scan_callback.start_scan_cb)]( + BleOperationStatus ble_status) { + Status status; + if (ble_status == BleOperationStatus::kSucceeded) { + status = Status{.value = Status::Value::kSuccess}; + } else { + status = Status{.value = Status::Value::kError}; + } + start_scan_client(status); + }, + // TODO(b/256686710): Track known devices + .advertisement_found_cb = + [this, id](BlePeripheral& peripheral, + BleAdvertisementData data) { + RunOnServiceControllerThread( + "notify-found-ble", + [this, id, data = std::move(data), + address = peripheral.GetAddress()]() + ABSL_EXCLUSIVE_LOCKS_REQUIRED(*executor_) { + NotifyFoundBle(id, data, address); + }); + }}; + scan_sessions_.insert( + {id, ScanSessionState{ + .request = scan_request, + .callback = std::move(scan_callback), + .decoder = AdvertisementDecoder(credential_manager_, + scan_request), + .scanning_session = mediums_->GetBle().StartScanning( + scan_request, std::move(callback))}}); + }); + return id; +} + +void ScanManager::StopScan(ScanSessionId id) { + RunOnServiceControllerThread( + "stop-scan", [this, id]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(*executor_) { + auto it = scan_sessions_.find(id); + if (it == scan_sessions_.end()) { + return; } - BleOperationStatus st = scanning_session_internal->stop_scanning(); - if (st != BleOperationStatus::kSucceeded) { - return Status{.value = Status::Value::kError}; + if (it->second.scanning_session) { + it->second.scanning_session->stop_scanning(); } - return Status{.value = Status::Value::kSuccess}; + scan_sessions_.erase(it); }); } -void ScanManager::NotifyFoundBle(BleAdvertisementData data, - const BlePeripheral& peripheral) { - std::vector> callbacks; - { - absl::MutexLock lock(&mutex_); - auto advertisement_data = - data.service_data[kPresenceServiceUuid].AsStringView(); - for (const auto& entry : scanning_callbacks_) { - auto candidate = entry.second; - auto advert = candidate.decoder.DecodeAdvertisement(advertisement_data); - if (!advert.ok()) { - // This advertisement is not relevant to the current element, skip. - continue; - } - if (candidate.decoder.MatchesScanFilter(advert.value())) { - std::string bt_addr = peripheral.GetAddress(); - internal::DeviceMetadata metadata; - metadata.set_bluetooth_mac_address(bt_addr); - callbacks.push_back({candidate.callback, metadata}); - } - } +void ScanManager::NotifyFoundBle(ScanSessionId id, BleAdvertisementData data, + absl::string_view remote_address) { + auto advertisement_data = + data.service_data[kPresenceServiceUuid].AsStringView(); + auto it = scan_sessions_.find(id); + if (it == scan_sessions_.end()) { + return; } - // TODO(b/256913915): Provide more information in PresenceDevice once fully - // implemented - for (const auto& callback : callbacks) { - callback.first.on_discovered_cb(PresenceDevice(callback.second)); + auto advert = it->second.decoder.DecodeAdvertisement(advertisement_data); + if (!advert.ok()) { + // This advertisement is not relevant to the current element, skip. + return; } + if (it->second.decoder.MatchesScanFilter(advert.value())) { + // TODO(b/256913915): Provide more information in PresenceDevice once + // fully implemented + internal::DeviceMetadata metadata; + metadata.set_bluetooth_mac_address(remote_address); + it->second.callback.on_discovered_cb(PresenceDevice(metadata)); + } +} + +int ScanManager::ScanningCallbacksLengthForTest() { + ::location::nearby::Future count; + RunOnServiceControllerThread("callbacks-size", + [&]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(*executor_) { + count.Set(scan_sessions_.size()); + }); + return count.Get().GetResult(); } } // namespace presence diff --git a/presence/implementation/scan_manager.h b/presence/implementation/scan_manager.h index 6de0953f..436b227a 100644 --- a/presence/implementation/scan_manager.h +++ b/presence/implementation/scan_manager.h @@ -21,7 +21,8 @@ #include #include "absl/container/flat_hash_map.h" -#include "absl/synchronization/mutex.h" +#include "absl/random/random.h" +#include "internal/platform/single_thread_executor.h" #include "presence/data_types.h" #include "presence/implementation/advertisement_decoder.h" #include "presence/implementation/credential_manager.h" @@ -31,45 +32,51 @@ namespace nearby { namespace presence { -/* - * The instance of ScanManager is owned by {@code ServiceControllerImpl}. - * Helping service controller to manage scan requests and callbacks. - */ +// The instance of ScanManager is owned by `ServiceControllerImpl`. +// Helping service controller to manage scan requests and callbacks. class ScanManager { public: - ScanManager(Mediums& mediums, CredentialManager& credential_manager) { + using SingleThreadExecutor = ::location::nearby::SingleThreadExecutor; + using Mutex = ::location::nearby::Mutex; + using MutexLock = ::location::nearby::MutexLock; + using ScanningSession = + ::location::nearby::api::ble_v2::BleMedium::ScanningSession; + using Runnable = ::location::nearby::Runnable; + using BleAdvertisementData = + ::location::nearby::api::ble_v2::BleAdvertisementData; + + ScanManager(Mediums& mediums, CredentialManager& credential_manager, + SingleThreadExecutor& executor) { mediums_ = &mediums, credential_manager_ = &credential_manager; + executor_ = &executor; } ~ScanManager() = default; - std::unique_ptr StartScan(ScanRequest scan_request, - ScanCallback cb) - ABSL_LOCKS_EXCLUDED(mutex_); + ScanSessionId StartScan(ScanRequest scan_request, ScanCallback cb); + void StopScan(ScanSessionId session_id); // Below functions are test only. // Reference: go/totw/135#augmenting-the-public-api-for-tests - int ScanningCallbacksLengthForTest() ABSL_LOCKS_EXCLUDED(mutex_) { - absl::MutexLock lock(&mutex_); - return scanning_callbacks_.size(); - } + int ScanningCallbacksLengthForTest(); private: - struct MapElement { + struct ScanSessionState { ScanRequest request; ScanCallback callback; AdvertisementDecoder decoder; + std::unique_ptr scanning_session; }; - void AddScanCallback(uint64_t id, MapElement element) { - absl::MutexLock lock(&mutex_); - scanning_callbacks_.insert({id, element}); + void NotifyFoundBle(ScanSessionId id, BleAdvertisementData data, + absl::string_view remote_address) + ABSL_EXCLUSIVE_LOCKS_REQUIRED(*executor_); + void RunOnServiceControllerThread(absl::string_view name, Runnable runnable) { + executor_->Execute(std::string(name), std::move(runnable)); } - mutable absl::Mutex mutex_; Mediums* mediums_; CredentialManager* credential_manager_; - absl::flat_hash_map scanning_callbacks_ - ABSL_GUARDED_BY(mutex_); - void NotifyFoundBle( - location::nearby::api::ble_v2::BleAdvertisementData data, - const location::nearby::api::ble_v2::BlePeripheral& peripheral); + absl::flat_hash_map scan_sessions_ + ABSL_GUARDED_BY(*executor_); + SingleThreadExecutor* executor_; + absl::BitGen bit_gen_; }; } // namespace presence diff --git a/presence/implementation/scan_manager_test.cc b/presence/implementation/scan_manager_test.cc index 1c30f8ca..16b60f4c 100644 --- a/presence/implementation/scan_manager_test.cc +++ b/presence/implementation/scan_manager_test.cc @@ -31,6 +31,7 @@ #include "internal/platform/count_down_latch.h" #include "internal/platform/logging.h" #include "internal/platform/medium_environment.h" +#include "internal/platform/single_thread_executor.h" #include "presence/implementation/advertisement_factory.h" #include "presence/implementation/base_broadcast_request.h" #include "presence/implementation/credential_manager_impl.h" @@ -46,6 +47,7 @@ using AdvertisingSession = ::location::nearby::api::ble_v2::BleMedium::AdvertisingSession; using AdvertisingCallback = ::location::nearby::api::ble_v2::BleMedium::AdvertisingCallback; +using ::location::nearby::SingleThreadExecutor; using CountDownLatch = ::location::nearby::CountDownLatch; @@ -117,11 +119,12 @@ class ScanManagerTest : public testing::Test { location::nearby::MediumEnvironment::Instance()}; CountDownLatch start_latch_{1}; CountDownLatch found_latch_{1}; + SingleThreadExecutor executor_; }; TEST_F(ScanManagerTest, CanStartThenStopScanning) { Mediums mediums; - ScanManager manager(mediums, credential_manager_); + ScanManager manager(mediums, credential_manager_, executor_); // Set up advertiser location::nearby::BluetoothAdapter server_adapter; Ble ble2(server_adapter); @@ -129,20 +132,20 @@ TEST_F(ScanManagerTest, CanStartThenStopScanning) { StartAdvertisingOn(ble2); // Start scanning - auto scan_session = + ScanSessionId scan_session = manager.StartScan(MakeDefaultScanRequest(), MakeDefaultScanCallback()); EXPECT_EQ(manager.ScanningCallbacksLengthForTest(), 1); EXPECT_TRUE(start_latch_.Await().Ok()); EXPECT_TRUE(found_latch_.Await().Ok()); - EXPECT_TRUE(scan_session->StopScan().Ok()); + manager.StopScan(scan_session); EXPECT_EQ(manager.ScanningCallbacksLengthForTest(), 0); } TEST_F(ScanManagerTest, CannotStopScanTwice) { Mediums mediums; - ScanManager manager(mediums, credential_manager_); + ScanManager manager(mediums, credential_manager_, executor_); - auto scan_session = + ScanSessionId scan_session = manager.StartScan(MakeDefaultScanRequest(), MakeDefaultScanCallback()); NEARBY_LOGS(INFO) << "Start scan"; @@ -150,14 +153,16 @@ TEST_F(ScanManagerTest, CannotStopScanTwice) { // Ensure that we have started scanning before we try to stop. env_.Sync(); NEARBY_LOGS(INFO) << "Stop scan"; - EXPECT_TRUE(scan_session->StopScan().Ok()); + manager.StopScan(scan_session); + EXPECT_EQ(manager.ScanningCallbacksLengthForTest(), 0); NEARBY_LOGS(INFO) << "Stop scan again"; - EXPECT_FALSE(scan_session->StopScan().Ok()); + manager.StopScan(scan_session); + EXPECT_EQ(manager.ScanningCallbacksLengthForTest(), 0); } TEST_F(ScanManagerTest, TestNoFilter) { Mediums mediums; - ScanManager manager(mediums, credential_manager_); + ScanManager manager(mediums, credential_manager_, executor_); // Set up advertiser location::nearby::BluetoothAdapter server_adapter; Ble ble2(server_adapter); @@ -167,20 +172,20 @@ TEST_F(ScanManagerTest, TestNoFilter) { // Start scanning ScanRequest scan_request_no_filter = MakeDefaultScanRequest(); scan_request_no_filter.scan_filters.clear(); - auto scan_session = + ScanSessionId scan_session = manager.StartScan(scan_request_no_filter, MakeDefaultScanCallback()); ASSERT_EQ(manager.ScanningCallbacksLengthForTest(), 1); ASSERT_TRUE(mediums.GetBle().IsAvailable()); EXPECT_TRUE(start_latch_.Await().Ok()); EXPECT_TRUE(found_latch_.Await().Ok()); - EXPECT_TRUE(scan_session->StopScan().Ok()); + manager.StopScan(scan_session); EXPECT_EQ(manager.ScanningCallbacksLengthForTest(), 0); } TEST_F(ScanManagerTest, PresenceDeviceMetadataIsRetained) { Mediums mediums; - ScanManager manager(mediums, credential_manager_); + ScanManager manager(mediums, credential_manager_, executor_); // Set up advertiser location::nearby::BluetoothAdapter server_adapter; Ble ble2(server_adapter); @@ -204,20 +209,19 @@ TEST_F(ScanManagerTest, PresenceDeviceMetadataIsRetained) { // Start scanning ScanRequest scan_request_no_filter = MakeDefaultScanRequest(); scan_request_no_filter.scan_filters.clear(); - auto scan_session = - manager.StartScan(scan_request_no_filter, callback); + auto scan_session = manager.StartScan(scan_request_no_filter, callback); ASSERT_EQ(manager.ScanningCallbacksLengthForTest(), 1); ASSERT_TRUE(mediums.GetBle().IsAvailable()); EXPECT_TRUE(start_latch_.Await().Ok()); - EXPECT_TRUE(found_latch_.Await(absl::Milliseconds(1000)).result()); - EXPECT_TRUE(scan_session->StopScan().Ok()); + EXPECT_TRUE(found_latch_.Await().Ok()); + manager.StopScan(scan_session); EXPECT_EQ(manager.ScanningCallbacksLengthForTest(), 0); } TEST_F(ScanManagerTest, StopOneSessionFromAnotherDeadlock) { Mediums mediums; - ScanManager manager(mediums, credential_manager_); + ScanManager manager(mediums, credential_manager_, executor_); CountDownLatch start_latch2{1}; CountDownLatch found_latch2{1}; @@ -240,23 +244,23 @@ TEST_F(ScanManagerTest, StopOneSessionFromAnotherDeadlock) { }; // we use scan_request_mismatch so this session's discovery doesn't get // triggered. - auto scan_session = + ScanSessionId scan_session = manager.StartScan(scan_request_mismatch, MakeDefaultScanCallback()); - ScanCallback scanning_callback2 = { - .start_scan_cb = - [&start_latch2](Status status) { - if (status.Ok()) { - start_latch2.CountDown(); - } - }, - .on_discovered_cb = - [&found_latch2, &scan_session](PresenceDevice pd) { - NEARBY_LOGS(INFO) << "scansession2 found"; - found_latch2.CountDown(); - scan_session->StopScan(); - }}; - auto scan_session2 = manager.StartScan(MakeDefaultScanRequest(), - std::move(scanning_callback2)); + ScanCallback scanning_callback2 = {.start_scan_cb = + [&](Status status) { + if (status.Ok()) { + start_latch2.CountDown(); + } + }, + .on_discovered_cb = + [&](PresenceDevice pd) { + NEARBY_LOGS(INFO) + << "scansession2 found"; + found_latch2.CountDown(); + manager.StopScan(scan_session); + }}; + ScanSessionId scan_session2 = manager.StartScan( + MakeDefaultScanRequest(), std::move(scanning_callback2)); ASSERT_EQ(manager.ScanningCallbacksLengthForTest(), 2); @@ -266,56 +270,15 @@ TEST_F(ScanManagerTest, StopOneSessionFromAnotherDeadlock) { std::unique_ptr advertising_session = StartAdvertisingOn(ble2); - EXPECT_TRUE(found_latch2.Await(absl::Milliseconds(1500)).result()); - EXPECT_FALSE(found_latch_.Await(absl::Milliseconds(1500)).result()); + EXPECT_TRUE(found_latch2.Await().Ok()); // Session was stopped before, this should not be able to stop successfully. - EXPECT_FALSE(scan_session->StopScan().Ok()); + manager.StopScan(scan_session); EXPECT_EQ(manager.ScanningCallbacksLengthForTest(), 1); - EXPECT_TRUE(scan_session2->StopScan().Ok()); + ASSERT_TRUE(mediums.GetBle().IsAvailable()); + manager.StopScan(scan_session2); EXPECT_EQ(manager.ScanningCallbacksLengthForTest(), 0); } -TEST_F(ScanManagerTest, StopWhenScopeEnds) { - Mediums mediums; - ScanManager manager(mediums, credential_manager_); - ScanCallback scanning_callback = ScanCallback{ - .start_scan_cb = - [this](Status status) { - if (status.Ok()) { - start_latch_.CountDown(); - } - }, - }; - { - auto scan_session = manager.StartScan(MakeDefaultScanRequest(), - std::move(scanning_callback)); - EXPECT_EQ(manager.ScanningCallbacksLengthForTest(), 1); - // Ensure that we start scanning before we go out of scope. - env_.Sync(); - } - EXPECT_EQ(manager.ScanningCallbacksLengthForTest(), 0); -} - -TEST_F(ScanManagerTest, MoveDoesNotTriggerDestructor) { - Mediums mediums; - ScanManager manager(mediums, credential_manager_); - ScanCallback scanning_callback = ScanCallback{ - .start_scan_cb = - [this](Status status) { - if (status.Ok()) { - start_latch_.CountDown(); - } - }, - }; - auto scan_session = - manager.StartScan(MakeDefaultScanRequest(), std::move(scanning_callback)); - EXPECT_EQ(manager.ScanningCallbacksLengthForTest(), 1); - env_.Sync(); - auto scan_session_moved = std::move(scan_session); - // Make sure we don't trigger the destructor. - EXPECT_EQ(manager.ScanningCallbacksLengthForTest(), 1); -} - } // namespace } // namespace presence } // namespace nearby diff --git a/presence/implementation/service_controller.h b/presence/implementation/service_controller.h index 7ecc07d3..0426115d 100644 --- a/presence/implementation/service_controller.h +++ b/presence/implementation/service_controller.h @@ -33,8 +33,9 @@ class ServiceController { public: ServiceController() = default; virtual ~ServiceController() = default; - virtual std::unique_ptr StartScan(ScanRequest scan_request, - ScanCallback callback) = 0; + virtual absl::StatusOr StartScan(ScanRequest scan_request, + ScanCallback callback) = 0; + virtual void StopScan(ScanSessionId session_id) = 0; virtual absl::StatusOr StartBroadcast( BroadcastRequest broadcast_request, BroadcastCallback callback) = 0; virtual void StopBroadcast(BroadcastSessionId session_id) = 0; diff --git a/presence/implementation/service_controller_impl.cc b/presence/implementation/service_controller_impl.cc index 86067ec6..9683cf4a 100644 --- a/presence/implementation/service_controller_impl.cc +++ b/presence/implementation/service_controller_impl.cc @@ -46,10 +46,13 @@ Status ConvertBleStatus(BleOperationStatus status) { } // namespace -std::unique_ptr ServiceControllerImpl::StartScan( +absl::StatusOr ServiceControllerImpl::StartScan( ScanRequest scan_request, ScanCallback callback) { return scan_manager_.StartScan(scan_request, callback); } +void ServiceControllerImpl::StopScan(ScanSessionId id) { + scan_manager_.StopScan(id); +} absl::StatusOr ServiceControllerImpl::StartBroadcast( BroadcastRequest broadcast_request, BroadcastCallback callback) { diff --git a/presence/implementation/service_controller_impl.h b/presence/implementation/service_controller_impl.h index 51c4ff4e..8ec26ebd 100644 --- a/presence/implementation/service_controller_impl.h +++ b/presence/implementation/service_controller_impl.h @@ -49,8 +49,9 @@ class ServiceControllerImpl : public ServiceController { ServiceControllerImpl() = default; ~ServiceControllerImpl() override { executor_.Shutdown(); } - std::unique_ptr StartScan(ScanRequest scan_request, - ScanCallback callback) override; + absl::StatusOr StartScan(ScanRequest scan_request, + ScanCallback callback) override; + void StopScan(ScanSessionId session_id) override; absl::StatusOr StartBroadcast( BroadcastRequest broadcast_request, BroadcastCallback callback) override; void StopBroadcast(BroadcastSessionId) override; @@ -85,8 +86,8 @@ class ServiceControllerImpl : public ServiceController { Mediums mediums_; // NOLINT: further impl will use it. CredentialManagerImpl credential_manager_; // NOLINT: further impl will use it. - ScanManager scan_manager_{ - mediums_, credential_manager_}; // NOLINT: further impl will use it. + ScanManager scan_manager_{mediums_, credential_manager_, + executor_}; // NOLINT: further impl will use it. absl::flat_hash_map sessions_ ABSL_GUARDED_BY(executor_); absl::BitGen bit_gen_; diff --git a/presence/presence_client.cc b/presence/presence_client.cc index 13859af7..37b10c15 100644 --- a/presence/presence_client.cc +++ b/presence/presence_client.cc @@ -26,14 +26,23 @@ namespace nearby { namespace presence { -std::unique_ptr PresenceClient::StartScan(ScanRequest scan_request, - ScanCallback callback) { +absl::StatusOr PresenceClient::StartScan( + ScanRequest scan_request, ScanCallback callback) { ::location::nearby::Borrowed borrowed = service_.Borrow(); if (!borrowed) { - return nullptr; + return absl::FailedPreconditionError( + "Can't start scan, presence service is gone"); } return (*borrowed)->StartScan(scan_request, callback); } + +void PresenceClient::StopScan(ScanSessionId id) { + ::location::nearby::Borrowed borrowed = service_.Borrow(); + if (borrowed) { + (*borrowed)->StopScan(id); + } +} + absl::StatusOr PresenceClient::StartBroadcast( BroadcastRequest broadcast_request, BroadcastCallback callback) { ::location::nearby::Borrowed borrowed = service_.Borrow(); diff --git a/presence/presence_client.h b/presence/presence_client.h index f07443d7..e33e7579 100644 --- a/presence/presence_client.h +++ b/presence/presence_client.h @@ -44,21 +44,23 @@ class PresenceClient { PresenceClient(PresenceClient&&) = default; PresenceClient& operator=(const PresenceClient&) = delete; - /** - * Starts a Nearby Presence scan and registers {@link ScanCallback} - * which will be invoked when a matching {@link PresenceDevice} is detected, - * lost, and status changed. - * - *

The {@link ScanCallback} is kept at the Nearby Presence service. - * Returning unique_ptr of ScanSession including stop scan callback - * for client to invoke later. - * - *

The {@link ScanRequest} contains the options like scan power mode - * and type; the filters including credentials, actions and extended - * properties. - */ - std::unique_ptr StartScan(ScanRequest scan_request, - ScanCallback callback); + // Starts a Nearby Presence scan and registers `ScanCallback` + // which will be invoked when a matching `PresenceDevice` is detected, + // lost, and status changed. + // The session can be terminated with `StopScan()`. + // + // `ScanCallback` is kept in the Nearby Presence service until `StopScan()` is + // called. + // + // `ScanRequest` contains the options like scan power mode + // and type; the filters including credentials, actions and extended + // properties. + absl::StatusOr StartScan(ScanRequest scan_request, + ScanCallback callback); + + // Terminates the scan session. Does nothing if the session is already + // terminated. + void StopScan(ScanSessionId session_id); // Starts a Nearby Presence broadcast and registers `BroadcastCallback` // which will be invoked after broadcast is started. diff --git a/presence/presence_client_test.cc b/presence/presence_client_test.cc index a95a6c42..a315cb4b 100644 --- a/presence/presence_client_test.cc +++ b/presence/presence_client_test.cc @@ -73,16 +73,17 @@ TEST_F(PresenceClientTest, StartBroadcastFailsWhenPresenceServiceIsGone) { TEST_F(PresenceClientTest, StartScanWithDefaultConstructor) { env_.Start(); - Status scan_result = {Status::Value::kError}; + ::location::nearby::Future scan_result; ScanCallback scan_callback = { - .start_scan_cb = [&](Status status) { scan_result = status; }, + .start_scan_cb = [&](Status status) { scan_result.Set(status); }, }; PresenceService presence_service; PresenceClient presence_client = presence_service.CreatePresenceClient(); - presence_client.StartScan({}, scan_callback); + EXPECT_OK(presence_client.StartScan({}, scan_callback)); - EXPECT_TRUE(scan_result.Ok()); + EXPECT_TRUE(scan_result.Get().ok()); + EXPECT_TRUE(scan_result.Get().GetResult().Ok()); env_.Stop(); } @@ -93,8 +94,10 @@ TEST_F(PresenceClientTest, StartScanFailsWhenPresenceServiceIsGone) { .start_scan_cb = [&](Status status) { scan_result = status; }, }; - CreateDefunctPresenceClient().StartScan({}, scan_callback); + absl::StatusOr session_id = + CreateDefunctPresenceClient().StartScan({}, scan_callback); + EXPECT_THAT(session_id, StatusIs(absl::StatusCode::kFailedPrecondition)); EXPECT_FALSE(scan_result.Ok()); env_.Stop(); } diff --git a/presence/presence_service.cc b/presence/presence_service.cc index 82e13c88..7226bee0 100644 --- a/presence/presence_service.cc +++ b/presence/presence_service.cc @@ -30,10 +30,13 @@ PresenceClient PresenceService::CreatePresenceClient() { return PresenceClient(lender_.GetBorrowable()); } -std::unique_ptr PresenceService::StartScan( +absl::StatusOr PresenceService::StartScan( ScanRequest scan_request, ScanCallback callback) { return service_controller_->StartScan(scan_request, callback); } +void PresenceService::StopScan(ScanSessionId id) { + service_controller_->StopScan(id); +} absl::StatusOr PresenceService::StartBroadcast( BroadcastRequest broadcast_request, BroadcastCallback callback) { diff --git a/presence/presence_service.h b/presence/presence_service.h index 71e4ce20..c4ba1b5e 100644 --- a/presence/presence_service.h +++ b/presence/presence_service.h @@ -37,8 +37,10 @@ class PresenceService { PresenceClient CreatePresenceClient(); - std::unique_ptr StartScan(ScanRequest scan_request, - ScanCallback callback); + absl::StatusOr StartScan(ScanRequest scan_request, + ScanCallback callback); + void StopScan(ScanSessionId session_id); + absl::StatusOr StartBroadcast( BroadcastRequest broadcast_request, BroadcastCallback callback); diff --git a/presence/presence_service_test.cc b/presence/presence_service_test.cc index 0b6e213a..182e6436 100644 --- a/presence/presence_service_test.cc +++ b/presence/presence_service_test.cc @@ -14,6 +14,8 @@ #include "presence/presence_service.h" +#include "gmock/gmock.h" +#include "protobuf-matchers/protocol-buffer-matchers.h" #include "gtest/gtest.h" #include "internal/platform/medium_environment.h" #include "presence/presence_client.h" @@ -42,22 +44,19 @@ TEST_F(PresenceServiceTest, StartThenStopScan) { PresenceService presence_service; PresenceClient client = presence_service.CreatePresenceClient(); - auto scan_session = client.StartScan( + absl::StatusOr scan_session = client.StartScan( {}, { .start_scan_cb = [&](Status status) { scan_result = status; }, }); - auto scan_session_with_default_params = + absl::StatusOr scan_session_with_default_params = client.StartScan(ScanRequest(), ScanCallback()); - EXPECT_NE(scan_session, nullptr); - EXPECT_NE(scan_session_with_default_params, nullptr); + ASSERT_OK(scan_session); + ASSERT_OK(scan_session_with_default_params); + EXPECT_NE(*scan_session, *scan_session_with_default_params); - Status stop_scan_session_status = scan_session->StopScan(); - Status scan_session_with_default_params_status = - scan_session_with_default_params->StopScan(); - - EXPECT_TRUE(stop_scan_session_status.Ok()); - EXPECT_TRUE(scan_session_with_default_params_status.Ok()); + client.StopScan(*scan_session); + client.StopScan(*scan_session_with_default_params); env_.Stop(); }