Improvements

TactilityKernel/Include/tactility/concurrent/mutex.h

@@ -18,12 +18,10 @@ struct Mutex {
 };
 
 inline static void mutex_construct(struct Mutex* mutex) {
-    assert(mutex->handle == NULL);
     mutex->handle = xSemaphoreCreateMutex();
 }
 
 inline static void mutex_destruct(struct Mutex* mutex) {
-    assert(mutex->handle != NULL);
     vPortAssertIfInISR();
     vSemaphoreDelete(mutex->handle);
     mutex->handle = NULL;

TactilityKernel/Source/device.cpp

@@ -15,19 +15,19 @@
 
 struct DeviceInternal {
     /** Address of the API exposed by the device instance. */
-    struct Driver* driver;
+    struct Driver* driver = nullptr;
     /** The driver data for this device (e.g. a mutex) */
-    void* driver_data;
+    void* driver_data = nullptr;
     /** The mutex for device operations */
-    struct Mutex mutex;
+    struct Mutex mutex {};
     /** The device state */
     struct {
-        int start_result;
-        bool started : 1;
-        bool added : 1;
+        int start_result = 0;
+        bool started : 1 = false;
+        bool added : 1 = false;
     } state;
     /** Attached child devices */
-    std::vector<Device*> children;
+    std::vector<Device*> children {};
 };
 
 struct DeviceLedger {
@@ -55,6 +55,9 @@ extern "C" {
 #define ledger_lock() mutex_lock(&ledger.mutex)
 #define ledger_unlock() mutex_unlock(&ledger.mutex)
 
+#define lock_internal(internal) mutex_lock(&internal->mutex)
+#define unlock_internal(internal) mutex_unlock(&internal->mutex)
+
 error_t device_construct(Device* device) {
     device->internal = new(std::nothrow) DeviceInternal;
     if (device->internal == nullptr) {
@@ -66,16 +69,22 @@ error_t device_construct(Device* device) {
 }
 
 error_t device_destruct(Device* device) {
-    if (device->internal->state.started || device->internal->state.added) {
+    lock_internal(device->internal);
+
+    auto* internal = device->internal;
+
+    if (internal->state.started || internal->state.added) {
         return ERROR_INVALID_STATE;
     }
-    if (!device->internal->children.empty()) {
+    if (!internal->children.empty()) {
         return ERROR_INVALID_STATE;
     }
     LOG_D(TAG, "destruct %s", device->name);
-    mutex_destruct(&device->internal->mutex);
-    delete device->internal;
+
     device->internal = nullptr;
+    mutex_unlock(&internal->mutex);
+    delete internal;
+
     return ERROR_NONE;
 }
 

TactilityKernel/Source/driver.cpp

@@ -13,22 +13,13 @@
 #define TAG "driver"
 
 struct DriverInternal {
-    Mutex mutex { 0 };
     int use_count = 0;
     bool destroying = false;
-
-    DriverInternal() {
-        mutex_construct(&mutex);
-    }
-
-    ~DriverInternal() {
-        mutex_destruct(&mutex);
-    }
 };
 
 struct DriverLedger {
     std::vector<Driver*> drivers;
-    Mutex mutex { 0 };
+    Mutex mutex {};
 
     DriverLedger() { mutex_construct(&mutex); }
     ~DriverLedger() { mutex_destruct(&mutex); }
@@ -37,16 +28,7 @@ struct DriverLedger {
     void unlock() { mutex_unlock(&mutex); }
 };
 
-static DriverLedger& get_ledger() {
-    static DriverLedger ledger;
-    return ledger;
-}
-
-#define ledger get_ledger()
-
-#define get_driver_internal(driver) static_cast<DriverInternal*>(driver->internal)
-#define driver_lock(driver) mutex_lock(&get_driver_internal(driver)->mutex);
-#define driver_unlock(driver) mutex_unlock(&get_driver_internal(driver)->mutex);
+static DriverLedger ledger;
 
 extern "C" {
 
@@ -59,21 +41,18 @@ error_t driver_construct(Driver* driver) {
 }
 
 error_t driver_destruct(Driver* driver) {
-    driver_lock(driver);
-    // No module means the system owns it and it cannot be destroyed
+    auto* internal = driver->internal;
+
     if (driver->owner == nullptr) {
-        driver_unlock(driver);
         return ERROR_NOT_ALLOWED;
     }
-    if (get_driver_internal(driver)->use_count != 0 || get_driver_internal(driver)->destroying) {
-        driver_unlock(driver);
+    if (internal->use_count != 0 || internal->destroying) {
         return ERROR_INVALID_STATE;
     }
-    get_driver_internal(driver)->destroying = true;
+    internal->destroying = true;
 
-    DriverInternal* internal = get_driver_internal(driver);
+    // Remove the internal reference before unlocking so it cannot be accidentally locked again
     driver->internal = nullptr;
-    mutex_unlock(&internal->mutex);
     delete internal;
 
     return ERROR_NONE;
@@ -144,10 +123,8 @@ Driver* driver_find_compatible(const char* compatible) {
 }
 
 error_t driver_bind(Driver* driver, Device* device) {
-    driver_lock(driver);
-
     error_t error = ERROR_NONE;
-    if (get_driver_internal(driver)->destroying || !device_is_added(device)) {
+    if (driver->internal->destroying || !device_is_added(device)) {
         error = ERROR_INVALID_STATE;
         goto error;
     }
@@ -159,23 +136,18 @@ error_t driver_bind(Driver* driver, Device* device) {
         }
     }
 
-    get_driver_internal(driver)->use_count++;
-    driver_unlock(driver);
+    driver->internal->use_count++;
 
     LOG_I(TAG, "bound %s to %s", driver->name, device->name);
     return ERROR_NONE;
 
 error:
-
-    driver_unlock(driver);
     return error;
 }
 
 error_t driver_unbind(Driver* driver, Device* device) {
-    driver_lock(driver);
-
     error_t error = ERROR_NONE;
-    if (get_driver_internal(driver)->destroying || !device_is_added(device)) {
+    if (driver->internal->destroying || !device_is_added(device)) {
         error = ERROR_INVALID_STATE;
         goto error;
     }
@@ -187,16 +159,13 @@ error_t driver_unbind(Driver* driver, Device* device) {
         }
     }
 
-    get_driver_internal(driver)->use_count--;
-    driver_unlock(driver);
+    driver->internal->use_count--;
 
     LOG_I(TAG, "unbound %s from %s", driver->name, device->name);
 
     return ERROR_NONE;
 
 error:
-
-    driver_unlock(driver);
     return error;
 }
 

TactilityKernel/Source/module.cpp

@@ -13,7 +13,7 @@ struct ModuleInternal {
 
 struct ModuleLedger {
     std::vector<struct Module*> modules;
-    struct Mutex mutex = { 0 };
+    struct Mutex mutex {};
 
     ModuleLedger() { mutex_construct(&mutex); }
     ~ModuleLedger() { mutex_destruct(&mutex); }

Tests/TactilityKernel/Source/DeviceTest.cpp

@@ -26,7 +26,12 @@ TEST_CASE("device_construct and device_destruct should set and unset the interna
 }
 
 TEST_CASE("device_add should add the device to the list of all devices") {
-    Device device = { 0 };
+    Device device = {
+        .name = "device",
+        .config = nullptr,
+        .parent = nullptr,
+        .internal = nullptr
+    };
     CHECK_EQ(device_construct(&device), ERROR_NONE);
     CHECK_EQ(device_add(&device), ERROR_NONE);
 
@@ -46,12 +51,18 @@ TEST_CASE("device_add should add the device to the list of all devices") {
 }
 
 TEST_CASE("device_add should add the device to its parent") {
-    Device parent = { 0 };
+    Device parent = {
+        .name = "parent",
+        .config = nullptr,
+        .parent = nullptr,
+        .internal = nullptr
+    };
 
     Device child = {
-        .name = nullptr,
+        .name = "child",
         .config = nullptr,
-        .parent = &parent
+        .parent = &parent,
+        .internal = nullptr
     };
 
     CHECK_EQ(device_construct(&parent), ERROR_NONE);
@@ -79,7 +90,13 @@ TEST_CASE("device_add should add the device to its parent") {
 }
 
 TEST_CASE("device_add should set the state to 'added'") {
-    Device device = { 0 };
+    Device device = {
+        .name = "device",
+        .config = nullptr,
+        .parent = nullptr,
+        .internal = nullptr
+    };
+
     CHECK_EQ(device_construct(&device), ERROR_NONE);
 
     CHECK_EQ(device_is_added(&device), false);
@@ -91,7 +108,13 @@ TEST_CASE("device_add should set the state to 'added'") {
 }
 
 TEST_CASE("device_remove should remove it from the list of all devices") {
-    Device device = { 0 };
+    Device device = {
+        .name = "device",
+        .config = nullptr,
+        .parent = nullptr,
+        .internal = nullptr
+    };
+
     CHECK_EQ(device_construct(&device), ERROR_NONE);
     CHECK_EQ(device_add(&device), ERROR_NONE);
     CHECK_EQ(device_remove(&device), ERROR_NONE);
@@ -110,12 +133,18 @@ TEST_CASE("device_remove should remove it from the list of all devices") {
 }
 
 TEST_CASE("device_remove should remove the device from its parent") {
-    Device parent = { 0 };
+    Device parent = {
+        .name = "parent",
+        .config = nullptr,
+        .parent = nullptr,
+        .internal = nullptr
+    };
 
     Device child = {
-        .name = nullptr,
+        .name = "child",
         .config = nullptr,
-        .parent = &parent
+        .parent = &parent,
+        .internal = nullptr
     };
 
     CHECK_EQ(device_construct(&parent), ERROR_NONE);
@@ -142,7 +171,13 @@ TEST_CASE("device_remove should remove the device from its parent") {
 }
 
 TEST_CASE("device_remove should clear the state 'added'") {
-    Device device = { 0 };
+    Device device = {
+        .name = "device",
+        .config = nullptr,
+        .parent = nullptr,
+        .internal = nullptr
+    };
+
     CHECK_EQ(device_construct(&device), ERROR_NONE);
 
     CHECK_EQ(device_add(&device), ERROR_NONE);