diff --git a/presence/BUILD b/presence/BUILD index 2569f8c7..ba1eb44d 100644 --- a/presence/BUILD +++ b/presence/BUILD @@ -29,7 +29,8 @@ cc_library( ":types", "//internal/platform:types", "//presence/implementation:internal", # build_cleaner: keep - "@com_google_absl//absl/log:die_if_null", + "@com_google_absl//absl/status", + "@com_google_absl//absl/status:statusor", ], ) diff --git a/presence/data_types.h b/presence/data_types.h index 3ebf9315..e90c17e2 100644 --- a/presence/data_types.h +++ b/presence/data_types.h @@ -76,17 +76,8 @@ struct ScanCallback { std::function on_lost_cb = [](PresenceDevice) {}; }; -/** - * Holds the callback of stop broadcast for client to invoke later. - */ -struct BroadcastSession { - // Nearby library would provide the implementation of this callback in - // runtime. Assiging with a default value NotImplemented to surface potential - // issue where library failed to provide the implementation. - absl::AnyInvocable stop_broadcast_callback = []() { - return Status{Status::Value::kNotImplemented}; - }; -}; +// Unique Broadcast Session Identifier. +using BroadcastSessionId = 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 b43c10d5..0c26cfd6 100644 --- a/presence/implementation/mock_service_controller.h +++ b/presence/implementation/mock_service_controller.h @@ -33,7 +33,7 @@ class MockServiceController : public ServiceController { MOCK_METHOD(std::unique_ptr, StartScan, (ScanRequest scan_request, ScanCallback callback), (override)); - MOCK_METHOD(std::unique_ptr, StartBroadcast, + MOCK_METHOD(absl::StatusOr, StartBroadcast, (BroadcastRequest broadcast_request, BroadcastCallback callback), (override)); diff --git a/presence/implementation/service_controller.h b/presence/implementation/service_controller.h index 5e239f32..7ecc07d3 100644 --- a/presence/implementation/service_controller.h +++ b/presence/implementation/service_controller.h @@ -35,8 +35,9 @@ class ServiceController { virtual ~ServiceController() = default; virtual std::unique_ptr StartScan(ScanRequest scan_request, ScanCallback callback) = 0; - virtual std::unique_ptr StartBroadcast( + virtual absl::StatusOr StartBroadcast( BroadcastRequest broadcast_request, BroadcastCallback callback) = 0; + virtual void StopBroadcast(BroadcastSessionId session_id) = 0; }; } // namespace presence diff --git a/presence/implementation/service_controller_impl.cc b/presence/implementation/service_controller_impl.cc index ec14f99c..4d8c84ca 100644 --- a/presence/implementation/service_controller_impl.cc +++ b/presence/implementation/service_controller_impl.cc @@ -18,8 +18,11 @@ #include #include +#include "absl/random/random.h" +#include "absl/status/status.h" #include "absl/status/statusor.h" #include "internal/platform/implementation/ble_v2.h" +#include "presence/data_types.h" #include "presence/implementation/advertisement_factory.h" #include "presence/implementation/base_broadcast_request.h" #include "presence/implementation/mediums/advertisement_data.h" @@ -48,7 +51,7 @@ std::unique_ptr ServiceControllerImpl::StartScan( return scan_manager_.StartScan(scan_request, callback); } -std::unique_ptr ServiceControllerImpl::StartBroadcast( +absl::StatusOr ServiceControllerImpl::StartBroadcast( BroadcastRequest broadcast_request, BroadcastCallback callback) { absl::StatusOr request = BaseBroadcastRequest::Create(broadcast_request); @@ -56,7 +59,7 @@ std::unique_ptr ServiceControllerImpl::StartBroadcast( NEARBY_LOGS(WARNING) << "Invalid broadcast request, reason: " << request.status(); callback.start_broadcast_cb(Status{Status::Value::kError}); - return nullptr; + return request.status(); } absl::StatusOr advertisement = AdvertisementFactory(&credential_manager_).CreateAdvertisement(*request); @@ -64,7 +67,7 @@ std::unique_ptr ServiceControllerImpl::StartBroadcast( NEARBY_LOGS(WARNING) << "Can't create advertisement, reason: " << advertisement.status(); callback.start_broadcast_cb(Status{Status::Value::kError}); - return nullptr; + return advertisement.status(); } std::unique_ptr session = mediums_.GetBle().StartAdvertising( @@ -75,15 +78,29 @@ std::unique_ptr ServiceControllerImpl::StartBroadcast( ConvertBleStatus(status)); }}); if (!session) { - NEARBY_LOGS(WARNING) << "Failed to start broadcasting"; callback.start_broadcast_cb(Status{Status::Value::kError}); - return nullptr; + return absl::UnavailableError("Failed to start broadcasting"); } - return std::make_unique(BroadcastSession{ - .stop_broadcast_callback = [session = std::move(session)]() { - return ConvertBleStatus(session->stop_advertising()); - }}); + BroadcastSessionId id = GenerateBroadcastSessionId(); + sessions_.insert({id, Session{.advertising_session = std::move(session)}}); + + return id; +} + +void ServiceControllerImpl::StopBroadcast(BroadcastSessionId id) { + auto it = sessions_.find(id); + if (it != sessions_.end()) { + it->second.advertising_session->stop_advertising(); + sessions_.erase(it); + } else { + NEARBY_LOGS(WARNING) << absl::StrFormat( + "BroadcastSessionId(0x%x) not found", id); + } +} + +BroadcastSessionId ServiceControllerImpl::GenerateBroadcastSessionId() { + return absl::Uniform(bit_gen_); } } // namespace presence diff --git a/presence/implementation/service_controller_impl.h b/presence/implementation/service_controller_impl.h index 19039160..908cbb02 100644 --- a/presence/implementation/service_controller_impl.h +++ b/presence/implementation/service_controller_impl.h @@ -17,6 +17,7 @@ #include +#include "absl/random/random.h" #include "presence/broadcast_request.h" #include "presence/data_types.h" #include "presence/implementation/credential_manager_impl.h" @@ -33,21 +34,31 @@ namespace presence { class ServiceControllerImpl : public ServiceController { public: + using AdvertisingSession = + location::nearby::api::ble_v2::BleMedium::AdvertisingSession; ServiceControllerImpl() = default; std::unique_ptr StartScan(ScanRequest scan_request, ScanCallback callback) override; - std::unique_ptr StartBroadcast( + absl::StatusOr StartBroadcast( BroadcastRequest broadcast_request, BroadcastCallback callback) override; + void StopBroadcast(BroadcastSessionId) override; // Gives tests access to mediums. Mediums& GetMediums() { return mediums_; } private: + struct Session { + std::unique_ptr advertising_session; + }; + + BroadcastSessionId GenerateBroadcastSessionId(); 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. + absl::flat_hash_map sessions_; + absl::BitGen bit_gen_; }; } // namespace presence diff --git a/presence/implementation/service_controller_impl_test.cc b/presence/implementation/service_controller_impl_test.cc index bf40050d..d9e65206 100644 --- a/presence/implementation/service_controller_impl_test.cc +++ b/presence/implementation/service_controller_impl_test.cc @@ -32,6 +32,7 @@ namespace { using FeatureFlags = ::location::nearby::FeatureFlags::Flags; using internal::IdentityType; using ::location::nearby::MediumEnvironment; +using ::testing::status::StatusIs; constexpr FeatureFlags kTestCases[] = { FeatureFlags{}, @@ -76,12 +77,12 @@ INSTANTIATE_TEST_SUITE_P(ParametrisedServiceControllerImplTest, testing::ValuesIn(kTestCases)); TEST_P(ServiceControllerImplTest, StartBroadcastPublicIdentity) { - std::unique_ptr session = + absl::StatusOr session = service_controller_.StartBroadcast( CreateBroadcastRequest(internal::IDENTITY_TYPE_PUBLIC), broadcast_callback_); - EXPECT_TRUE(session); + EXPECT_OK(session); EXPECT_TRUE(start_broadcast_status_.Get().ok()); EXPECT_EQ(start_broadcast_status_.Get().GetResult(), Status{Status::Value::kSuccess}); @@ -92,14 +93,13 @@ TEST_P(ServiceControllerImplTest, StartBroadcastPublicIdentity) { } TEST_P(ServiceControllerImplTest, StartAndStopBroadcast) { - std::unique_ptr session = + absl::StatusOr session = service_controller_.StartBroadcast( CreateBroadcastRequest(internal::IDENTITY_TYPE_PUBLIC), broadcast_callback_); - ASSERT_TRUE(session); - EXPECT_EQ(session->stop_broadcast_callback(), - Status{Status::Value::kSuccess}); + ASSERT_OK(session); + service_controller_.StopBroadcast(*session); MediumEnvironment::Instance().Sync(); EXPECT_FALSE(MediumEnvironment::Instance() .GetBleV2MediumStatus( @@ -107,12 +107,27 @@ TEST_P(ServiceControllerImplTest, StartAndStopBroadcast) { ->is_advertising); } +TEST_P(ServiceControllerImplTest, StopBroadcastTwiceNoSideEffects) { + absl::StatusOr session = + service_controller_.StartBroadcast( + CreateBroadcastRequest(internal::IDENTITY_TYPE_PUBLIC), + broadcast_callback_); + ASSERT_OK(session); + service_controller_.StopBroadcast(*session); + + service_controller_.StopBroadcast(*session); +} + +TEST_P(ServiceControllerImplTest, StopBroadcastInvalidSessionNoSideEffects) { + service_controller_.StopBroadcast(123456); +} + TEST_P(ServiceControllerImplTest, StartBroadcastInvalidRequestFails) { - std::unique_ptr session = + absl::StatusOr session = service_controller_.StartBroadcast(BroadcastRequest{}, broadcast_callback_); - EXPECT_FALSE(session); + EXPECT_THAT(session, StatusIs(absl::StatusCode::kInvalidArgument)); EXPECT_TRUE(start_broadcast_status_.Get().ok()); EXPECT_EQ(start_broadcast_status_.Get().GetResult(), Status{Status::Value::kError}); @@ -120,12 +135,12 @@ TEST_P(ServiceControllerImplTest, StartBroadcastInvalidRequestFails) { TEST_P(ServiceControllerImplTest, StartBroadcastPrivateIdentityFails) { // TODO(b/256249404): Support private identity. - std::unique_ptr session = + absl::StatusOr session = service_controller_.StartBroadcast( CreateBroadcastRequest(internal::IDENTITY_TYPE_PRIVATE), broadcast_callback_); - EXPECT_FALSE(session); + EXPECT_THAT(session, StatusIs(absl::StatusCode::kUnavailable)); EXPECT_TRUE(start_broadcast_status_.Get().ok()); EXPECT_EQ(start_broadcast_status_.Get().GetResult(), Status{Status::Value::kError}); diff --git a/presence/presence_client.cc b/presence/presence_client.cc index fac81e60..13859af7 100644 --- a/presence/presence_client.cc +++ b/presence/presence_client.cc @@ -15,9 +15,12 @@ #include "presence/presence_client.h" #include +#include #include +#include "absl/status/status.h" #include "internal/platform/borrowable.h" +#include "internal/platform/logging.h" #include "presence/presence_service.h" namespace nearby { @@ -31,14 +34,24 @@ std::unique_ptr PresenceClient::StartScan(ScanRequest scan_request, } return (*borrowed)->StartScan(scan_request, callback); } -std::unique_ptr PresenceClient::StartBroadcast( +absl::StatusOr PresenceClient::StartBroadcast( BroadcastRequest broadcast_request, BroadcastCallback callback) { ::location::nearby::Borrowed borrowed = service_.Borrow(); if (!borrowed) { - return nullptr; + return absl::FailedPreconditionError( + "Can't start broadcast, presence service is gone"); } return (*borrowed)->StartBroadcast(broadcast_request, callback); } +void PresenceClient::StopBroadcast(BroadcastSessionId session_id) { + ::location::nearby::Borrowed borrowed = service_.Borrow(); + if (borrowed) { + (*borrowed)->StopBroadcast(session_id); + } else { + NEARBY_LOGS(VERBOSE) << "Session already finished, id: " << session_id; + } +} + } // namespace presence } // namespace nearby diff --git a/presence/presence_client.h b/presence/presence_client.h index db104692..f07443d7 100644 --- a/presence/presence_client.h +++ b/presence/presence_client.h @@ -19,6 +19,7 @@ #include #include +#include "absl/status/statusor.h" #include "internal/platform/borrowable.h" #include "presence/broadcast_request.h" #include "presence/data_types.h" @@ -59,21 +60,23 @@ class PresenceClient { std::unique_ptr StartScan(ScanRequest scan_request, ScanCallback callback); - /** - * Starts a Nearby Presence broadcast and registers {@link BroadcastCallback} - * which will be invoked after broadcast is started. - * - *

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

The {@link BroadcastRequest} contains the options like tx_power, - * the credential info like salt and private credential, the actions and - * extended properties. - */ - std::unique_ptr StartBroadcast( + // Starts a Nearby Presence broadcast and registers `BroadcastCallback` + // which will be invoked after broadcast is started. + // The session can be terminated with `StopBroadcast()`. + // + // `BroadcastCallback` is kept in the Nearby Presence service until + // `StopBroadcast()` is called. + // + // `BroadcastRequest` contains the options like tx_power, + // the credential info like salt and private credential, the actions and + // extended properties. + absl::StatusOr StartBroadcast( BroadcastRequest broadcast_request, BroadcastCallback callback); + // Terminates a broadcast session. Does nothing if the session is already + // terminated. + void StopBroadcast(BroadcastSessionId session_id); + private: BorrowablePresenceService service_; }; diff --git a/presence/presence_client_test.cc b/presence/presence_client_test.cc index 4ee6b1df..a95a6c42 100644 --- a/presence/presence_client_test.cc +++ b/presence/presence_client_test.cc @@ -14,6 +14,8 @@ #include "presence/presence_client.h" +#include "gmock/gmock.h" +#include "protobuf-matchers/protocol-buffer-matchers.h" #include "gtest/gtest.h" #include "internal/platform/medium_environment.h" #include "presence/data_types.h" @@ -24,6 +26,8 @@ namespace nearby { namespace presence { namespace { +using ::testing::status::StatusIs; + // Creates a PresenceClient and destroys PresenceService that was used to create // it. PresenceClient CreateDefunctPresenceClient() { @@ -46,7 +50,7 @@ TEST_F(PresenceClientTest, StartBroadcastWithDefaultConstructor) { PresenceService presence_service; PresenceClient presence_client = presence_service.CreatePresenceClient(); - presence_client.StartBroadcast({}, broadcast_callback); + auto unused = presence_client.StartBroadcast({}, broadcast_callback); EXPECT_FALSE(broadcast_result.Ok()); env_.Stop(); @@ -59,8 +63,10 @@ TEST_F(PresenceClientTest, StartBroadcastFailsWhenPresenceServiceIsGone) { .start_broadcast_cb = [&](Status status) { broadcast_result = status; }, }; - CreateDefunctPresenceClient().StartBroadcast({}, broadcast_callback); + absl::StatusOr session_id = + CreateDefunctPresenceClient().StartBroadcast({}, broadcast_callback); + EXPECT_THAT(session_id, StatusIs(absl::StatusCode::kFailedPrecondition)); EXPECT_FALSE(broadcast_result.Ok()); env_.Stop(); } diff --git a/presence/presence_service.cc b/presence/presence_service.cc index 08f86f94..82e13c88 100644 --- a/presence/presence_service.cc +++ b/presence/presence_service.cc @@ -35,10 +35,14 @@ std::unique_ptr PresenceService::StartScan( return service_controller_->StartScan(scan_request, callback); } -std::unique_ptr PresenceService::StartBroadcast( +absl::StatusOr PresenceService::StartBroadcast( BroadcastRequest broadcast_request, BroadcastCallback callback) { return service_controller_->StartBroadcast(broadcast_request, callback); } +void PresenceService::StopBroadcast(BroadcastSessionId session) { + service_controller_->StopBroadcast(session); +} + } // namespace presence } // namespace nearby diff --git a/presence/presence_service.h b/presence/presence_service.h index 48c3341e..71e4ce20 100644 --- a/presence/presence_service.h +++ b/presence/presence_service.h @@ -39,9 +39,11 @@ class PresenceService { std::unique_ptr StartScan(ScanRequest scan_request, ScanCallback callback); - std::unique_ptr StartBroadcast( + absl::StatusOr StartBroadcast( BroadcastRequest broadcast_request, BroadcastCallback callback); + void StopBroadcast(BroadcastSessionId session_id); + private: std::unique_ptr service_controller_; ::location::nearby::Lender lender_{this};