From f955a0ec5ca2ee7a09afd9683a06d5101cde48b1 Mon Sep 17 00:00:00 2001 From: Ken Van Hoeylandt Date: Fri, 6 Feb 2026 15:51:10 +0100 Subject: [PATCH] Improvements --- .../Include/tactility/concurrent/mutex.h | 2 - TactilityKernel/Source/device.cpp | 31 +++++++---- TactilityKernel/Source/driver.cpp | 53 ++++-------------- TactilityKernel/Source/module.cpp | 2 +- Tests/TactilityKernel/Source/DeviceTest.cpp | 55 +++++++++++++++---- 5 files changed, 77 insertions(+), 66 deletions(-) diff --git a/TactilityKernel/Include/tactility/concurrent/mutex.h b/TactilityKernel/Include/tactility/concurrent/mutex.h index 4d5e3b98..a6e0d579 100644 --- a/TactilityKernel/Include/tactility/concurrent/mutex.h +++ b/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; diff --git a/TactilityKernel/Source/device.cpp b/TactilityKernel/Source/device.cpp index 4d5012a9..cba6175d 100644 --- a/TactilityKernel/Source/device.cpp +++ b/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 children; + std::vector 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; } diff --git a/TactilityKernel/Source/driver.cpp b/TactilityKernel/Source/driver.cpp index f6df3afe..c8da9169 100644 --- a/TactilityKernel/Source/driver.cpp +++ b/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 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(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; } diff --git a/TactilityKernel/Source/module.cpp b/TactilityKernel/Source/module.cpp index 1105d971..c84b1f8a 100644 --- a/TactilityKernel/Source/module.cpp +++ b/TactilityKernel/Source/module.cpp @@ -13,7 +13,7 @@ struct ModuleInternal { struct ModuleLedger { std::vector modules; - struct Mutex mutex = { 0 }; + struct Mutex mutex {}; ModuleLedger() { mutex_construct(&mutex); } ~ModuleLedger() { mutex_destruct(&mutex); } diff --git a/Tests/TactilityKernel/Source/DeviceTest.cpp b/Tests/TactilityKernel/Source/DeviceTest.cpp index 819c5fe9..897eda9c 100644 --- a/Tests/TactilityKernel/Source/DeviceTest.cpp +++ b/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);