From faa1bea66da763c6b3657468fb05e05ce9f0dc14 Mon Sep 17 00:00:00 2001 From: Alex Deymo Date: Fri, 21 Oct 2022 11:29:03 +0000 Subject: [PATCH] pw_bluetooth: Specify the Deleter when using std::unique_ptr In several cases we use std::unique_ptr to return an object with RAII semantics, meaning that some action is needed by the backend to release the resource when the object goes out of scope. However, using using std::unique_ptr with the default deleter forces the pw_bluetooth backend to use new/delete to allocate the T interface implementation. This patch introduces a RaiiPtr template class as an alias of std::unique_ptr with a custom deleter that calls the Api::OnDestroy parametric method instead of calling "delete". This template class is used instead of the plain std::unique_ptr when returning objects from the pw_bluetooth API for the all the "Api" classes with only pure virtual methods. In all 5 cases where this pattern was used the abstract class has one more private pure virtual method (the one used as Api::OnDestroy parameter) which must be implemented by the backend. This allows a backend to manage the memory for the returned objects with something other than new/delete, for example it can return a RaiiPtr to a std::optional instance or to one of the existing pre-allocated objects and re-use them when needed. This is particularly useful in cases where there can only be a fixed small number of objects for the given type, like in the Scan or AdvertisingParameters case where there is only one object at a time. Bug: 250930246 Change-Id: I724ff5135e0f73f5fb74c7fc92abdffa6a9e2d2d Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/113210 Reviewed-by: Wyatt Hepler Commit-Queue: Alex Deymo Reviewed-by: Ben Lawson --- pw_bluetooth/BUILD.bazel | 1 + pw_bluetooth/BUILD.gn | 1 + pw_bluetooth/CMakeLists.txt | 1 + .../public/pw_bluetooth/gatt/client.h | 15 ++++- .../public/pw_bluetooth/gatt/server.h | 28 ++++++-- .../public/pw_bluetooth/internal/raii_ptr.h | 67 +++++++++++++++++++ .../public/pw_bluetooth/low_energy/central.h | 33 ++++++--- .../pw_bluetooth/low_energy/connection.h | 12 ++++ .../pw_bluetooth/low_energy/peripheral.h | 28 +++++--- 9 files changed, 158 insertions(+), 28 deletions(-) create mode 100644 pw_bluetooth/public/pw_bluetooth/internal/raii_ptr.h diff --git a/pw_bluetooth/BUILD.bazel b/pw_bluetooth/BUILD.bazel index d1c28a7140..6677e8de08 100644 --- a/pw_bluetooth/BUILD.bazel +++ b/pw_bluetooth/BUILD.bazel @@ -35,6 +35,7 @@ pw_cc_library( "public/pw_bluetooth/hci.h", "public/pw_bluetooth/host.h", "public/pw_bluetooth/internal/hex.h", + "public/pw_bluetooth/internal/raii_ptr.h", "public/pw_bluetooth/low_energy/advertising_data.h", "public/pw_bluetooth/low_energy/bond_data.h", "public/pw_bluetooth/low_energy/central.h", diff --git a/pw_bluetooth/BUILD.gn b/pw_bluetooth/BUILD.gn index 34f2264974..53d8163723 100644 --- a/pw_bluetooth/BUILD.gn +++ b/pw_bluetooth/BUILD.gn @@ -41,6 +41,7 @@ pw_source_set("pw_bluetooth") { "public/pw_bluetooth/hci.h", "public/pw_bluetooth/host.h", "public/pw_bluetooth/internal/hex.h", + "public/pw_bluetooth/internal/raii_ptr.h", "public/pw_bluetooth/low_energy/advertising_data.h", "public/pw_bluetooth/low_energy/bond_data.h", "public/pw_bluetooth/low_energy/central.h", diff --git a/pw_bluetooth/CMakeLists.txt b/pw_bluetooth/CMakeLists.txt index a19bb9e1a0..6bd9488be6 100644 --- a/pw_bluetooth/CMakeLists.txt +++ b/pw_bluetooth/CMakeLists.txt @@ -23,6 +23,7 @@ pw_add_module_library(pw_bluetooth public/pw_bluetooth/gatt/server.h public/pw_bluetooth/gatt/types.h public/pw_bluetooth/internal/hex.h + public/pw_bluetooth/internal/raii_ptr.h public/pw_bluetooth/low_energy/advertising_data.h public/pw_bluetooth/low_energy/bond_data.h public/pw_bluetooth/low_energy/central.h diff --git a/pw_bluetooth/public/pw_bluetooth/gatt/client.h b/pw_bluetooth/public/pw_bluetooth/gatt/client.h index 069b472401..a352f0ecbb 100644 --- a/pw_bluetooth/public/pw_bluetooth/gatt/client.h +++ b/pw_bluetooth/public/pw_bluetooth/gatt/client.h @@ -18,6 +18,7 @@ #include "pw_bluetooth/gatt/constants.h" #include "pw_bluetooth/gatt/error.h" #include "pw_bluetooth/gatt/types.h" +#include "pw_bluetooth/internal/raii_ptr.h" #include "pw_bluetooth/result.h" #include "pw_bluetooth/types.h" #include "pw_containers/vector.h" @@ -236,6 +237,17 @@ class RemoteService { // Stops notifications for the characteristic with the given `handle`. void StopNotifications(Handle handle); + + private: + // Disconnect from the remote service. This method is called by the + // ~RemoteService::Ptr() when it goes out of scope, the API client should + // never call this method. + void Disconnect(); + + public: + // Movable RemoteService smart pointer. The remote server will remain + // connected until the returned RemoteService::Ptr is destroyed. + using Ptr = internal::RaiiPtr; }; // Represents a GATT client that interacts with services on a GATT server. @@ -278,8 +290,7 @@ class Client { // // This may fail with the following errors: // kInvalidParameters - `handle` does not correspond to a known service. - virtual Result> ConnectToService( - Handle handle) = 0; + virtual Result ConnectToService(Handle handle) = 0; }; } // namespace pw::bluetooth::gatt diff --git a/pw_bluetooth/public/pw_bluetooth/gatt/server.h b/pw_bluetooth/public/pw_bluetooth/gatt/server.h index 3868430c35..3cf6fc0851 100644 --- a/pw_bluetooth/public/pw_bluetooth/gatt/server.h +++ b/pw_bluetooth/public/pw_bluetooth/gatt/server.h @@ -18,6 +18,7 @@ #include "pw_bluetooth/gatt/error.h" #include "pw_bluetooth/gatt/types.h" +#include "pw_bluetooth/internal/raii_ptr.h" #include "pw_bluetooth/result.h" #include "pw_bluetooth/types.h" #include "pw_containers/vector.h" @@ -55,8 +56,8 @@ class LocalServiceDelegate { // Called when there is a fatal error related to this service that forces the // service to close. LocalServiceDelegate methods will no longer be called. // This invalidates the associated LocalService. It is OK to destroy both - // LocalServiceDelegate and the associated LocalService from within this - // method. + // `LocalServiceDelegate` and the associated `LocalService::Ptr` from within + // this method. virtual void OnError(Error error) = 0; // This notifies the current configuration of a particular @@ -124,6 +125,7 @@ class LocalServiceDelegate { virtual void MtuUpdate(PeerId peer_id, uint16_t mtu) = 0; }; +// Interface provided by the backend to interact with a published service. // LocalService is valid for the lifetime of a published GATT service. It is // used to control the service and send notifications/indications. class LocalService { @@ -183,6 +185,17 @@ class LocalService { // this callback is called. virtual void IndicateValue(const ValueChangedParameters& parameters, Function)>&& confirmation) = 0; + + private: + // Unpublish the local service. This method is called by the + // ~LocalService::Ptr() when it goes out of scope, the API client should never + // call this method. + virtual void UnpublishService() = 0; + + public: + // Movable LocalService smart pointer. When the LocalService::Ptr object is + // destroyed the service will be unpublished. + using Ptr = internal::RaiiPtr; }; // Interface for a GATT server that serves many GATT services. @@ -204,6 +217,9 @@ class Server { kInvalidIncludes = 4, }; + // The Result passed by PublishService. + using PublishServiceResult = Result; + virtual ~Server() = default; // Publishes the service defined by `info` and implemented by `delegate` so @@ -211,15 +227,13 @@ class Server { // // The caller must assign distinct handles to the characteristics and // descriptors listed in `info`. These identifiers will be used in requests - // sent to `delegate`. On success, a `LocalService` is returned. When the - // `LocalService` is destroyed or an error occurs + // sent to `delegate`. On success, a `LocalService::Ptr` is returned. When the + // `LocalService::Ptr` is destroyed or an error occurs // (LocalServiceDelegate.OnError), the service will be unpublished. virtual void PublishService( const LocalServiceInfo& info, LocalServiceDelegate* delegate, - Function< - void(Result>)>&& - result_callback) = 0; + Function&& result_callback) = 0; }; } // namespace pw::bluetooth::gatt diff --git a/pw_bluetooth/public/pw_bluetooth/internal/raii_ptr.h b/pw_bluetooth/public/pw_bluetooth/internal/raii_ptr.h new file mode 100644 index 0000000000..4d61f62d44 --- /dev/null +++ b/pw_bluetooth/public/pw_bluetooth/internal/raii_ptr.h @@ -0,0 +1,67 @@ +// Copyright 2022 The Pigweed Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); you may not +// use this file except in compliance with the License. You may obtain a copy of +// the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +// License for the specific language governing permissions and limitations under +// the License. +#pragma once + +#include +#include + +namespace pw::bluetooth::internal { + +// Helper deleter struct to call the OnDestroy template parameter method when +// RaiiPtr is destroyed. +template +struct RaiiPtrDeleter { + void operator()(Api* api) { (api->*OnDestroy)(); } +}; + +// Smart pointer class to expose an API with RAII semantics that calls a +// specific method of the API implementation when it is destroyed, instead of +// calling delete on the API object like the default std::unique_ptr deleter +// behavior. This allows to be used like if it was a std::unique_ptr but +// leaving the memory management implementation details up to the backend +// implementing the API. +// +// Example usage: +// class SomeApi { +// public: +// virtual ~SomeApi() = default; +// +// virtual void MethodOne() = 0; +// virtual void MethodTwo(int) = 0; +// +// private: +// // Method used to release the resource in the backend. +// virtual void DeleteResource() = 0; +// +// public: +// using Ptr = RaiiPtr; +// }; +// +// // The concrete backend implementation. +// class BackendImplementation final : public SomeApi { +// public: +// void MethodOne() override { ... } +// void MethodTwo(int) override { ... } +// void DeleteResource() override { ... } +// }; +// +// // Example using a static global resource object. GetResource should check +// // whether the global resource is in use. +// BackendImplementation backend_impl; +// SomeApi::Ptr GetResource() { ...; return &backend_impl; } +// +template +using RaiiPtr = std::unique_ptr>; + +} // namespace pw::bluetooth::internal diff --git a/pw_bluetooth/public/pw_bluetooth/low_energy/central.h b/pw_bluetooth/public/pw_bluetooth/low_energy/central.h index 2ce553e7dd..2753feb4be 100644 --- a/pw_bluetooth/public/pw_bluetooth/low_energy/central.h +++ b/pw_bluetooth/public/pw_bluetooth/low_energy/central.h @@ -16,6 +16,7 @@ #include #include +#include "pw_bluetooth/internal/raii_ptr.h" #include "pw_bluetooth/low_energy/advertising_data.h" #include "pw_bluetooth/low_energy/connection.h" #include "pw_bluetooth/result.h" @@ -39,6 +40,16 @@ class Central { // Set a callback that will be called if the scan is stopped due to an error // in the BLE stack. virtual void SetErrorCallback(Function&& callback) = 0; + + private: + // Stop the current scan. This method is called by the ~Scan::Ptr() when it + // goes out of scope, the API client should never call this method. + virtual void StopScan() = 0; + + public: + // Movable Scan smart pointer. The controller will continue scanning until + // the returned Scan::Ptr is destroyed. + using Ptr = internal::RaiiPtr; }; // Filter parameters for use during a scan. A discovered peer only matches the @@ -195,6 +206,9 @@ class Central { kInternal, }; + // The Result type returned by Connect() via the passed callback. + using ConnectResult = Result; + virtual ~Central() = default; // Connect to the peer with the given identifier. @@ -213,16 +227,14 @@ class Central { // error occurs. // // Possible errors are documented in `ConnectError`. - virtual void Connect( - PeerId peer_id, - ConnectionOptions options, - Function>)>&& - callback) = 0; + virtual void Connect(PeerId peer_id, + ConnectionOptions options, + Function&& callback) = 0; // Scans for nearby LE peripherals and broadcasters. The lifetime of the scan // session is tied to the returned `Scan` object. Once a scan is started, // `scan_result_callback` will be called with scan results. Only 1 scan may be - // active at a time. It is OK to destroy the `Scan` object in + // active at a time. It is OK to destroy the `Scan::Ptr` object in // `scan_result_callback` to stop scanning (no more results will be returned). // // Parameters: @@ -234,11 +246,10 @@ class Central { // call. // `scan_started_callback` - Called with a `Scan` object if the // scan successfully starts, or a `ScanError` otherwise. - virtual void Scan( - ScanOptions options, - Function&& scan_result_callback, - Function>)>&& - scan_started_callback) = 0; + virtual void Scan(ScanOptions options, + Function&& scan_result_callback, + Function)>&& + scan_started_callback) = 0; }; } // namespace pw::bluetooth::low_energy diff --git a/pw_bluetooth/public/pw_bluetooth/low_energy/connection.h b/pw_bluetooth/public/pw_bluetooth/low_energy/connection.h index f9e38c12de..e7d823faa4 100644 --- a/pw_bluetooth/public/pw_bluetooth/low_energy/connection.h +++ b/pw_bluetooth/public/pw_bluetooth/low_energy/connection.h @@ -14,6 +14,7 @@ #pragma once #include "pw_bluetooth/gatt/client.h" +#include "pw_bluetooth/internal/raii_ptr.h" #include "pw_bluetooth/types.h" namespace pw::bluetooth::low_energy { @@ -156,6 +157,17 @@ class Connection { virtual void RequestConnectionParameterUpdate( RequestedConnectionParameters parameters, Function)>&& callback) = 0; + + private: + // Request to disconnect this connection. This method is called by the + // ~Connection::Ptr() when it goes out of scope, the API client should never + // call this method. + virtual void Disconnect() = 0; + + public: + // Movable Connection smart pointer. When Connection::Ptr is destroyed the + // Connection will disconnect automatically. + using Ptr = internal::RaiiPtr; }; } // namespace pw::bluetooth::low_energy diff --git a/pw_bluetooth/public/pw_bluetooth/low_energy/peripheral.h b/pw_bluetooth/public/pw_bluetooth/low_energy/peripheral.h index 057881d92e..23c56d170e 100644 --- a/pw_bluetooth/public/pw_bluetooth/low_energy/peripheral.h +++ b/pw_bluetooth/public/pw_bluetooth/low_energy/peripheral.h @@ -16,6 +16,7 @@ #include #include +#include "pw_bluetooth/internal/raii_ptr.h" #include "pw_bluetooth/low_energy/advertising_data.h" #include "pw_bluetooth/low_energy/connection.h" #include "pw_bluetooth/result.h" @@ -28,12 +29,11 @@ namespace pw::bluetooth::low_energy { // `AdvertisedPeripheral` instances are valid for the duration of advertising. class AdvertisedPeripheral { public: - // Destroying an instance will stop advertising. virtual ~AdvertisedPeripheral() = default; // Set a callback that will be called when an error occurs and advertising - // has been stopped (invalidating this object). It is OK to destroy this - // object from within `callback`. + // has been stopped (invalidating this object). It is OK to destroy the + // `AdvertisedPeripheral::Ptr` object from within `callback`. virtual void SetErrorCallback(Closure callback) = 0; // For connectable advertisements, this callback will be called when an LE @@ -50,9 +50,21 @@ class AdvertisedPeripheral { // // If advertising is not stopped, this callback may be called with multiple // connections over the lifetime of an advertisement. It is OK to destroy - // this object from within `callback` in order to stop advertising. + // the `AdvertisedPeripheral::Ptr` object from within `callback` in order to + // stop advertising. virtual void SetConnectionCallback( - Function)>&& callback) = 0; + Function&& callback) = 0; + + private: + // Stop advertising. This method is called by the ~AdvertisedPeripheral::Ptr() + // when it goes out of scope, the API client should never call this method. + virtual void StopAdvertising() = 0; + + public: + // Movable AdvertisedPeripheral smart pointer. The peripheral will continue + // advertising until the returned AdvertisedPeripheral::Ptr is destroyed. + using Ptr = internal::RaiiPtr; }; // Represents the LE Peripheral role, which advertises and is connected to. @@ -117,15 +129,15 @@ class Peripheral { kFailed = 6, }; - using AdvertiseCallback = Function>)>; + using AdvertiseCallback = + Function)>; virtual ~Peripheral() = default; // Start advertising continuously as a LE peripheral. If advertising cannot // be initiated then `result_callback` will be called with an error. Once // started, advertising can be stopped by destroying the returned - // `AdvertisedPeripheral`. + // `AdvertisedPeripheral::Ptr`. // // If the system supports multiple advertising, this may be called as many // times as there are advertising slots. To reconfigure an advertisement,