GPIO refactored #495
GPIO refactored #495
Conversation
📝 WalkthroughWalkthroughThis pull request refactors the GPIO control architecture from a device-pointer and pin-number based model to a descriptor-based abstraction. A new 🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 7
🧹 Nitpick comments (4)
TactilityC/Source/tt_hal_gpio.cpp (1)
1-4: Unused includes can be removed.With the function bodies stubbed out, headers
Gpio.h,device.h, andgpio_controller.hare no longer referenced. Onlytt_hal_gpio.his needed.Suggested cleanup
`#include` "tt_hal_gpio.h" -#include <Tactility/hal/gpio/Gpio.h> -#include <tactility/device.h> -#include <tactility/drivers/gpio_controller.h>TactilityKernel/Include/tactility/drivers/gpio.h (1)
58-59:gpio_level_tis defined but not used by the functional API.The descriptor struct stores
gpio_level_t level, but the actualset_level/get_levelAPIs usebool high. If this type is intended for caching the pin state in the descriptor, consider actually writing todescriptor->levelin the set/get implementations. Otherwise, thelevelfield inGpioDescriptorwill remain stale aftercallocinitialization.TactilityKernel/Include/tactility/drivers/gpio_descriptor.h (1)
1-20: Missingextern "C"guards for consistency with sibling headers.Other headers in this directory (
gpio.h,gpio_controller.h) wrap declarations inextern "C"blocks. While struct definitions don't strictly require linkage guards, adding them keeps the header consistent and avoids surprises if functions are added later.Proposed fix
`#pragma` once `#include` "gpio.h" +#ifdef __cplusplus +extern "C" { +#endif + struct GpioDescriptor { /** `@brief` The controller that owns this pin */ struct Device* controller; /** `@brief` Physical pin number */ gpio_pin_t pin; /** `@brief` Current owner */ enum GpioOwnerType owner_type; /** `@brief` Owner identity for validation */ void* owner_context; /** `@brief` Pin level */ gpio_level_t level; /** `@brief` Pin configuration flags */ gpio_flags_t flags; /** `@brief` Implementation-specific context (e.g. from esp32 controller internally) */ void* controller_context; }; + +#ifdef __cplusplus +} +#endifPlatforms/PlatformEsp32/Source/drivers/esp32_gpio.cpp (1)
94-98: Null guard contradicts project convention for internal APIs.
get_pin_numberadds a null check fordescriptor, but the project convention (Linux kernel style) is to not add null pointer guards in internal APIs — crashes on invalid pointers are bugs caught during development. The other descriptor-based functions in this file (e.g.,set_level,get_level) don't include such guards.Based on learnings, internal APIs should not add null pointer guards; crashes on invalid pointers are bugs detected during development.
Proposed fix
static error_t get_pin_number(struct GpioDescriptor* descriptor, gpio_pin_t* pin) { - if (!descriptor) return ERROR_INVALID_ARGUMENT; *pin = descriptor->pin; return ERROR_NONE; }
| bool tt_hal_gpio_get_level(GpioPin pin) { | ||
| Device* device_result = find_first_gpio_controller(); | ||
| if (device_result == nullptr) return false; | ||
| bool pin_state = false; | ||
| if (gpio_controller_get_level(device_result, pin, &pin_state) != ERROR_NONE) return false; | ||
| return pin_state; | ||
| return false; | ||
| } | ||
|
|
||
| int tt_hal_gpio_get_pin_count() { | ||
| Device* device_result = find_first_gpio_controller(); | ||
| if (device_result == nullptr) return 0; | ||
| uint32_t pin_count = 0; | ||
| if (gpio_controller_get_pin_count(device_result, &pin_count) != ERROR_NONE) return 0; | ||
| return (int)pin_count; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Stubbed functions silently return incorrect values — callers will get wrong behavior with no indication.
Returning hardcoded false / 0 makes these functions silently broken rather than properly deprecated. Any existing caller will observe incorrect GPIO state without any signal that the API is non-functional. This is worse than a loud failure.
Consider at minimum one of:
- Adding a
TT_LOG_W(or equivalent) so usage is visible at runtime. - Using a compile-time deprecation attribute (
[[deprecated("Use GpioDescriptor API")]]) on the declarations. - Removing the functions entirely if no callers remain.
Example: add runtime warning
bool tt_hal_gpio_get_level(GpioPin pin) {
+ TT_LOG_W("gpio", "tt_hal_gpio_get_level is deprecated and non-functional");
return false;
}
int tt_hal_gpio_get_pin_count() {
+ TT_LOG_W("gpio", "tt_hal_gpio_get_pin_count is deprecated and non-functional");
return 0;
}| /** | ||
| * @brief Gets the pin number associated with a descriptor. | ||
| * @param[in] descriptor the pin descriptor | ||
| * @param[out] pin pointer to store the pin number | ||
| * @return ERROR_NONE if successful | ||
| */ | ||
| error_t gpio_descriptor_pin_number(struct GpioDescriptor* descriptor, gpio_pin_t* pin); |
There was a problem hiding this comment.
Function name mismatch: declaration vs implementation.
This declares gpio_descriptor_pin_number but the implementation in gpio_controller.cpp defines gpio_descriptor_get_pin_number. This will cause a linker error for callers using the header name.
Pick one name and use it consistently across the header, implementation, and symbol table.
| /** | ||
| * @brief Gets the number of pins supported by the controller. | ||
| * @param[in] device the GPIO controller device | ||
| * @param[out] count pointer to store the number of pins | ||
| * @return ERROR_NONE if successful | ||
| */ | ||
| error_t gpio_controller_get_pin_count(struct Device* device, uint32_t* count); | ||
| * @brief Initializes GPIO descriptors for a controller. | ||
| * @param[in,out] device the GPIO controller device | ||
| * @param[in] owner_data pointer to store in the descriptor's owner_data field | ||
| * @return ERROR_NONE if successful | ||
| */ | ||
| error_t gpio_controller_init_descriptors(struct Device* device, uint32_t pin_count, void* owner_data); |
There was a problem hiding this comment.
Misleading parameter name and documentation for init_descriptors.
The header declares the third parameter as void* owner_data and the doc says "pointer to store in the descriptor's owner_data field," but the implementation names it controller_context and stores it in descriptor->controller_context. This is confusing — the doc suggests per-owner data, while the implementation treats it as controller-level context.
Align the parameter name and doc with the implementation:
Proposed fix
/**
- * `@brief` Initializes GPIO descriptors for a controller.
- * `@param`[in,out] device the GPIO controller device
- * `@param`[in] owner_data pointer to store in the descriptor's owner_data field
- * `@return` ERROR_NONE if successful
+ * `@brief` Initializes GPIO descriptors for a controller.
+ * `@param`[in,out] device the GPIO controller device
+ * `@param`[in] pin_count number of GPIO pins to allocate descriptors for
+ * `@param`[in] controller_context implementation-specific context stored in each descriptor
+ * `@return` ERROR_NONE if successful
*/
-error_t gpio_controller_init_descriptors(struct Device* device, uint32_t pin_count, void* owner_data);
+error_t gpio_controller_init_descriptors(struct Device* device, uint32_t pin_count, void* controller_context);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * @brief Gets the number of pins supported by the controller. | |
| * @param[in] device the GPIO controller device | |
| * @param[out] count pointer to store the number of pins | |
| * @return ERROR_NONE if successful | |
| */ | |
| error_t gpio_controller_get_pin_count(struct Device* device, uint32_t* count); | |
| * @brief Initializes GPIO descriptors for a controller. | |
| * @param[in,out] device the GPIO controller device | |
| * @param[in] owner_data pointer to store in the descriptor's owner_data field | |
| * @return ERROR_NONE if successful | |
| */ | |
| error_t gpio_controller_init_descriptors(struct Device* device, uint32_t pin_count, void* owner_data); | |
| /** | |
| * `@brief` Initializes GPIO descriptors for a controller. | |
| * `@param`[in,out] device the GPIO controller device | |
| * `@param`[in] pin_count number of GPIO pins to allocate descriptors for | |
| * `@param`[in] controller_context implementation-specific context stored in each descriptor | |
| * `@return` ERROR_NONE if successful | |
| */ | |
| error_t gpio_controller_init_descriptors(struct Device* device, uint32_t pin_count, void* controller_context); |
| struct GpioDescriptor* acquire_descriptor( | ||
| struct Device* controller, | ||
| gpio_pin_t pin_number, | ||
| enum GpioOwnerType owner, | ||
| void* owner_context | ||
| ) { | ||
| auto* data = (struct GpioControllerData*)device_get_driver_data(controller); | ||
|
|
||
| mutex_lock(&data->mutex); | ||
| if (pin_number >= data->pin_count) { | ||
| mutex_unlock(&data->mutex); | ||
| return nullptr; | ||
| } | ||
|
|
||
| struct GpioDescriptor* desc = &data->descriptors[pin_number]; | ||
| if (desc->owner_type != GPIO_OWNER_NONE) { | ||
| mutex_unlock(&data->mutex); | ||
| return nullptr; | ||
| } | ||
|
|
||
| desc->owner_type = owner; | ||
| desc->owner_context = owner_context; | ||
| mutex_unlock(&data->mutex); | ||
|
|
||
| return desc; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
acquire_descriptor and release_descriptor are overly generic names for public symbols.
These have external linkage and are declared in a public header. The names are highly collision-prone. Consider prefixing them with gpio_controller_ to match the naming convention of the other public functions in this file (e.g., gpio_controller_acquire_descriptor, gpio_controller_release_descriptor).
This would require updating the header declaration, the ESP32 driver API struct assignment, and the kernel symbol table.
Also applies to: 46-50, 65-70
| error_t release_descriptor(struct GpioDescriptor* descriptor) { | ||
| descriptor->owner_type = GPIO_OWNER_NONE; | ||
| descriptor->owner_context = nullptr; | ||
| return ERROR_NONE; | ||
| } |
There was a problem hiding this comment.
Race condition: release_descriptor writes shared state without the mutex.
acquire_descriptor reads desc->owner_type under data->mutex (line 34), but release_descriptor writes to the same fields without any synchronization. A concurrent acquire_descriptor call on the same pin could observe a partially-written state.
The descriptor holds a controller pointer, so you can retrieve the mutex:
Proposed fix
error_t release_descriptor(struct GpioDescriptor* descriptor) {
+ auto* data = (struct GpioControllerData*)device_get_driver_data(descriptor->controller);
+ mutex_lock(&data->mutex);
descriptor->owner_type = GPIO_OWNER_NONE;
descriptor->owner_context = nullptr;
+ mutex_unlock(&data->mutex);
return ERROR_NONE;
}| error_t gpio_controller_init_descriptors(struct Device* device, uint32_t pin_count, void* controller_context) { | ||
| auto* data = (struct GpioControllerData*)malloc(sizeof(struct GpioControllerData)); | ||
| if (!data) return ERROR_OUT_OF_MEMORY; | ||
|
|
||
| data->pin_count = pin_count; | ||
| data->descriptors = (struct GpioDescriptor*)calloc(pin_count, sizeof(struct GpioDescriptor)); | ||
| if (!data->descriptors) { | ||
| free(data); | ||
| return ERROR_OUT_OF_MEMORY; | ||
| } | ||
|
|
||
| for (uint32_t i = 0; i < pin_count; ++i) { | ||
| data->descriptors[i].controller = device; | ||
| data->descriptors[i].pin = (gpio_pin_t)i; | ||
| data->descriptors[i].owner_type = GPIO_OWNER_NONE; | ||
| data->descriptors[i].controller_context = controller_context; | ||
| } | ||
|
|
||
| mutex_construct(&data->mutex); | ||
| device_set_driver_data(device, data); | ||
|
|
||
| return ERROR_NONE; | ||
| } |
There was a problem hiding this comment.
pin_count overflow: uint32_t silently truncated to gpio_pin_t (uint8_t).
Line 71 casts i (uint32_t) to gpio_pin_t (uint8_t). If pin_count > 255, pins above index 255 will have incorrect pin numbers due to truncation. Consider either asserting pin_count <= UINT8_MAX or widening gpio_pin_t.
Proposed assertion
error_t gpio_controller_init_descriptors(struct Device* device, uint32_t pin_count, void* controller_context) {
+ if (pin_count > UINT8_MAX) return ERROR_INVALID_ARGUMENT;
+
auto* data = (struct GpioControllerData*)malloc(sizeof(struct GpioControllerData));
if (!data) return ERROR_OUT_OF_MEMORY;| DEFINE_MODULE_SYMBOL(gpio_descriptor_set_level), | ||
| DEFINE_MODULE_SYMBOL(gpio_descriptor_get_level), | ||
| DEFINE_MODULE_SYMBOL(gpio_descriptor_set_options), | ||
| DEFINE_MODULE_SYMBOL(gpio_descriptor_get_options), | ||
| DEFINE_MODULE_SYMBOL(gpio_controller_get_pin_count), | ||
| DEFINE_MODULE_SYMBOL(gpio_controller_init_descriptors), | ||
| DEFINE_MODULE_SYMBOL(gpio_controller_deinit_descriptors), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the name mismatch and missing exports
echo "=== Searching for gpio_descriptor pin-number related functions in headers ==="
find . -name "*.h" -type f | xargs rg -n 'gpio_descriptor.*pin_number' 2>/dev/null || echo "No matches in headers"
echo ""
echo "=== Searching for gpio_descriptor pin-number related functions in implementations ==="
find . -name "*.cpp" -o -name "*.c" | xargs rg -n 'gpio_descriptor.*pin_number' 2>/dev/null || echo "No matches in implementations"
echo ""
echo "=== Checking for acquire_descriptor and release_descriptor declarations ==="
find . -name "*.h" -type f | xargs rg -n 'acquire_descriptor|release_descriptor' 2>/dev/null || echo "No matches found"
echo ""
echo "=== Checking kernel_symbols.c for these symbols ==="
cat -n TactilityKernel/Source/kernel_symbols.c | head -100Repository: TactilityProject/Tactility
Length of output: 6293
🏁 Script executed:
#!/bin/bash
# Additional verification: check the full kernel_symbols.c to confirm acquire_descriptor and release_descriptor are absent
echo "=== Full kernel_symbols.c to check for acquire/release descriptor exports ==="
grep -n 'acquire_descriptor\|release_descriptor' TactilityKernel/Source/kernel_symbols.c || echo "NOT FOUND in kernel_symbols.c"
echo ""
echo "=== Confirm both header declarations exist ==="
grep -n 'acquire_descriptor\|release_descriptor' TactilityKernel/Include/tactility/drivers/gpio_controller.hRepository: TactilityProject/Tactility
Length of output: 463
Fix symbol export mismatch and missing exports in kernel_symbols.c.
Three critical issues require fixes:
-
Name mismatch between header and implementation: The header declares
gpio_descriptor_pin_number(gpio_controller.h:88), but the implementation definesgpio_descriptor_get_pin_number(gpio_controller.cpp:113). This will cause linker errors for any code using the header-declared function. -
Missing symbol exports:
acquire_descriptorandrelease_descriptorare declared in the header as public functions (gpio_controller.h:65, 72) but are not exported in the kernel_symbols table. External modules cannot access these core descriptor-acquisition functions. -
Missing pin-number getter export: The pin-number getter function is absent from the symbol table entirely, preventing external module access.
Add the missing symbols to kernel_symbols.c and correct the function name mismatch.
1 participant
Work in progress
Summary by CodeRabbit
New Features
Refactor
Deprecation