Refactor services into classes (#183)

TactilityHeadless/Source/TactilityHeadless.cpp

@@ -21,20 +21,12 @@ namespace service::sdcard { extern const ServiceManifest manifest; }
 
 static Dispatcher mainDispatcher;
 
-static const service::ServiceManifest* const system_services[] = {
-    &service::sdcard::manifest,
-    &service::wifi::manifest
-};
-
 static const hal::Configuration* hardwareConfig = nullptr;
 
 static void register_and_start_system_services() {
     TT_LOG_I(TAG, "Registering and starting system services");
-    int app_count = sizeof(system_services) / sizeof(service::ServiceManifest*);
-    for (int i = 0; i < app_count; ++i) {
-        addService(system_services[i]);
-        tt_check(service::startService(system_services[i]->id));
-    }
+    addService(service::sdcard::manifest);
+    addService(service::wifi::manifest);
 }
 
 void initHeadless(const hal::Configuration& config) {

TactilityHeadless/Source/service/Service.h

@@ -0,0 +1,24 @@
+#pragma once
+
+#include <memory>
+
+namespace tt::service {
+
+// Forward declaration
+class ServiceContext;
+
+class Service {
+
+public:
+
+    Service() = default;
+    virtual ~Service() = default;
+
+    virtual void onStart(ServiceContext& serviceContext) {}
+    virtual void onStop(ServiceContext& serviceContext) {}
+};
+
+template<typename T>
+std::shared_ptr<Service> create() { return std::shared_ptr<T>(new T); }
+
+}

TactilityHeadless/Source/service/ServiceContext.h

@@ -22,10 +22,7 @@ public:
 
     /** @return a reference ot the service's manifest */
     virtual const service::ServiceManifest& getManifest() const = 0;
-    /** @return a shared pointer to the data that is attached to the service */
-    virtual std::shared_ptr<void> _Nullable getData() const = 0;
-    /** Set the data for a service. */
-    virtual void setData(std::shared_ptr<void> newData) = 0;
+
     /** Retrieve the paths that are relevant to this service */
     virtual std::unique_ptr<Paths> getPaths() const = 0;
 };

TactilityHeadless/Source/service/ServiceInstance.cpp

@@ -1,26 +1,14 @@
-#include <utility>
-
 #include "service/ServiceInstance.h"
 #include "service/ServiceInstancePaths.h"
 
 namespace tt::service {
 
-ServiceInstance::ServiceInstance(const service::ServiceManifest&manifest) : manifest(manifest) {}
+ServiceInstance::ServiceInstance(std::shared_ptr<const service::ServiceManifest> manifest) :
+    manifest(manifest),
+    service(manifest->createService())
+{}
 
-const service::ServiceManifest& ServiceInstance::getManifest() const { return manifest; }
-
-std::shared_ptr<void> ServiceInstance::getData() const {
-    mutex.acquire(TtWaitForever);
-    std::shared_ptr<void> result = data;
-    mutex.release();
-    return result;
-}
-
-void ServiceInstance::setData(std::shared_ptr<void> newData) {
-    mutex.acquire(TtWaitForever);
-    data = std::move(newData);
-    mutex.release();
-}
+const service::ServiceManifest& ServiceInstance::getManifest() const { return *manifest; }
 
 std::unique_ptr<Paths> ServiceInstance::getPaths() const {
     return std::make_unique<ServiceInstancePaths>(manifest);

TactilityHeadless/Source/service/ServiceInstancePaths.cpp

@@ -11,38 +11,38 @@
 namespace tt::service {
 
 std::string ServiceInstancePaths::getDataDirectory() const {
-    return PARTITION_PREFIX + DATA_PARTITION_NAME + "/service/" + manifest.id;
+    return PARTITION_PREFIX + DATA_PARTITION_NAME + "/service/" + manifest->id;
 }
 
 std::string ServiceInstancePaths::getDataDirectoryLvgl() const {
-    return LVGL_PATH_PREFIX + DATA_PARTITION_NAME + "/service/" + manifest.id;
+    return LVGL_PATH_PREFIX + DATA_PARTITION_NAME + "/service/" + manifest->id;
 }
 
 std::string ServiceInstancePaths::getDataPath(const std::string& childPath) const {
     assert(!childPath.starts_with('/'));
-    return PARTITION_PREFIX + DATA_PARTITION_NAME + "/service/" + manifest.id + '/' + childPath;
+    return PARTITION_PREFIX + DATA_PARTITION_NAME + "/service/" + manifest->id + '/' + childPath;
 }
 
 std::string ServiceInstancePaths::getDataPathLvgl(const std::string& childPath) const {
     assert(!childPath.starts_with('/'));
-    return LVGL_PATH_PREFIX + DATA_PARTITION_NAME + "/service/" + manifest.id + '/' + childPath;
+    return LVGL_PATH_PREFIX + DATA_PARTITION_NAME + "/service/" + manifest->id + '/' + childPath;
 }
 
 std::string ServiceInstancePaths::getSystemDirectory() const {
-    return PARTITION_PREFIX + SYSTEM_PARTITION_NAME + "/service/" + manifest.id;
+    return PARTITION_PREFIX + SYSTEM_PARTITION_NAME + "/service/" + manifest->id;
 }
 
 std::string ServiceInstancePaths::getSystemDirectoryLvgl() const {
-    return LVGL_PATH_PREFIX + SYSTEM_PARTITION_NAME + "/service/" + manifest.id;
+    return LVGL_PATH_PREFIX + SYSTEM_PARTITION_NAME + "/service/" + manifest->id;
 }
 
 std::string ServiceInstancePaths::getSystemPath(const std::string& childPath) const {
     assert(!childPath.starts_with('/'));
-    return PARTITION_PREFIX + SYSTEM_PARTITION_NAME + "/service/" + manifest.id + '/' + childPath;
+    return PARTITION_PREFIX + SYSTEM_PARTITION_NAME + "/service/" + manifest->id + '/' + childPath;
 }
 
 std::string ServiceInstancePaths::getSystemPathLvgl(const std::string& childPath) const {
-    return LVGL_PATH_PREFIX + SYSTEM_PARTITION_NAME + "/service/" + manifest.id + '/' + childPath;
+    return LVGL_PATH_PREFIX + SYSTEM_PARTITION_NAME + "/service/" + manifest->id + '/' + childPath;
 }
 
 }

TactilityHeadless/Source/service/ServiceManifest.h

@@ -1,24 +1,22 @@
 #pragma once
 
+#include "service/Service.h"
 #include <string>
 
 namespace tt::service {
 
+// Forward declarations
 class ServiceContext;
 
-typedef void (*ServiceOnStart)(ServiceContext& service);
-typedef void (*ServiceOnStop)(ServiceContext& service);
+typedef std::shared_ptr<Service>(*CreateService)();
 
 /** A ledger that describes the main parts of a service. */
 struct ServiceManifest {
     /** The identifier by which the app is launched by the system and other apps. */
     std::string id {};
 
-    /** Non-blocking method to call when service is started. */
-    const ServiceOnStart onStart = nullptr;
-
-    /** Non-blocking method to call when service is stopped. */
-    const ServiceOnStop onStop = nullptr;
+    /** Create the instance of the app */
+    CreateService createService = nullptr;
 };
 
 } // namespace

TactilityHeadless/Source/service/ServiceRegistry.cpp

@@ -3,7 +3,6 @@
 #include "Mutex.h"
 #include "service/ServiceInstance.h"
 #include "service/ServiceManifest.h"
-#include "TactilityCore.h"
 #include <string>
 #include <unordered_map>
 
@@ -11,8 +10,8 @@ namespace tt::service {
 
 #define TAG "service_registry"
 
-typedef std::unordered_map<std::string, const ServiceManifest*> ManifestMap;
-typedef std::unordered_map<std::string, ServiceInstance*> ServiceInstanceMap;
+typedef std::unordered_map<std::string, std::shared_ptr<const ServiceManifest>> ManifestMap;
+typedef std::unordered_map<std::string, std::shared_ptr<ServiceInstance>> ServiceInstanceMap;
 
 static ManifestMap service_manifest_map;
 static ServiceInstanceMap service_instance_map;
@@ -20,30 +19,41 @@ static ServiceInstanceMap service_instance_map;
 static Mutex manifest_mutex(Mutex::Type::Normal);
 static Mutex instance_mutex(Mutex::Type::Normal);
 
-void addService(const ServiceManifest* manifest) {
-    TT_LOG_I(TAG, "Adding %s", manifest->id.c_str());
+void addService(std::shared_ptr<const ServiceManifest> manifest, bool autoStart) {
+    // We'll move the manifest pointer, but we'll need to id later
+    std::string id = manifest->id;
 
-    manifest_mutex.acquire(TtWaitForever);
-    if (service_manifest_map[manifest->id] == nullptr) {
-        service_manifest_map[manifest->id] = manifest;
+    TT_LOG_I(TAG, "Adding %s", id.c_str());
+
+    manifest_mutex.lock(TtWaitForever);
+    if (service_manifest_map[id] == nullptr) {
+        service_manifest_map[id] = std::move(manifest);
     } else {
-        TT_LOG_E(TAG, "Service id in use: %s", manifest->id.c_str());
+        TT_LOG_E(TAG, "Service id in use: %s", id.c_str());
+    }
+    manifest_mutex.unlock();
+
+    if (autoStart) {
+        startService(id);
     }
-    manifest_mutex.release();
 }
 
-const ServiceManifest* _Nullable findManifestId(const std::string& id) {
+void addService(const ServiceManifest& manifest, bool autoStart) {
+    addService(std::make_shared<const ServiceManifest>(manifest), autoStart);
+}
+
+std::shared_ptr<const ServiceManifest> _Nullable findManifestId(const std::string& id) {
     manifest_mutex.acquire(TtWaitForever);
     auto iterator = service_manifest_map.find(id);
-    _Nullable const ServiceManifest * manifest = iterator != service_manifest_map.end() ? iterator->second : nullptr;
+    auto manifest = iterator != service_manifest_map.end() ? iterator->second : nullptr;
     manifest_mutex.release();
     return manifest;
 }
 
-static ServiceInstance* _Nullable service_registry_find_instance_by_id(const std::string& id) {
+static std::shared_ptr<ServiceInstance> _Nullable findServiceInstanceById(const std::string& id) {
     manifest_mutex.acquire(TtWaitForever);
     auto iterator = service_instance_map.find(id);
-    _Nullable ServiceInstance* service = iterator != service_instance_map.end() ? iterator->second : nullptr;
+    auto service = iterator != service_instance_map.end() ? iterator->second : nullptr;
     manifest_mutex.release();
     return service;
 }
@@ -51,42 +61,53 @@ static ServiceInstance* _Nullable service_registry_find_instance_by_id(const std
 // TODO: Return proper error/status instead of BOOL?
 bool startService(const std::string& id) {
     TT_LOG_I(TAG, "Starting %s", id.c_str());
-    const ServiceManifest* manifest = findManifestId(id);
+    auto manifest = findManifestId(id);
     if (manifest == nullptr) {
         TT_LOG_E(TAG, "manifest not found for service %s", id.c_str());
         return false;
     }
 
-    auto* service = new ServiceInstance(*manifest);
-    manifest->onStart(*service);
+    auto service_instance = std::make_shared<ServiceInstance>(manifest);
 
+    // Register first, so that a service can retrieve itself during onStart()
     instance_mutex.acquire(TtWaitForever);
-    service_instance_map[manifest->id] = service;
+    service_instance_map[manifest->id] = service_instance;
     instance_mutex.release();
+
+    service_instance->getService()->onStart(*service_instance);
+
     TT_LOG_I(TAG, "Started %s", id.c_str());
 
     return true;
 }
 
-_Nullable ServiceContext* findServiceById(const std::string& service_id) {
-    return static_cast<ServiceInstance*>(service_registry_find_instance_by_id(service_id));
+std::shared_ptr<ServiceContext> _Nullable findServiceContextById(const std::string& id) {
+    return findServiceInstanceById(id);
+}
+
+std::shared_ptr<Service> _Nullable findServiceById(const std::string& id) {
+    auto instance = findServiceInstanceById(id);
+    return instance != nullptr ? instance->getService() : nullptr;
 }
 
 bool stopService(const std::string& id) {
     TT_LOG_I(TAG, "Stopping %s", id.c_str());
-    ServiceInstance* service = service_registry_find_instance_by_id(id);
-    if (service == nullptr) {
+    auto service_instance = findServiceInstanceById(id);
+    if (service_instance == nullptr) {
         TT_LOG_W(TAG, "service not running: %s", id.c_str());
         return false;
     }
 
-    service->getManifest().onStop(*service);
-    delete service;
+    service_instance->getService()->onStop(*service_instance);
 
     instance_mutex.acquire(TtWaitForever);
     service_instance_map.erase(id);
     instance_mutex.release();
 
+    if (service_instance.use_count() > 1) {
+        TT_LOG_W(TAG, "Possible memory leak: service %s still has %ld references", service_instance->getManifest().id.c_str(), service_instance.use_count() - 1);
+    }
+
     TT_LOG_I(TAG, "Stopped %s", id.c_str());
 
     return true;

TactilityHeadless/Source/service/ServiceRegistry.h

@@ -1,20 +1,20 @@
 #pragma once
 
 #include "service/ServiceManifest.h"
+#include "service/Service.h"
+#include <memory>
 
 namespace tt::service {
 
-void initRegistry();
-
 /** Register a service.
  * @param[in] the service manifest
  */
-void addService(const ServiceManifest* manifest);
+void addService(std::shared_ptr<const ServiceManifest> manifest, bool autoStart = true);
 
-/** Unregister a service.
+/** Register a service.
  * @param[in] the service manifest
  */
-void removeService(const ServiceManifest* manifest);
+void addService(const ServiceManifest& manifest, bool autoStart = true);
 
 /** Start a service.
  * @param[in] the service id as defined in its manifest
@@ -32,12 +32,27 @@ bool stopService(const std::string& id);
  * @param[in] id the id as defined in the manifest
  * @return the matching manifest or nullptr when it wasn't found
  */
-const ServiceManifest* _Nullable findManifestId(const std::string& id);
+std::shared_ptr<const ServiceManifest> _Nullable findManifestId(const std::string& id);
 
-/** Find a service by its manifest id.
+/** Find a ServiceContext by its manifest id.
  * @param[in] id the id as defined in the manifest
  * @return the service context or nullptr when it wasn't found
  */
-ServiceContext* _Nullable findServiceById(const std::string& id);
+std::shared_ptr<ServiceContext> _Nullable findServiceContextById(const std::string& id);
+
+/** Find a Service by its manifest id.
+ * @param[in] id the id as defined in the manifest
+ * @return the service context or nullptr when it wasn't found
+ */
+std::shared_ptr<Service> _Nullable findServiceById(const std::string& id);
+
+/** Find a Service by its manifest id.
+ * @param[in] id the id as defined in the manifest
+ * @return the service context or nullptr when it wasn't found
+ */
+template <typename T>
+std::shared_ptr<T> _Nullable findServiceById(const std::string& id) {
+    return std::static_pointer_cast<T>(findServiceById(id));
+}
 
 } // namespace

TactilityHeadless/Source/service/sdcard/Sdcard.cpp

@@ -11,7 +11,10 @@ namespace tt::service::sdcard {
 
 extern const ServiceManifest manifest;
 
-struct ServiceData {
+class SdCardService final : public Service {
+
+private:
+
     Mutex mutex;
     std::unique_ptr<Timer> updateTimer;
     hal::SdCard::State lastState = hal::SdCard::State::Unmounted;
@@ -23,61 +26,59 @@ struct ServiceData {
     void unlock() const {
         tt_check(mutex.release() == TtStatusOk);
     }
+
+    void update() {
+        auto sdcard = tt::hal::getConfiguration()->sdcard;
+        tt_assert(sdcard);
+
+        if (lock(50)) {
+            auto new_state = sdcard->getState();
+
+            if (new_state == hal::SdCard::State::Error) {
+                TT_LOG_W(TAG, "Sdcard error - unmounting. Did you eject the card in an unsafe manner?");
+                sdcard->unmount();
+            }
+
+            if (new_state != lastState) {
+                lastState = new_state;
+            }
+
+            unlock();
+        } else {
+            TT_LOG_W(TAG, LOG_MESSAGE_MUTEX_LOCK_FAILED);
+        }
+    }
+
+    static void onUpdate(std::shared_ptr<void> context) {
+        auto service = std::static_pointer_cast<SdCardService>(context);
+        service->update();
+    }
+
+public:
+
+    void onStart(ServiceContext& serviceContext) final {
+        if (hal::getConfiguration()->sdcard != nullptr) {
+            auto service = findServiceById(manifest.id);
+            updateTimer = std::make_unique<Timer>(Timer::Type::Periodic, onUpdate, service);
+            // We want to try and scan more often in case of startup or scan lock failure
+            updateTimer->start(1000);
+        } else {
+            TT_LOG_I(TAG, "Timer not started: no SD card config");
+        }
+    }
+
+    void onStop(ServiceContext& serviceContext) final {
+        if (updateTimer != nullptr) {
+            // Stop thread
+            updateTimer->stop();
+            updateTimer = nullptr;
+        }
+    }
 };
 
-static void onUpdate(std::shared_ptr<void> context) {
-    auto sdcard = tt::hal::getConfiguration()->sdcard;
-    if (sdcard == nullptr) {
-        return;
-    }
-
-    auto data = std::static_pointer_cast<ServiceData>(context);
-
-    if (!data->lock(50)) {
-        TT_LOG_W(TAG, LOG_MESSAGE_MUTEX_LOCK_FAILED);
-        return;
-    }
-
-    auto new_state = sdcard->getState();
-
-    if (new_state == hal::SdCard::State::Error) {
-        TT_LOG_W(TAG, "Sdcard error - unmounting. Did you eject the card in an unsafe manner?");
-        sdcard->unmount();
-    }
-
-    if (new_state != data->lastState) {
-        data->lastState = new_state;
-    }
-
-    data->unlock();
-}
-
-static void onStart(ServiceContext& service) {
-    if (hal::getConfiguration()->sdcard != nullptr) {
-        auto data = std::make_shared<ServiceData>();
-        service.setData(data);
-
-        data->updateTimer = std::make_unique<Timer>(Timer::Type::Periodic, onUpdate, data);
-        // We want to try and scan more often in case of startup or scan lock failure
-        data->updateTimer->start(1000);
-    } else {
-        TT_LOG_I(TAG, "task not started due to config");
-    }
-}
-
-static void onStop(ServiceContext& service) {
-    auto data = std::static_pointer_cast<ServiceData>(service.getData());
-    if (data->updateTimer != nullptr) {
-        // Stop thread
-        data->updateTimer->stop();
-        data->updateTimer = nullptr;
-    }
-}
-
 extern const ServiceManifest manifest = {
     .id = "sdcard",
-    .onStart = onStart,
-    .onStop = onStop
+    .createService = create<SdCardService>
 };
 
 } // namespace

TactilityHeadless/Source/service/wifi/WifiEsp.cpp

@@ -936,52 +936,54 @@ void onAutoConnectTimer(std::shared_ptr<void> context) {
     }
 }
 
-static void onStart(ServiceContext& service) {
-    tt_assert(wifi_singleton == nullptr);
-    wifi_singleton = std::make_shared<Wifi>();
+class WifiService final : public Service {
 
-    service.setData(wifi_singleton);
+public:
 
-    wifi_singleton->autoConnectTimer = std::make_unique<Timer>(Timer::Type::Periodic, onAutoConnectTimer, wifi_singleton);
-    // We want to try and scan more often in case of startup or scan lock failure
-    wifi_singleton->autoConnectTimer->start(TT_MIN(2000, AUTO_SCAN_INTERVAL));
+    void onStart(ServiceContext& service) override {
+        tt_assert(wifi_singleton == nullptr);
+        wifi_singleton = std::make_shared<Wifi>();
 
-    if (settings::shouldEnableOnBoot()) {
-        TT_LOG_I(TAG, "Auto-enabling due to setting");
-        getMainDispatcher().dispatch(dispatchEnable, wifi_singleton);
-    }
-}
+        wifi_singleton->autoConnectTimer = std::make_unique<Timer>(Timer::Type::Periodic, onAutoConnectTimer, wifi_singleton);
+        // We want to try and scan more often in case of startup or scan lock failure
+        wifi_singleton->autoConnectTimer->start(TT_MIN(2000, AUTO_SCAN_INTERVAL));
 
-static void onStop(ServiceContext& service) {
-    auto wifi = wifi_singleton;
-    tt_assert(wifi != nullptr);
-
-    RadioState state = wifi->getRadioState();
-    if (state != RadioState::Off) {
-        dispatchDisable(wifi);
+        if (settings::shouldEnableOnBoot()) {
+            TT_LOG_I(TAG, "Auto-enabling due to setting");
+            getMainDispatcher().dispatch(dispatchEnable, wifi_singleton);
+        }
     }
 
-    wifi->autoConnectTimer->stop();
-    wifi->autoConnectTimer = nullptr; // Must release as it holds a reference to this Wifi instance
+    void onStop(ServiceContext& service) override {
+        auto wifi = wifi_singleton;
+        tt_assert(wifi != nullptr);
 
-    // Acquire all mutexes
-    wifi->dataMutex.acquire(TtWaitForever);
-    wifi->radioMutex.acquire(TtWaitForever);
+        RadioState state = wifi->getRadioState();
+        if (state != RadioState::Off) {
+            dispatchDisable(wifi);
+        }
 
-    // Detach
-    wifi_singleton = nullptr;
+        wifi->autoConnectTimer->stop();
+        wifi->autoConnectTimer = nullptr; // Must release as it holds a reference to this Wifi instance
 
-    // Release mutexes
-    wifi->dataMutex.release();
-    wifi->radioMutex.release();
+        // Acquire all mutexes
+        wifi->dataMutex.acquire(TtWaitForever);
+        wifi->radioMutex.acquire(TtWaitForever);
 
-    // Release (hopefully) last Wifi instance by scope
-}
+        // Detach
+        wifi_singleton = nullptr;
+
+        // Release mutexes
+        wifi->dataMutex.release();
+        wifi->radioMutex.release();
+
+        // Release (hopefully) last Wifi instance by scope
+    }
+};
 
 extern const ServiceManifest manifest = {
     .id = "Wifi",
-    .onStart = onStart,
-    .onStop = onStop
+    .createService = create<WifiService>
 };
 
 } // namespace

TactilityHeadless/Source/service/wifi/WifiMock.cpp

@@ -137,25 +137,25 @@ int getRssi() {
 
 // endregion Public functions
 
-static void service_start(TT_UNUSED ServiceContext& service) {
-    tt_check(wifi == nullptr);
-    wifi = new Wifi();
-}
+class WifiService final : public Service {
 
-static void service_stop(TT_UNUSED ServiceContext& service) {
-    tt_check(wifi != nullptr);
-    delete wifi;
-    wifi = nullptr;
-}
+public:
 
-void wifi_main(void*) {
-    // NO-OP
-}
+    void onStart(TT_UNUSED ServiceContext& service) final {
+        tt_check(wifi == nullptr);
+        wifi = new Wifi();
+    }
+
+    void onStop(TT_UNUSED ServiceContext& service) final {
+        tt_check(wifi != nullptr);
+        delete wifi;
+        wifi = nullptr;
+    }
+};
 
 extern const ServiceManifest manifest = {
     .id = "Wifi",
-    .onStart = &service_start,
-    .onStop = &service_stop
+    .createService = create<WifiService>
 };
 
 } // namespace