New kernel drivers and device migrations#563
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request migrates device profiles from legacy display and touch implementations to devicetree-backed modules. It adds ILI9341, ST7789 i8080, HX8357, XPT2046 soft-SPI, ESP32 i8080, and GPIO hog drivers. Touch calibration becomes configurable through device properties and Kconfig, with new LVGL APIs, setup integration, calibration UI, and runtime persistence. Device property generation is refactored to use flattened keys, and devicetree integer range validation is added. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
Devices/cyd-2432s028rv3/source/module.cpp (1)
8-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider using the new
gpio-hogbinding instead of a bespoke module for LED pin setup.This PR adds a
tactility,gpio-hogdriver for exactly this kind of "set GPIO to fixed level at boot" use case. Migrating the RGB LED off/on logic to agpio-hogdevicetree node would keep this board consistent with the devicetree-driven pattern the rest of the PR establishes, and let this file (and itsmodule.cpp) be removed entirely.Drivers/xpt2046-softspi-module/README.md (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse markdown link syntax for the datasheet URL.
A bare URL works but is fragile in some renderers. Consider wrapping it in proper link syntax.
🔗 Proposed fix
-See https://grobotronics.com/images/datasheets/xpt2046-datasheet.pdf +See [XPT2046 Datasheet](https://grobotronics.com/images/datasheets/xpt2046-datasheet.pdf)Drivers/xpt2046-softspi-module/include/drivers/xpt2046_softspi.h (1)
18-19: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider
uint32_tforx_max/y_maxto match devicetreeinttype.The binding YAML defines
x-maxandy-maxastype: int(typically 32-bit in devicetree), but the config struct usesuint16_t. Values above 65535 would silently truncate. While touch panel resolutions are well withinuint16_trange, usinguint32_twould avoid any potential mismatch with the devicetreeinttype and prevent future surprises.♻️ Proposed refactor
- uint16_t x_max; - uint16_t y_max; + uint32_t x_max; + uint32_t y_max;Tactility/Source/lvgl/Lvgl.cpp (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate "lock → get default indev → build/clear calibration → apply" skeleton across 4 sites. The same pattern of building an
LvglPointerCalibrationfrom aTouchCalibrationSettings(or clearing it) and applying it tolvgl_pointer_get_default()under the LVGL lock is repeated verbatim inLvgl.cppand three places inTouchCalibration.cpp. A single helper (e.g.applyTouchCalibration(const TouchCalibrationSettings*)in the touch settings module) would remove this duplication and keep the lock/lookup/apply logic in one place.
Tactility/Source/lvgl/Lvgl.cpp#L76-93: extract the load-settings-and-apply-to-default-indev block into a shared helper called from here.Tactility/Source/app/touchcalibration/TouchCalibration.cpp#L131-144: replace the inline calibration construction + lock/apply with a call to the same shared helper, passing the freshly computedsettings.Tactility/Source/app/touchcalibration/TouchCalibration.cpp#L181-190: replace with a call to the helper's "clear" path (or passnullptr/no-settings to the shared helper).Tactility/Source/app/touchcalibration/TouchCalibration.cpp#L192-213: replace the load-from-disk-and-apply block with the same shared helper used inLvgl.cpp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9663ee99-0112-4b1e-83a6-15f77698637c
📒 Files selected for processing (100)
Devices/cyd-2432s024r/CMakeLists.txtDevices/cyd-2432s024r/Source/Configuration.cppDevices/cyd-2432s024r/Source/devices/Display.cppDevices/cyd-2432s024r/Source/devices/Display.hDevices/cyd-2432s024r/cyd,2432s024r.dtsDevices/cyd-2432s024r/device.propertiesDevices/cyd-2432s024r/devicetree.yamlDevices/cyd-2432s024r/source/module.cppDevices/cyd-2432s028r/CMakeLists.txtDevices/cyd-2432s028r/Source/Configuration.cppDevices/cyd-2432s028r/Source/devices/Display.cppDevices/cyd-2432s028r/Source/devices/Display.hDevices/cyd-2432s028r/Source/module.cppDevices/cyd-2432s028r/cyd,2432s028r.dtsDevices/cyd-2432s028r/device.propertiesDevices/cyd-2432s028r/devicetree.yamlDevices/cyd-2432s028r/source/module.cppDevices/cyd-2432s028rv3/CMakeLists.txtDevices/cyd-2432s028rv3/Source/Configuration.cppDevices/cyd-2432s028rv3/Source/devices/Display.cppDevices/cyd-2432s028rv3/Source/devices/Display.hDevices/cyd-2432s028rv3/Source/module.cppDevices/cyd-2432s028rv3/cyd,2432s028rv3.dtsDevices/cyd-2432s028rv3/device.propertiesDevices/cyd-2432s028rv3/devicetree.yamlDevices/cyd-2432s028rv3/source/module.cppDevices/cyd-e32r28t/CMakeLists.txtDevices/cyd-e32r28t/Source/Configuration.cppDevices/cyd-e32r28t/Source/devices/Display.cppDevices/cyd-e32r28t/Source/devices/Display.hDevices/cyd-e32r28t/cyd,e32r28t.dtsDevices/cyd-e32r28t/device.propertiesDevices/cyd-e32r28t/devicetree.yamlDevices/cyd-e32r28t/source/module.cppDevices/cyd-e32r32p/device.propertiesDevices/elecrow-crowpanel-basic-28/device.propertiesDevices/elecrow-crowpanel-basic-35/device.propertiesDevices/lilygo-thmi/CMakeLists.txtDevices/lilygo-thmi/Source/Configuration.cppDevices/lilygo-thmi/Source/Init.cppDevices/lilygo-thmi/Source/devices/Display.cppDevices/lilygo-thmi/Source/devices/Display.hDevices/lilygo-thmi/Source/devices/Power.cppDevices/lilygo-thmi/Source/devices/Power.hDevices/lilygo-thmi/device.propertiesDevices/lilygo-thmi/devicetree.yamlDevices/lilygo-thmi/lilygo,thmi.dtsDevices/lilygo-thmi/source/module.cppDevices/unphone/device.propertiesDrivers/XPT2046/Source/Xpt2046Touch.cppDrivers/XPT2046/Source/Xpt2046Touch.hDrivers/XPT2046SoftSPI/Source/Xpt2046SoftSpi.cppDrivers/XPT2046SoftSPI/Source/Xpt2046SoftSpi.hDrivers/st7789-i8080-module/CMakeLists.txtDrivers/st7789-i8080-module/LICENSE-Apache-2.0.mdDrivers/st7789-i8080-module/README.mdDrivers/st7789-i8080-module/bindings/sitronix,st7789-i8080.yamlDrivers/st7789-i8080-module/devicetree.yamlDrivers/st7789-i8080-module/include/bindings/st7789_i8080.hDrivers/st7789-i8080-module/include/drivers/st7789_i8080.hDrivers/st7789-i8080-module/include/st7789_i8080_module.hDrivers/st7789-i8080-module/source/module.cppDrivers/st7789-i8080-module/source/st7789_i8080.cppDrivers/xpt2046-softspi-module/CMakeLists.txtDrivers/xpt2046-softspi-module/LICENSE-Apache-2.0.mdDrivers/xpt2046-softspi-module/README.mdDrivers/xpt2046-softspi-module/bindings/xptek,xpt2046-softspi.yamlDrivers/xpt2046-softspi-module/devicetree.yamlDrivers/xpt2046-softspi-module/include/bindings/xpt2046_softspi.hDrivers/xpt2046-softspi-module/include/drivers/xpt2046_softspi.hDrivers/xpt2046-softspi-module/include/xpt2046_softspi_module.hDrivers/xpt2046-softspi-module/source/module.cppDrivers/xpt2046-softspi-module/source/xpt2046_softspi.cppFirmware/KconfigModules/lvgl-module/include/tactility/lvgl_pointer.hModules/lvgl-module/source/lvgl_pointer.cPlatforms/platform-esp32/CMakeLists.txtPlatforms/platform-esp32/bindings/espressif,esp32-i8080.yamlPlatforms/platform-esp32/include/tactility/bindings/esp32_i8080.hPlatforms/platform-esp32/include/tactility/drivers/esp32_i8080.hPlatforms/platform-esp32/source/drivers/esp32_i8080.cppPlatforms/platform-esp32/source/module.cppTactility/Include/Tactility/app/touchcalibration/TouchCalibration.hTactility/Include/Tactility/settings/TouchCalibrationSettings.hTactility/Source/Tactility.cppTactility/Source/app/kerneldisplay/KernelDisplay.cppTactility/Source/app/setup/Setup.cppTactility/Source/app/timezone/TimeZone.cppTactility/Source/app/touchcalibration/TouchCalibration.cppTactility/Source/lvgl/Lvgl.cppTactility/Source/settings/TouchCalibrationSettings.cppTactilityKernel/bindings/i8080-controller.yamlTactilityKernel/bindings/tactility,gpio-hog.yamlTactilityKernel/include/tactility/bindings/gpio_hog.hTactilityKernel/include/tactility/drivers/gpio_hog.hTactilityKernel/include/tactility/drivers/i8080_controller.hTactilityKernel/source/drivers/gpio_hog.cppTactilityKernel/source/drivers/i8080_controller.cppTactilityKernel/source/kernel_init.cppdevice.py
💤 Files with no reviewable changes (23)
- Devices/cyd-2432s024r/Source/Configuration.cpp
- Devices/cyd-e32r28t/Source/devices/Display.h
- Devices/cyd-2432s028rv3/Source/module.cpp
- Devices/cyd-2432s028r/Source/Configuration.cpp
- Devices/cyd-2432s024r/Source/devices/Display.h
- Devices/cyd-2432s028rv3/Source/Configuration.cpp
- Devices/cyd-2432s028r/Source/module.cpp
- Devices/cyd-e32r28t/Source/Configuration.cpp
- Devices/cyd-2432s028rv3/Source/devices/Display.h
- Devices/cyd-2432s028r/Source/devices/Display.h
- Devices/lilygo-thmi/Source/devices/Display.h
- Devices/cyd-2432s024r/Source/devices/Display.cpp
- Devices/lilygo-thmi/Source/Configuration.cpp
- Drivers/XPT2046SoftSPI/Source/Xpt2046SoftSpi.cpp
- Drivers/XPT2046SoftSPI/Source/Xpt2046SoftSpi.h
- Devices/lilygo-thmi/Source/devices/Display.cpp
- Devices/cyd-2432s028r/Source/devices/Display.cpp
- Devices/lilygo-thmi/Source/Init.cpp
- Devices/lilygo-thmi/Source/devices/Power.h
- Devices/cyd-e32r28t/Source/devices/Display.cpp
- Devices/lilygo-thmi/Source/devices/Power.cpp
- Devices/cyd-2432s028rv3/Source/devices/Display.cpp
- Drivers/XPT2046/Source/Xpt2046Touch.h
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9002f61-e02b-4d06-a154-aee92e30156d
📒 Files selected for processing (16)
Devices/lilygo-thmi/lilygo,thmi.dtsDrivers/ili9341-module/bindings/ilitek,ili9341.yamlDrivers/ili9341-module/include/drivers/ili9341.hDrivers/ili9341-module/source/ili9341.cppDrivers/st7789-i8080-module/bindings/sitronix,st7789-i8080.yamlDrivers/st7789-i8080-module/include/drivers/st7789_i8080.hDrivers/st7789-i8080-module/source/st7789_i8080.cppDrivers/st7789-module/bindings/sitronix,st7789.yamlDrivers/st7789-module/include/drivers/st7789.hDrivers/st7789-module/source/st7789.cppPlatforms/platform-esp32/include/tactility/drivers/esp32_i8080.hPlatforms/platform-esp32/source/drivers/esp32_i8080.cppTactility/Include/Tactility/app/touchcalibration/TouchCalibration.hTactilityKernel/bindings/tactility,gpio-hog.yamlTactilityKernel/include/tactility/drivers/gpio_hog.hTactilityKernel/source/drivers/gpio_hog.cpp
🚧 Files skipped from review as they are similar to previous changes (8)
- Drivers/st7789-i8080-module/bindings/sitronix,st7789-i8080.yaml
- TactilityKernel/source/drivers/gpio_hog.cpp
- Drivers/st7789-i8080-module/include/drivers/st7789_i8080.h
- Tactility/Include/Tactility/app/touchcalibration/TouchCalibration.h
- Devices/lilygo-thmi/lilygo,thmi.dts
- Platforms/platform-esp32/include/tactility/drivers/esp32_i8080.h
- Platforms/platform-esp32/source/drivers/esp32_i8080.cpp
- Drivers/st7789-i8080-module/source/st7789_i8080.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6edf8067-e3cc-4deb-8991-465cbdc30f14
📒 Files selected for processing (4)
Devices/cyd-e32r32p/CMakeLists.txtDevices/cyd-e32r32p/cyd,e32r32p.dtsDevices/cyd-e32r32p/device.propertiesDevices/cyd-e32r32p/devicetree.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- Devices/cyd-e32r32p/device.properties
There was a problem hiding this comment.
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)
Devices/unphone/Source/InitBoot.cpp (1)
4-4: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
<tactility/check.h>(or keep the umbrella include)check()is not provided by the remaining headers;LOG_I/LOG_Eand the device APIs already are.
🧹 Nitpick comments (2)
Buildscripts/DevicetreeCompiler/source/generator.py (1)
121-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider gating on
type == "int"explicitly.The function works correctly today (non-numeric values are safely skipped via the try/except), but explicitly checking
binding_property.typebefore attempting the int parse would make the "only applies to int-typed properties" contract in the docstring self-enforcing rather than implicit.Drivers/hx8357-module/source/hx8357.cpp (1)
396-406: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: name the remaining magic opcodes for consistency.
0x28(DISPOFF) and0x10(SLPIN) are inlined with comments while every other opcode is a namedconstexpralongside the others at the top of the file.♻️ Proposed fix
constexpr uint8_t HX8357_SLPOUT = 0x11; +constexpr uint8_t HX8357_SLPIN = 0x10; constexpr uint8_t HX8357_INVOFF = 0x20; ... constexpr uint8_t HX8357_DISPON = 0x29; +constexpr uint8_t HX8357_DISPOFF = 0x28;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8bf64dcc-6603-499e-8a29-2eb27ef993ee
📒 Files selected for processing (46)
Buildscripts/DevicetreeCompiler/source/binding_parser.pyBuildscripts/DevicetreeCompiler/source/generator.pyBuildscripts/DevicetreeCompiler/source/models.pyBuildscripts/DevicetreeCompiler/tests/test_integration.pyDevices/cyd-e32r32p/cyd,e32r32p.dtsDevices/elecrow-crowpanel-basic-28/CMakeLists.txtDevices/elecrow-crowpanel-basic-28/Source/Configuration.cppDevices/elecrow-crowpanel-basic-28/Source/devices/Display.cppDevices/elecrow-crowpanel-basic-28/Source/devices/Display.hDevices/elecrow-crowpanel-basic-28/device.propertiesDevices/elecrow-crowpanel-basic-28/devicetree.yamlDevices/elecrow-crowpanel-basic-28/elecrow,crowpanel-basic-28.dtsDevices/elecrow-crowpanel-basic-28/source/module.cppDevices/unphone/CMakeLists.txtDevices/unphone/Source/Configuration.cppDevices/unphone/Source/InitBoot.cppDevices/unphone/Source/devices/Hx8357Display.cppDevices/unphone/Source/devices/Hx8357Display.hDevices/unphone/Source/devices/Touch.cppDevices/unphone/Source/devices/Touch.hDevices/unphone/Source/hx8357/README.mdDevices/unphone/Source/hx8357/disp_spi.cDevices/unphone/Source/hx8357/disp_spi.hDevices/unphone/Source/hx8357/hx8357.cDevices/unphone/Source/hx8357/hx8357.hDevices/unphone/Source/lvgl_spi_conf.hDevices/unphone/devicetree.yamlDevices/unphone/unphone.dtsDrivers/XPT2046/CMakeLists.txtDrivers/XPT2046/README.mdDrivers/XPT2046/Source/Xpt2046Touch.cppDrivers/XPT2046/Source/Xpt2046Touch.hDrivers/hx8357-module/CMakeLists.txtDrivers/hx8357-module/LICENSE-Apache-2.0.mdDrivers/hx8357-module/README.mdDrivers/hx8357-module/bindings/himax,hx8357.yamlDrivers/hx8357-module/devicetree.yamlDrivers/hx8357-module/include/bindings/hx8357.hDrivers/hx8357-module/include/drivers/hx8357.hDrivers/hx8357-module/include/hx8357_module.hDrivers/hx8357-module/source/hx8357.cppDrivers/hx8357-module/source/module.cppDrivers/ili9341-module/bindings/ilitek,ili9341.yamlDrivers/ili9488-module/bindings/ilitek,ili9488.yamlDrivers/st7789-i8080-module/bindings/sitronix,st7789-i8080.yamlDrivers/st7789-module/bindings/sitronix,st7789.yaml
💤 Files with no reviewable changes (17)
- Drivers/XPT2046/README.md
- Devices/unphone/Source/lvgl_spi_conf.h
- Drivers/XPT2046/CMakeLists.txt
- Drivers/XPT2046/Source/Xpt2046Touch.h
- Devices/elecrow-crowpanel-basic-28/Source/devices/Display.h
- Devices/unphone/Source/hx8357/hx8357.h
- Devices/unphone/Source/hx8357/disp_spi.h
- Devices/unphone/Source/hx8357/README.md
- Devices/unphone/Source/devices/Touch.cpp
- Devices/elecrow-crowpanel-basic-28/Source/devices/Display.cpp
- Devices/elecrow-crowpanel-basic-28/Source/Configuration.cpp
- Devices/unphone/Source/devices/Touch.h
- Devices/unphone/Source/hx8357/disp_spi.c
- Devices/unphone/Source/hx8357/hx8357.c
- Devices/unphone/Source/devices/Hx8357Display.cpp
- Devices/unphone/Source/devices/Hx8357Display.h
- Drivers/XPT2046/Source/Xpt2046Touch.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- Drivers/st7789-i8080-module/bindings/sitronix,st7789-i8080.yaml
- Drivers/ili9341-module/bindings/ilitek,ili9341.yaml
- Devices/cyd-e32r32p/cyd,e32r32p.dts
Uh oh!
There was an error while loading. Please reload this page.