Refactor Device and Driver

This commit is contained in:
Ken Van Hoeylandt 2026-02-06 00:18:19 +01:00
parent 1de6f52604
commit 73b85e0f86
13 changed files with 93 additions and 106 deletions

View File

@ -120,7 +120,7 @@ Driver hal_device_driver = {
.api = nullptr,
.device_type = &HAL_DEVICE_TYPE,
.owner = &hal_device_module,
.driver_private = nullptr
.internal = nullptr
};
}

View File

@ -123,7 +123,7 @@ Driver esp32_gpio_driver = {
.api = (void*)&esp32_gpio_api,
.device_type = &GPIO_CONTROLLER_TYPE,
.owner = &platform_module,
.driver_private = nullptr
.internal = nullptr
};
} // extern "C"

View File

@ -216,7 +216,7 @@ Driver esp32_i2c_driver = {
.api = (void*)&esp32_i2c_api,
.device_type = &I2C_CONTROLLER_TYPE,
.owner = &platform_module,
.driver_private = nullptr
.internal = nullptr
};
} // extern "C"

View File

@ -236,7 +236,7 @@ Driver esp32_i2s_driver = {
.api = (void*)&esp32_i2s_api,
.device_type = &I2S_CONTROLLER_TYPE,
.owner = &platform_module,
.driver_private = nullptr
.internal = nullptr
};
} // extern "C"

View File

@ -16,7 +16,7 @@ extern "C" {
#endif
struct Driver;
struct DevicePrivate;
struct DeviceInternal;
/** Enables discovering devices of the same type */
struct DeviceType {
@ -33,22 +33,7 @@ struct Device {
/** The parent device that this device belongs to. Can be NULL, but only the root device should have a NULL parent. */
struct Device* parent;
/** Internal data */
struct {
/** Address of the API exposed by the device instance. */
struct Driver* driver;
/** The driver data for this device (e.g. a mutex) */
void* driver_data;
/** The mutex for device operations */
struct Mutex mutex;
/** The device state */
struct {
int start_result;
bool started : 1;
bool added : 1;
} state;
/** Private data */
struct DevicePrivate* device_private;
} internal;
struct DeviceInternal* internal;
};
/**

View File

@ -12,7 +12,7 @@ extern "C" {
struct Device;
struct DeviceType;
struct Module;
struct DriverPrivate;
struct DriverInternal;
struct Driver {
/** The driver name */
@ -30,7 +30,7 @@ struct Driver {
/** The module that owns this driver. When it is NULL, the system owns the driver and it cannot be removed from registration. */
const struct Module* owner;
/** Internal data */
struct DriverPrivate* driver_private;
struct DriverInternal* internal;
};
/**

View File

@ -25,6 +25,8 @@ struct ModuleSymbol {
const void* symbol;
};
struct ModuleInternal;
/**
* A module is a collection of drivers or other functionality that can be loaded and unloaded at runtime.
*/
@ -59,7 +61,7 @@ struct Module {
*/
const struct ModuleSymbol* symbols;
void* internal;
struct ModuleInternal* internal;
};
/**

View File

@ -13,7 +13,20 @@
#define TAG "device"
struct DevicePrivate {
struct DeviceInternal {
/** Address of the API exposed by the device instance. */
struct Driver* driver;
/** The driver data for this device (e.g. a mutex) */
void* driver_data;
/** The mutex for device operations */
struct Mutex mutex;
/** The device state */
struct {
int start_result;
bool started : 1;
bool added : 1;
} state;
/** Attached child devices */
std::vector<Device*> children;
};
@ -42,47 +55,44 @@ extern "C" {
#define ledger_lock() mutex_lock(&ledger.mutex)
#define ledger_unlock() mutex_unlock(&ledger.mutex)
#define get_device_private(device) static_cast<DevicePrivate*>(device->internal.device_private)
error_t device_construct(Device* device) {
device->internal.device_private = new(std::nothrow) DevicePrivate;
if (device->internal.device_private == nullptr) {
device->internal = new(std::nothrow) DeviceInternal;
if (device->internal == nullptr) {
return ERROR_OUT_OF_MEMORY;
}
LOG_D(TAG, "construct %s", device->name);
mutex_construct(&device->internal.mutex);
mutex_construct(&device->internal->mutex);
return ERROR_NONE;
}
error_t device_destruct(Device* device) {
if (device->internal.state.started || device->internal.state.added) {
if (device->internal->state.started || device->internal->state.added) {
return ERROR_INVALID_STATE;
}
if (!get_device_private(device)->children.empty()) {
if (!device->internal->children.empty()) {
return ERROR_INVALID_STATE;
}
LOG_D(TAG, "destruct %s", device->name);
mutex_destruct(&device->internal.mutex);
delete get_device_private(device);
device->internal.device_private = nullptr;
mutex_destruct(&device->internal->mutex);
delete device->internal;
device->internal = nullptr;
return ERROR_NONE;
}
/** Add a child to the list of children */
static void device_add_child(struct Device* device, struct Device* child) {
device_lock(device);
assert(device->internal.state.added);
get_device_private(device)->children.push_back(child);
check(device->internal->state.added);
device->internal->children.push_back(child);
device_unlock(device);
}
/** Remove a child from the list of children */
static void device_remove_child(struct Device* device, struct Device* child) {
device_lock(device);
auto* parent_data = get_device_private(device);
const auto iterator = std::ranges::find(parent_data->children, child);
if (iterator != parent_data->children.end()) {
parent_data->children.erase(iterator);
const auto iterator = std::ranges::find(device->internal->children, child);
if (iterator != device->internal->children.end()) {
device->internal->children.erase(iterator);
}
device_unlock(device);
}
@ -91,7 +101,7 @@ error_t device_add(Device* device) {
LOG_D(TAG, "add %s", device->name);
// Already added
if (device->internal.state.started || device->internal.state.added) {
if (device->internal->state.started || device->internal->state.added) {
return ERROR_INVALID_STATE;
}
@ -106,14 +116,14 @@ error_t device_add(Device* device) {
device_add_child(parent, device);
}
device->internal.state.added = true;
device->internal->state.added = true;
return ERROR_NONE;
}
error_t device_remove(Device* device) {
LOG_D(TAG, "remove %s", device->name);
if (device->internal.state.started || !device->internal.state.added) {
if (device->internal->state.started || !device->internal->state.added) {
return ERROR_INVALID_STATE;
}
@ -132,7 +142,7 @@ error_t device_remove(Device* device) {
ledger.devices.erase(iterator);
ledger_unlock();
device->internal.state.added = false;
device->internal->state.added = false;
return ERROR_NONE;
failed_ledger_lookup:
@ -147,41 +157,41 @@ failed_ledger_lookup:
error_t device_start(Device* device) {
LOG_I(TAG, "start %s", device->name);
if (!device->internal.state.added) {
if (!device->internal->state.added) {
return ERROR_INVALID_STATE;
}
if (device->internal.driver == nullptr) {
if (device->internal->driver == nullptr) {
return ERROR_INVALID_STATE;
}
// Already started
if (device->internal.state.started) {
if (device->internal->state.started) {
return ERROR_NONE;
}
error_t bind_error = driver_bind(device->internal.driver, device);
device->internal.state.started = (bind_error == ERROR_NONE);
device->internal.state.start_result = bind_error;
error_t bind_error = driver_bind(device->internal->driver, device);
device->internal->state.started = (bind_error == ERROR_NONE);
device->internal->state.start_result = bind_error;
return bind_error == ERROR_NONE ? ERROR_NONE : ERROR_RESOURCE;
}
error_t device_stop(struct Device* device) {
LOG_I(TAG, "stop %s", device->name);
if (!device->internal.state.added) {
if (!device->internal->state.added) {
return ERROR_INVALID_STATE;
}
if (!device->internal.state.started) {
if (!device->internal->state.started) {
return ERROR_NONE;
}
if (driver_unbind(device->internal.driver, device) != ERROR_NONE) {
if (driver_unbind(device->internal->driver, device) != ERROR_NONE) {
return ERROR_RESOURCE;
}
device->internal.state.started = false;
device->internal.state.start_result = 0;
device->internal->state.started = false;
device->internal->state.start_result = 0;
return ERROR_NONE;
}
@ -236,7 +246,7 @@ on_construct_add_error:
}
void device_set_parent(Device* device, Device* parent) {
assert(!device->internal.state.started);
assert(!device->internal->state.started);
device->parent = parent;
}
@ -245,43 +255,43 @@ Device* device_get_parent(struct Device* device) {
}
void device_set_driver(struct Device* device, struct Driver* driver) {
device->internal.driver = driver;
device->internal->driver = driver;
}
struct Driver* device_get_driver(struct Device* device) {
return device->internal.driver;
return device->internal->driver;
}
bool device_is_ready(const struct Device* device) {
return device->internal.state.started;
return device->internal->state.started;
}
void device_set_driver_data(struct Device* device, void* driver_data) {
device->internal.driver_data = driver_data;
device->internal->driver_data = driver_data;
}
void* device_get_driver_data(struct Device* device) {
return device->internal.driver_data;
return device->internal->driver_data;
}
bool device_is_added(const struct Device* device) {
return device->internal.state.added;
return device->internal->state.added;
}
void device_lock(struct Device* device) {
mutex_lock(&device->internal.mutex);
mutex_lock(&device->internal->mutex);
}
bool device_try_lock(struct Device* device) {
return mutex_try_lock(&device->internal.mutex);
return mutex_try_lock(&device->internal->mutex);
}
void device_unlock(struct Device* device) {
mutex_unlock(&device->internal.mutex);
mutex_unlock(&device->internal->mutex);
}
const struct DeviceType* device_get_type(struct Device* device) {
return device->internal.driver ? device->internal.driver->device_type : NULL;
return device->internal->driver ? device->internal->driver->device_type : NULL;
}
void device_for_each(void* callback_context, bool(*on_device)(Device* device, void* context)) {
@ -295,8 +305,7 @@ void device_for_each(void* callback_context, bool(*on_device)(Device* device, vo
}
void device_for_each_child(Device* device, void* callbackContext, bool(*on_device)(struct Device* device, void* context)) {
auto* data = get_device_private(device);
for (auto* child_device : data->children) {
for (auto* child_device : device->internal->children) {
if (!on_device(child_device, callbackContext)) {
break;
}
@ -306,7 +315,7 @@ void device_for_each_child(Device* device, void* callbackContext, bool(*on_devic
void device_for_each_of_type(const DeviceType* type, void* callbackContext, bool(*on_device)(Device* device, void* context)) {
ledger_lock();
for (auto* device : ledger.devices) {
auto* driver = device->internal.driver;
auto* driver = device->internal->driver;
if (driver != nullptr) {
if (driver->device_type == type) {
if (!on_device(device, callbackContext)) {

View File

@ -12,16 +12,16 @@
#define TAG "driver"
struct DriverPrivate {
struct DriverInternal {
Mutex mutex { 0 };
int use_count = 0;
bool destroying = false;
DriverPrivate() {
DriverInternal() {
mutex_construct(&mutex);
}
~DriverPrivate() {
~DriverInternal() {
mutex_destruct(&mutex);
}
};
@ -44,15 +44,15 @@ static DriverLedger& get_ledger() {
#define ledger get_ledger()
#define get_driver_private(driver) static_cast<DriverPrivate*>(driver->driver_private)
#define get_driver_private(driver) static_cast<DriverInternal*>(driver->internal)
#define driver_lock(driver) mutex_lock(&get_driver_private(driver)->mutex);
#define driver_unlock(driver) mutex_unlock(&get_driver_private(driver)->mutex);
extern "C" {
error_t driver_construct(Driver* driver) {
driver->driver_private = new(std::nothrow) DriverPrivate;
if (driver->driver_private == nullptr) {
driver->internal = new(std::nothrow) DriverInternal;
if (driver->internal == nullptr) {
return ERROR_OUT_OF_MEMORY;
}
return ERROR_NONE;
@ -73,7 +73,7 @@ error_t driver_destruct(Driver* driver) {
driver_unlock(driver);
delete get_driver_private(driver);
driver->driver_private = nullptr;
driver->internal = nullptr;
return ERROR_NONE;
}

View File

@ -12,26 +12,17 @@ static Module module = {
.stop = nullptr
};
TEST_CASE("device_construct and device_destruct should set and unset the correct fields") {
TEST_CASE("device_construct and device_destruct should set and unset the internal field") {
Device device = { 0 };
error_t error = device_construct(&device);
CHECK_EQ(error, ERROR_NONE);
CHECK_NE(device.internal.device_private, nullptr);
CHECK_NE(device.internal.mutex.handle, nullptr);
CHECK_NE(device.internal, nullptr);
CHECK_EQ(device_destruct(&device), ERROR_NONE);
CHECK_EQ(device.internal.device_private, nullptr);
CHECK_EQ(device.internal.mutex.handle, nullptr);
Device comparison_device = { 0 };
comparison_device.internal.device_private = device.internal.device_private;
comparison_device.internal.mutex.handle = device.internal.mutex.handle;
// Check that no other data was set
CHECK_EQ(memcmp(&device, &comparison_device, sizeof(Device)), 0);
CHECK_EQ(device.internal, nullptr);
}
TEST_CASE("device_add should add the device to the list of all devices") {
@ -91,9 +82,9 @@ TEST_CASE("device_add should set the state to 'added'") {
Device device = { 0 };
CHECK_EQ(device_construct(&device), ERROR_NONE);
CHECK_EQ(device.internal.state.added, false);
CHECK_EQ(device_is_added(&device), false);
CHECK_EQ(device_add(&device), ERROR_NONE);
CHECK_EQ(device.internal.state.added, true);
CHECK_EQ(device_is_added(&device), true);
CHECK_EQ(device_remove(&device), ERROR_NONE);
CHECK_EQ(device_destruct(&device), ERROR_NONE);
@ -155,9 +146,9 @@ TEST_CASE("device_remove should clear the state 'added'") {
CHECK_EQ(device_construct(&device), ERROR_NONE);
CHECK_EQ(device_add(&device), ERROR_NONE);
CHECK_EQ(device.internal.state.added, true);
CHECK_EQ(device_is_added(&device), true);
CHECK_EQ(device_remove(&device), ERROR_NONE);
CHECK_EQ(device.internal.state.added, false);
CHECK_EQ(device_is_added(&device), false);
CHECK_EQ(device_destruct(&device), ERROR_NONE);
}
@ -172,7 +163,7 @@ TEST_CASE("device_is_ready should return true only when it is started") {
.api = nullptr,
.device_type = nullptr,
.owner = &module,
.driver_private = nullptr
.internal = nullptr
};
Device device = { 0 };
@ -180,17 +171,17 @@ TEST_CASE("device_is_ready should return true only when it is started") {
CHECK_EQ(driver_construct_add(&driver), ERROR_NONE);
CHECK_EQ(device_construct(&device), ERROR_NONE);
CHECK_EQ(device.internal.state.started, false);
CHECK_EQ(device_is_ready(&device), false);
device_set_driver(&device, &driver);
CHECK_EQ(device.internal.state.started, false);
CHECK_EQ(device_is_ready(&device), false);
CHECK_EQ(device_add(&device), ERROR_NONE);
CHECK_EQ(device.internal.state.started, false);
CHECK_EQ(device_is_ready(&device), false);
CHECK_EQ(device_start(&device), ERROR_NONE);
CHECK_EQ(device.internal.state.started, true);
CHECK_EQ(device_is_ready(&device), true);
CHECK_EQ(device_stop(&device), ERROR_NONE);
CHECK_EQ(device.internal.state.started, false);
CHECK_EQ(device_is_ready(&device), false);
CHECK_EQ(device_remove(&device), ERROR_NONE);
CHECK_EQ(device.internal.state.started, false);
CHECK_EQ(device_is_ready(&device), false);
CHECK_EQ(device_destruct(&device), ERROR_NONE);
CHECK_EQ(driver_remove_destruct(&driver), ERROR_NONE);

View File

@ -38,7 +38,7 @@ static Driver integration_driver = {
.api = nullptr,
.device_type = nullptr,
.owner = &module,
.driver_private = nullptr,
.internal = nullptr,
};
TEST_CASE("driver with with start success and stop success should start and stop a device") {

View File

@ -15,10 +15,10 @@ TEST_CASE("driver_construct and driver_destruct should set and unset the correct
CHECK_EQ(driver_construct(&driver), ERROR_NONE);
CHECK_EQ(driver_add(&driver), ERROR_NONE);
CHECK_NE(driver.driver_private, nullptr);
CHECK_NE(driver.internal, nullptr);
CHECK_EQ(driver_remove(&driver), ERROR_NONE);
CHECK_EQ(driver_destruct(&driver), ERROR_NONE);
CHECK_EQ(driver.driver_private, nullptr);
CHECK_EQ(driver.internal, nullptr);
}
TEST_CASE("a driver without a module should not be destructible") {
@ -40,7 +40,7 @@ TEST_CASE("driver_is_compatible should return true if a compatible value is foun
.api = nullptr,
.device_type = nullptr,
.owner = &module,
.driver_private = nullptr
.internal = nullptr
};
CHECK_EQ(driver_is_compatible(&driver, "test_compatible"), true);
CHECK_EQ(driver_is_compatible(&driver, "nope"), false);
@ -57,7 +57,7 @@ TEST_CASE("driver_find should only find a compatible driver when the driver was
.api = nullptr,
.device_type = nullptr,
.owner = &module,
.driver_private = nullptr
.internal = nullptr
};
Driver* found_driver = driver_find_compatible("test_compatible");

View File

@ -28,7 +28,7 @@ TEST_CASE("Module construction and destruction") {
// Test successful construction
CHECK_EQ(module_construct(&module), ERROR_NONE);
CHECK_EQ(module.internal.started, false);
CHECK_EQ(module_is_started(&module), false);
// Test successful destruction
CHECK_EQ(module_destruct(&module), ERROR_NONE);