Implement new I2C driver#531
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds a new esp32_i2c_master driver and devicetree/binding types, updates platform/module build wiring and SDK defaults, extends the I2C controller API with an optional probe callback, and refactors Tab5Keyboard to accept a ::Device* I2C controller and use i2c_controller_* register/read/write/probe routines instead of direct ESP-IDF i2c_master calls. Device-tree now declares a low-power I2C bus for the keyboard and per-board properties were consolidated. 🚥 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Platforms/platform-esp32/source/module.cpp (1)
78-86:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: Driver teardown order violates dependency chain.
In
stop(), drivers are removed in the SAME order as they were constructed instart(), rather than reverse order. This violates the dependency relationship whereesp32_i2c_master_driverdepends onesp32_gpio_driver(it acquires GPIO descriptors duringstart()and releases them duringstop()).Currently:
- Line 78 removes
esp32_gpio_driver- Line 80 removes
esp32_i2c_master_driverWhen
esp32_i2c_master_driver.stop()executes at line 80, it callsgpio_descriptor_release()(line 230-231 inesp32_i2c_master.cpp), but the GPIO driver has already been destructed at line 78. This could cause crashes or undefined behavior.Drivers must be torn down in reverse construction order.
🔧 Proposed fix: reverse teardown order for core drivers
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 */ `#if` SOC_USB_OTG_SUPPORTED check(driver_remove_destruct(&esp32_usbhost_msc_driver) == ERROR_NONE); check(driver_remove_destruct(&esp32_usbhost_midi_driver) == ERROR_NONE); check(driver_remove_destruct(&esp32_usbhost_hid_driver) == ERROR_NONE); check(driver_remove_destruct(&esp32_usbhost_driver) == ERROR_NONE); `#endif` `#if` defined(CONFIG_BT_NIMBLE_ENABLED) check(driver_remove_destruct(&esp32_ble_hid_device_driver) == ERROR_NONE); check(driver_remove_destruct(&esp32_ble_midi_driver) == ERROR_NONE); check(driver_remove_destruct(&esp32_ble_serial_driver) == ERROR_NONE); check(driver_remove_destruct(&esp32_bluetooth_driver) == ERROR_NONE); `#endif` - check(driver_remove_destruct(&esp32_gpio_driver) == ERROR_NONE); - check(driver_remove_destruct(&esp32_i2c_driver) == ERROR_NONE); - check(driver_remove_destruct(&esp32_i2c_master_driver) == ERROR_NONE); - check(driver_remove_destruct(&esp32_i2s_driver) == ERROR_NONE); + check(driver_remove_destruct(&esp32_uart_driver) == ERROR_NONE); + check(driver_remove_destruct(&esp32_spi_driver) == ERROR_NONE); `#if` SOC_SDMMC_HOST_SUPPORTED check(driver_remove_destruct(&esp32_sdmmc_driver) == ERROR_NONE); `#endif` - check(driver_remove_destruct(&esp32_spi_driver) == ERROR_NONE); - check(driver_remove_destruct(&esp32_uart_driver) == ERROR_NONE); + check(driver_remove_destruct(&esp32_i2s_driver) == ERROR_NONE); + check(driver_remove_destruct(&esp32_i2c_master_driver) == ERROR_NONE); + check(driver_remove_destruct(&esp32_i2c_driver) == ERROR_NONE); + check(driver_remove_destruct(&esp32_gpio_driver) == ERROR_NONE); return ERROR_NONE; }
🧹 Nitpick comments (1)
Platforms/platform-esp32/source/drivers/esp32_i2c_master.cpp (1)
163-163: ClarifyGPIO_FLAG_PULL_UPsemantics for ESP32 I2C internal pull-ups
esp32_i2c_master.cppuses a single ESP-IDF bus flagi2c_master_bus_config_t.flags.enable_internal_pullup, which enables internal pull-ups for both SDA and SCL simultaneously. That matches the current logic:.enable_internal_pullup = ((sda_flags & GPIO_FLAG_PULL_UP) != 0) || ((scl_flags & GPIO_FLAG_PULL_UP) != 0),In this repo’s device trees,
GPIO_FLAG_PULL_UPis currently specified together for both pins (e.g.Devices/m5stack-tab5/m5stack,tab5.dtssets bothpin-sdaandpin-scltoGPIO_FLAG_PULL_UP), so there’s no existing “SDA-only vs SCL-only” mismatch.If the intended meaning of
GPIO_FLAG_PULL_UPis “per-pin” (rather than “request bus pull-ups”), document that the DT flag is treated as a bus-level request and/or consider switching to an AND policy (sda_flags && scl_flags) to require both pins be marked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e11aac1-bcaf-4a2b-acd3-4f990fa07366
📒 Files selected for processing (13)
Buildscripts/sdkconfig/default.propertiesDevices/m5stack-tab5/Source/Configuration.cppDevices/m5stack-tab5/Source/devices/Tab5Keyboard.cppDevices/m5stack-tab5/Source/devices/Tab5Keyboard.hDevices/m5stack-tab5/device.propertiesDevices/m5stack-tab5/m5stack,tab5.dtsPlatforms/platform-esp32/CMakeLists.txtPlatforms/platform-esp32/bindings/espressif,esp32-i2c-master.yamlPlatforms/platform-esp32/bindings/espressif,esp32-i2c.yamlPlatforms/platform-esp32/include/tactility/bindings/esp32_i2c_master.hPlatforms/platform-esp32/include/tactility/drivers/esp32_i2c_master.hPlatforms/platform-esp32/source/drivers/esp32_i2c_master.cppPlatforms/platform-esp32/source/module.cpp
💤 Files with no reviewable changes (1)
- Devices/m5stack-tab5/device.properties
|
Thanks for the contribution, @Shadowtrance ! |
1 participant
Summary by CodeRabbit
Bug Fixes
New Features
Refactor