Refactor code to use abstract classes and PIMPL #370

jcarrano · 2024-12-06T23:18:07Z

This paves the way towards having multiple implementations at runtime. It also centralized the defintions of the "base" or "implementation" classes (such as AdapterBase, PeripheralBase) so that by explicitly deriving from these ABCs the interfaces of each implemetations are correct.

The public interface for "frontend" classes (the ones used by the library user) are unchanged. The main differences are:

They now include an "internal_" shared pointer (the PIMPL)
Some logic is implemented in the frontend (i.e. checking that a peripheral is connected before invoking read_inner, write_inner, etc).
The logic to detect if the instance is initialized is centralized into a private "operator->()"
The "safe" classes no longer inherit from the non safe ones. They are castable, though, so most code should continue working unmodified.

An additional concept, with the associated classes is introduced, the "Backend". For the time being this is not exposed in the public API. The idea is that all static methods in the Adapter will be moved to the Backend as non-static methods and the Backend frontend class will have a static get_backends(). Note that in many cases having a "BackendIMPL.h" header is unncecessary and thus ommited.

This paves the way towards having multiple implementations at runtime. It also centralized the defintions of the "base" or "implementation" classes (such as AdapterBase, PeripheralBase) so that by explicitly deriving from these ABCs the interfaces of each implemetations are correct. The public interface for "frontend" classes (the ones used by the library user) are unchanged. The main differences are: - They now include an "internal_" shared pointer (the PIMPL) - Some logic is implemented in the frontend (i.e. checking that a peripheral is connected before invoking read_inner, write_inner, etc). - The logic to detect if the instance is initialized is centralized into a private "operator->()" - The "safe" classes no longer inherit from the non safe ones. They are castable, though, so most code should continue working unmodified. An additional concept, with the associated classes is introduced, the "Backend". For the time being this is not exposed in the public API. The idea is that all static methods in the Adapter will be moved to the Backend as non-static methods and the Backend frontend class will have a static get_backends(). Note that in many cases having a "BackendIMPL.h" header is unncecessary and thus ommited.

jcarrano · 2024-12-06T23:48:52Z

The windows build fails on CI because it complains that a prototype declared "extern" is not called. Maybe the line that specifies WX should be removed:

# /WX -> Treats all warnings as errors.
list(APPEND PRIVATE_COMPILE_OPTIONS "/WX")

Or I can add some MSVC-specifi pragmas, or I can figure out how to do the backend selection differently.

kdewald · 2024-12-11T18:27:49Z

simpleble/src/frontends/base/Backend.cpp

+static std::shared_ptr<BackendBase> _get_enabled_backend(){
+    SIMPLEBLE_ENABLE(BACKEND_LINUX) SIMPLEBLE_ENABLE(BACKEND_WINDOWS) SIMPLEBLE_ENABLE(BACKEND_ANDROID)
+        SIMPLEBLE_ENABLE(BACKEND_MACOS) SIMPLEBLE_ENABLE(BACKEND_PLAIN)}


I think using plain IFDEFs is OK for this one, no need to make it overly complicated.

kdewald · 2024-12-11T18:38:51Z

simpleble/src/backends/common/BackendBase.h

+
+    virtual std::vector<std::shared_ptr<AdapterBase>> get_adapters() = 0;
+    virtual bool bluetooth_enabled() = 0;
+    virtual std::string backend_name() const noexcept = 0;


Can we just call the method name? Or does it conflict somewhere?

kdewald · 2024-12-11T18:40:04Z

simpleble/src/frontends/base/Adapter.cpp

@@ -1,105 +1,99 @@
+#include "fmt/format.h"


Is this header used here?

kdewald · 2024-12-11T18:41:59Z

simpleble/src/backends/common/PeripheralBase.h

+    virtual ByteArray read_inner(BluetoothUUID const& service, BluetoothUUID const& characteristic) = 0;
+    virtual void write_request_inner(BluetoothUUID const& service, BluetoothUUID const& characteristic, ByteArray const& data) = 0;
+    virtual void write_command_inner(BluetoothUUID const& service, BluetoothUUID const& characteristic, ByteArray const& data) = 0;
+    virtual void notify_inner(BluetoothUUID const& service, BluetoothUUID const& characteristic, std::function<void(ByteArray payload)> callback) = 0;
+    virtual void indicate_inner(BluetoothUUID const& service, BluetoothUUID const& characteristic, std::function<void(ByteArray payload)> callback) = 0;
+    virtual void unsubscribe_inner(BluetoothUUID const& service, BluetoothUUID const& characteristic) = 0;
+
+    virtual ByteArray read_inner(BluetoothUUID const& service, BluetoothUUID const& characteristic, BluetoothUUID const& descriptor) = 0;
+    virtual void write_inner(BluetoothUUID const& service, BluetoothUUID const& characteristic, BluetoothUUID const& descriptor, ByteArray const& data) = 0;


Can we skip adding the _inner suffix? It's sort of understandable if you're calling the internal pointer that you're referring to an inner method.

I put the _inner to indicate that it is not just that the frontend method calls the implementation method, but it also checks whether it is connected, so the _inner methods are only called on a connected peripheral. If that indication is not important, I can change the name back.

I don't think it's important to have that distinction. I'd rather keep the same name.

kdewald · 2024-12-11T18:42:48Z

simpleble/include/simpleble/AdapterSafe.h

+    /**
+     * Cast to the underlying adapter object.
+     */
+    operator SimpleBLE::Adapter() const noexcept;


Is this implemented? I couldn't find it.

kdewald · 2024-12-11T18:45:34Z

simpleble/src/CommonUtils.h

+
+namespace SimpleBLE {
+template <typename T>
+using vec_of_shared = std::vector<std::shared_ptr<T>>;


Can we use SharedPtrVector instead?

kdewald · 2024-12-11T19:03:02Z

simpleble/src/backends/linux/AdapterLinux.cpp


-void* AdapterBase::underlying() const { return adapter_.get(); }
+// TODO: why is this necessary?
+void* AdapterLinux::underlying() const { return adapter_.get(); }


You can add a developer note for this one. For certain compatibility with external libraries, we sometimes need to expose the actual OS handle to the user. This is particularly important for MacOS right now, but we might do something different in the future.

kdewald · 2024-12-11T19:05:32Z

simpleble/src/backends/linux/AdapterLinux.cpp

-
-    return enabled;
-}
+bool AdapterLinux::bluetooth_enabled() { return adapter_->powered(); }


This doesn't match what the original implementation did. Unfortunately we need to check every individual adapter.

Actually, on a second pass, given that this functionality is currently provided by the backend, this function doesn't need to exist as part of the adapter.

The thing with bluetooth_enabled is that there are different levels of granularity supported by different systems. Up to now, it was a static method, which means it reported the system-wide (and thus backend-wide, because only one backend could be enabled) setting.

In this PR I split the adapter-specific and backend-general enabled settings (which for some systems is the same).

On linux, it seems like one can query the powered status of individual adapters separately and the implementation was emulating a system-wide setting by returning true if at least one adapter was enabled. A problem with this is that the specific adapter corresponding to the Adapter object that the user has may not be the one that is enabled.

Yeah, we'll need to redesign this at some point, but I think for now your approach is fine, let's ship it.

kdewald · 2024-12-11T19:14:09Z

simpleble/src/backends/macos/BackendMac.mm

+
+// There is no need to make this class a singleton, as there is no state (no
+// member variables)
+class BackendMac : public BackendBase {


Could you rename this class and file to BackendCoreBluetooth?

kdewald · 2024-12-11T19:14:55Z

simpleble/src/backends/windows/BackendWindows.cpp

+
+std::shared_ptr<BackendBase> BACKEND_WINDOWS() { return BackendWindows::get(); }
+
+BackendWindows::BackendWindows(buildToken) { initialize_winrt(); }


Could we rename this backend to BackendWinRT?

jcarrano · 2024-12-12T15:04:30Z

simpleble/include/simpleble/Adapter.h

  protected:
+    AdapterBase* operator->();
+    const AdapterBase* operator->() const;
+
    std::shared_ptr<AdapterBase> internal_;


@kdewald This repetition in each frontend class could be avoided by having a base PIMPL class that is (privately) inherited. I did not do it not to make the header look "complicated" but it would save on code.

I'm fine having the duplication at this point, the most important thing is that the user-facing API is easy to read and understand right away. The extra lines of code are negligible.

jcarrano force-pushed the backends-multi1 branch from 2e810c4 to c4d1cde Compare December 6, 2024 23:41

kdewald self-requested a review December 11, 2024 18:01

kdewald reviewed Dec 11, 2024

View reviewed changes

simpleble/src/frontends/base/Adapter.cpp

@@ -1,105 +1,99 @@

#include "fmt/format.h"

Copy link

Member

kdewald Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this header used here?

kdewald reviewed Dec 11, 2024

View reviewed changes

jcarrano commented Dec 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor code to use abstract classes and PIMPL #370

Refactor code to use abstract classes and PIMPL #370

jcarrano commented Dec 6, 2024

jcarrano commented Dec 6, 2024

kdewald Dec 11, 2024

kdewald Dec 11, 2024

kdewald Dec 11, 2024

kdewald Dec 11, 2024

jcarrano Dec 12, 2024

kdewald Dec 12, 2024

kdewald Dec 11, 2024

kdewald Dec 11, 2024

kdewald Dec 11, 2024

kdewald Dec 11, 2024

kdewald Dec 11, 2024

jcarrano Dec 12, 2024

kdewald Dec 12, 2024

kdewald Dec 11, 2024

kdewald Dec 11, 2024

jcarrano Dec 12, 2024

kdewald Dec 12, 2024


		std::shared_ptr<BackendBase> BACKEND_WINDOWS() { return BackendWindows::get(); }

		BackendWindows::BackendWindows(buildToken) { initialize_winrt(); }

Refactor code to use abstract classes and PIMPL #370

Are you sure you want to change the base?

Refactor code to use abstract classes and PIMPL #370

Conversation

jcarrano commented Dec 6, 2024

jcarrano commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment