Kernel improvements #468
Kernel improvements #468
Conversation
📝 WalkthroughWalkthroughDevicetree generator now emits a null‑terminated 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (5)
TactilityKernel/Include/tactility/driver.h (1)
28-31: Add a forward declaration forstruct Modulefor clarity.This keeps the header consistent with the existing
Device/DeviceTypeforward declarations.♻️ Suggested tweak
struct Device; struct DeviceType; +struct Module;TactilityKernel/Include/tactility/module.h (1)
12-41: Clarify thatModulemust be zero-initialized before use.Without an init helper, stack-allocated modules can leave
internal.started/parentuninitialized, which affectsmodule_set_parent()andmodule_start()behavior.📝 Suggested doc addition
/** * A module is a collection of drivers or other functionality that can be loaded and unloaded at runtime. + * Must be zero-initialized (e.g., = {0}) before use. */ struct Module {Tactility/Include/Tactility/Tactility.h (1)
20-23: Document newrun()parameters (especially devicetree termination).The public header now exposes new parameters, but the comment doesn’t explain expectations (non-null, array termination). This is easy to misuse for external callers.
Proposed fix
/** * Attempts to initialize Tactility and all configured hardware. + * `@param` platformModule Platform module to start (non-null). + * `@param` deviceModule Device module to start (non-null). + * `@param` devicetreeDevices Null-terminated array (device == nullptr marks end). */ void run(const Configuration& config, Module* platformModule, Module* deviceModule, CompatibleDevice devicetreeDevices[]);TactilityKernel/Include/tactility/kernel_init.h (1)
11-16: Clarify devicetree array termination in the API doc.
kernel_inititerates untildevice == nullptr(TactilityKernel/Source/kernel_init.cpp, Lines 17-53). Please document the required terminator to prevent overruns.Proposed fix
- * `@param` devicetree_devices The list of generated devices from the devicetree. + * `@param` devicetree_devices The list of generated devices from the devicetree. + * The array must be terminated by an entry where device == nullptr.TactilityKernel/Source/kernel_init.cpp (1)
21-48: Consider cleanup/rollback on failure.
If initialization fails after partial progress, the kernel module parent, drivers, or already-started modules/devices may be left in an inconsistent state. Consider stopping what was started and destructingkernel_module_parentbefore returning.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (31)
Devices/elecrow-crowpanel-advance-35/Source/module.cpp (1)
17-17: Consider using a device-specific module name.The name
"Module"is generic. For better identification in logs and debugging, consider using a descriptive name like"elecrow-crowpanel-advance-35"that identifies which device this module belongs to.struct Module device_module = { - .name = "Module", + .name = "elecrow-crowpanel-advance-35", .start = start, .stop = stop };Devices/cyd-e32r32p/Source/module.cpp (1)
16-19: Consider using a device-specific module name for better diagnostics.The module name
"Module"is generic and doesn't identify this specific device. For debugging and logging purposes, a more descriptive name like"cyd-e32r32p"would help distinguish this module from others.That said, the relevant code snippets show this same generic naming pattern is used consistently across all device modules, so this may be intentional.
💡 Optional: Use device-specific name
struct Module device_module = { - .name = "Module", + .name = "cyd-e32r32p", .start = start, .stop = stop };Devices/waveshare-s3-touch-lcd-128/Source/module.cpp (2)
15-17: Consider a more descriptive module name.The name
"Module"is generic. For easier debugging and logging, consider using a device-specific name like"waveshare-s3-touch-lcd-128"to distinguish this module from others.💡 Suggested improvement
/** `@warning` The variable name must be exactly "device_module" */ struct Module device_module = { - .name = "Module", + .name = "waveshare-s3-touch-lcd-128", .start = start, .stop = stop };
15-15: Doxygen syntax: use@warninginstead of@warn.The correct Doxygen tag is
@warning, not@warn.📝 Suggested fix
-/** `@warn` The variable name must be exactly "device_module" */ +/** `@warning` The variable name must be exactly "device_module" */Devices/m5stack-stickc-plus/Source/module.cpp (1)
16-17: Consider using a more descriptive module name.The module name
"Module"is generic and may cause confusion in logs or debugging when multiple device modules are loaded. Consider using a device-specific name like"m5stack-stickc-plus"to match the device this module represents.💡 Suggested improvement
struct Module device_module = { - .name = "Module", + .name = "m5stack-stickc-plus", .start = start, .stop = stop };Devices/m5stack-cardputer-adv/Source/module.cpp (1)
15-17: Consider using a more descriptive module name.The
.nameis set to the generic string"Module". For better debuggability and logging clarity, consider using a device-specific name like"m5stack-cardputer-adv"to distinguish this module from others.💡 Suggested change
/** `@warn` The variable name must be exactly "device_module" */ struct Module device_module = { - .name = "Module", + .name = "m5stack-cardputer-adv", .start = start, .stop = stop };Devices/guition-jc8048w550c/Source/module.cpp (2)
16-20: Consider using a device-specific module name instead of "Module".The name
"Module"is generic and may not uniquely identify this device in logs or debugging contexts. A device-specific name like"guition-jc8048w550c"would improve traceability and consistency.Suggested change
struct Module device_module = { - .name = "Module", + .name = "guition-jc8048w550c", .start = start, .stop = stop };
15-15: Use standard Doxygen tag@warninginstead of@warn.The
@warntag is not recognized by Doxygen; the correct tag is@warning.Suggested change
-/** `@warn` The variable name must be exactly "device_module" */ +/** `@warning` The variable name must be exactly "device_module" */Devices/guition-jc1060p470ciwy/Source/module.cpp (1)
15-19: Consider a more descriptive module name for diagnostics.Right now
.name = "Module"is generic; a device-specific name helps logs and module listings without changing behavior.♻️ Example tweak
struct Module device_module = { - .name = "Module", + .name = "guition-jc1060p470ciwy", .start = start, .stop = stop };Devices/generic-esp32c6/Source/module.cpp (1)
15-20: Consider using a device-specific module name.The
.namefield is set to the generic"Module"string. For better debuggability and logging, consider using a more descriptive name like"Generic ESP32-C6"that identifies the device, similar to howDevices/lilygo-tlora-pager/Source/module.cppuses"LilyGO T-Lora Pager".Suggested change
/** `@warn` The variable name must be exactly "device_module" */ struct Module device_module = { - .name = "Module", + .name = "Generic ESP32-C6", .start = start, .stop = stop };Devices/cyd-2432s028rv3/Source/module.cpp (1)
15-20: Consider using a device-specific module name.The
.namefield uses"Module"which is generic. Consider using a descriptive name like"CYD-2432S028RV3"for better identification in logs and debugging.Suggested change
/** `@warn` The variable name must be exactly "device_module" */ struct Module device_module = { - .name = "Module", + .name = "CYD-2432S028RV3", .start = start, .stop = stop };Devices/guition-jc2432w328c/Source/module.cpp (1)
15-20: Consider using a device-specific module name.Same as other device modules, consider using a descriptive name like
"Guition JC2432W328C"instead of the generic"Module".Suggested change
/** `@warn` The variable name must be exactly "device_module" */ struct Module device_module = { - .name = "Module", + .name = "Guition JC2432W328C", .start = start, .stop = stop };Devices/cyd-2432s024c/Source/module.cpp (1)
15-20: Consider using a device-specific module name.Consider using a descriptive name like
"CYD-2432S024C"instead of"Module"for better identification.Suggested change
/** `@warn` The variable name must be exactly "device_module" */ struct Module device_module = { - .name = "Module", + .name = "CYD-2432S024C", .start = start, .stop = stop };Devices/wireless-tag-wt32-sc01-plus/Source/module.cpp (1)
15-20: Consider using a device-specific module name.Consider using a descriptive name like
"Wireless Tag WT32-SC01 Plus"instead of the generic"Module"for consistency withDevices/lilygo-tlora-pager/Source/module.cppwhich uses"LilyGO T-Lora Pager".Suggested change
/** `@warn` The variable name must be exactly "device_module" */ struct Module device_module = { - .name = "Module", + .name = "Wireless Tag WT32-SC01 Plus", .start = start, .stop = stop };Devices/unphone/Source/module.cpp (1)
16-20: Consider using a device-specific name for better observability.The module name
"Module"is generic. Using a device-specific name like"unPhone"would improve debugging and logging when multiple devices are involved.Suggested change
struct Module device_module = { - .name = "Module", + .name = "unPhone", .start = start, .stop = stop };Devices/generic-esp32/Source/module.cpp (1)
16-20: Consider using a device-specific name for better observability.Using
"Generic ESP32"instead of"Module"would make logs and debugging clearer.Suggested change
struct Module device_module = { - .name = "Module", + .name = "Generic ESP32", .start = start, .stop = stop };Devices/cyd-4848s040c/Source/module.cpp (1)
16-20: Consider using a device-specific name for better observability.Using
"CYD-4848S040C"instead of"Module"would improve traceability.Suggested change
struct Module device_module = { - .name = "Module", + .name = "CYD-4848S040C", .start = start, .stop = stop };Devices/elecrow-crowpanel-basic-50/Source/module.cpp (1)
16-20: Consider using a device-specific name for better observability.Using
"Elecrow Crowpanel Basic 50"instead of"Module"would improve traceability.Suggested change
struct Module device_module = { - .name = "Module", + .name = "Elecrow Crowpanel Basic 50", .start = start, .stop = stop };Devices/elecrow-crowpanel-advance-50/Source/module.cpp (1)
16-20: Consider using a device-specific name for better observability.Using
"Elecrow Crowpanel Advance 50"instead of"Module"would improve traceability.Suggested change
struct Module device_module = { - .name = "Module", + .name = "Elecrow Crowpanel Advance 50", .start = start, .stop = stop };Devices/m5stack-cores3/Source/module.cpp (1)
16-20: Consider using a device-specific name for better observability.Using
"M5Stack CoreS3"instead of"Module"would improve traceability.Suggested change
struct Module device_module = { - .name = "Module", + .name = "M5Stack CoreS3", .start = start, .stop = stop };Devices/m5stack-core2/Source/module.cpp (1)
16-20: Consider using a device-specific module name for better observability.The
.name = "Module"is generic and identical across all device modules. Using a device-specific name (e.g.,"m5stack-core2") would improve debugging and logging clarity when multiple modules are in use.Suggested change
struct Module device_module = { - .name = "Module", + .name = "m5stack-core2", .start = start, .stop = stop };Devices/waveshare-s3-touch-lcd-147/Source/module.cpp (1)
16-20: Consider using a descriptive module name.The generic name
"Module"provides no diagnostic value when debugging or logging. Other devices in this PR (e.g.,btt-panda-touch) use descriptive names like"BigTreeTech Panda Touch / K Touch".Proposed fix
struct Module device_module = { - .name = "Module", + .name = "Waveshare S3 Touch LCD 1.47", .start = start, .stop = stop };Devices/generic-esp32s3/Source/module.cpp (1)
16-20: Consider using a descriptive module name.Using
"Generic ESP32-S3"or similar would aid debugging and logging.Proposed fix
struct Module device_module = { - .name = "Module", + .name = "Generic ESP32-S3", .start = start, .stop = stop };Devices/cyd-2432s032c/Source/module.cpp (1)
16-20: Consider using a descriptive module name.A device-specific name like
"CYD 2432S032C"would improve debuggability.Proposed fix
struct Module device_module = { - .name = "Module", + .name = "CYD 2432S032C", .start = start, .stop = stop };Devices/elecrow-crowpanel-basic-28/Source/module.cpp (1)
16-20: Consider using a descriptive module name.A device-specific name like
"Elecrow CrowPanel Basic 2.8"would improve debuggability.Proposed fix
struct Module device_module = { - .name = "Module", + .name = "Elecrow CrowPanel Basic 2.8", .start = start, .stop = stop };Devices/m5stack-cardputer/Source/module.cpp (1)
16-20: Consider using a descriptive module name.A device-specific name like
"M5Stack Cardputer"would improve debuggability.Proposed fix
struct Module device_module = { - .name = "Module", + .name = "M5Stack Cardputer", .start = start, .stop = stop };Devices/generic-esp32p4/Source/module.cpp (1)
16-20: Consider using a descriptive module name.A device-specific name like
"Generic ESP32-P4"would improve debuggability.Proposed fix
struct Module device_module = { - .name = "Module", + .name = "Generic ESP32-P4", .start = start, .stop = stop };Devices/heltec-wifi-lora-32-v3/Source/module.cpp (1)
15-20: Consider using a device-specific module name.The
.namefield is set to the generic string"Module", which won't help distinguish this device's module in logs or debugging output. Consider using the device name (e.g.,"heltec-wifi-lora-32-v3") for better observability.💡 Suggested improvement
/** `@warn` The variable name must be exactly "device_module" */ struct Module device_module = { - .name = "Module", + .name = "heltec-wifi-lora-32-v3", .start = start, .stop = stop };Devices/lilygo-tdongle-s3/Source/module.cpp (1)
15-20: Consider using a device-specific module name.Same as other device modules - the
.namefield uses generic"Module"instead of the device identifier. Using"lilygo-tdongle-s3"would improve debugging/logging clarity.TactilityKernel/Include/tactility/module.h (2)
54-68: Use standard Doxygen tag@warninginstead of@warn.The
@warntag is not a standard Doxygen command. Use@warningfor proper documentation generation.📝 Suggested fix
/** * `@brief` Initialize the module parent. - * `@warn` This function does no validation on input or state. + * `@warning` This function does no validation on input or state. * `@param` parent parent module * `@return` ERROR_NONE if successful, ERROR_OUT_OF_MEMORY if allocation fails */ error_t module_parent_construct(struct ModuleParent* parent); /** * `@brief` Deinitialize the module parent. Must have no children when calling this. - * `@warn` This function does no validation on input. + * `@warning` This function does no validation on input. * `@param` parent parent module * `@return` ERROR_NONE if successful or ERROR_INVALID_STATE if the parent has children */ error_t module_parent_destruct(struct ModuleParent* parent);
37-41: Internal state is publicly visible.The
internalstruct exposesstartedandparentfields publicly, allowing callers to bypass the lifecycle API and directly manipulate module state. This could lead to inconsistent state if misused.If this is intentional for C compatibility or low-level embedded use, consider adding a documentation comment warning against direct manipulation of these fields.
📝 Suggested documentation addition
+ /** + * Internal state - do not modify directly. Use module_* functions instead. + */ struct { bool started; struct ModuleParent* parent; } internal;
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TactilityKernel/Source/driver.cpp (1)
173-190:⚠️ Potential issue | 🟡 MinorPrevent
use_countunderflow on unbind.
Callingdriver_unbindwithout a prior successful bind drivesuse_countnegative and can permanently block teardown.🔧 Suggested guard
- if (get_driver_private(driver)->destroying || !device_is_added(device)) { + auto* priv = get_driver_private(driver); + if (priv->destroying || !device_is_added(device)) { error = ERROR_INVALID_STATE; goto error; } + if (priv->use_count == 0) { + error = ERROR_INVALID_STATE; + goto error; + } @@ - get_driver_private(driver)->use_count--; + priv->use_count--;
🧹 Nitpick comments (19)
Devices/cyd-8048s043c/Source/module.cpp (1)
1-22: LGTM! Structure matches the established module pattern across devices.The implementation is consistent with other device modules (e.g.,
btt-panda-touch,cyd-2432s024c).One optional nit: the
.name = "Module"is generic. Consider using a device-specific name like"cyd-8048s043c"to aid debugging and logging.💡 Optional: Use device-specific module name
struct Module device_module = { - .name = "Module", + .name = "cyd-8048s043c", .start = start, .stop = stop };Regarding the static analysis hint about
'tactility/module.h' file not found: this is a false positive as the header is part of the module system introduced in this PR and is resolved via the build system's include paths.,
Devices/cyd-2432s024c/Source/module.cpp (1)
15-20: Consider using a device-specific module name.The
.name = "Module"is generic and won't help identify this module during debugging, logging, or runtime inspection. A more descriptive name would improve observability.♻️ Suggested improvement
/** `@warning` The variable name must be exactly "device_module" */ struct Module device_module = { - .name = "Module", + .name = "cyd-2432s024c", .start = start, .stop = stop };Devices/elecrow-crowpanel-advance-28/Source/module.cpp (1)
15-20: Consider using a descriptive module name.The
.name = "Module"is generic. If multiple device modules all share this same name, it could make debugging or logging output confusing when identifying which module is involved.Consider using a device-specific name:
struct Module device_module = { - .name = "Module", + .name = "elecrow-crowpanel-advance-28", .start = start, .stop = stop };Devices/cyd-e32r32p/Source/module.cpp (1)
15-20: Use a device-specific module name for better identification.The
.name = "Module"is too generic and won't help identify this device during debugging or logging. Consider using the device identifier.♻️ Suggested improvement
/** `@warning` The variable name must be exactly "device_module" */ struct Module device_module = { - .name = "Module", + .name = "cyd-e32r32p", .start = start, .stop = stop };Devices/elecrow-crowpanel-advance-50/Source/module.cpp (1)
16-19: Use a device-specific module name for clarity.Line 17’s
"Module"is very generic and can make logs or module registries ambiguous. Consider a device-specific name to avoid collisions and improve traceability.♻️ Suggested tweak
struct Module device_module = { - .name = "Module", + .name = "elecrow-crowpanel-advance-50", .start = start, .stop = stop };Devices/elecrow-crowpanel-basic-28/Source/module.cpp (1)
16-19: Use a device-specific module name for clearer diagnostics.A more descriptive
.namehelps logs and debugging when multiple modules exist.Suggested tweak
struct Module device_module = { - .name = "Module", + .name = "elecrow-crowpanel-basic-28", .start = start, .stop = stop };Devices/waveshare-s3-touch-lcd-128/Source/module.cpp (1)
15-19: Consider a descriptive module name for diagnostics.Using the device name here helps logs and module listings.
♻️ Suggested tweak
- .name = "Module", + .name = "waveshare-s3-touch-lcd-128",Platforms/PlatformEsp32/Source/module.cpp (1)
18-24: Consider reversing destruction order (LIFO pattern).The
stop()function destructs drivers in the same order they were constructed. Typically, resources should be released in reverse order of acquisition (LIFO) to handle potential dependencies correctly.♻️ Suggested change
static error_t stop() { /* We crash when destruct fails, because if a single driver fails to destruct, * there is no guarantee that the previously destroyed drivers can be recovered */ - check(driver_destruct(&esp32_gpio_driver) == ERROR_NONE); check(driver_destruct(&esp32_i2c_driver) == ERROR_NONE); + check(driver_destruct(&esp32_gpio_driver) == ERROR_NONE); return ERROR_NONE; }Devices/guition-jc8048w550c/Source/module.cpp (1)
16-20: Consider using a device-specific module name.The module name is set to the generic string
"Module". For better debugging and logging, consider using a device-specific name like"guition-jc8048w550c"that identifies this particular device.♻️ Suggested change
/** `@warning` The variable name must be exactly "device_module" */ struct Module device_module = { - .name = "Module", + .name = "guition-jc8048w550c", .start = start, .stop = stop };Devices/cyd-2432s028r/Source/module.cpp (1)
16-20: Consider using a device-specific module name.Same as other device modules - the generic name
"Module"should ideally be"cyd-2432s028r"for better traceability.Devices/guition-jc2432w328c/Source/module.cpp (1)
16-20: Consider using a device-specific module name.Same as other device modules - suggest using
"guition-jc2432w328c"instead of the generic"Module".Devices/waveshare-s3-lcd-13/Source/module.cpp (1)
16-20: Consider using a device-specific module name.Same as other device modules - suggest using
"waveshare-s3-lcd-13"instead of the generic"Module".Devices/m5stack-stickc-plus/Source/module.cpp (1)
16-20: Consider using a device-specific module name instead of generic "Module".All device modules use
.name = "Module", which could make debugging and logging harder when multiple modules are active. Consider using a descriptive name like"m5stack-stickc-plus"to aid in traceability.💡 Suggested improvement
struct Module device_module = { - .name = "Module", + .name = "m5stack-stickc-plus", .start = start, .stop = stop };Devices/waveshare-s3-touch-lcd-43/Source/module.cpp (1)
16-20: Consider using a device-specific module name.Same observation as other device modules - using
"waveshare-s3-touch-lcd-43"instead of generic"Module"would improve debuggability.Devices/generic-esp32s3/Source/module.cpp (1)
16-20: Consider using a device-specific module name.Using
"generic-esp32s3"instead of"Module"would aid debugging.TactilityKernel/Include/tactility/kernel_init.h (1)
3-9: Move includes outsideextern "C"block.Including headers inside
extern "C"can cause issues if those headers contain C++ constructs or have their ownextern "C"guards. Best practice is to include headers before the linkage specification.♻️ Proposed fix
`#pragma` once +#include <tactility/device.h> +#include <tactility/error.h> +#include <tactility/module.h> + `#ifdef` __cplusplus extern "C" { `#endif` -#include <tactility/device.h> -#include <tactility/error.h> -#include <tactility/module.h> - /** * Initialize the kernel with platform and device modules, and a device tree.Devices/generic-esp32/Source/module.cpp (1)
16-20: Consider using a device-specific module name.Using
"generic-esp32"instead of"Module"would aid debugging.Devices/m5stack-stickc-plus2/Source/module.cpp (1)
17-17: Consider using a more descriptive module name.The generic name
"Module"may make debugging and logging harder when multiple devices are involved. Consider using the device name (e.g.,"m5stack-stickc-plus2") for better traceability.Tests/TactilityKernel/ModuleTest.cpp (1)
4-16: Global test state may cause issues with parallel test execution.The static variables
test_start_result,start_called,test_stop_result, andstop_calledare shared across all tests. If tests run in parallel, this could lead to flaky test results. Consider using test fixtures or local state if the test framework supports parallel execution.
Summary by CodeRabbit
New Features
Refactor
Changes
✏️ Tip: You can customize this high-level summary in your review settings.