New kernel drivers#560
Conversation
📝 WalkthroughWalkthroughThis change replaces touch placeholders with pointer placeholders, adds GT911, ST7789, ADC, backlight, keyboard, trackball, and battery-sense drivers, and introduces kernel driver APIs. LVGL gains device-backed display and input adapters. The LilyGO T-Deck device tree now describes its display, touch controller, keyboard, backlight, trackball, ADC, and battery sensing hardware. Boot, power, display, keyboard, status bar, launcher, and device registration paths are updated to use the new kernel devices. 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
Drivers/st7789-module/source/st7789.cpp (1)
106-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
esp_lcd_panel_invert_colorcall during bring-up.When
config->invert_coloris true,esp_lcd_panel_invert_color(panel, true)is called on line 109 (in the reset→init→invert chain) and again on line 118 (after swap_xy/mirror). On the ST7789, inversion (INVON/INVOFF, 0x20/0x21) is independent of the MADCTL register (0x36) that swap_xy/mirror modify, so the second call is a no-op. If the first call succeeds, the second is redundant; if the first fails,okis already false and the second is skipped via short-circuit. Line 118 adds no value.♻️ Remove redundant invert call
ok = ok && (!config->swap_xy || esp_lcd_panel_swap_xy(internal->panel_handle, true) == ESP_OK); ok = ok && ((!config->mirror_x && !config->mirror_y) || esp_lcd_panel_mirror(internal->panel_handle, config->mirror_x, config->mirror_y) == ESP_OK); -ok = ok && (!config->invert_color || esp_lcd_panel_invert_color(internal->panel_handle, true) == ESP_OK); ok = ok && esp_lcd_panel_disp_on_off(internal->panel_handle, true) == ESP_OK;Devices/lilygo-tdeck/Source/tdeck_keyboard_backlight.cpp (2)
60-64: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClear driver data after freeing in
stop.After
free(internal), the device still holds a dangling pointer viadevice_get_driver_data. Clearing it prevents use-after-free if any API call reaches the device post-stop.🛡️ Proposed fix
static error_t stop(Device* device) { auto* internal = static_cast<TdeckKeyboardBacklightInternal*>(device_get_driver_data(device)); free(internal); + device_set_driver_data(device, nullptr); return ERROR_NONE; }
44-57: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winState desync when initial brightness write fails.
startreturnsERROR_NONEeven if the I2C brightness writes at lines 47–55 fail. When theCMD_BRIGHTNESSwrite fails,current_brightnessstays at 0 while the hardware may have a different value, causingget_brightnessto report stale data.Consider either returning an error when the initial brightness write fails, or reading back the hardware state to initialize the cache.
Drivers/gt911-module/CMakeLists.txt (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding
CONFIGURE_DEPENDStoGLOB_RECURSE.Without
CONFIGURE_DEPENDS, CMake won't re-run when source files are added or removed undersource/, leading to stale builds. Sincecmake_minimum_requiredis 3.20 (well above the 3.12 requirement), the flag is available.♻️ Proposed fix
-file(GLOB_RECURSE SOURCE_FILES "source/*.c*") +file(GLOB_RECURSE SOURCE_FILES CONFIGURE_DEPENDS "source/*.c*")Tactility/Source/app/kerneldisplay/KernelDisplay.cpp (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace fragile relative includes with proper include paths.
Lines 1–2 use
../../../../TactilityKernel/include/...relative paths, while every other include in the file uses proper angle-bracket paths (e.g.,<tactility/drivers/backlight.h>,<tactility/device.h>). The project clearly has include directories configured for these paths. The relative includes will break if the file moves and are inconsistent with the codebase convention.♻️ Proposed fix
-#include "../../../../TactilityKernel/include/tactility/drivers/display.h" -#include "../../../../TactilityKernel/include/tactility/error.h" +#include <tactility/drivers/display.h> +#include <tactility/error.h>TactilityKernel/include/tactility/drivers/display.h (1)
226-227: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove duplicate
display_get_color_formatdeclaration.Lines 226 and 227 both declare
display_get_color_formatwith identical signatures. This is a copy-paste error — while valid in C, it's clearly unintentional.♻️ Proposed fix
enum DisplayColorFormat display_get_color_format(struct Device* device); -enum DisplayColorFormat display_get_color_format(struct Device* device);TactilityKernel/source/drivers/pointer.cpp (2)
9-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse consistent
Device*(withoutstructkeyword) across all function signatures.Line 9 uses
struct Device*while all other functions (lines 14–54) useDevice*. Both are equivalent in C++, but the inconsistency is noticeable.♻️ Consistency fix
-error_t pointer_enter_sleep(struct Device* device) { +error_t pointer_enter_sleep(Device* device) {
59-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop
structfromDeviceTypedeclaration for consistency withsdcard.cpp.
sdcard.cppwas just changed fromconst struct DeviceTypetoconst DeviceTypein this PR. The newPOINTER_TYPEdefinition here still usesstruct.♻️ Consistency fix
-const struct DeviceType POINTER_TYPE { +const DeviceType POINTER_TYPE {Tactility/Source/app/boot/Boot.cpp (1)
85-113: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSkip
backlight_set_brightnesswhendevice_startfails.If
device_start(backlight)fails on line 91, the code proceeds to callbacklight_set_brightness(backlight, ...)on line 102, which will also fail and produce a redundant error log. Consider early-returning or skipping the brightness call when the backlight device failed to start.♻️ Proposed refactor
if (!device_is_ready(backlight)) { if (device_start(backlight) != ERROR_NONE) { LOG_E(TAG, "Failed to start %s", backlight->name); + return; } }Devices/lilygo-tdeck/Source/module.cpp (1)
49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
statictosubscribe_events()for consistency.All other file-scope functions (
power_on,start,stop) are declaredstatic, butsubscribe_events()is not. This gives it unnecessary external linkage and risks symbol collision.♻️ Proposed fix
-void subscribe_events() { +static void subscribe_events() {Modules/lvgl-module/source/lvgl_display.c (1)
130-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
bpptobytes_per_pixel.lv_color_format_get_sizereturns bytes per pixel, sobppreads like bits-per-pixel and makes the buffer-size math easier to misread.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4eb3e1c7-91ef-48f9-a545-4a8a2fe3f52e
📒 Files selected for processing (100)
Buildscripts/DevicetreeCompiler/source/generator.pyDevices/cyd-2432s024r/cyd,2432s024r.dtsDevices/cyd-2432s028r/cyd,2432s028r.dtsDevices/cyd-2432s028rv3/cyd,2432s028rv3.dtsDevices/cyd-e32r28t/cyd,e32r28t.dtsDevices/cyd-e32r32p/cyd,e32r32p.dtsDevices/elecrow-crowpanel-basic-28/elecrow,crowpanel-basic-28.dtsDevices/elecrow-crowpanel-basic-35/elecrow,crowpanel-basic-35.dtsDevices/lilygo-tdeck/CMakeLists.txtDevices/lilygo-tdeck/Source/Configuration.cppDevices/lilygo-tdeck/Source/Drivers.cppDevices/lilygo-tdeck/Source/Init.cppDevices/lilygo-tdeck/Source/KeyboardBacklight/KeyboardBacklight.cppDevices/lilygo-tdeck/Source/KeyboardBacklight/KeyboardBacklight.hDevices/lilygo-tdeck/Source/bindings/tdeck_keyboard.hDevices/lilygo-tdeck/Source/bindings/tdeck_keyboard_backlight.hDevices/lilygo-tdeck/Source/devices/Display.cppDevices/lilygo-tdeck/Source/devices/Display.hDevices/lilygo-tdeck/Source/devices/KeyboardBacklight.cppDevices/lilygo-tdeck/Source/devices/KeyboardBacklight.hDevices/lilygo-tdeck/Source/devices/TdeckKeyboard.cppDevices/lilygo-tdeck/Source/devices/TdeckKeyboard.hDevices/lilygo-tdeck/Source/drivers/tdeck_keyboard.hDevices/lilygo-tdeck/Source/drivers/tdeck_keyboard_backlight.hDevices/lilygo-tdeck/Source/module.cppDevices/lilygo-tdeck/Source/tdeck_keyboard.cppDevices/lilygo-tdeck/Source/tdeck_keyboard_backlight.cppDevices/lilygo-tdeck/bindings/lilygo,tdeck-keyboard-backlight.yamlDevices/lilygo-tdeck/bindings/lilygo,tdeck-keyboard.yamlDevices/lilygo-tdeck/devicetree.yamlDevices/lilygo-tdeck/lilygo,tdeck.dtsDevices/lilygo-thmi/lilygo,thmi.dtsDevices/unphone/unphone.dtsDrivers/gt911-module/CMakeLists.txtDrivers/gt911-module/bindings/goodix,gt911.yamlDrivers/gt911-module/devicetree.yamlDrivers/gt911-module/include/bindings/gt911.hDrivers/gt911-module/include/drivers/gt911.hDrivers/gt911-module/include/gt911_module.hDrivers/gt911-module/source/gt911.cppDrivers/gt911-module/source/module.cppDrivers/st7789-module/CMakeLists.txtDrivers/st7789-module/bindings/sitronix,st7789.yamlDrivers/st7789-module/devicetree.yamlDrivers/st7789-module/include/bindings/st7789.hDrivers/st7789-module/include/drivers/st7789.hDrivers/st7789-module/include/st7789_module.hDrivers/st7789-module/source/module.cppDrivers/st7789-module/source/st7789.cppModules/lvgl-module/include/tactility/lvgl_display.hModules/lvgl-module/include/tactility/lvgl_keyboard.hModules/lvgl-module/include/tactility/lvgl_pointer.hModules/lvgl-module/source/arch/lvgl_esp32.cModules/lvgl-module/source/lvgl_devices.cModules/lvgl-module/source/lvgl_display.cModules/lvgl-module/source/lvgl_keyboard.cModules/lvgl-module/source/lvgl_pointer.cPlatforms/platform-esp32/bindings/espressif,esp32-ledc-backlight.yamlPlatforms/platform-esp32/include/tactility/bindings/esp32_ledc_backlight.hPlatforms/platform-esp32/include/tactility/drivers/esp32_ledc_backlight.hPlatforms/platform-esp32/source/drivers/esp32_ledc_backlight.cppPlatforms/platform-esp32/source/module.cppTactility/Include/Tactility/hal/display/KernelDisplayDriver.hTactility/Include/Tactility/hal/power/PowerDevice.hTactility/Include/Tactility/hal/touch/KernelTouchDriver.hTactility/Include/Tactility/hal/touch/TouchDriver.hTactility/Source/Tactility.cppTactility/Source/app/boot/Boot.cppTactility/Source/app/display/Display.cppTactility/Source/app/kerneldisplay/KernelDisplay.cppTactility/Source/app/keyboard/KeyboardSettings.cppTactility/Source/app/power/Power.cppTactility/Source/app/poweroff/PowerOff.cppTactility/Source/hal/display/KernelDisplayDriver.cppTactility/Source/hal/power/PowerDevice.cppTactility/Source/hal/touch/KernelTouchDriver.cppTactility/Source/lvgl/Keyboard.cppTactility/Source/lvgl/Lvgl.cppTactility/Source/service/keyboardidle/KeyboardIdle.cppTactilityKernel/bindings/pointer-placeholder.yamlTactilityKernel/bindings/touch-placeholder.yamlTactilityKernel/include/tactility/bindings/pointer_placeholder.hTactilityKernel/include/tactility/drivers/backlight.hTactilityKernel/include/tactility/drivers/display.hTactilityKernel/include/tactility/drivers/keyboard.hTactilityKernel/include/tactility/drivers/pointer.hTactilityKernel/include/tactility/drivers/pointer_placeholder.hTactilityKernel/include/tactility/drivers/power_supply.hTactilityKernel/source/drivers/backlight.cppTactilityKernel/source/drivers/display.cppTactilityKernel/source/drivers/grove.cppTactilityKernel/source/drivers/keyboard.cppTactilityKernel/source/drivers/pointer.cppTactilityKernel/source/drivers/pointer_placeholder.cppTactilityKernel/source/drivers/power_supply.cppTactilityKernel/source/drivers/rtc.cppTactilityKernel/source/drivers/sdcard.cppTactilityKernel/source/drivers/spi_controller.cppTactilityKernel/source/drivers/touch.cppTactilityKernel/source/kernel_init.cpp
💤 Files with no reviewable changes (12)
- TactilityKernel/bindings/touch-placeholder.yaml
- Devices/lilygo-tdeck/Source/devices/Display.h
- Devices/lilygo-tdeck/Source/Drivers.cpp
- Devices/lilygo-tdeck/Source/KeyboardBacklight/KeyboardBacklight.h
- TactilityKernel/source/drivers/touch.cpp
- Devices/lilygo-tdeck/Source/devices/KeyboardBacklight.h
- Devices/lilygo-tdeck/Source/Init.cpp
- Devices/lilygo-tdeck/Source/devices/TdeckKeyboard.cpp
- Devices/lilygo-tdeck/Source/devices/KeyboardBacklight.cpp
- Devices/lilygo-tdeck/Source/devices/Display.cpp
- Devices/lilygo-tdeck/Source/devices/TdeckKeyboard.h
- Devices/lilygo-tdeck/Source/KeyboardBacklight/KeyboardBacklight.cpp
| static error_t stop(Device* device) { | ||
| auto* internal = static_cast<Gt911Internal*>(device_get_driver_data(device)); | ||
|
|
||
| if (esp_lcd_touch_del(internal->touch_handle) != ESP_OK) { | ||
| LOG_E(TAG, "Failed to delete touch handle"); | ||
| return ERROR_RESOURCE; | ||
| } | ||
|
|
||
| free(internal); | ||
| return ERROR_NONE; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the driver and related cleanup calls.
git ls-files 'Drivers/gt911-module/source/gt911.cpp' 'Drivers/gt911-module/**' | sed 's#^`#FILE` #'
echo
rg -n "esp_lcd_touch_del|esp_lcd_panel_io_del|stop\\(|start\\(" Drivers/gt911-module/source/gt911.cpp Drivers/gt911-module -S
echo
# Show the relevant section with line numbers.
cat -n Drivers/gt911-module/source/gt911.cpp | sed -n '1,220p'Repository: TactilityProject/Tactility
Length of output: 10697
🌐 Web query:
esp_lcd_touch_del does it delete esp_lcd_panel_io handle GT911 ESP-IDF
💡 Result:
No, esp_lcd_touch_del does not delete or free the esp_lcd_panel_io_handle_t handle [1][2]. The function esp_lcd_touch_del is responsible only for cleaning up resources specific to the touch controller itself, such as resetting GPIO pins (e.g., interrupt and reset pins) and freeing the memory allocated for the touch controller handle [1][3]. It does not have ownership of the communication interface (panel IO) that was passed into the touch initialization function [4][5]. To properly clean up resources, you must delete the handles in the correct order: 1. Call esp_lcd_touch_del(touch_handle) to free the touch controller resources [1][6]. 2. Call esp_lcd_panel_io_del(io_handle) to free the LCD panel IO handle [7][8]. This separation of concerns is necessary because the same panel IO handle might be shared or used by other components, and the touch driver simply consumes it as a dependency for communication [4][5].
Citations:
- 1: https://github.com/espressif/esp-bsp/blob/master/components/lcd_touch/esp_lcd_touch_gt911/esp_lcd_touch_gt911.c
- 2: https://github.com/espressif/esp-bsp/blob/4b3c07fb/components/lcd_touch/esp_lcd_touch/esp_lcd_touch.c
- 3: espressif/esp-bsp@098bd8e
- 4: https://github.com/espressif/esp-iot-solution/blob/master/docs/en/input%5Fdevice/touch%5Fpanel.rst
- 5: https://github.com/espressif/esp-bsp/tree/master/components/lcd_touch/esp_lcd_touch_gt911
- 6: https://components.espressif.com/components/espressif/m5dial/versions/3.1.0/api?language=en
- 7: Make LVGL/esp_lvgl_port to switch/reinitiate between big and little framebuffers espressif/esp-bsp#707
- 8: https://github.com/espressif/esp-idf/blob/12f36a02/components/esp_lcd/include/esp_lcd_panel_io.h
Delete the panel IO handle in stop() and always free internal
esp_lcd_touch_del() only releases the touch-side resources; internal->io_handle is still owned here and needs its own esp_lcd_panel_io_del(). Also, the early return on esp_lcd_touch_del() failure leaves internal allocated.
| initialized = true; | ||
|
|
||
| lvgl_devices_attach(); | ||
|
|
||
| if (lvgl_module_config.on_start) lvgl_module_config.on_start(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if lvgl_lock is called anywhere in the arch start path
rg -n "lvgl_lock\|lv_lock\|port_lock" Modules/lvgl-module/source/ --type c
# Check the esp_lvgl_port init behavior regarding task creation
rg -n "lvgl_port_init\|xTaskCreate\|task_create" Modules/lvgl-module/ Platforms/platform-esp32/ --type c --type cppRepository: TactilityProject/Tactility
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant files
fd -a 'lvgl_esp32.c|lvgl_devices_attach|esp_lvgl_port' Modules/lvgl-module Platforms . || true
# Find exact definitions/usages
rg -n "lvgl_devices_attach|lvgl_port_init|lvgl_lock|lv_lock|port_lock|xTaskCreate" Modules/lvgl-module Platforms -S || true
# Show file outline for the target file if present
if [ -f "Modules/lvgl-module/source/arch/lvgl_esp32.c" ]; then
ast-grep outline Modules/lvgl-module/source/arch/lvgl_esp32.c --view expanded || true
fiRepository: TactilityProject/Tactility
Length of output: 3120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a 'lvgl_esp32.c|lvgl_devices_attach|esp_lvgl_port' Modules/lvgl-module Platforms . || true
rg -n "lvgl_devices_attach|lvgl_port_init|lvgl_lock|lv_lock|port_lock|xTaskCreate" Modules/lvgl-module Platforms -S || true
if [ -f "Modules/lvgl-module/source/arch/lvgl_esp32.c" ]; then
ast-grep outline Modules/lvgl-module/source/arch/lvgl_esp32.c --view expanded || true
fiRepository: TactilityProject/Tactility
Length of output: 3120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file =="
git ls-files 'Modules/lvgl-module/source/arch/lvgl_esp32.c' || true
echo "== possible attach definition/usages =="
rg -n "lvgl_devices_attach" Modules/lvgl-module -S || true
echo "== likely init paths =="
rg -n "lvgl_port_init|lvgl_arch_start|on_start|lvgl_pointer_add|lvgl_keyboard_add" Modules/lvgl-module -S || true
echo "== target file excerpt =="
if [ -f "Modules/lvgl-module/source/arch/lvgl_esp32.c" ]; then
sed -n '1,140p' Modules/lvgl-module/source/arch/lvgl_esp32.c
fiRepository: TactilityProject/Tactility
Length of output: 5092
🌐 Web query:
esp_lvgl_port lvgl_port_init task starts before return lock requirement lvgl_port_lock docs
💡 Result:
The esp_lvgl_port component is designed so that lvgl_port_init initializes the LVGL subsystem and spawns a background task to handle LVGL processing (such as timers and input devices) [1][2][3]. Regarding your query on initialization and locking, here is the breakdown: Initialization Timing and Task Synchronization: Historical versions of esp_lvgl_port had issues where the background task would start before initialization was fully completed, potentially causing crashes if called from high-priority tasks [4][5][6]. Recent versions have addressed this by implementing task notification and synchronization mechanisms to ensure lvgl_port_init completes its necessary setup before returning [4]. You should ensure you are using a recent version of the esp_lvgl_port component to benefit from these fixes [6][7]. LVGL Port Lock Requirement: The lvgl_port_lock and lvgl_port_unlock functions are mandatory for protecting all LVGL API calls [7][8][9]. Because LVGL is not thread-safe by default, any interaction with LVGL objects (like creating UI elements, updating labels, or changing screens) from a task other than the one running the LVGL background handler must be enclosed within these lock/unlock commands [8][9]. Usage Pattern: 1. Initialize the port: Call lvgl_port_init in your application setup (typically in app_main) [3]. 2. Protect API calls: Always wrap your LVGL function calls with a lock/unlock pair [7][8]. Example: lvgl_port_lock(0); // 0 blocks indefinitely until the mutex is acquired lv_obj_t * screen = lv_disp_get_scr_act(disp_handle); lv_obj_t * obj = lv_label_create(screen); lvgl_port_unlock; // Release the mutex Always refer to the official esp_lvgl_port component documentation for the version you are using, as implementation details can evolve [7][3].
Citations:
- 1: https://github.com/espressif/esp-bsp/blob/4b3c07fb/components/esp_lvgl_port/src/lvgl9/esp_lvgl_port.c
- 2: https://github.com/espressif/esp-bsp/blob/4b3c07fb/components/esp_lvgl_port/src/lvgl8/esp_lvgl_port.c
- 3: https://lvgl.io/docs/open/9.5/integration/chip_vendors/espressif/add_lvgl_to_esp32_idf_project
- 4: fix(esp_lvgl_port): Make sure LVGL is initialize at the end of lvgl_port_init() call espressif/esp-bsp#468
- 5: Potential Exception Due to Task Priority in
esp_lvgl_portwith LVGL9 (BSP-596) espressif/esp-bsp#455 - 6: https://components.espressif.com/components/espressif/esp_lvgl_port/versions/2.7.0/changelog?language=
- 7: https://components.espressif.com/components/espressif/esp_lvgl_port
- 8: https://github.com/espressif/esp-bsp/tree/master/components/esp_lvgl_port
- 9: https://github.com/espressif/esp-bsp/blob/4b3c07fb/components/esp_lvgl_port/README.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' Modules/lvgl-module/source/lvgl_devices.cRepository: TactilityProject/Tactility
Length of output: 2271
Hold the LVGL lock around lvgl_devices_attach(). lvgl_pointer_add() and lvgl_keyboard_add() require the caller to own the lock, and this runs after lvgl_port_init() has already brought up the LVGL task. Wrap the attach call in lvgl_lock()/lvgl_unlock() or move it into a locked start hook.
| void lvgl_devices_attach() { | ||
| lv_disp_t* lvgl_display = NULL; | ||
| struct Device* kernel_display_device = device_find_first_by_type(&DISPLAY_TYPE); | ||
| if (kernel_display_device != NULL) { | ||
| struct LvglDisplayConfig lvgl_display_config = {}; | ||
| if (lvgl_display_add(kernel_display_device, &lvgl_display_config, &lvgl_display) == ERROR_NONE) { | ||
| LOG_I(TAG, "Bound %s to LVGL", kernel_display_device->name); | ||
| } else { | ||
| LOG_E(TAG, "Failed to bind %s to LVGL", kernel_display_device->name); | ||
| } | ||
| } | ||
|
|
||
| struct Device* kernel_pointer_device = device_find_first_by_type(&POINTER_TYPE); | ||
| lv_indev_t* lvgl_pointer_device; | ||
| if (kernel_pointer_device != NULL) { | ||
| if (lvgl_pointer_add(kernel_pointer_device, lvgl_display, &lvgl_pointer_device) == ERROR_NONE) { | ||
| LOG_I(TAG, "Bound %s to LVGL", kernel_pointer_device->name); | ||
| } else { | ||
| LOG_E(TAG, "Failed to bind %s to LVG", kernel_pointer_device->name); | ||
| } | ||
| } | ||
|
|
||
| struct Device* kernel_keyboard_device = device_find_first_by_type(&KEYBOARD_TYPE); | ||
| lv_indev_t* lvgl_keyboard_device; | ||
| if (kernel_keyboard_device != NULL) { | ||
| if (lvgl_keyboard_add(kernel_keyboard_device, lvgl_display, &lvgl_keyboard_device) == ERROR_NONE) { | ||
| LOG_I(TAG, "Bound %s to LVGL", kernel_keyboard_device->name); | ||
| } else { | ||
| LOG_E(TAG, "Failed to bind %s to LVGL", kernel_keyboard_device->name); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if lvgl_display_remove exists
rg -n "lvgl_display_remove\|lv_display_remove\|lv_disp_remove" Modules/lvgl-module/ --type c --type h
# Check what lvgl_display_add allocates
rg -n "lvgl_display_add" Modules/lvgl-module/ --type c --type h -C5Repository: TactilityProject/Tactility
Length of output: 3686
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect detach implementation and related display lifecycle code
ast-grep outline Modules/lvgl-module/source/lvgl_devices.c --view expanded
printf '\n--- lvgl_devices.c (detach region) ---\n'
sed -n '1,220p' Modules/lvgl-module/source/lvgl_devices.c
printf '\n--- lvgl_display.h ---\n'
sed -n '1,140p' Modules/lvgl-module/include/tactility/lvgl_display.h
printf '\n--- lvgl_display.c (remove implementation) ---\n'
rg -n "lvgl_display_remove|lvgl_display_add|lv_display_delete|lv_display_remove|lv_indev_delete" Modules/lvgl-module/source/lvgl_display.c -C 6Repository: TactilityProject/Tactility
Length of output: 5878
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all callers/usages of the attach/detach and display remove APIs
rg -n "lvgl_devices_attach|lvgl_devices_detach|lvgl_display_remove|lvgl_display_add" Modules/lvgl-module/ -C 3
printf '\n--- module start/stop wiring ---\n'
rg -n "on_start|on_stop|lvgl_lock|lvgl_devices_" Modules/lvgl-module/ -C 3
printf '\n--- LVGL display lifecycle details ---\n'
sed -n '196,240p' Modules/lvgl-module/source/lvgl_display.cRepository: TactilityProject/Tactility
Length of output: 17113
Call lvgl_display_remove during detach. lvgl_devices_attach() creates an lv_display_t, but lvgl_devices_detach() only deletes indevs. lvgl_display_remove() already frees the display and any owned buffers, so keep that handle around and remove it before shutdown.
| void lvgl_devices_detach() { | ||
| lv_indev_t* device = lv_indev_get_next(NULL); | ||
| while (device != NULL) { | ||
| lv_indev_delete(device); | ||
| // Always get the first item, because getting the next one doesn't work as the current pointer just became corrupt | ||
| device = lv_indev_get_next(NULL); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Memory leak: lvgl_devices_detach bypasses context cleanup.
lv_indev_delete does not free the LvglPointerCtx/LvglKeyboardCtx structs that were malloc'd in lvgl_pointer_add/lvgl_keyboard_add. Every attach/detach cycle leaks these allocations. Use lvgl_pointer_remove and lvgl_keyboard_remove instead, which properly retrieve and free the driver data before deleting the indev.
🔒 Proposed fix: use typed remove functions in detach
void lvgl_devices_detach() {
lv_indev_t* device = lv_indev_get_next(NULL);
while (device != NULL) {
- lv_indev_delete(device);
- // Always get the first item, because getting the next one doesn't work as the current pointer just became corrupt
- device = lv_indev_get_next(NULL);
+ lv_indev_type_t type = lv_indev_get_type(device);
+ if (type == LV_INDEV_TYPE_POINTER) {
+ lvgl_pointer_remove(device);
+ } else if (type == LV_INDEV_TYPE_KEYPAD) {
+ lvgl_keyboard_remove(device);
+ } else {
+ lv_indev_delete(device);
+ }
+ // Always get the first item, because getting the next one doesn't work as the current pointer just became corrupt
+ device = lv_indev_get_next(NULL);
}
}📝 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.
| void lvgl_devices_detach() { | |
| lv_indev_t* device = lv_indev_get_next(NULL); | |
| while (device != NULL) { | |
| lv_indev_delete(device); | |
| // Always get the first item, because getting the next one doesn't work as the current pointer just became corrupt | |
| device = lv_indev_get_next(NULL); | |
| } | |
| } | |
| void lvgl_devices_detach() { | |
| lv_indev_t* device = lv_indev_get_next(NULL); | |
| while (device != NULL) { | |
| lv_indev_type_t type = lv_indev_get_type(device); | |
| if (type == LV_INDEV_TYPE_POINTER) { | |
| lvgl_pointer_remove(device); | |
| } else if (type == LV_INDEV_TYPE_KEYPAD) { | |
| lvgl_keyboard_remove(device); | |
| } else { | |
| lv_indev_delete(device); | |
| } | |
| // Always get the first item, because getting the next one doesn't work as the current pointer just became corrupt | |
| device = lv_indev_get_next(NULL); | |
| } | |
| } |
| static error_t stop(Device* device) { | ||
| auto* internal = static_cast<Esp32LedcBacklightInternal*>(device_get_driver_data(device)); | ||
| free(internal); | ||
| return ERROR_NONE; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the reported lines.
FILE="Platforms/platform-esp32/source/drivers/esp32_ledc_backlight.cpp"
wc -l "$FILE"
sed -n '1,180p' "$FILE"
# Find LEDC usage and any cleanup paths in the esp32 platform sources.
rg -n "ledc_stop|ledc_timer|ledc_channel|stop\(Device\* device\)|Esp32LedcBacklightInternal|device_get_driver_data" Platforms/platform-esp32 -S
# If there are other backlight drivers, inspect their stop/cleanup pattern.
fd -a ".*backlight.*\.cpp$" Platforms/platform-esp32/source || trueRepository: TactilityProject/Tactility
Length of output: 15882
🌐 Web query:
ESP-IDF ledc_stop semantics channel timer remain configured reuse output off documentation
💡 Result:
In the ESP-IDF framework, the function ledc_stop is used to suspend the signal generation on a specific LEDC channel [1][2]. Its semantics regarding the channel and timer configuration are as follows: 1. Channel Configuration: Calling ledc_stop disables the LEDC output for the specified channel and sets the output to the idle level defined by the idle_level parameter [1][3][2]. It does not deconfigure or destroy the channel's association with its assigned timer; the channel remains configured and ready to be restarted (typically by re-initializing the duty cycle or calling ledc_channel_config if necessary) [1][4]. 2. Timer Configuration: The function ledc_stop does not affect the configuration or the running state of the timer bound to the channel [1][4]. The timer continues to operate independently of the stop status of individual channels [1][4]. If you need to stop the timer, you must use ledc_timer_pause [5][6]. 3. Reusability: Because ledc_stop only affects the output state of the channel, the channel and its assigned timer remain fully configured in the driver's internal state [1][4]. If your goal is to "reuse" these resources, they remain reserved and configured as they were before the stop command [1][4]. If you intend to completely remove a channel's configuration or a timer to free it for a different use (e.g., re-binding to a different pin or changing settings entirely), you generally do not need to call ledc_stop as a prerequisite for reconfiguration, but you may need to call ledc_channel_config again with new parameters to overwrite the old settings [1][4]. Timers, specifically, must be paused before they can be deconfigured using the deconfigure field in ledc_timer_config_t [1][4]. In summary, ledc_stop is a light-weight suspension mechanism that does not release or clear the underlying hardware configuration [1][2].
Citations:
- 1: https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/peripherals/ledc.html
- 2: https://docs.espressif.com/projects/esp-idf/en/v6.0.1/esp32/api-reference/peripherals/ledc.html
- 3: https://github.com/espressif/esp-idf/blob/v5.5/components/esp_driver_ledc/include/driver/ledc.h
- 4: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/peripherals/ledc.html?highlight=ledc
- 5: https://github.com/espressif/esp-idf/blob/master/components/esp_driver_ledc/src/ledc.c
- 6: https://github.com/espressif/esp-idf/blob/78a04b36/components/esp_driver_ledc/include/driver/ledc.h
Turn off the LEDC output in stop()
Call ledc_stop() before freeing the internal state; otherwise the backlight stays at its last duty cycle after the device is stopped. ledc_stop() only disables the channel output, so it won’t deconfigure the timer/channel.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
TactilityKernel/source/drivers/battery_sense.cpp (1)
11-12: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
ADC_MAX_RAWis hardcoded for 12-bit ADC.The comment acknowledges this limitation. For the T-Deck's
ADC_BITWIDTH_DEFAULTon ESP32 (12-bit), this is correct. However, if this driver is reused on a board configured with a different ADC bitwidth, the voltage calculation will be silently incorrect. Consider documenting this as a driver constraint or exposing the ADC resolution through the generic ADC API in a future iteration.Platforms/platform-esp32/source/drivers/esp32_adc_oneshot.cpp (1)
25-38: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDocument the ignored
timeoutargument in the ESP32 oneshot ADC backend.
adc_oneshot_read()has no timeout parameter, so this driver always drops the API’stimeoutvalue. Add a note in the backend or interface so callers know this path does not enforce per-call deadlines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b658f5a7-eb3b-41df-b037-8198069b4daf
📒 Files selected for processing (24)
Devices/lilygo-tdeck/CMakeLists.txtDevices/lilygo-tdeck/Source/Configuration.cppDevices/lilygo-tdeck/Source/devices/Power.cppDevices/lilygo-tdeck/Source/devices/Power.hDevices/lilygo-tdeck/Source/module.cppDevices/lilygo-tdeck/Source/tdeck_keyboard.cppDevices/lilygo-tdeck/lilygo,tdeck.dtsPlatforms/platform-esp32/CMakeLists.txtPlatforms/platform-esp32/bindings/espressif,esp32-adc-oneshot.yamlPlatforms/platform-esp32/include/tactility/bindings/esp32_adc_oneshot.hPlatforms/platform-esp32/include/tactility/drivers/esp32_adc_oneshot.hPlatforms/platform-esp32/source/drivers/esp32_adc_oneshot.cppPlatforms/platform-esp32/source/module.cppTactility/Source/Tactility.cppTactility/Source/service/statusbar/Statusbar.cppTactilityKernel/bindings/adc-controller.yamlTactilityKernel/bindings/battery-sense.yamlTactilityKernel/include/tactility/bindings/battery_sense.hTactilityKernel/include/tactility/drivers/adc.hTactilityKernel/include/tactility/drivers/adc_controller.hTactilityKernel/include/tactility/drivers/battery_sense.hTactilityKernel/source/drivers/adc_controller.cppTactilityKernel/source/drivers/battery_sense.cppTactilityKernel/source/kernel_init.cpp
💤 Files with no reviewable changes (3)
- Devices/lilygo-tdeck/Source/devices/Power.cpp
- Devices/lilygo-tdeck/Source/devices/Power.h
- Devices/lilygo-tdeck/Source/Configuration.cpp
✅ Files skipped from review due to trivial changes (2)
- TactilityKernel/bindings/adc-controller.yaml
- TactilityKernel/include/tactility/bindings/battery_sense.h
🚧 Files skipped from review as they are similar to previous changes (4)
- Devices/lilygo-tdeck/CMakeLists.txt
- Tactility/Source/Tactility.cpp
- Devices/lilygo-tdeck/Source/tdeck_keyboard.cpp
- Devices/lilygo-tdeck/Source/module.cpp
| static error_t stop(Device* device) { | ||
| LOG_I(TAG, "stop %s", device->name); | ||
| adc_oneshot_del_unit(GET_HANDLE(device)); | ||
| device_set_driver_data(device, nullptr); | ||
| return ERROR_NONE; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unchecked return value of adc_oneshot_del_unit in stop().
If adc_oneshot_del_unit fails, the function still returns ERROR_NONE, potentially masking a resource leak. Consider logging the error for debuggability.
🔧 Suggested improvement
static error_t stop(Device* device) {
LOG_I(TAG, "stop %s", device->name);
- adc_oneshot_del_unit(GET_HANDLE(device));
+ esp_err_t esp_error = adc_oneshot_del_unit(GET_HANDLE(device));
+ if (esp_error != ESP_OK) {
+ LOG_E(TAG, "Failed to delete ADC unit: %s", esp_err_to_name(esp_error));
+ }
device_set_driver_data(device, nullptr);
return ERROR_NONE;
}📝 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.
| static error_t stop(Device* device) { | |
| LOG_I(TAG, "stop %s", device->name); | |
| adc_oneshot_del_unit(GET_HANDLE(device)); | |
| device_set_driver_data(device, nullptr); | |
| return ERROR_NONE; | |
| } | |
| static error_t stop(Device* device) { | |
| LOG_I(TAG, "stop %s", device->name); | |
| esp_err_t esp_error = adc_oneshot_del_unit(GET_HANDLE(device)); | |
| if (esp_error != ESP_OK) { | |
| LOG_E(TAG, "Failed to delete ADC unit: %s", esp_err_to_name(esp_error)); | |
| } | |
| device_set_driver_data(device, nullptr); | |
| return ERROR_NONE; | |
| } |
| io-channels: | ||
| type: phandles | ||
| required: true | ||
| description: The ADC channel connected to the battery sense circuit, e.g. `<&adc1 3>` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
io-channels type may not parse the channel index argument.
The DTS uses io-channels = <&adc0 0> where 0 is the channel index, and BatterySenseConfig.io_channel is an AdcChannelSpec with both adc_controller and channel fields. However, the binding declares type: phandles, which in standard devicetree semantics only parses phandle references without arguments. The ADC oneshot binding uses type: phandle-array with element-type for similar phandle+argument patterns.
If the custom devicetree compiler's phandles type doesn't parse the argument, the channel field would default to 0 — which happens to match the T-Deck DTS but would silently break on boards with a non-zero channel index.
#!/bin/bash
# Check the battery_sense binding header to see how io-channels is mapped
cat TactilityKernel/include/tactility/bindings/battery_sense.h
# Check the adc-controller base binding for reference
cat TactilityKernel/bindings/adc-controller.yaml
# Check how the devicetree compiler handles phandles vs phandle-array
rg -n "phandle" Buildscripts/DevicetreeCompiler/source/generator.py
# Check the esp32_adc_oneshot binding header for comparison
cat Platforms/platform-esp32/include/tactility/bindings/esp32_adc_oneshot.h
# Search for other bindings using io-channels or phandle-array for comparison
rg -n "io-channels|phandle-array|phandles" TactilityKernel/bindings/ Platforms/*/bindings/ Drivers/*/bindings/ Devices/*/bindings/ --type yaml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Devices/lilygo-tdeck/source/module.cpp (1)
99-112: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMissing driver cleanup on error path in
start().If
driver_construct_add(&tdeck_keyboard_backlight_driver)ordriver_construct_add(&tdeck_trackball_driver)fails, previously registered drivers are not unregistered, leaking resources. Add rollback for each failure case.🔧 Proposed fix: add cleanup on error paths
static error_t start() { LOG_I(TAG, LOG_MESSAGE_POWER_ON_START); if (!power_on()) { LOG_E(TAG, LOG_MESSAGE_POWER_ON_FAILED); return ERROR_RESOURCE; } if (driver_construct_add(&tdeck_keyboard_driver) != ERROR_NONE) { LOG_E(TAG, "Failed to register keyboard driver"); return ERROR_RESOURCE; } if (driver_construct_add(&tdeck_keyboard_backlight_driver) != ERROR_NONE) { LOG_E(TAG, "Failed to register keyboard backlight driver"); + driver_remove_destruct(&tdeck_keyboard_driver); return ERROR_RESOURCE; } if (driver_construct_add(&tdeck_trackball_driver) != ERROR_NONE) { LOG_E(TAG, "Failed to register trackball driver"); + driver_remove_destruct(&tdeck_keyboard_backlight_driver); + driver_remove_destruct(&tdeck_keyboard_driver); return ERROR_RESOURCE; } subscribe_events(); return ERROR_NONE; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef94159f-7c42-4f25-9df2-8f8c860e1727
📒 Files selected for processing (20)
Devices/lilygo-tdeck/CMakeLists.txtDevices/lilygo-tdeck/Source/Trackball/Trackball.cppDevices/lilygo-tdeck/Source/devices/TrackballDevice.cppDevices/lilygo-tdeck/Source/devices/TrackballDevice.hDevices/lilygo-tdeck/bindings/lilygo,tdeck-trackball.yamlDevices/lilygo-tdeck/include/bindings/tdeck_keyboard.hDevices/lilygo-tdeck/include/bindings/tdeck_keyboard_backlight.hDevices/lilygo-tdeck/include/bindings/tdeck_trackball.hDevices/lilygo-tdeck/include/drivers/tdeck_keyboard.hDevices/lilygo-tdeck/include/drivers/tdeck_keyboard_backlight.hDevices/lilygo-tdeck/include/drivers/tdeck_trackball.hDevices/lilygo-tdeck/include/drivers/trackball.hDevices/lilygo-tdeck/lilygo,tdeck.dtsDevices/lilygo-tdeck/source/drivers/tdeck_keyboard.cppDevices/lilygo-tdeck/source/drivers/tdeck_keyboard_backlight.cppDevices/lilygo-tdeck/source/drivers/tdeck_trackball.cppDevices/lilygo-tdeck/source/drivers/trackball.cppDevices/lilygo-tdeck/source/module.cppTactility/Source/app/launcher/Launcher.cppTactilityKernel/source/drivers/battery_sense.cpp
💤 Files with no reviewable changes (3)
- Devices/lilygo-tdeck/Source/devices/TrackballDevice.h
- Devices/lilygo-tdeck/Source/devices/TrackballDevice.cpp
- Devices/lilygo-tdeck/Source/Trackball/Trackball.cpp
✅ Files skipped from review due to trivial changes (1)
- Devices/lilygo-tdeck/include/drivers/tdeck_trackball.h
🚧 Files skipped from review as they are similar to previous changes (2)
- Devices/lilygo-tdeck/CMakeLists.txt
- Devices/lilygo-tdeck/lilygo,tdeck.dts
1 participant
Summary by CodeRabbit
New Features
Bug Fixes