Grove driver#532
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new Grove driver for ESP32 that enables dynamic switching between UART and I2C communication modes. The implementation follows a layered architecture: a public 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8de92cb0-4616-4082-aa46-e22743183e0b
📒 Files selected for processing (7)
Platforms/platform-esp32/bindings/espressif,esp32-grove.yamlPlatforms/platform-esp32/include/tactility/bindings/esp32_grove.hPlatforms/platform-esp32/include/tactility/drivers/esp32_grove.hPlatforms/platform-esp32/source/drivers/esp32_grove.cppPlatforms/platform-esp32/source/module.cppTactilityKernel/include/tactility/drivers/grove.hTactilityKernel/source/drivers/grove.cpp
| defaultMode: | ||
| type: int | ||
| required: true | ||
| description: "One of enum Esp32GroveMode" |
There was a problem hiding this comment.
Inconsistent enum name in documentation.
The description references Esp32GroveMode, but the actual enum is GroveMode (defined in TactilityKernel/include/tactility/drivers/grove.h).
📝 Proposed fix
- description: "One of enum Esp32GroveMode"
+ description: "One of enum GroveMode"📝 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.
| description: "One of enum Esp32GroveMode" | |
| description: "One of enum GroveMode" |
| static error_t stop_child(struct Device* device) { | ||
| auto* data = GET_DATA(device); | ||
| if (!data) return ERROR_NONE; | ||
|
|
||
| if (data->child_device) { | ||
| if (data->child_device->internal) { | ||
| if (device_is_added(data->child_device)) { | ||
| if (device_is_ready(data->child_device)) { | ||
| if (device_stop(data->child_device) != ERROR_NONE) { | ||
| LOG_E(TAG, "%s: failed to stop child device", device->name); | ||
| return ERROR_RESOURCE; | ||
| } | ||
| } | ||
| if (device_remove(data->child_device) != ERROR_NONE) { | ||
| LOG_E(TAG, "%s: failed to remove child device", device->name); | ||
| return ERROR_RESOURCE; | ||
| } | ||
| } | ||
| check(device_destruct(data->child_device) == ERROR_NONE); | ||
| } | ||
| delete data->child_device; | ||
| data->child_device = nullptr; | ||
| } | ||
|
|
||
| if (data->child_config) { | ||
| if (data->current_mode == GROVE_MODE_UART) { | ||
| delete (struct Esp32UartConfig*)data->child_config; | ||
| } else if (data->current_mode == GROVE_MODE_I2C) { | ||
| delete (struct Esp32I2cConfig*)data->child_config; | ||
| } | ||
| data->child_config = nullptr; | ||
| } | ||
|
|
||
| delete[] data->child_name; | ||
| data->child_name = nullptr; | ||
|
|
||
| data->current_mode = GROVE_MODE_DISABLED; | ||
| return ERROR_NONE; | ||
| } |
There was a problem hiding this comment.
Resource leak: stop_child early return leaves child resources allocated.
When device_stop (line 37) or device_remove (line 42) fails, stop_child returns ERROR_RESOURCE without cleaning up child_device, child_config, or child_name. Since stop_device (line 168) ignores the error return and unconditionally deletes data, the pointers to these allocated resources are lost, causing a leak.
Impact: If the child device enters a failure state where it cannot be stopped or removed, the Grove driver will leak memory on every subsequent mode change or device shutdown.
♻️ Recommended fix: Best-effort cleanup in `stop_child`
Continue cleanup even if device_stop or device_remove fails. Log errors but don't abort the cleanup sequence:
if (device_is_ready(data->child_device)) {
- if (device_stop(data->child_device) != ERROR_NONE) {
+ error_t stop_err = device_stop(data->child_device);
+ if (stop_err != ERROR_NONE) {
LOG_E(TAG, "%s: failed to stop child device", device->name);
- return ERROR_RESOURCE;
+ // Continue with cleanup
}
}
- if (device_remove(data->child_device) != ERROR_NONE) {
+ error_t remove_err = device_remove(data->child_device);
+ if (remove_err != ERROR_NONE) {
LOG_E(TAG, "%s: failed to remove child device", device->name);
- return ERROR_RESOURCE;
+ // Continue with cleanup
}
}Also propagate the error from stop_child in stop_device:
static error_t stop_device(struct Device* device) {
auto* data = GET_DATA(device);
if (!data) return ERROR_NONE;
- stop_child(device);
+ error_t err = stop_child(device);
delete data;
device_set_driver_data(device, nullptr);
- return ERROR_NONE;
+ return err;
}| if (data->child_config) { | ||
| if (data->current_mode == GROVE_MODE_UART) { | ||
| delete (struct Esp32UartConfig*)data->child_config; | ||
| } else if (data->current_mode == GROVE_MODE_I2C) { | ||
| delete (struct Esp32I2cConfig*)data->child_config; | ||
| } | ||
| data->child_config = nullptr; | ||
| } |
There was a problem hiding this comment.
Critical memory leak: child_config not deleted when current_mode is GROVE_MODE_DISABLED.
When start_child fails after allocating child_config (line 113 for UART or line 130 for I2C) but before setting current_mode (line 151), the internally-called stop_child (line 147) will leak child_config:
Failure scenario:
start_child(device, GROVE_MODE_UART)is calledchild_device,child_name, anduart_cfgare successfully allocated (lines 79–113)device_construct_add_startfails (line 144)stop_childis called (line 147)- At this point,
current_modeis stillGROVE_MODE_DISABLED(line 151 never executed) - In
stop_child, line 53 condition is true (child_configis not nullptr) - Lines 54 and 56 both evaluate to false (current_mode is
DISABLED, notUARTorI2C) - Line 59 sets
child_config = nullptrwithout deleting the allocated config → memory leak
🔧 Proposed fix: Set `current_mode` before calling `device_construct_add_start`
Move the current_mode assignment to immediately after allocating and assigning child_config, so stop_child can correctly identify the type even if device_construct_add_start fails:
For UART mode:
uart_cfg->pin_cts = GPIO_PIN_SPEC_NONE;
uart_cfg->pin_rts = GPIO_PIN_SPEC_NONE;
data->child_config = uart_cfg;
+ data->current_mode = GROVE_MODE_UART;
compatible = "espressif,esp32-uart";
LOG_I(TAG, "%s: starting UART mode on port %d", device->name, (int)uart_cfg->port);For I2C mode:
i2c_cfg->pinSda = config->pinSdaRx;
i2c_cfg->pinScl = config->pinSclTx;
data->child_config = i2c_cfg;
+ data->current_mode = GROVE_MODE_I2C;
compatible = "espressif,esp32-i2c";
LOG_I(TAG, "%s: starting I2C mode on port %d", device->name, (int)i2c_cfg->port);Then remove the assignment at the end:
if (err != ERROR_NONE) {
LOG_E(TAG, "%s: failed to start child device: %d", device->name, err);
stop_child(device);
return err;
}
- data->current_mode = mode;
return ERROR_NONE;
}| static error_t start_device(struct Device* device) { | ||
| const auto* config = GET_CONFIG(device); | ||
| auto* data = new(std::nothrow) Esp32GroveInternal(); | ||
| if (!data) return ERROR_OUT_OF_MEMORY; | ||
| device_set_driver_data(device, data); | ||
|
|
||
| return start_child(device, config->defaultMode); | ||
| } |
There was a problem hiding this comment.
Memory leak: data not freed when start_child fails.
If start_child fails (line 161), start_device returns the error but leaves the allocated Esp32GroveInternal instance in driver_data. Assuming the device framework does not call stop_device when start_device fails (typical driver lifecycle), this leaks data.
🛠️ Proposed fix: Clean up `data` on `start_child` failure
static error_t start_device(struct Device* device) {
const auto* config = GET_CONFIG(device);
auto* data = new(std::nothrow) Esp32GroveInternal();
if (!data) return ERROR_OUT_OF_MEMORY;
device_set_driver_data(device, data);
- return start_child(device, config->defaultMode);
+ error_t err = start_child(device, config->defaultMode);
+ if (err != ERROR_NONE) {
+ delete data;
+ device_set_driver_data(device, nullptr);
+ }
+ return err;
}1 participant
Summary by CodeRabbit