M5Stack Tab5 support #449
M5Stack Tab5 support #449
Conversation
📝 WalkthroughWalkthroughThis pull request adds support for the M5Stack Tab5 device. It introduces the M5Stack Tab5 as a new build target with ESP32P4 architecture by adding device configuration files, hardware initialization routines, display driver integration for an ILI9881C MIPI-DSI display with touch support, SD card mounting functionality, and firmware configuration options. The changes also update a device name in an existing device configuration, modify logging terminology in the hardware initialization layer, and update application UI text. A new ESP-IDF component dependency for the ILI9881C LCD controller is added to support the display hardware. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 7
🤖 Fix all issues with AI agents
In @Devices/guition-jc1060p470ciwy/device.properties:
- Line 3: Update all remaining usages of the old device name format
`JC1060P470CIWY` to the new hyphenated form `JC1060P470C-I-W-Y`: change the
Kconfig symbol `TT_DEVICE_GUITION_JC1060P470CIWY` and its human-readable
description "Guition JC1060P470CIWY" to use the new name, update the logger
reference in Devices/guition-jc1060p470ciwy/Source/devices/Display.cpp that
emits the device name, and revise the comment mentions in
Devices/guition-jc1060p470ciwy/Source/devices/SdCard.h and
Devices/m5stack-tab5/Source/devices/SdCard.h to the hyphenated
`JC1060P470C-I-W-Y` so all configuration, runtime identifiers, and documentation
are consistent.
In @Devices/m5stack-tab5/Source/Configuration.cpp:
- Around line 70-82: The LOGGER.error messages are swapped: the call to
i2c::masterWriteRegisterArray with IO_EXPANDER2_ADDRESS should log "IO expander
2 ..." and the call with IO_EXPANDER1_ADDRESS should log "IO expander 1 ...".
Update the error strings in those two LOGGER.error calls to match the
corresponding IO_EXPANDER1_ADDRESS and IO_EXPANDER2_ADDRESS symbols (e.g.,
change "IO expander 1 init failed in phase 2" to reference expander 2, and "IO
expander 2 init failed" to reference expander 1).
In @Devices/m5stack-tab5/Source/devices/Display.cpp:
- Around line 62-64: The code uses std::reinterpret_pointer_cast to convert the
shared_ptr of Ili9881cDisplay to tt::hal::display::DisplayDevice; replace this
with std::static_pointer_cast because Ili9881cDisplay inherits through
EspLcdDisplayV2 to DisplayDevice, so use
std::static_pointer_cast<tt::hal::display::DisplayDevice>(display) to perform a
safe cast that respects the inheritance hierarchy (locate the conversion in the
return statement where display is created as
std::make_shared<Ili9881cDisplay>(configuration)).
In @Devices/m5stack-tab5/Source/devices/Ili9881cDisplay.cpp:
- Around line 116-133: The vendor_config (ili9881c_vendor_config_t
vendor_config) is stack-local but its address is stored in mutable_panel_config
and passed to esp_lcd_new_panel_ili9881c, leaving a dangling pointer after the
function returns; fix by making vendor_config persist for the panel
lifetime—e.g., move it out of the function into a persistent storage: add a
member ili9881c_vendor_config_t vendor_config; to the Ili9881cDisplay class (or
declare it static within the function if only one instance is expected),
initialize that persistent vendor_config (set init_cmds, init_cmds_size,
mipi_config) and then assign mutable_panel_config.vendor_config =
&this->vendor_config before calling esp_lcd_new_panel_ili9881c so the pointer
remains valid for the panelHandle lifetime.
In @Devices/m5stack-tab5/Source/devices/SdCard.cpp:
- Around line 62-76: The LDO channel acquired via esp_ldo_acquire_channel (local
variable ldo_handle) in mount() is never released, causing a resource leak; make
ldo_handle a persistent member (e.g., class field) and assign the acquired
handle to it in mount(), then call esp_ldo_release_channel (or the appropriate
release API) on that member in unmount() and in error/cleanup paths (including
mount failure and destructor) and null out the member after release to ensure
safe repeated mount/unmount cycles.
In @Devices/m5stack-tab5/Source/devices/SdCard.h:
- Around line 5-6: The file-level comment incorrectly names the device as
"jc1060p470ciwy"; update the comment above the createSdCard declaration to
reference the correct device "M5Stack Tab5" (keeping the rest of the comment
about SDMMC slot 0 (4-bit) as appropriate) so the comment accurately describes
the SdCardDevice created by createSdCard().
In @Tactility/Source/app/appsettings/AppSettings.cpp:
- Line 29: The toolbar title "Installed Apps" (created via
toolbar_create(parent, "Installed Apps")) doesn't match the current filtering
which only shows external apps (manifest->appLocation.isExternal()); update one
of these to be consistent: either change the toolbar title to "External Apps",
or remove/adjust the manifest->appLocation.isExternal() filter so internal apps
are included, and if keeping the external-only filter then also change the "No
apps installed" string to "No external apps installed". Make the change in the
AppSettings display code where toolbar_create is called and where the
manifest->appLocation.isExternal() check and "No apps installed" message are
produced.
🧹 Nitpick comments (3)
Devices/m5stack-tab5/Source/devices/SdCard.cpp (2)
23-26: Consider whether NoLock is appropriate for concurrent file access.The
NoLockimplementation provides no synchronization. TheSdCardDevice::getLock()interface suggests callers may rely on this lock for thread-safe file access. If multiple tasks could access the SD card concurrently, this no-op lock may lead to data corruption.If single-threaded access is guaranteed by design for this device, consider adding a brief comment documenting this assumption.
44-96: Early return on mount leaves mountPath potentially stale.If
mount()is called when already mounted (line 45-47), it returnstruebut doesn't verify that the existingmountPathmatchesnewMountPath. This could cause confusion if a caller expects mounting at a different path.Consider verifying mount path consistency
bool mount(const std::string& newMountPath) override { if (mounted) { + if (mountPath != newMountPath) { + LOGGER.warn("Already mounted at {}, ignoring request for {}", mountPath, newMountPath); + } return true; }Devices/m5stack-tab5/Source/devices/Ili9881cDisplay.h (1)
11-14: Consider extractingNoLockto a shared utility.The
NoLockclass is duplicated in bothSdCard.cppandIli9881cDisplay.h. Consider extracting it to a shared header (e.g., inTactility/) to reduce duplication.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/build.ymlDevices/guition-jc1060p470ciwy/device.propertiesDevices/m5stack-tab5/CMakeLists.txtDevices/m5stack-tab5/Source/Configuration.cppDevices/m5stack-tab5/Source/devices/Display.cppDevices/m5stack-tab5/Source/devices/Display.hDevices/m5stack-tab5/Source/devices/Ili9881cDisplay.cppDevices/m5stack-tab5/Source/devices/Ili9881cDisplay.hDevices/m5stack-tab5/Source/devices/SdCard.cppDevices/m5stack-tab5/Source/devices/SdCard.hDevices/m5stack-tab5/Source/devices/disp_init_data.hDevices/m5stack-tab5/device.propertiesFirmware/KconfigFirmware/idf_component.ymlTactility/Source/app/appsettings/AppSettings.cppTactility/Source/hal/Hal.cpp
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-29T14:45:02.914Z
Learnt from: fonix232
Repo: ByteWelder/Tactility PR: 380
File: Boards/m5stack-papers3/Source/devices/Display.cpp:9-18
Timestamp: 2025-10-29T14:45:02.914Z
Learning: For M5Stack PaperS3 in Boards/m5stack-papers3/Source/devices/Display.cpp, the GT911 touch configuration must use PAPERS3_EPD_HORIZONTAL_RESOLUTION (960) as yMax, PAPERS3_EPD_VERTICAL_RESOLUTION (540) as xMax, with swapXY=true and mirrorX=true. Alternative configurations (swapping the resolution parameters or setting swapXY=false) result in badly resolved touch positions. This specific configuration is empirically verified to work correctly.
Applied to files:
Devices/m5stack-tab5/Source/devices/Display.cpp
📚 Learning: 2025-10-27T22:33:23.840Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 394
File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50
Timestamp: 2025-10-27T22:33:23.840Z
Learning: In the Tactility project, when configuring SPI for displays that use esp_lvgl_port, the `.lock` field must be set to `tt::lvgl::getSyncLock()` rather than `tt::hal::spi::getLock()`, because esp_lvgl_port creates and manages its own synchronization lock for display operations.
Applied to files:
Devices/m5stack-tab5/Source/devices/Display.cppDevices/m5stack-tab5/Source/devices/Ili9881cDisplay.hDevices/m5stack-tab5/Source/devices/Ili9881cDisplay.cpp
📚 Learning: 2025-10-28T19:44:35.263Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Drivers/ST7789-i8080/Source/St7789i8080Display.cpp:326-329
Timestamp: 2025-10-28T19:44:35.263Z
Learning: In the LilyGo T-Display S3 ST7789 i8080 display driver (Drivers/ST7789-i8080/Source/St7789i8080Display.cpp), both callback registrations are required: setting on_color_trans_done during panel IO creation in configurePanelIO() and calling esp_lcd_panel_io_register_event_callbacks() in initializeLvgl() after the LVGL display is created. This dual registration is intentional and necessary for proper operation on this hardware.
Applied to files:
Devices/m5stack-tab5/Source/devices/Display.cppDevices/m5stack-tab5/Source/devices/Ili9881cDisplay.hDevices/m5stack-tab5/Source/devices/Ili9881cDisplay.cpp
📚 Learning: 2025-10-28T05:42:03.394Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Boards/LilygoTdisplayS3/Source/devices/Display.cpp:183-199
Timestamp: 2025-10-28T05:42:03.394Z
Learning: The LilyGo T-Display S3 board (Boards/LilygoTdisplayS3) requires a non-standard LVGL flush pattern in its ST7789 i8080 display driver. In st7789_send_color_cb, lv_display_flush_ready() must be called immediately after esp_lcd_panel_io_tx_color() with a vTaskDelay(1), rather than using standard DMA completion callbacks via esp_lcd_panel_io_register_event_callbacks. Standard DMA callback approaches cause display corruption on this specific board. This pattern was tested against multiple alternatives (callback registration, buffer tuning, clock reduction) and is the only stable solution for T-Display S3 hardware.
Applied to files:
Devices/m5stack-tab5/Source/devices/Display.cppDevices/m5stack-tab5/Source/devices/Ili9881cDisplay.hDevices/m5stack-tab5/Source/devices/Ili9881cDisplay.cpp
🧬 Code graph analysis (5)
Tactility/Source/app/appsettings/AppSettings.cpp (1)
Tactility/Source/lvgl/Toolbar.cpp (4)
toolbar_create(78-150)toolbar_create(78-78)toolbar_create(152-154)toolbar_create(152-152)
Devices/m5stack-tab5/Source/devices/Display.h (2)
Tactility/Include/Tactility/hal/display/DisplayDevice.h (1)
DisplayDevice(15-47)Devices/m5stack-tab5/Source/devices/Display.cpp (2)
createDisplay(28-64)createDisplay(28-28)
Devices/m5stack-tab5/Source/devices/SdCard.h (2)
Tactility/Include/Tactility/hal/sdcard/SdCardDevice.h (1)
SdCardDevice(15-81)Devices/m5stack-tab5/Source/devices/SdCard.cpp (2)
createSdCard(125-127)createSdCard(125-125)
Devices/m5stack-tab5/Source/devices/SdCard.cpp (2)
Tactility/Include/Tactility/hal/sdcard/SdCardDevice.h (1)
SdCardDevice(15-81)TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Devices/m5stack-tab5/Source/devices/Ili9881cDisplay.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
🪛 Clang (14.0.6)
Devices/m5stack-tab5/Source/devices/Display.h
[error] 3-3: 'Tactility/hal/display/DisplayDevice.h' file not found
(clang-diagnostic-error)
Devices/m5stack-tab5/Source/devices/disp_init_data.h
[error] 7-7: 'esp_lcd_ili9881c.h' file not found
(clang-diagnostic-error)
Devices/m5stack-tab5/Source/devices/SdCard.h
[error] 3-3: 'Tactility/hal/sdcard/SdCardDevice.h' file not found
(clang-diagnostic-error)
Devices/m5stack-tab5/Source/devices/Ili9881cDisplay.h
[error] 3-3: 'EspLcdDisplayV2.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: run
- GitHub Check: Linux
- GitHub Check: macOS
🔇 Additional comments (18)
Tactility/Source/hal/Hal.cpp (1)
82-85: LGTM!The log messages now correctly reflect the callback name
initBoot, improving consistency between the code and logs..github/workflows/build.yml (1)
65-65: LGTM!The new board entry follows the existing naming conventions and correctly specifies
esp32p4architecture, which is already supported in the SDK build matrix.Devices/m5stack-tab5/Source/devices/Display.h (1)
1-6: LGTM!Clean factory function declaration that follows the established device pattern in the codebase. The static analysis error about the missing include is a false positive—the ESP-IDF build system configures include paths appropriately.
Firmware/idf_component.yml (1)
46-50: LGTM. The dependency follows the established pattern for target-conditional LCD drivers, and version 1.1.0 is valid on the ESP Component Registry. Version pinning ensures reproducible builds.Devices/m5stack-tab5/device.properties (1)
1-28: LGTM!The device properties are well-structured with appropriate hardware specifications for the M5Stack Tab5. The ESP-Hosted configuration with SDIO interface for the ESP32C6 slave aligns with the ESP32P4's external WiFi requirements.
Firmware/Kconfig (1)
72-73: LGTM!The new Kconfig entry follows the established naming convention and is properly placed among the other M5Stack device options.
Devices/m5stack-tab5/CMakeLists.txt (1)
1-7: LGTM!The CMakeLists configuration follows the ESP-IDF component pattern used by other device directories in the project. The dependency list correctly covers all required components for display, touch, backlight, and SD card functionality.
Devices/m5stack-tab5/Source/devices/disp_init_data.h (1)
420-431: LGTM - Final display enable sequence.The CMD_Page 0 block correctly ends the initialization with the tear effect control (0x35) and display on (0x29) commands as expected for ILI9881C panels.
Devices/m5stack-tab5/Source/devices/Display.cpp (2)
36-40: Document the GPIO 23 touch interrupt workaround.The workaround is referenced with a link, but consider adding a brief inline explanation that the resistor pulls the line high, preventing GT911 from responding to I2C, and driving it low allows communication.
13-26: LGTM - Touch configuration.The GT911 touch configuration with 720x1280 resolution and no axis transformations is appropriate for the Tab5's portrait-oriented display.
Devices/m5stack-tab5/Source/devices/Ili9881cDisplay.h (1)
9-44: LGTM - Class structure is well-designed.The
Ili9881cDisplayclass properly overrides the necessary methods for MIPI-DSI panel integration with EspLcdDisplayV2. The protected interface for panel IO/config creation and the public device info methods are appropriately designed.Devices/m5stack-tab5/Source/Configuration.cpp (2)
10-15: LGTM!Clean factory pattern for device creation.
87-126: LGTM!Hardware configuration is well-structured with clear I2C bus naming and appropriate settings for 400kHz fast mode.
Devices/m5stack-tab5/Source/devices/Ili9881cDisplay.cpp (5)
9-19: LGTM!Proper resource cleanup with null checks and pointer reset to prevent double-free.
21-55: LGTM!Proper LDO and MIPI DSI bus initialization with error handling. The 2-lane configuration at 1 Gbps is appropriate for a 720x1280 RGB565 display.
57-74: LGTM!Clean lazy initialization of the MIPI DSI bus with proper error propagation.
76-87: LGTM!Clean panel configuration factory with appropriate comment about deferred vendor_config assignment.
140-147: LGTM!Reasonable choice to disable tearing avoidance for stability, with clear documentation of the rationale.
| esp_ldo_channel_handle_t ldo_handle = nullptr; | ||
| esp_ldo_channel_config_t ldo_config = { | ||
| .chan_id = 4, // LDO channel 4 for SD power | ||
| .voltage_mv = 3300, // 3.3V | ||
| .flags { | ||
| .adjustable = 0, | ||
| .owned_by_hw = 0, | ||
| .bypass = 0 | ||
| } | ||
| }; | ||
|
|
||
| esp_err_t ldo_ret = esp_ldo_acquire_channel(&ldo_config, &ldo_handle); | ||
| if (ldo_ret != ESP_OK) { | ||
| LOGGER.warn("Failed to acquire LDO for SD power: {} (continuing anyway)", esp_err_to_name(ldo_ret)); | ||
| } |
There was a problem hiding this comment.
LDO channel handle is acquired but never released.
The LDO channel is acquired during mount but is never released on unmount, mount failure, or destruction. This leads to a resource leak, and on repeated mount/unmount cycles, the LDO channel may fail to be reacquired.
🔧 Proposed fix to track and release LDO handle
Store the LDO handle as a member variable and release it in unmount():
class SdCardDeviceImpl final : public SdCardDevice {
// ...
std::shared_ptr<tt::Lock> lock = std::make_shared<NoLock>();
sdmmc_card_t* card = nullptr;
+ esp_ldo_channel_handle_t ldoHandle = nullptr;
bool mounted = false;
std::string mountPath;In mount(), store the handle:
- esp_ldo_channel_handle_t ldo_handle = nullptr;
// ...
- esp_err_t ldo_ret = esp_ldo_acquire_channel(&ldo_config, &ldo_handle);
+ esp_err_t ldo_ret = esp_ldo_acquire_channel(&ldo_config, &ldoHandle);In unmount(), release the LDO:
card = nullptr;
mounted = false;
+ if (ldoHandle != nullptr) {
+ esp_ldo_release_channel(ldoHandle);
+ ldoHandle = nullptr;
+ }
LOGGER.info("SD card unmounted");🤖 Prompt for AI Agents
In @Devices/m5stack-tab5/Source/devices/SdCard.cpp around lines 62 - 76, The LDO
channel acquired via esp_ldo_acquire_channel (local variable ldo_handle) in
mount() is never released, causing a resource leak; make ldo_handle a persistent
member (e.g., class field) and assign the acquired handle to it in mount(), then
call esp_ldo_release_channel (or the appropriate release API) on that member in
unmount() and in error/cleanup paths (including mount failure and destructor)
and null out the member after release to ensure safe repeated mount/unmount
cycles.
| // Create SD card device for jc1060p470ciwy using SDMMC slot 0 (4-bit) | ||
| std::shared_ptr<tt::hal::sdcard::SdCardDevice> createSdCard(); |
There was a problem hiding this comment.
Incorrect device name in comment.
The comment references jc1060p470ciwy but this file is for the M5Stack Tab5 device.
📝 Proposed fix
-// Create SD card device for jc1060p470ciwy using SDMMC slot 0 (4-bit)
+// Create SD card device for M5Stack Tab5 using SDMMC slot 0 (4-bit)
std::shared_ptr<tt::hal::sdcard::SdCardDevice> createSdCard();📝 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.
| // Create SD card device for jc1060p470ciwy using SDMMC slot 0 (4-bit) | |
| std::shared_ptr<tt::hal::sdcard::SdCardDevice> createSdCard(); | |
| // Create SD card device for M5Stack Tab5 using SDMMC slot 0 (4-bit) | |
| std::shared_ptr<tt::hal::sdcard::SdCardDevice> createSdCard(); |
🤖 Prompt for AI Agents
In @Devices/m5stack-tab5/Source/devices/SdCard.h around lines 5 - 6, The
file-level comment incorrectly names the device as "jc1060p470ciwy"; update the
comment above the createSdCard declaration to reference the correct device
"M5Stack Tab5" (keeping the rest of the comment about SDMMC slot 0 (4-bit) as
appropriate) so the comment accurately describes the SdCardDevice created by
createSdCard().
|
|
||
| void onShow(TT_UNUSED AppContext& app, lv_obj_t* parent) override { | ||
| auto* toolbar = lvgl::toolbar_create(parent, "External Apps"); | ||
| auto* toolbar = lvgl::toolbar_create(parent, "Installed Apps"); |
There was a problem hiding this comment.
Title-implementation mismatch: "Installed Apps" displays only external apps.
The toolbar title "Installed Apps" suggests all installed apps will be displayed, but the implementation at line 45 only shows external apps via manifest->appLocation.isExternal(). This inconsistency could confuse users who expect to see all installed apps, including internal/built-in ones. Additionally, the "No apps installed" message at line 53 may be misleading if internal apps exist.
Consider either:
- Keeping "External Apps" as the title to match the implementation
- Updating the filter to show all installed apps (both internal and external)
- Changing the "No apps installed" message to "No external apps installed" for clarity
🤖 Prompt for AI Agents
In @Tactility/Source/app/appsettings/AppSettings.cpp at line 29, The toolbar
title "Installed Apps" (created via toolbar_create(parent, "Installed Apps"))
doesn't match the current filtering which only shows external apps
(manifest->appLocation.isExternal()); update one of these to be consistent:
either change the toolbar title to "External Apps", or remove/adjust the
manifest->appLocation.isExternal() filter so internal apps are included, and if
keeping the external-only filter then also change the "No apps installed" string
to "No external apps installed". Make the change in the AppSettings display code
where toolbar_create is called and where the manifest->appLocation.isExternal()
check and "No apps installed" message are produced.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @Devices/m5stack-tab5/Source/devices/Ili9881cDisplay.cpp:
- Around line 117-126: The static ili9881c_vendor_config_t vendor_config
captures the instance member mipiDsiBus which can become stale if
Ili9881cDisplay instances are recreated or multiple instances exist; make
vendor_config an instance-owned object (remove the static and move vendor_config
into the Ili9881cDisplay class as a member initialized with .mipi_config.dsi_bus
= mipiDsiBus and other fields set from disp_init_data/dpi_config) so its
lifetime matches the instance, or if you must keep it static, add a clear
runtime guard or static assertion enforcing single-instance usage and document
the requirement.
🧹 Nitpick comments (1)
Devices/m5stack-tab5/Source/devices/Ili9881cDisplay.cpp (1)
48-51: Consider releasing LDO channel on bus creation failure.If
esp_lcd_new_dsi_busfails, the LDO channel acquired at line 32 remains held. While the destructor will eventually clean it up, releasing immediately on failure keeps the object in a consistent state and follows fail-fast cleanup patterns.♻️ Suggested cleanup on failure
if (esp_lcd_new_dsi_bus(&bus_config, &mipiDsiBus) != ESP_OK) { LOGGER.error("Failed to create bus"); + esp_ldo_release_channel(ldoChannel); + ldoChannel = nullptr; return false; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Devices/guition-jc1060p470ciwy/Source/devices/Display.cppDevices/m5stack-tab5/Source/Configuration.cppDevices/m5stack-tab5/Source/devices/Display.cppDevices/m5stack-tab5/Source/devices/Ili9881cDisplay.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- Devices/m5stack-tab5/Source/Configuration.cpp
- Devices/m5stack-tab5/Source/devices/Display.cpp
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-28T05:42:03.394Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Boards/LilygoTdisplayS3/Source/devices/Display.cpp:183-199
Timestamp: 2025-10-28T05:42:03.394Z
Learning: The LilyGo T-Display S3 board (Boards/LilygoTdisplayS3) requires a non-standard LVGL flush pattern in its ST7789 i8080 display driver. In st7789_send_color_cb, lv_display_flush_ready() must be called immediately after esp_lcd_panel_io_tx_color() with a vTaskDelay(1), rather than using standard DMA completion callbacks via esp_lcd_panel_io_register_event_callbacks. Standard DMA callback approaches cause display corruption on this specific board. This pattern was tested against multiple alternatives (callback registration, buffer tuning, clock reduction) and is the only stable solution for T-Display S3 hardware.
Applied to files:
Devices/m5stack-tab5/Source/devices/Ili9881cDisplay.cpp
📚 Learning: 2025-10-28T19:44:35.263Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Drivers/ST7789-i8080/Source/St7789i8080Display.cpp:326-329
Timestamp: 2025-10-28T19:44:35.263Z
Learning: In the LilyGo T-Display S3 ST7789 i8080 display driver (Drivers/ST7789-i8080/Source/St7789i8080Display.cpp), both callback registrations are required: setting on_color_trans_done during panel IO creation in configurePanelIO() and calling esp_lcd_panel_io_register_event_callbacks() in initializeLvgl() after the LVGL display is created. This dual registration is intentional and necessary for proper operation on this hardware.
Applied to files:
Devices/m5stack-tab5/Source/devices/Ili9881cDisplay.cpp
📚 Learning: 2025-10-27T22:33:23.840Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 394
File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50
Timestamp: 2025-10-27T22:33:23.840Z
Learning: In the Tactility project, when configuring SPI for displays that use esp_lvgl_port, the `.lock` field must be set to `tt::lvgl::getSyncLock()` rather than `tt::hal::spi::getLock()`, because esp_lvgl_port creates and manages its own synchronization lock for display operations.
Applied to files:
Devices/m5stack-tab5/Source/devices/Ili9881cDisplay.cpp
🧬 Code graph analysis (1)
Devices/m5stack-tab5/Source/devices/Ili9881cDisplay.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: run
- GitHub Check: macOS
- GitHub Check: Linux
🔇 Additional comments (6)
Devices/guition-jc1060p470ciwy/Source/devices/Display.cpp (1)
64-64: LGTM! Good improvement to type safety.Using
static_pointer_castinstead ofreinterpret_pointer_castis the correct choice when upcasting to a base class type in an inheritance hierarchy. It provides compile-time type checking whereasreinterpret_pointer_castbypasses type safety entirely.Devices/m5stack-tab5/Source/devices/Ili9881cDisplay.cpp (5)
1-7: LGTM!Includes and logger setup follow the project's established patterns.
9-19: LGTM!Destructor properly cleans up resources in the correct order (bus before LDO). The TODO about moving cleanup to
::stop()is a reasonable future improvement for lifecycle management.
57-74: LGTM!Clean lazy initialization of the MIPI DSI bus with proper error handling.
76-87: LGTM!Panel config creation is straightforward with clear documentation about deferred
vendor_configassignment.
142-149: LGTM!Disabling tearing avoidance is a reasonable choice for this hardware, and the comment clearly explains the rationale regarding blocking operations.
| // TODO: undo static | ||
| static ili9881c_vendor_config_t vendor_config = { | ||
| .init_cmds = disp_init_data, | ||
| .init_cmds_size = std::size(disp_init_data), | ||
| .mipi_config = { | ||
| .dsi_bus = mipiDsiBus, | ||
| .dpi_config = &dpi_config, | ||
| .lane_num = 2, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Static vendor_config captures instance member mipiDsiBus - potential correctness issue.
The static vendor_config at line 118 references mipiDsiBus (line 122), which is a per-instance member. This creates a subtle bug: if Ili9881cDisplay is ever destroyed and recreated (or if multiple instances exist), the static vendor_config would reference stale or incorrect bus handles.
While the TODO suggests this is known, the current implementation is unsafe for object lifecycle scenarios. Consider either:
- Making
vendor_confignon-static (preferred) - Adding a static assertion or runtime guard to ensure single-instance usage
🐛 Proposed fix - remove static
- // TODO: undo static
- static esp_lcd_dpi_panel_config_t dpi_config = {
+ esp_lcd_dpi_panel_config_t dpi_config = {
.virtual_channel = 0,
.dpi_clk_src = MIPI_DSI_DPI_CLK_SRC_DEFAULT,
.dpi_clock_freq_mhz = 60,
...
};
- // TODO: undo static
- static ili9881c_vendor_config_t vendor_config = {
+ ili9881c_vendor_config_t vendor_config = {
.init_cmds = disp_init_data,
.init_cmds_size = std::size(disp_init_data),
.mipi_config = {
.dsi_bus = mipiDsiBus,
.dpi_config = &dpi_config,
.lane_num = 2,
},
};Note: If removing static causes issues with the library expecting these configs to persist beyond the function call, store them as class members instead.
🤖 Prompt for AI Agents
In @Devices/m5stack-tab5/Source/devices/Ili9881cDisplay.cpp around lines 117 -
126, The static ili9881c_vendor_config_t vendor_config captures the instance
member mipiDsiBus which can become stale if Ili9881cDisplay instances are
recreated or multiple instances exist; make vendor_config an instance-owned
object (remove the static and move vendor_config into the Ili9881cDisplay class
as a member initialized with .mipi_config.dsi_bus = mipiDsiBus and other fields
set from disp_init_data/dpi_config) so its lifetime matches the instance, or if
you must keep it static, add a clear runtime guard or static assertion enforcing
single-instance usage and document the requirement.
Support for the original Tab5 hardware. The new revision is not supported yet.