Skip to content

Conversation

@KenVanHoeylandt
Copy link
Contributor

@KenVanHoeylandt KenVanHoeylandt commented Feb 9, 2026

Summary by CodeRabbit

  • Refactor

    • GPIO subsystem moved to a descriptor-based model for per-pin ownership and runtime pin management; many platform drivers now acquire/release descriptors.
    • Device trees and drivers now use GPIO phandle-style pin specifications across all boards and all drivers.
  • Behavior

    • Device list now encodes per-device status (ok/disabled); boot will skip disabled devices accordingly.
  • Deprecation

    • Legacy GPIO HAL marked deprecated and replaced with descriptor-based interfaces.
  • Chores

    • Bindings and platform configs updated to the new GPIO pin-spec format.

@coderabbitai
Copy link

coderabbitai bot commented Feb 9, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR refactors GPIO to a descriptor-centric model. It adds struct GpioDescriptor and enum GpioOwnerType, replaces GpioPinConfig with GpioPinSpec, and renames option macros to GPIO_FLAG_*. Controller APIs and drivers now use descriptor-based signatures (set/get level, set/get flags, get_native_pin_number) and add descriptor lifecycle functions (acquire/release, init/destruct). Device-tree bindings and many board DTS files switch numeric pins to phandle GpioPinSpec entries. Kernel symbols, init paths, and several platform drivers were updated; legacy HAL GPIO functions were stubbed/deprecated.

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'GPIO refactored' is vague and generic, using a non-descriptive term that doesn't convey meaningful information about the substantial architectural changes in this pull request. Revise the title to be more specific about the core refactoring, such as 'Refactor GPIO API to use descriptor-based architecture' or 'Migrate GPIO from pin-based to descriptor-centric API' to clearly communicate the primary change to reviewers.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch gpio-refactored

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, and gpio_controller.h are no longer referenced. Only tt_hal_gpio.h is 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_t is defined but not used by the functional API.

The descriptor struct stores gpio_level_t level, but the actual set_level/get_level APIs use bool high. If this type is intended for caching the pin state in the descriptor, consider actually writing to descriptor->level in the set/get implementations. Otherwise, the level field in GpioDescriptor will remain stale after calloc initialization.

TactilityKernel/Include/tactility/drivers/gpio_descriptor.h (1)

1-20: Missing extern "C" guards for consistency with sibling headers.

Other headers in this directory (gpio.h, gpio_controller.h) wrap declarations in extern "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
+}
+#endif
Platforms/PlatformEsp32/Source/drivers/esp32_gpio.cpp (1)

94-98: Null guard contradicts project convention for internal APIs.

get_pin_number adds a null check for descriptor, 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;
 }

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
Devices/unphone/unphone.dts (1)

25-32: ⚠️ Potential issue | 🟡 Minor

SPI pins not yet migrated to the new GPIO phandle format.

The I2C pins on lines 21–22 were updated to use <&gpio0 N GPIO_FLAG_NONE>, but the SPI pins (pin-mosi, pin-miso, pin-sclk) still use bare integer literals. If the SPI driver has been (or will be) updated to expect GPIO phandle references, these should be converted as well for consistency.

Given the PR is marked WIP, this may already be planned — flagging to ensure it's not missed.

Devices/wireless-tag-wt32-sc01-plus/wireless-tag,wt32-sc01-plus.dts (1)

25-31: ⚠️ Potential issue | 🟠 Major

Migrate SPI pins to GPIO phandle bindings for consistency.

Other device trees in the repository (lilygo-tdisplay, lilygo-tdisplay-s3) have already migrated pin-mosi and pin-sclk to GPIO phandle bindings (e.g., <&gpio0 18 GPIO_FLAG_NONE>), but this file still uses raw literals. Update lines 28-30 to match the established pattern:

pin-mosi = <&gpio0 40 GPIO_FLAG_NONE>;
pin-miso = <38>;
pin-sclk = <&gpio0 39 GPIO_FLAG_NONE>;
Devices/m5stack-cores3/m5stack,cores3.dts (1)

52-79: ⚠️ Potential issue | 🟠 Major

Migrate SPI pins to GPIO phandle syntax for consistency with I2C.

The I2C nodes use <&gpio0 PIN GPIO_FLAG_NONE> syntax, but spi0 still uses plain integers. Other devices in the codebase (e.g., lilygo-tdisplay-s3) have already migrated their SPI pins to phandle syntax, so at minimum the SPI pins here should be updated for consistency.

I2S and UART drivers don't yet support phandle-style pins in this codebase, so those can remain as plain integers for now.

Platforms/PlatformEsp32/Source/drivers/esp32_i2c.cpp (1)

192-202: ⚠️ Potential issue | 🟠 Major

Descriptor resource leak on I2C initialization failure.

If i2c_param_config or i2c_driver_install fails, the function returns without releasing sda_descriptor and scl_descriptor. Those pins will remain in an acquired state, preventing future acquisition attempts.

Proposed fix
     esp_err_t error = i2c_param_config(dts_config->port, &esp_config);
     if (error != ESP_OK) {
         LOG_E(TAG, "Failed to configure port %d: %s", static_cast<int>(dts_config->port), esp_err_to_name(error));
+        gpio_descriptor_release(sda_descriptor);
+        gpio_descriptor_release(scl_descriptor);
         return ERROR_RESOURCE;
     }
 
     error = i2c_driver_install(dts_config->port, esp_config.mode, 0, 0, 0);
     if (error != ESP_OK) {
         LOG_E(TAG, "Failed to install driver at port %d: %s", static_cast<int>(dts_config->port), esp_err_to_name(error));
+        gpio_descriptor_release(sda_descriptor);
+        gpio_descriptor_release(scl_descriptor);
         return ERROR_RESOURCE;
     }
🧹 Nitpick comments (10)
Devices/lilygo-tdeck/lilygo,tdeck.dts (1)

36-57: Remaining nodes (i2s0, spi0, uart1) still use raw numeric pin values.

The I2C nodes were migrated to GPIO phandle references, but these three nodes still use the old <N> format. Since the PR is WIP, flagging this as a reminder to migrate them for consistency once the corresponding platform drivers support phandle-based pin resolution.

Devices/guition-jc8048w550c/guition,jc8048w550c.dts (1)

34-47: SPI and UART pin properties not yet migrated to GPIO phandle format.

The spi0 and uart1 nodes still use bare integer pin assignments while the i2c nodes have been migrated to the new <&gpio0 PIN FLAGS> descriptor format. Since the PR description notes this is WIP, flagging for tracking.

Firmware/CMakeLists.txt (1)

55-58: Consider removing the debug echo before merging.

The added echo on lines 55–56 prints the compile command to stdout on every build. If this was added for debugging during development, consider removing it before merge to keep build output clean. If you want to keep it, CMake's COMMENT on line 61 already serves a similar purpose.

Devices/cyd-2432s032c/cyd,2432s032c.dts (1)

12-15: GPIO node should use gpio0 for consistency with all other device trees.

This file uniquely names its GPIO controller gpio1 while all 40+ other device tree files in the codebase use gpio0. The phandle references on lines 21–22 are internally consistent, but renaming to gpio0 would align with the established naming convention.

TactilityKernel/Include/tactility/drivers/gpio_descriptor.h (1)

1-14: Consider adding a forward declaration for struct Device.

struct Device* is used on line 7. While struct Device is transitively available through gpio.h<tactility/device.h>, an explicit forward declaration (struct Device;) makes the dependency clearer and protects against future include chain changes.

Suggested fix
 `#pragma` once
 
 `#include` "gpio.h"
 
+struct Device;
+
 struct GpioDescriptor {
TactilityC/Source/tt_init.cpp (1)

378-381: Exporting raw ESP-IDF GPIO functions bypasses the new descriptor-based API.

These exports (gpio_config, gpio_get_level, gpio_set_level) give ELF modules direct hardware GPIO access, bypassing the descriptor-based ownership/acquisition model introduced in this PR. This means external modules can manipulate pins without going through gpio_descriptor_acquire, potentially conflicting with pins owned by kernel drivers.

If this is intentional (e.g., for backward compatibility or low-level module needs), consider documenting this tradeoff. Otherwise, consider exporting the descriptor-based wrappers instead.

TactilityKernel/Include/tactility/drivers/gpio_controller.h (2)

54-54: void* pin_number in get_native_pin_number is type-unsafe.

Both the vtable function pointer (Line 54) and the free function (Line 119) use void* pin_number as an output parameter. This loses type safety — callers must know the expected type (e.g., gpio_num_t on ESP32) and cast correctly. A wrong-sized destination causes memory corruption.

Consider using a fixed-width type like int32_t* or uint32_t* if cross-platform pin numbers fit in 32 bits, or document the required destination type prominently.

Also applies to: 119-119


57-61: Document the ownership semantics and failure mode of gpio_descriptor_acquire.

This function returns a pointer (presumably NULL on failure), takes an owner enum, but there's no doc comment explaining:

  • What happens if the pin is already acquired by another owner?
  • Whether acquiring the same pin twice with the same owner is allowed?
  • The caller's responsibility to call gpio_descriptor_release.

These semantics are critical for correct usage of the ownership model.

Platforms/PlatformEsp32/Source/drivers/esp32_spi.cpp (1)

19-25: Constructor parameters are unused.

Esp32SpiInternal accepts two GpioDescriptor* parameters that are neither stored nor referenced. The constructor body only initializes the mutex. On line 54, both are passed as nullptr.

If descriptor-based SPI pin handling is planned for a future iteration, consider leaving a // TODO comment. Otherwise, revert to the default constructor to avoid confusion.

Proposed fix (if descriptors aren't needed yet)
-    Esp32SpiInternal(GpioDescriptor* pDescriptor, GpioDescriptor* pGpioDescriptor) {
+    Esp32SpiInternal() {
         recursive_mutex_construct(&mutex);
     }

And in start():

-    auto* data = new (std::nothrow) Esp32SpiInternal(nullptr, nullptr);
+    auto* data = new (std::nothrow) Esp32SpiInternal();
Platforms/PlatformEsp32/Source/drivers/esp32_i2c.cpp (1)

176-176: Nit: double semicolon.

-    gpio_flags_t sda_flags, scl_flags;;
+    gpio_flags_t sda_flags, scl_flags;

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Platforms/PlatformEsp32/Source/drivers/esp32_i2c.cpp (1)

214-231: ⚠️ Potential issue | 🟠 Major

Resource leak on i2c_driver_delete failure: descriptors and driver_data are not released.

If i2c_driver_delete fails (line 220), the function returns ERROR_RESOURCE at line 222 without releasing the GPIO descriptors or freeing driver_data. This leaks both the descriptors (keeping the pins locked) and the Esp32SpiInternal allocation.

Proposed fix — always clean up, then report the error
 static error_t stop(Device* device) {
     ESP_LOGI(TAG, "stop %s", device->name);
     auto* driver_data = static_cast<Esp32SpiInternal*>(device_get_driver_data(device));
 
     i2c_port_t port = GET_CONFIG(device)->port;
     esp_err_t result = i2c_driver_delete(port);
     if (result != ESP_OK) {
         LOG_E(TAG, "Failed to delete driver at port %d: %s", static_cast<int>(port), esp_err_to_name(result));
-        return ERROR_RESOURCE;
     }
 
     check(gpio_descriptor_release(driver_data->sda_descriptor) == ERROR_NONE);
     check(gpio_descriptor_release(driver_data->scl_descriptor) == ERROR_NONE);
 
     device_set_driver_data(device, nullptr);
     delete driver_data;
-    return ERROR_NONE;
+    return (result == ESP_OK) ? ERROR_NONE : ERROR_RESOURCE;
 }
🧹 Nitpick comments (5)
Platforms/PlatformEsp32/Source/drivers/esp32_spi.cpp (2)

23-25: explicit on a zero-argument constructor has no effect.

The explicit specifier prevents implicit conversions, but a constructor with no parameters cannot participate in implicit conversions. This is harmless but unnecessary.


6-6: Remove unused include of gpio_descriptor.h.

This file doesn't reference GpioDescriptor or any descriptor API. The SPI driver continues to use raw pin numbers from configuration.

TactilityKernel/Include/tactility/drivers/gpio_descriptor.h (1)

1-16: Missing SPDX license header.

Other source files in the project start with // SPDX-License-Identifier: Apache-2.0. This new public header should follow the same convention.

Proposed fix
+// SPDX-License-Identifier: Apache-2.0
 `#pragma` once
 
 `#include` "gpio.h"
Platforms/PlatformEsp32/Source/drivers/esp32_i2c.cpp (1)

15-30: The struct is named Esp32SpiInternal but this is the I2C driver.

This is likely a pre-existing copy-paste issue, but since the struct internals were refactored here (adding descriptor members and a new constructor), this would be a good time to rename it to Esp32I2cInternal for clarity.

TactilityKernel/Include/tactility/drivers/gpio_controller.h (1)

48-54: void* pin_number is inherently type-unsafe.

Callers must know the platform-specific type to cast correctly (e.g., gpio_num_t on ESP32). If multiple platforms are targeted, consider documenting the expected concrete type per platform, or using a uintptr_t/int that can hold any platform's pin type without a void pointer.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
Platforms/PlatformEsp32/Source/drivers/esp32_i2c.cpp (1)

217-234: ⚠️ Potential issue | 🟠 Major

Descriptor and memory leak when i2c_driver_delete fails.

If i2c_driver_delete returns an error at line 223, stop() returns early without releasing the GPIO descriptors or freeing driver_data. This leaves both descriptors permanently acquired and the Esp32SpiInternal allocation leaked.

Proposed fix — always clean up, propagate original error
 static error_t stop(Device* device) {
     ESP_LOGI(TAG, "stop %s", device->name);
     auto* driver_data = static_cast<Esp32SpiInternal*>(device_get_driver_data(device));
 
     i2c_port_t port = GET_CONFIG(device)->port;
     esp_err_t result = i2c_driver_delete(port);
     if (result != ESP_OK) {
         LOG_E(TAG, "Failed to delete driver at port %d: %s", static_cast<int>(port), esp_err_to_name(result));
-        return ERROR_RESOURCE;
     }
 
     check(gpio_descriptor_release(driver_data->sda_descriptor) == ERROR_NONE);
     check(gpio_descriptor_release(driver_data->scl_descriptor) == ERROR_NONE);
 
     device_set_driver_data(device, nullptr);
     delete driver_data;
-    return ERROR_NONE;
+    return (result != ESP_OK) ? ERROR_RESOURCE : ERROR_NONE;
 }
🧹 Nitpick comments (1)
Platforms/PlatformEsp32/Source/drivers/esp32_i2c.cpp (1)

17-32: Esp32SpiInternal is misleading — this is an I2C driver.

The struct is named Esp32SpiInternal but lives in the I2C driver file and manages I2C resources. This appears to be a copy-paste artifact from the SPI driver. Should be Esp32I2cInternal.

Proposed rename
-struct Esp32SpiInternal {
+struct Esp32I2cInternal {
     Mutex mutex {};
     GpioDescriptor* sda_descriptor = nullptr;
     GpioDescriptor* scl_descriptor = nullptr;
 
-    Esp32SpiInternal(GpioDescriptor* sda_descriptor, GpioDescriptor* scl_descriptor) :
+    Esp32I2cInternal(GpioDescriptor* sda_descriptor, GpioDescriptor* scl_descriptor) :
         sda_descriptor(sda_descriptor),
         scl_descriptor(scl_descriptor)
     {
         mutex_construct(&mutex);
     }
 
-    ~Esp32SpiInternal() {
+    ~Esp32I2cInternal() {
         mutex_destruct(&mutex);
     }
 };

Update the macro and all usages accordingly:

-#define GET_DATA(device) ((Esp32SpiInternal*)device_get_driver_data(device))
+#define GET_DATA(device) ((Esp32I2cInternal*)device_get_driver_data(device))

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
Platforms/PlatformEsp32/Source/drivers/esp32_i2c.cpp (1)

221-237: ⚠️ Potential issue | 🟠 Major

stop() leaks descriptors and Esp32I2cInternal on i2c_driver_delete failure.

If i2c_driver_delete fails (line 226-230), the function returns early without releasing the GPIO descriptors or deleting driver_data. This leaks both the descriptors and the Esp32I2cInternal allocation (including its mutex).

Proposed fix — clean up regardless of driver-delete outcome
 static error_t stop(Device* device) {
     ESP_LOGI(TAG, "stop %s", device->name);
     auto* driver_data = static_cast<Esp32I2cInternal*>(device_get_driver_data(device));

     i2c_port_t port = GET_CONFIG(device)->port;
     esp_err_t result = i2c_driver_delete(port);
     if (result != ESP_OK) {
         LOG_E(TAG, "Failed to delete driver at port %d: %s", static_cast<int>(port), esp_err_to_name(result));
-        return ERROR_RESOURCE;
     }

     check(gpio_descriptor_release(driver_data->sda_descriptor) == ERROR_NONE);
     check(gpio_descriptor_release(driver_data->scl_descriptor) == ERROR_NONE);

     device_set_driver_data(device, nullptr);
     delete driver_data;
-    return ERROR_NONE;
+    return result != ESP_OK ? ERROR_RESOURCE : ERROR_NONE;
 }
Platforms/PlatformEsp32/Source/drivers/esp32_i2s.cpp (1)

241-257: ⚠️ Potential issue | 🟠 Major

Memory leak: data not deleted when init_pins fails.

If init_pins returns false (line 249), the function returns ERROR_RESOURCE without deleting data. The Esp32I2sInternal object (including its mutex) is leaked.

Proposed fix
     if (!data->init_pins(dts_config)) {
         LOG_E(TAG, "Failed to init one or more pins");
+        delete data;
         return ERROR_RESOURCE;
     }
🧹 Nitpick comments (10)
Devices/cyd-8048s043c/cyd,8048s043c.dts (1)

42-47: Good: UART1 disabled to avoid GPIO 17/18 conflict with i2c_external.

GPIO pins 17 and 18 are shared between i2c_external (lines 30–31) and uart1 (lines 46–47). Setting status = "disabled" on uart1 prevents the conflict at runtime, which aligns with the commit message about fixing duplicate pin usage. Worth adding a brief comment in the DTS noting why UART1 is disabled, so future maintainers don't accidentally re-enable it and cause a pin conflict.

📝 Suggested comment
 	uart1 {
 		compatible = "espressif,esp32-uart";
+		/* disabled: GPIO 17/18 shared with i2c_external */
 		status = "disabled";
 		port = <UART_NUM_1>;
 		pin-tx = <&gpio0 18 GPIO_FLAG_NONE>;
 		pin-rx = <&gpio0 17 GPIO_FLAG_NONE>;
 	};
Devices/m5stack-cardputer/m5stack,cardputer.dts (1)

24-25: Pin overlap between i2c_port_a and uart_port_a on GPIO 1 & 2.

i2c_port_a uses pins 1 (SCL) and 2 (SDA), while uart_port_a uses the same pins 1 (TX) and 2 (RX). The UART is correctly marked status = "disabled", so there's no runtime conflict. A brief inline comment on the UART node (e.g., // Shares pins with i2c_port_a) would make the mutual exclusivity obvious to future editors.

📝 Suggested comment for clarity
+	// Directly conflicts with i2c_port_a (shared GPIO 1 & 2)
 	uart_port_a: uart1 {
 		compatible = "espressif,esp32-uart";
 		status = "disabled";

Also applies to: 54-60

Devices/m5stack-cores3/m5stack,cores3.dts (1)

76-82: Pin overlap between uart_port_a and i2c_port_a is safely guarded by status = "disabled".

uart_port_a (TX=GPIO1, RX=GPIO2) shares pins with i2c_port_a (SCL=GPIO1, SDA=GPIO2). Since uart_port_a is disabled, there's no runtime conflict. Worth a brief inline comment in the DTS to call out the shared-pin relationship for future maintainers.

📝 Suggested documentation comment
+	// Note: uart_port_a shares GPIO 1 and 2 with i2c_port_a; only one can be enabled at a time.
 	uart_port_a: uart1 {
 		compatible = "espressif,esp32-uart";
 		status = "disabled";
Platforms/PlatformEsp32/Include/tactility/drivers/esp32_uart.h (1)

12-18: Inconsistent naming convention: pinTx/pinRx (camelCase) vs pin_sclk/pin_mosi (snake_case) in esp32_spi.h.

The UART config uses camelCase for pin fields (pinTx, pinRx, pinCts, pinRts), while the SPI config (see esp32_spi.h lines 15–23) and I2S config (esp32_i2s.h lines 14–18) use snake_case (pin_sclk, pin_mosi, pin_bclk, etc.). Consider aligning to one convention across all ESP32 peripheral configs for consistency.

♻️ Proposed fix to align with snake_case convention
 struct Esp32UartConfig {
     uart_port_t port;
-    struct GpioPinSpec pinTx;
-    struct GpioPinSpec pinRx;
-    struct GpioPinSpec pinCts;
-    struct GpioPinSpec pinRts;
+    struct GpioPinSpec pin_tx;
+    struct GpioPinSpec pin_rx;
+    struct GpioPinSpec pin_cts;
+    struct GpioPinSpec pin_rts;
 };
TactilityKernel/Include/tactility/dts.h (1)

1-28: Clean header definition for the DTS device model.

The struct/enum design is straightforward and appropriate for generated DTS code consumption. One minor note: the DTS_DEVICE_TERMINATOR macro on Line 28 uses NULL, but this header doesn't include <stddef.h> (or <stdlib.h>). This works as long as every translation unit that expands the macro has already pulled in NULL through another path, which is almost certainly the case for generated code—but adding the include would make the header self-contained.

Optional: make header self-contained
 `#pragma` once
 
+#include <stddef.h>
+
 struct Device;
Platforms/PlatformEsp32/Source/drivers/esp32_spi.cpp (2)

112-118: Inconsistent cleanup order: delete before device_set_driver_data(nullptr).

On lines 88–90, the failure path does device_set_driver_data(nullptr) before delete data, but here (lines 114–115) the order is reversed. For consistency and to avoid a window where device_get_driver_data returns a dangling pointer, consider matching the order used elsewhere.

Suggested reorder
     if (ret != ESP_OK) {
         data->cleanup_pins();
-        delete data;
         device_set_driver_data(device, nullptr);
+        delete data;
         ESP_LOGE(TAG, "Failed to initialize SPI bus: %s", esp_err_to_name(ret));
         return ERROR_RESOURCE;
     }

31-31: Nit: explicit on a zero-argument constructor has no effect.

explicit prevents implicit conversions, which only applies to single-argument (or implicitly-convertible) constructors. On a default constructor it's harmless but unnecessary.

Platforms/PlatformEsp32/Private/tactility/drivers/esp32_gpio_helpers.h (1)

7-9: Consider adding brief doc comments for release_pin and acquire_pin_or_set_null.

get_native_pin and is_pin_inverted have doc comments documenting nullability semantics, but these two don't. The nullable-pointer and optional-pin behavior is worth documenting for callers.

Buildscripts/DevicetreeCompiler/source/generator.py (1)

80-93: Use DevicetreeException instead of bare Exception for consistency.

Line 93 raises a generic Exception while the rest of the file consistently uses DevicetreeException. This also addresses the Ruff TRY002 hint.

Proposed fix
-            raise Exception(f"Unsupported phandle-array type for {property.value}")
+            raise DevicetreeException(f"Unsupported phandle-array type for {property.value}")
Platforms/PlatformEsp32/Source/drivers/esp32_gpio.cpp (1)

94-98: get_native_pin_number uses void* — inherently type-unsafe but consistent with the generic API.

This matches the cross-platform pattern in gpio_controller.cpp (line 127-130) where the platform-specific pin type is opaque. Callers must know to pass the correct type — worth a brief doc comment if not already documented in the header.

@KenVanHoeylandt KenVanHoeylandt merged commit 26c1798 into main Feb 11, 2026
53 checks passed
@KenVanHoeylandt KenVanHoeylandt deleted the gpio-refactored branch February 11, 2026 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant