From e64f9859e19c4d54795048f9bd105dd4d941f91d Mon Sep 17 00:00:00 2001 From: Janusz Sobczak Date: Thu, 22 Dec 2022 10:17:46 -0800 Subject: [PATCH] Replace BleOperationStatus with absl::Status PiperOrigin-RevId: 497191477 --- internal/platform/BUILD | 3 +- internal/platform/ble_v2.cc | 4 +- internal/platform/ble_v2_test.cc | 18 +---- internal/platform/implementation/ble_v2.h | 17 ++--- internal/platform/implementation/g3/ble_v2.cc | 16 ++-- .../platform/implementation/windows/ble_v2.cc | 1 - internal/platform/medium_environment.cc | 4 +- presence/implementation/broadcast_manager.cc | 21 ++---- presence/implementation/mediums/ble_test.cc | 29 ++++---- presence/implementation/scan_manager.cc | 73 ++++++++----------- presence/implementation/scan_manager_test.cc | 4 +- 11 files changed, 79 insertions(+), 111 deletions(-) diff --git a/internal/platform/BUILD b/internal/platform/BUILD index a6ea1c0d..f6ee4cac 100644 --- a/internal/platform/BUILD +++ b/internal/platform/BUILD @@ -220,11 +220,12 @@ cc_library( deps = [ ":base", ":logging", + ":types", ":uuid", - "//internal/platform:types", "//internal/platform/implementation:comm", "@com_google_absl//absl/container:flat_hash_map", "@com_google_absl//absl/container:flat_hash_set", + "@com_google_absl//absl/status", "@com_google_absl//absl/strings", "@com_google_absl//absl/types:optional", ], diff --git a/internal/platform/ble_v2.cc b/internal/platform/ble_v2.cc index 60990d10..f5e76fbb 100644 --- a/internal/platform/ble_v2.cc +++ b/internal/platform/ble_v2.cc @@ -112,10 +112,10 @@ BleV2Medium::StartScanning(const Uuid& service_uuid, service_uuid, tx_power_level, api::ble_v2::BleMedium::ScanningCallback{ .start_scanning_result = - [this, callback](api::ble_v2::BleOperationStatus status) { + [this, callback](absl::Status status) { { MutexLock lock(&mutex_); - if (status == api::ble_v2::BleOperationStatus::kSucceeded) { + if (status.ok()) { scanning_enabled_ = true; } } diff --git a/internal/platform/ble_v2_test.cc b/internal/platform/ble_v2_test.cc index 70ad2292..1a9ac811 100644 --- a/internal/platform/ble_v2_test.cc +++ b/internal/platform/ble_v2_test.cc @@ -355,11 +355,7 @@ TEST_F(BleV2MediumTest, StartThenStopAsyncScanning) { }); EXPECT_TRUE(env_.GetBleV2MediumStatus(*ble_a.GetImpl()).value().is_scanning); - api::ble_v2::BleOperationStatus stop_result = - scanning_session_a->stop_scanning(); - - EXPECT_EQ(stop_result, api::ble_v2::BleOperationStatus::kSucceeded); - + EXPECT_OK(scanning_session_a->stop_scanning()); EXPECT_FALSE(env_.GetBleV2MediumStatus(*ble_a.GetImpl()).value().is_scanning); env_.Stop(); } @@ -403,12 +399,8 @@ TEST_F(BleV2MediumTest, CanStartMultipleAsyncScanning) { EXPECT_TRUE(env_.GetBleV2MediumStatus(*ble_a.GetImpl()).value().is_scanning); EXPECT_TRUE(env_.GetBleV2MediumStatus(*ble_b.GetImpl()).value().is_scanning); - api::ble_v2::BleOperationStatus stop_result_a = - scanning_session_a->stop_scanning(); - EXPECT_EQ(stop_result_a, api::ble_v2::BleOperationStatus::kSucceeded); - api::ble_v2::BleOperationStatus stop_result_b = - scanning_session_b->stop_scanning(); - EXPECT_EQ(stop_result_b, api::ble_v2::BleOperationStatus::kSucceeded); + EXPECT_OK(scanning_session_a->stop_scanning()); + EXPECT_OK(scanning_session_b->stop_scanning()); EXPECT_FALSE(env_.GetBleV2MediumStatus(*ble_a.GetImpl()).value().is_scanning); EXPECT_FALSE(env_.GetBleV2MediumStatus(*ble_b.GetImpl()).value().is_scanning); @@ -451,9 +443,7 @@ TEST_F(BleV2MediumTest, CanStartAsyncScanningAndAdvertising) { EXPECT_TRUE( env_.GetBleV2MediumStatus(*ble_b.GetImpl()).value().is_advertising); EXPECT_TRUE(found_latch.Await(kWaitDuration).result()); - api::ble_v2::BleOperationStatus stop_scanning_result = - scanning_session->stop_scanning(); - EXPECT_EQ(stop_scanning_result, api::ble_v2::BleOperationStatus::kSucceeded); + EXPECT_OK(scanning_session->stop_scanning()); EXPECT_TRUE(ble_b.StopAdvertising()); EXPECT_FALSE(env_.GetBleV2MediumStatus(*ble_a.GetImpl()).value().is_scanning); diff --git a/internal/platform/implementation/ble_v2.h b/internal/platform/implementation/ble_v2.h index 82f250ad..cf30b192 100644 --- a/internal/platform/implementation/ble_v2.h +++ b/internal/platform/implementation/ble_v2.h @@ -25,6 +25,7 @@ #include #include "absl/container/flat_hash_map.h" +#include "absl/status/status.h" #include "absl/strings/string_view.h" #include "internal/platform/byte_array.h" #include "internal/platform/cancellation_flag.h" @@ -48,12 +49,6 @@ enum class TxPowerLevel { kHigh = 4, }; -enum class BleOperationStatus { - kUnknown = 0, - kSucceeded = 1, - kFailed = 2, -}; - // https://developer.android.com/reference/android/bluetooth/le/AdvertisingSetParameters.Builder // // The preferences for Advertising. @@ -305,11 +300,11 @@ class BleMedium { AdvertiseParameters advertise_set_parameters) = 0; struct AdvertisingCallback { - std::function start_advertising_result; + std::function start_advertising_result; }; struct AdvertisingSession { - std::function stop_advertising; + std::function stop_advertising; }; // Async interface for StartAdertising. @@ -366,12 +361,12 @@ class BleMedium { virtual bool StopScanning() = 0; struct ScanningSession { - std::function stop_scanning; + std::function stop_scanning; }; struct ScanningCallback { - std::function start_scanning_result = - DefaultCallback(); + std::function start_scanning_result = + DefaultCallback(); std::function advertisement_found_cb = diff --git a/internal/platform/implementation/g3/ble_v2.cc b/internal/platform/implementation/g3/ble_v2.cc index c3a6f7ab..6b0b8f3d 100644 --- a/internal/platform/implementation/g3/ble_v2.cc +++ b/internal/platform/implementation/g3/ble_v2.cc @@ -22,6 +22,7 @@ #include #include +#include "absl/status/status.h" #include "absl/strings/escaping.h" #include "absl/synchronization/mutex.h" #include "internal/platform/cancellation_flag_listener.h" @@ -36,7 +37,6 @@ namespace g3 { namespace { using ::location::nearby::api::ble_v2::BleAdvertisementData; -using ::location::nearby::api::ble_v2::BleOperationStatus; using ::location::nearby::api::ble_v2::TxPowerLevel; std::string TxPowerLevelToName(TxPowerLevel power_mode) { @@ -263,15 +263,16 @@ std::unique_ptr BleV2Medium::StartAdvertising( } if (callback.start_advertising_result) { - callback.start_advertising_result(BleOperationStatus::kSucceeded); + callback.start_advertising_result(absl::OkStatus()); } absl::MutexLock lock(&mutex_); MediumEnvironment::Instance().UpdateBleV2MediumForAdvertising( /*enabled=*/true, *this, adapter_->GetPeripheralV2(), advertising_data); return std::make_unique( AdvertisingSession{.stop_advertising = [this] { - return StopAdvertising() ? BleOperationStatus::kSucceeded - : BleOperationStatus::kFailed; + return StopAdvertising() + ? absl::OkStatus() + : absl::InternalError("Failed to stop advertising"); }}); } @@ -313,7 +314,7 @@ std::unique_ptr BleV2Medium::StartScanning( /*enabled=*/true, service_uuid, internal_session_id, callback, *this); scanning_internal_session_ids_.insert({service_uuid, internal_session_id}); } - callback.start_scanning_result(api::ble_v2::BleOperationStatus::kSucceeded); + callback.start_scanning_result(absl::OkStatus()); return std::make_unique(ScanningSession{ .stop_scanning = [this, service_uuid = service_uuid, @@ -323,14 +324,15 @@ std::unique_ptr BleV2Medium::StartScanning( {service_uuid, internal_session_id}) == scanning_internal_session_ids_.end()) { // can't find the provided internal session. - return BleOperationStatus::kFailed; + return absl::NotFoundError( + "can't find the provided internal session"); } MediumEnvironment::Instance().UpdateBleV2MediumForScanning( /*enabled=*/false, service_uuid, internal_session_id, /*callback=*/{}, *this); scanning_internal_session_ids_.erase( {service_uuid, internal_session_id}); - return BleOperationStatus::kSucceeded; + return absl::OkStatus(); }, }); } diff --git a/internal/platform/implementation/windows/ble_v2.cc b/internal/platform/implementation/windows/ble_v2.cc index 6abc40a8..1d52aaa5 100644 --- a/internal/platform/implementation/windows/ble_v2.cc +++ b/internal/platform/implementation/windows/ble_v2.cc @@ -33,7 +33,6 @@ namespace { using ::location::nearby::api::ble_v2::AdvertiseParameters; using ::location::nearby::api::ble_v2::BleAdvertisementData; -using ::location::nearby::api::ble_v2::BleOperationStatus; using ::location::nearby::api::ble_v2::BleServerSocket; using ::location::nearby::api::ble_v2::BleSocket; using ::location::nearby::api::ble_v2::GattClient; diff --git a/internal/platform/medium_environment.cc b/internal/platform/medium_environment.cc index 638a283f..4fff2550 100644 --- a/internal/platform/medium_environment.cc +++ b/internal/platform/medium_environment.cc @@ -25,6 +25,7 @@ #include #include "absl/container/flat_hash_set.h" +#include "absl/status/status.h" #include "internal/platform/count_down_latch.h" #include "internal/platform/feature_flags.h" #include "internal/platform/implementation/ble_v2.h" @@ -597,8 +598,7 @@ void MediumEnvironment::UpdateBleV2MediumForScanning( << ", enabled=" << enabled; if (enabled) { context.scanning = true; - callback.start_scanning_result( - api::ble_v2::BleOperationStatus::kSucceeded); + callback.start_scanning_result(absl::OkStatus()); context.scan_callback_map.insert( {{scanning_service_uuid, internal_session_id}, std::move(callback)}); absl::flat_hash_set scanning_service_uuids; diff --git a/presence/implementation/broadcast_manager.cc b/presence/implementation/broadcast_manager.cc index 7e953fbe..2915a6ee 100644 --- a/presence/implementation/broadcast_manager.cc +++ b/presence/implementation/broadcast_manager.cc @@ -25,18 +25,11 @@ namespace nearby { namespace presence { namespace { -using ::location::nearby::api::ble_v2::BleOperationStatus; using AdvertisingCallback = ::location::nearby::api::ble_v2::BleMedium::AdvertisingCallback; using AdvertisingSession = ::location::nearby::api::ble_v2::BleMedium::AdvertisingSession; -absl::Status ConvertBleStatus(BleOperationStatus status) { - return status == BleOperationStatus::kSucceeded - ? absl::OkStatus() - : absl::InternalError(absl::StrFormat("BleOperationStatus(%d)", - static_cast(status))); -} } // namespace absl::StatusOr BroadcastManager::StartBroadcast( @@ -114,11 +107,10 @@ void BroadcastManager::Advertise(BroadcastSessionId id, std::unique_ptr session = mediums_->GetBle().StartAdvertising( *advertisement, it->second.GetPowerMode(), - AdvertisingCallback{.start_advertising_result = - [this, id](BleOperationStatus status) { - NotifyStartCallbackStatus( - id, ConvertBleStatus(status)); - }}); + AdvertisingCallback{ + .start_advertising_result = [this, id](absl::Status status) { + NotifyStartCallbackStatus(id, status); + }}); if (!session) { NotifyStartCallbackStatus(id, absl::InternalError("Can't start advertising")); @@ -179,7 +171,10 @@ void BroadcastManager::BroadcastSessionState::StopAdvertising() { std::unique_ptr advertising_session = std::move(advertising_session_); if (advertising_session) { - advertising_session->stop_advertising(); + absl::Status status = advertising_session->stop_advertising(); + if (!status.ok()) { + NEARBY_LOGS(WARNING) << "StopAdvertising error: " << status; + } } } diff --git a/presence/implementation/mediums/ble_test.cc b/presence/implementation/mediums/ble_test.cc index c9d1610a..001369e2 100644 --- a/presence/implementation/mediums/ble_test.cc +++ b/presence/implementation/mediums/ble_test.cc @@ -36,7 +36,6 @@ namespace presence { namespace { using FeatureFlags = ::location::nearby::FeatureFlags::Flags; -using BleOperationStatus = ::location::nearby::api::ble_v2::BleOperationStatus; using BleV2MediumStatus = ::location::nearby::MediumEnvironment::BleV2MediumStatus; using ScanningSession = @@ -111,21 +110,20 @@ TEST_P(BleTest, CanStartThenStopScanning) { location::nearby::CountDownLatch started_scanning_latch(1); std::unique_ptr scannning_session = ble.StartScanning( - scan_request, - ScanningCallback{ - .start_scanning_result = - [&started_scanning_latch](BleOperationStatus status) { - if (status == BleOperationStatus::kSucceeded) { - started_scanning_latch.CountDown(); - } - }, - }); + scan_request, ScanningCallback{ + .start_scanning_result = + [&started_scanning_latch](absl::Status status) { + if (status.ok()) { + started_scanning_latch.CountDown(); + } + }, + }); EXPECT_TRUE(started_scanning_latch.Await(kWaitDuration).result()); EXPECT_TRUE(GetBleStatus(ble).has_value() && GetBleStatus(ble).value().is_scanning == true); - BleOperationStatus stop_scanning_status = scannning_session->stop_scanning(); - EXPECT_EQ(BleOperationStatus::kSucceeded, stop_scanning_status); + absl::Status stop_scanning_status = scannning_session->stop_scanning(); + EXPECT_OK(stop_scanning_status); EXPECT_TRUE(GetBleStatus(ble).has_value() && GetBleStatus(ble).value().is_scanning == false); env_.Stop(); @@ -159,15 +157,14 @@ TEST_P(BleTest, AdvertiseAndScan) { advertising_session = server.StartAdvertising( advert_data, PowerMode::kBalanced, AdvertisingCallback{ - .start_advertising_result = [&](BleOperationStatus status) { + .start_advertising_result = [&](absl::Status status) { advertise_latch.CountDown(); }}); EXPECT_TRUE(advertise_latch.Await(kWaitDuration).result()); EXPECT_TRUE(scan_latch.Await(kWaitDuration).result()); - EXPECT_EQ(scanning_session->stop_scanning(), BleOperationStatus::kSucceeded); - EXPECT_EQ(advertising_session->stop_advertising(), - BleOperationStatus::kSucceeded); + EXPECT_OK(scanning_session->stop_scanning()); + EXPECT_OK(advertising_session->stop_advertising()); ASSERT_FALSE(advertisements.empty()); EXPECT_EQ(advertisements[0] .service_data.find(kPresenceServiceUuid) diff --git a/presence/implementation/scan_manager.cc b/presence/implementation/scan_manager.cc index 382cef7e..6a250cfe 100644 --- a/presence/implementation/scan_manager.cc +++ b/presence/implementation/scan_manager.cc @@ -38,7 +38,6 @@ namespace { using BleAdvertisementData = ::location::nearby::api::ble_v2::BleAdvertisementData; using BlePeripheral = ::location::nearby::api::ble_v2::BlePeripheral; -using BleOperationStatus = ::location::nearby::api::ble_v2::BleOperationStatus; using ScanningSession = ::location::nearby::api::ble_v2::BleMedium::ScanningSession; using ScanningCallback = @@ -52,45 +51,34 @@ ScanSessionId ScanManager::StartScan(ScanRequest scan_request, ScanCallback cb) { ScanSessionId id = ::crypto::RandData(); 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) { - absl::Status status; - if (ble_status == BleOperationStatus::kSucceeded) { - status = absl::OkStatus(); - } else { - status = absl::InternalError( - absl::StrFormat("BleOperationStatus(%d)", - static_cast(ble_status))); - } - 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); - }); - }}; - FetchCredentials(id, scan_request); - scan_sessions_.insert( - {id, ScanSessionState{ - .request = scan_request, - .callback = std::move(scan_callback), - .decoder = AdvertisementDecoder(scan_request), - .scanning_session = mediums_->GetBle().StartScanning( - scan_request, std::move(callback))}}); - }); + "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)]( + absl::Status ble_status) { start_scan_client(ble_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); + }); + }}; + FetchCredentials(id, scan_request); + scan_sessions_.insert( + {id, ScanSessionState{ + .request = scan_request, + .callback = std::move(scan_callback), + .decoder = AdvertisementDecoder(scan_request), + .scanning_session = mediums_->GetBle().StartScanning( + scan_request, std::move(callback))}}); + }); return id; } @@ -102,7 +90,10 @@ void ScanManager::StopScan(ScanSessionId id) { return; } if (it->second.scanning_session) { - it->second.scanning_session->stop_scanning(); + absl::Status status = it->second.scanning_session->stop_scanning(); + if (!status.ok()) { + NEARBY_LOGS(WARNING) << "StopScan error: " << status; + } } scan_sessions_.erase(it); }); diff --git a/presence/implementation/scan_manager_test.cc b/presence/implementation/scan_manager_test.cc index 50d0d6a1..2c1fae59 100644 --- a/presence/implementation/scan_manager_test.cc +++ b/presence/implementation/scan_manager_test.cc @@ -41,7 +41,6 @@ namespace nearby { namespace presence { namespace { -using BleOperationStatus = ::location::nearby::api::ble_v2::BleOperationStatus; using AdvertisingSession = ::location::nearby::api::ble_v2::BleMedium::AdvertisingSession; using AdvertisingCallback = @@ -70,8 +69,7 @@ class ScanManagerTest : public testing::Test { EXPECT_OK(advertisement); std::unique_ptr session = ble.StartAdvertising( advertisement.value(), PowerMode::kLowPower, - AdvertisingCallback{ - .start_advertising_result = [](BleOperationStatus) {}}); + AdvertisingCallback{.start_advertising_result = [](absl::Status) {}}); env_.Sync(); return session; }