Add Guition JC1060P470CIWY and update other Guition device IDs #447
Conversation
📝 WalkthroughWalkthroughThe PR replaces CYD board entries with Guition boards in CI and Kconfig, adds a new guition-jc1060p470ciwy device (device.properties, CMake, device Configuration, display, JD9165 MIPI‑DSI driver, power ADC device, SD card device), updates vendor fields for two other devices, extends EspLcdCompat with DSI hooks and DSI display path handling, tightens Wi‑Fi compile guards to use TT_WIFI_ENABLED with exclusions for ESP_WIFI_REMOTE_ENABLED, updates system locale format/region, and adds idf_component dependencies for esp_hosted, esp_wifi_remote, and esp_lcd_jd9165. 🚥 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. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-11-01T19:57:58.233ZApplied to files:
🧬 Code graph analysis (2)TactilityC/Source/symbols/pthread.cpp (1)
TactilityC/Source/symbols/gcc_soft_float_p4.cpp (2)
⏰ 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)
🔇 Additional comments (5)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tactility/Source/service/wifi/WifiEsp.cpp (1)
979-979: Stale#endifcomment.The closing comment references
ESP_PLATFORMbut the guard was changed toCONFIG_SOC_WIFI_SUPPORTED || CONFIG_SLAVE_SOC_WIFI_SUPPORTEDon line 5. Update the comment to match the actual guard.Suggested fix
-#endif // ESP_PLATFORM +#endif // CONFIG_SOC_WIFI_SUPPORTED || CONFIG_SLAVE_SOC_WIFI_SUPPORTED
🤖 Fix all issues with AI agents
In @Devices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.cpp:
- Around line 78-104: The LDO handle acquired in createMipiDsiBus (ldo_mipi_phy
via esp_ldo_acquire_channel) is a local variable and never released; make
ldo_mipi_phy a class member (e.g., esp_ldo_channel_handle_t ldoMipiPhy) on the
Jd9165Display class, assign to that member in createMipiDsiBus, and call
esp_ldo_release_channel(ldoMipiPhy) in the Jd9165Display destructor (and set the
member to nullptr after release); ensure any error paths and object move/cleanup
semantics also account for the member so the LDO channel is always released when
the display is destroyed.
In @Devices/guition-jc1060p470ciwy/Source/devices/Power.cpp:
- Around line 92-122: The tryInitCalibration function mixes logging APIs:
replace the TT_LOG_I(TAG, "ADC calibration (line fitting) enabled") call with
the consistent LOGGER.info("ADC calibration (line fitting) enabled") used
elsewhere (remove the undefined TAG), so both the line-fitting and curve-fitting
branches use LOGGER.info and the fallback uses LOGGER.warn; ensure no TT_LOG_*
macros remain in tryInitCalibration and keep calScheme and caliHandle handling
unchanged.
- Around line 63-90: In ensureInit(), when adc_oneshot_config_channel(adcHandle,
ADC_CHANNEL, &chan_cfg) fails you must release the ADC unit created by
adc_oneshot_new_unit to avoid a resource leak: call the appropriate deletion
function (e.g., adc_oneshot_del_unit or the SDK equivalent) with adcHandle,
clear/reset adcHandle, and then return false; also ensure you only mark
initialized = true after both unit creation and channel configuration succeed
and that calibrated is set afterwards.
In @Devices/guition-jc1060p470ciwy/Source/devices/SdCard.cpp:
- Around line 57-84: Replace the Spanish inline comments with clear English
equivalents in this block: change "más estable para inicialización" to "more
stable for initialization", "Forzar modo 4-bit" to "Force 4-bit mode",
"Configurar alimentación LDO para el SD card (crítico en ESP32-P4)" to
"Configure LDO power for SD card (critical on ESP32-P4)", "Slot 0 usa IO MUX -
los pines son fijos y no se especifican" to "Slot 0 uses IO MUX - pins are fixed
and not specified", and the cd/wp comments "No card detect" and "No write
protect" are fine but standardize casing; update comments near sdmmc_host_t
host, esp_ldo_channel_config_t ldo_config, esp_ldo_acquire_channel/LOGGER.warn,
and slot_config to use the new English text.
- Around line 18-19: Translate the Spanish comments at the top of SdCard.cpp to
English: replace "// ESP32-P4 Slot 0 usa IO MUX (pines fijos, no configurables
manualmente)" and the following pin mapping comment with an English equivalent
that preserves the technical details (e.g., "// ESP32-P4 Slot 0 uses IO MUX
(fixed pins, not manually configurable)" and "// CLK=43, CMD=44, D0=39, D1=40,
D2=41, D3=42 (defined automatically by hardware)"). Ensure wording is clear and
retain exact pin numbers and the note about hardware-defined pins.
In @Firmware/idf_component.yml:
- Around line 2-9: Replace the wildcard versions for espressif/esp_hosted and
espressif/esp_wifi_remote with pinned versions to ensure reproducible builds:
change the version value for the component named espressif/esp_hosted from "*"
to "2.8.5" and for espressif/esp_wifi_remote from "*" to "1.2.3", keeping their
existing rules (the if: "target == esp32p4" entries) intact.
In @Firmware/Kconfig:
- Around line 44-49: Three Kconfig menu labels use inconsistent vendor
capitalization: TT_DEVICE_GUITION_JC1060P470CIWY, TT_DEVICE_GUITION_JC2432W328C,
and TT_DEVICE_GUITION_JC8048W550C; update the human-readable strings so the
vendor name is cased consistently (e.g., change the labels currently using
"GUITION ..." to "Guition ...") by editing the bool "..." text for those config
entries to match the chosen capitalization across all three entries.
🧹 Nitpick comments (8)
Tactility/Source/service/wifi/WifiEsp.cpp (1)
5-5: Consider using||instead oforfor preprocessor consistency.While
oris valid in C++ as an alternative token for||, preprocessor directives conventionally use||. This ensures consistency with C-style macros and avoids potential confusion.Suggested fix
-#if defined(CONFIG_SOC_WIFI_SUPPORTED) or defined(CONFIG_SLAVE_SOC_WIFI_SUPPORTED) +#if defined(CONFIG_SOC_WIFI_SUPPORTED) || defined(CONFIG_SLAVE_SOC_WIFI_SUPPORTED)Devices/guition-jc1060p470ciwy/Source/devices/Power.h (1)
7-7: Consider wrapping factory function in a namespace.The
createPower()function is declared in the global scope, which could potentially cause naming conflicts if other device implementations use the same pattern. Consider wrapping it in a device-specific namespace (e.g.,guition::jc1060p470ciwy).♻️ Proposed refactor
+namespace guition::jc1060p470ciwy { + // Battery measurement via ADC2 channel 4 with 85k/100k divider std::shared_ptr<tt::hal::power::PowerDevice> createPower(); + +}Devices/guition-jc8048w550c/Source/devices/Display.h (1)
5-5: Consider wrapping factory function in a namespace.Similar to the
createPower()function in the JC1060P470CIWY device, this factory function is in the global scope. For consistency and to avoid potential naming conflicts, consider wrapping device-specific factories in namespaces.♻️ Proposed refactor
+namespace guition::jc8048w550c { + std::shared_ptr<tt::hal::display::DisplayDevice> createDisplay(); + +}Firmware/Kconfig (1)
99-99: Trailing whitespace.Line 99 contains trailing whitespace. While this doesn't affect functionality, removing it would improve consistency.
🧹 Proposed fix
- + config TT_WIFI_ENABLEDDevices/guition-jc1060p470ciwy/Source/devices/Display.cpp (1)
37-41: Use the TAG constant consistently for logging.Line 40 instantiates a new Logger with the tag
"jc1060p470ciwy", but theTAGconstant"Display"is already defined on Line 9. For consistency, consider using the existingTAGconstant.♻️ Proposed fix
std::shared_ptr<tt::hal::display::DisplayDevice> createDisplay() { // Initialize PWM backlight if (!driver::pwmbacklight::init(LCD_PIN_BACKLIGHT, 20000, LEDC_TIMER_1, LEDC_CHANNEL_0)) { - tt::Logger("jc1060p470ciwy").warn(TAG, "Failed to initialize backlight"); + tt::Logger(TAG).warn("Failed to initialize backlight"); }Note: This assumes the Logger API accepts a single-argument
warn()method. If the current signature iswarn(tag, message), then the TAG is already being used for categorization and the Logger tag can remain as is.Drivers/JD9165/esp_lcd_jd9165.c (1)
25-38: Fix typo in comment.Line 29 contains a typo: "surrent" should be "current".
📝 Proposed fix
typedef struct { esp_lcd_panel_io_handle_t io; int reset_gpio_num; uint8_t madctl_val; // save current value of LCD_CMD_MADCTL register - uint8_t colmod_val; // save surrent value of LCD_CMD_COLMOD register + uint8_t colmod_val; // save current value of LCD_CMD_COLMOD register const jd9165_lcd_init_cmd_t *init_cmds;Drivers/JD9165/include/esp_lcd_jd9165.h (1)
107-162: Remove duplicate comment block between macro definitions.Lines 130-138 contain a duplicate comment block that appears to be leftover from copying the previous macro definition. The comment for
JD9165_1024_600_PANEL_60HZ_DPI_CONFIG_CFshould start at line 131, making lines 130-138 redundant.📝 Proposed fix
.disable_lp = 0, \ }, \ - }/** - * @brief MIPI DPI configuration structure - * - * @note refresh_rate = (dpi_clock_freq_mhz * 1000000) / (h_res + hsync_pulse_width + hsync_back_porch + hsync_front_porch) - * / (v_res + vsync_pulse_width + vsync_back_porch + vsync_front_porch) - * - * @param[in] color_format Input color format of the panel - * - */ + } + +/** + * @brief MIPI DPI configuration structure + * + * @note refresh_rate = (dpi_clock_freq_mhz * 1000000) / (h_res + hsync_pulse_width + hsync_back_porch + hsync_front_porch) + * / (v_res + vsync_pulse_width + vsync_back_porch + vsync_front_porch) + * + * @param[in] color_format Input color format of the panel + * + */ #define JD9165_1024_600_PANEL_60HZ_DPI_CONFIG_CF(color_format) \Devices/guition-jc1060p470ciwy/Source/devices/Power.cpp (1)
145-162: Potential issue with calibration handle cleanup when not calibrated.The
deinit()method checksif (calibrated && caliHandle)before cleanup. However, if calibration initialization partially fails (handle created but function returns error),caliHandlemight be non-null whilecalibratedis false, causing a leak. Consider checking justcaliHandle != nullptrfor cleanup.♻️ Proposed fix
void deinit() { if (adcHandle) { adc_oneshot_del_unit(adcHandle); adcHandle = nullptr; } - if (calibrated && caliHandle) { + if (caliHandle) { if (calScheme == CaliScheme::Line) { #if ADC_CALI_SCHEME_LINE_FITTING_SUPPORTED adc_cali_delete_scheme_line_fitting(caliHandle); #endif #if ADC_CALI_SCHEME_CURVE_FITTING_SUPPORTED } else if (calScheme == CaliScheme::Curve) { adc_cali_delete_scheme_curve_fitting(caliHandle); #endif } caliHandle = nullptr; + calibrated = false; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.github/workflows/build.ymlData/data/settings/system.propertiesDevices/guition-jc1060p470ciwy/CMakeLists.txtDevices/guition-jc1060p470ciwy/Source/Configuration.cppDevices/guition-jc1060p470ciwy/Source/devices/Display.cppDevices/guition-jc1060p470ciwy/Source/devices/Display.hDevices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.cppDevices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.hDevices/guition-jc1060p470ciwy/Source/devices/Power.cppDevices/guition-jc1060p470ciwy/Source/devices/Power.hDevices/guition-jc1060p470ciwy/Source/devices/SdCard.cppDevices/guition-jc1060p470ciwy/Source/devices/SdCard.hDevices/guition-jc1060p470ciwy/device.propertiesDevices/guition-jc2432w328c/CMakeLists.txtDevices/guition-jc2432w328c/Source/Configuration.cppDevices/guition-jc2432w328c/Source/devices/Display.cppDevices/guition-jc2432w328c/Source/devices/Display.hDevices/guition-jc2432w328c/Source/devices/SdCard.cppDevices/guition-jc2432w328c/Source/devices/SdCard.hDevices/guition-jc2432w328c/device.propertiesDevices/guition-jc8048w550c/CMakeLists.txtDevices/guition-jc8048w550c/Source/Configuration.cppDevices/guition-jc8048w550c/Source/devices/Display.cppDevices/guition-jc8048w550c/Source/devices/Display.hDevices/guition-jc8048w550c/Source/devices/SdCard.cppDevices/guition-jc8048w550c/Source/devices/SdCard.hDevices/guition-jc8048w550c/device.propertiesDrivers/EspLcdCompat/Source/EspLcdDisplayV2.cppDrivers/EspLcdCompat/Source/EspLcdDisplayV2.hDrivers/JD9165/CMakeLists.txtDrivers/JD9165/esp_lcd_jd9165.cDrivers/JD9165/include/esp_lcd_jd9165.hFirmware/KconfigFirmware/idf_component.ymlTactility/Source/Tactility.cppTactility/Source/app/chat/ChatApp.cppTactility/Source/app/wifimanage/WifiManage.cppTactility/Source/service/espnow/EspNow.cppTactility/Source/service/espnow/EspNowService.cppTactility/Source/service/espnow/EspNowWifi.cppTactility/Source/service/wifi/WifiEsp.cppTactilityCore/Source/CpuAffinity.cpp
🧰 Additional context used
🧠 Learnings (7)
📚 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:
Drivers/JD9165/CMakeLists.txtDrivers/JD9165/esp_lcd_jd9165.cDevices/guition-jc1060p470ciwy/Source/devices/Display.cppDevices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.hDrivers/EspLcdCompat/Source/EspLcdDisplayV2.cppDrivers/EspLcdCompat/Source/EspLcdDisplayV2.hDevices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.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:
Drivers/JD9165/esp_lcd_jd9165.cDevices/guition-jc1060p470ciwy/Source/devices/Display.cppDevices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.hDrivers/EspLcdCompat/Source/EspLcdDisplayV2.cppDrivers/EspLcdCompat/Source/EspLcdDisplayV2.hDevices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.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/guition-jc1060p470ciwy/Source/devices/Display.cppDevices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.hTactility/Source/app/chat/ChatApp.cppDrivers/EspLcdCompat/Source/EspLcdDisplayV2.cppDrivers/EspLcdCompat/Source/EspLcdDisplayV2.hDevices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.cpp
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Devices/guition-jc1060p470ciwy/Source/devices/Display.cpp
📚 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/guition-jc1060p470ciwy/Source/devices/Display.cpp
📚 Learning: 2025-11-01T19:57:58.233Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 405
File: Tactility/Source/hal/usb/UsbTusb.cpp:12-12
Timestamp: 2025-11-01T19:57:58.233Z
Learning: In ESP-IDF projects like Tactility, the wear levelling header can be included directly as `#include <wear_levelling.h>` without requiring a directory prefix like `wear_levelling/wear_levelling.h`.
Applied to files:
Tactility/Source/service/espnow/EspNow.cppTactility/Source/service/espnow/EspNowWifi.cpp
📚 Learning: 2025-11-11T17:22:20.238Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 411
File: sdkconfig.board.waveshare-s3-touch-lcd-43:39-39
Timestamp: 2025-11-11T17:22:20.238Z
Learning: In Tactility sdkconfig.board.* files, board names (CONFIG_TT_BOARD_NAME) that include screen size measurements intentionally use escaped quotes (e.g., "WaveShare S3 Touch LCD 4.3\"") where the \" represents the inch symbol (") in the final board name string. This is not a syntax error but deliberate formatting.
Applied to files:
Tactility/Source/service/espnow/EspNowService.cppFirmware/Kconfig
🧬 Code graph analysis (8)
Devices/guition-jc1060p470ciwy/Source/devices/Power.h (2)
Tactility/Include/Tactility/hal/power/PowerDevice.h (1)
PowerDevice(8-47)Devices/guition-jc1060p470ciwy/Source/devices/Power.cpp (2)
createPower(175-177)createPower(175-175)
Devices/guition-jc1060p470ciwy/Source/devices/Display.cpp (3)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)Devices/guition-jc2432w328c/Source/devices/Display.cpp (2)
createTouch(7-15)createTouch(7-7)Devices/guition-jc8048w550c/Source/devices/Display.cpp (2)
createTouch(7-17)createTouch(7-7)
Devices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.h (1)
Devices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.cpp (11)
Jd9165Display(71-76)createMipiDsiBus(78-104)createMipiDsiBus(78-78)createIoHandle(106-123)createIoHandle(106-106)createPanelConfig(125-136)createPanelConfig(125-125)createPanelHandle(138-163)createPanelHandle(138-138)getLvglPortDisplayDsiConfig(165-172)getLvglPortDisplayDsiConfig(165-165)
Drivers/JD9165/include/esp_lcd_jd9165.h (1)
Drivers/JD9165/esp_lcd_jd9165.c (1)
esp_lcd_new_panel_jd9165(49-119)
Devices/guition-jc1060p470ciwy/Source/devices/SdCard.cpp (3)
Tactility/Include/Tactility/hal/sdcard/SdCardDevice.h (1)
SdCardDevice(15-81)TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)Devices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.h (1)
lock(11-11)
Devices/guition-jc8048w550c/Source/devices/Display.h (4)
Tactility/Include/Tactility/hal/display/DisplayDevice.h (1)
DisplayDevice(15-47)Devices/guition-jc1060p470ciwy/Source/devices/Display.cpp (2)
createDisplay(37-67)createDisplay(37-37)Devices/guition-jc2432w328c/Source/devices/Display.cpp (2)
createDisplay(17-43)createDisplay(17-17)Devices/guition-jc8048w550c/Source/devices/Display.cpp (2)
createDisplay(19-104)createDisplay(19-19)
Devices/guition-jc1060p470ciwy/Source/devices/Power.cpp (2)
Tactility/Include/Tactility/hal/power/PowerDevice.h (1)
PowerDevice(8-47)TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Devices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.cpp (2)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)Drivers/JD9165/esp_lcd_jd9165.c (1)
esp_lcd_new_panel_jd9165(49-119)
⏰ 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: macOS
- GitHub Check: Linux
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: run
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
🔇 Additional comments (34)
TactilityCore/Source/CpuAffinity.cpp (1)
38-39: The default core 0 affinity is correct and intentional.The code change is consistent and well-designed. Both
getEspWifiAffinity()(lines 22-23) andgetEspMainSchedulerAffinity()(lines 38-39) follow identical logic, ensuring they always return the same affinity value. This satisfies the assertion at line 99 (assert(esp.wifi == esp.mainDispatcher)) for 2-core systems.The comment at line 22 ("Assume core 0 (risky, but safer than 'None')") demonstrates the developers deliberately chose this trade-off, understanding the implications. The explicit return at lines 38-39 improves code clarity without introducing any inconsistency.
Devices/guition-jc2432w328c/device.properties (1)
2-2: LGTM!The vendor rename from CYD to Guition aligns with the PR objectives and the broader rebranding effort across the codebase.
Devices/guition-jc1060p470ciwy/Source/devices/SdCard.h (1)
1-6: LGTM!The header file is clean and well-structured. The factory function signature is clear, and the comment provides useful context about the hardware configuration (SDMMC slot 0, 4-bit mode).
Tactility/Source/app/chat/ChatApp.cpp (1)
5-5: LGTM!The compilation guard update is consistent with the broader WiFi configuration refactoring across the codebase. The more restrictive condition (
CONFIG_TT_WIFI_ENABLEDand notCONFIG_ESP_WIFI_REMOTE_ENABLED) appropriately gates ESP-NOW/WiFi functionality.Tactility/Source/service/espnow/EspNow.cpp (1)
5-5: LGTM!The compilation guard update is consistent with the WiFi refactoring changes applied across ESP-NOW components. The conditional compilation logic properly gates the ESP-NOW service implementation.
Data/data/settings/system.properties (1)
3-4: Clarify the rationale for changing the default locale from US to EU.The default system locale has been changed to DD/MM/YYYY date format and EU region as part of the Guition device support commit, but no rationale is documented. This affects the initial user experience across all devices.
Was this an intentional change to match a specific deployment, or should the default remain region-agnostic with users configuring their preference during setup?
Tactility/Source/app/wifimanage/WifiManage.cpp (1)
123-128: LGTM! Enhanced observability for WiFi state.The additional logging of the connection target improves debuggability. The empty-string handling is safe and provides clear output.
Devices/guition-jc1060p470ciwy/device.properties (4)
1-3: LGTM! General configuration is correct.The vendor and name configuration aligns with the PR objectives and device naming conventions.
5-11: LGTM! Hardware configuration is appropriate for ESP32P4.The hardware specifications (16MB flash, octal SPI RAM at 200MHz) are well-suited for a 7" display board with ESP32P4 target.
13-19: LGTM! Display and LVGL configuration is appropriate.The 7" display with 187 DPI and 16-bit color depth is well-suited for the ESP32P4 hardware configuration.
21-28: Verify ESP-IDF version compatibility with esp-hosted Kconfig.The sdkconfig options (
CONFIG_ESP_HOSTED_ENABLED,CONFIG_ESP_HOSTED_SDIO_HOST_INTERFACE,CONFIG_ESP_HOSTED_P4_DEV_BOARD_FUNC_BOARD,CONFIG_SLAVE_IDF_TARGET_ESP32C6) are valid and documented in the esp-hosted and esp_wifi_remote components. However, there is a known Kconfig issue where the$ESP_IDF_VERSIONvariable fails to expand in certain ESP-IDF patch versions (e.g., 5.5.1), which can prevent the slave-target Kconfig menu from loading. If you encounter missing slave-target options duringidf.py menuconfig, apply the recommended workaround (create a symlink matching your ESP-IDF patch directory, or useCONFIG_ESP_HOSTED_IDF_SLAVE_TARGET="esp32c6"in sdkconfig directly).Tactility/Source/service/espnow/EspNowService.cpp (1)
5-5: LGTM! Consistent WiFi configuration guard.The compilation guard change is consistent with the pattern applied in other EspNow-related files (EspNowWifi.cpp). This ensures uniform build behavior across the EspNow subsystem.
Tactility/Source/service/espnow/EspNowWifi.cpp (1)
5-5: WiFi configuration flags are properly defined and consistently applied.The compilation guard uses
CONFIG_TT_WIFI_ENABLED(defined in Firmware/Kconfig) andCONFIG_ESP_WIFI_REMOTE_ENABLED(from the esp_wifi_remote dependency), which is the correct approach. The same guard pattern is consistently applied across all EspNow-related files and the main Tactility.cpp, and the build matrix properly handles these flags with conditional inclusion of esp_wifi_remote only for ESP32-P4 targets.Tactility/Source/service/wifi/WifiEsp.cpp (1)
416-430: Good improvement for handling incomplete scan results.The conditional logging correctly filters out AP records with missing SSID or channel information, providing cleaner debug output and better handling of edge cases seen on certain hardware configurations.
Drivers/EspLcdCompat/Source/EspLcdDisplayV2.cpp (2)
44-47: Good optimization to skip gap setting when not needed.Skipping the
esp_lcd_panel_set_gapcall when both gap values are zero avoids unnecessary API calls and potential issues with panels that may not support the gap setting.
125-133: Clean integration of DSI panel support.The branching logic for DSI panels follows the existing RGB panel pattern. The
useDsiPanel()andgetLvglPortDisplayDsiConfig()virtual hooks allow subclasses to customize DSI behavior while maintaining backward compatibility for non-DSI panels.Drivers/EspLcdCompat/Source/EspLcdDisplayV2.h (1)
58-63: Well-designed extensibility for DSI panels.The virtual hooks provide a clean extension point for DSI panel support while maintaining backward compatibility. The default implementations (
useDsiPanel() = false,avoid_tearing = 0) ensure existing non-DSI subclasses continue to work without modification.Devices/guition-jc8048w550c/device.properties (1)
2-2: Vendor naming update looks correct.The vendor change from CYD to Guition aligns with the PR objective of updating Guition device IDs.
Devices/guition-jc1060p470ciwy/CMakeLists.txt (1)
1-8: Component registration looks complete.The dependencies cover all expected peripherals for this board: display (JD9165, esp_lcd, EspLcdCompat), touch (GT911), backlight (PwmBacklight), SD card (vfs, fatfs), and power (esp_adc, EstimatedPower).
Note:
GLOB_RECURSEis convenient but CMake won't automatically detect new source files without re-running cmake. This is a common pattern in ESP-IDF projects and acceptable here.Drivers/JD9165/CMakeLists.txt (1)
1-3: Clean component registration for the JD9165 driver.The dependencies (
esp_lcd,driver) are appropriate for an ESP-LCD panel driver implementation.Tactility/Source/Tactility.cpp (1)
48-50: Guard changes for ESP-NOW are consistent and logically sound, pending CONFIG_ESP_WIFI_REMOTE_ENABLED definition.CONFIG_TT_WIFI_ENABLED is properly defined in Firmware/Kconfig. The guard syntax is consistently applied across all four locations in Tactility.cpp (declarations and registrations for both service and app) plus additional source files. The logic is correct: when CONFIG_ESP_WIFI_REMOTE_ENABLED is eventually added to the build configuration, it will properly exclude ESP-NOW, which requires direct WiFi hardware access.
Note: CONFIG_ESP_WIFI_REMOTE_ENABLED does not yet exist in the build configuration, so the second part of the guard currently has no effect. Until that config is defined, the feature is effectively gated only by CONFIG_TT_WIFI_ENABLED.
Devices/guition-jc1060p470ciwy/Source/devices/Power.h (1)
1-4: LGTM!The includes are appropriate and the use of
#pragma onceis a clean approach for include guards.Devices/guition-jc8048w550c/Source/devices/Display.h (1)
1-3: LGTM!Clean header structure with appropriate includes.
.github/workflows/build.yml (1)
50-52: LGTM!The new GUITION board entries are properly formatted and positioned alphabetically in the build matrix. The architecture assignments (esp32p4 for JC1060P470CIWY, esp32 for JC2432W328C, and esp32s3 for JC8048W550C) are appropriate.
Firmware/Kconfig (1)
100-105: LGTM!The WiFi configuration option is well-structured with clear documentation. The help text appropriately explains the different WiFi implementations (native vs ESP-Hosted) for different chip families.
Devices/guition-jc1060p470ciwy/Source/Configuration.cpp (3)
1-7: LGTM!Clean include structure with local device headers followed by the framework Configuration header. The
using namespace tt::haldeclaration is appropriately scoped to this implementation file.
9-15: LGTM!The
createDevices()helper function properly encapsulates device initialization with static linkage. The device order (Display, SdCard, Power) appears logical, though you may want to verify if initialization order matters for device dependencies.
17-38: LGTM!The hardware configuration is well-structured with appropriate I2C settings:
- 400kHz clock speed is standard I2C fast mode
- Pullups are enabled on both SDA and SCL lines
- Configuration is correctly marked as immutable
- Framework-managed initialization (ByTactility) ensures proper lifecycle management
Ensure that GPIO 7 (SDA) and GPIO 8 (SCL) are the correct pins for the I2C bus on the JC1060P470CIWY hardware.
Devices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.h (1)
10-13: NoLock is appropriate for the JD9165 MIPI-DSI display.MIPI-DSI panels manage synchronization internally as part of the DSI protocol. Unlike SPI displays that use
esp_lvgl_port's external synchronization, DSI displays uselvgl_port_add_disp_dsi()which does not accept or require a lock parameter.NoLockfollows the established pattern in the codebase for self-synchronized subsystems.Likely an incorrect or invalid review comment.
Devices/guition-jc1060p470ciwy/Source/devices/Power.cpp (2)
139-142: Verify voltage divider formula orientation.The formula computes
(UPPER + LOWER) / LOWER * mv, which assumes the ADC measures across the lower resistor in a voltage divider from battery to ground. Based on the resistor values (85kΩ upper, 100kΩ lower), this yields a multiplier of ~1.85. Please confirm this matches the actual hardware schematic, as swapping upper/lower would significantly change the result.
1-11: LGTM!The overall structure is clean: clear separation of concerns with a well-defined factory function, proper use of ESP-IDF ADC oneshot APIs, and appropriate fallback handling when calibration is unavailable.
Also applies to: 175-177
Devices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.cpp (3)
17-69: Large init command table looks correct.The initialization sequence follows the ESP32-P4 Function EV Board reference. The sleep-out (0x11) and display-on (0x29) commands have appropriate delays (120ms and 20ms respectively) per MIPI DCS specifications.
165-172: LGTM!Disabling
avoid_tearingis a reasonable choice given the comment about preventing stalls during flash writes. The unused parameters are properly marked.
138-163: No undefined behavior risk from stack-local configuration variables.The code is safe. While
dpi_configis created on the stack, it is not retained byjd9165_panel_tafter panel creation. The configuration is only passed toesp_lcd_new_panel_dpi()during initialization, which must copy the struct into its own heap-allocated structures. Unlikeinit_cmds(which is properly declaredstatic constand is retained injd9165_panel_t), thedpi_configpointer is not stored in any structure for later access aftercreatePanelHandle()returns.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @Devices/guition-jc1060p470ciwy/Source/devices/SdCard.cpp:
- Around line 23-26: The NoLock class currently subclasses tt::Lock and returns
true/no-op, removing synchronization for SD operations; replace it with a real
mutex-based lock by removing NoLock and introducing a tt::Mutex (or a class that
wraps tt::Mutex) used where NoLock was referenced (e.g., in SdCard mount/unmount
and filesystem operation paths). Specifically, change usages of NoLock to
acquire/release the tt::Mutex (or provide a tt::Lock-compatible wrapper that
delegates to a tt::Mutex) so all calls to mount, unmount and other SD methods
serialize correctly and prevent concurrent access.
In @Tactility/Source/service/wifi/WifiEsp.cpp:
- Line 5: The preprocessor guards for WiFi are inconsistent: WifiEsp.cpp uses
CONFIG_SOC_WIFI_SUPPORTED || CONFIG_SLAVE_SOC_WIFI_SUPPORTED while other files
(CpuAffinity.cpp, WifiMock.cpp, EspNow* ) use CONFIG_ESP_WIFI_ENABLED and
services use CONFIG_TT_WIFI_ENABLED && !CONFIG_ESP_WIFI_REMOTE_ENABLED; fix by
choosing one consistent approach across the codebase—either (A) revert
WifiEsp.cpp to use CONFIG_ESP_WIFI_ENABLED so all source files use the existing
CONFIG_ESP_WIFI_ENABLED and service guards remain valid, or (B) update Kconfig
to introduce CONFIG_SOC_WIFI_SUPPORTED and CONFIG_SLAVE_SOC_WIFI_SUPPORTED and
add proper depends on CONFIG_TT_WIFI_ENABLED (and ensure CONFIG_ESP_WIFI_ENABLED
is aliased/linked if needed), then update all source files (CpuAffinity.cpp,
WifiMock.cpp, EspNow* and service guards referencing
CONFIG_ESP_WIFI_ENABLED/CONFIG_ESP_WIFI_REMOTE_ENABLED) to use the new SOC
flags; pick one option and apply it consistently across the listed files and
related Kconfig so the build flags and dependencies align.
🧹 Nitpick comments (2)
Devices/guition-jc1060p470ciwy/Source/devices/SdCard.cpp (1)
49-55: Consider verifying the 64KB allocation unit size.The
allocation_unit_sizeof 64KB means every file consumes at least 64KB of space, which wastes storage if you expect many small files (logs, config files, etc.). Typical values are 4KB–16KB.Verify this size aligns with your expected file sizes and usage patterns.
Tactility/Source/service/wifi/WifiEsp.cpp (1)
416-430: Good defensive logging improvement, but fix typo in comment.The conditional logging correctly handles incomplete scan records that may occur on ESP32-P4 with C6 configurations. However, there's a typo in the comment on line 429.
📝 Proposed fix for typo
} else { - LOGGER.info(" - (missing channel info)"); // Behaviour on on P4 with C6 + LOGGER.info(" - (missing channel info)"); // Behaviour on P4 with C6 }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Devices/guition-jc1060p470ciwy/CMakeLists.txtDevices/guition-jc1060p470ciwy/Source/devices/Display.cppDevices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.cppDevices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.hDevices/guition-jc1060p470ciwy/Source/devices/Power.cppDevices/guition-jc1060p470ciwy/Source/devices/SdCard.cppFirmware/KconfigFirmware/idf_component.ymlTactility/Source/service/wifi/WifiEsp.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- Firmware/idf_component.yml
- Devices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.h
- Devices/guition-jc1060p470ciwy/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (6)
📚 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/guition-jc1060p470ciwy/Source/devices/Display.cppDevices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.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/guition-jc1060p470ciwy/Source/devices/Display.cppDevices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.cpp
📚 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/guition-jc1060p470ciwy/Source/devices/Display.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/guition-jc1060p470ciwy/Source/devices/Display.cppDevices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.cpp
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Devices/guition-jc1060p470ciwy/Source/devices/Display.cpp
📚 Learning: 2025-11-11T17:22:20.238Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 411
File: sdkconfig.board.waveshare-s3-touch-lcd-43:39-39
Timestamp: 2025-11-11T17:22:20.238Z
Learning: In Tactility sdkconfig.board.* files, board names (CONFIG_TT_BOARD_NAME) that include screen size measurements intentionally use escaped quotes (e.g., "WaveShare S3 Touch LCD 4.3\"") where the \" represents the inch symbol (") in the final board name string. This is not a syntax error but deliberate formatting.
Applied to files:
Firmware/Kconfig
🧬 Code graph analysis (3)
Devices/guition-jc1060p470ciwy/Source/devices/Display.cpp (3)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)Devices/guition-jc2432w328c/Source/devices/Display.cpp (2)
createTouch(7-15)createTouch(7-7)Devices/guition-jc8048w550c/Source/devices/Display.cpp (2)
createTouch(7-17)createTouch(7-7)
Devices/guition-jc1060p470ciwy/Source/devices/Power.cpp (2)
Tactility/Include/Tactility/hal/power/PowerDevice.h (1)
PowerDevice(8-47)TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Devices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.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-esp32p4, esp32p4)
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: macOS
- GitHub Check: Linux
- GitHub Check: run
🔇 Additional comments (21)
Devices/guition-jc1060p470ciwy/Source/devices/Power.cpp (8)
1-20: LGTM! Clean setup with well-documented constants.The includes, logger setup, and ADC configuration constants are clear and properly documented. The voltage divider resistor values are explicitly noted to match the brookesia configuration.
23-23: Verify the charge estimator voltage range for LiPo batteries.The
ChargeFromVoltageestimator is initialized with a minimum voltage of 3.3V. However, LiPo batteries typically have a safe discharge voltage down to approximately 3.0V. Using 3.3V as the minimum may cause the charge level to report 0% while the battery still has ~10-15% capacity remaining in the 3.0V-3.3V range.If this 3.3V minimum is intentional (e.g., to provide a safety margin or match specific battery characteristics), please confirm. Otherwise, consider adjusting to 3.0f for a more accurate charge estimation across the full discharge curve.
63-92: Previous resource leak issue has been resolved.The resource leak flagged in the previous review has been properly fixed. Lines 84-85 now correctly clean up the ADC unit handle when channel configuration fails, preventing the handle from being leaked.
94-124: Previous logging inconsistency has been resolved.The logging inconsistency flagged in the previous review has been properly fixed. Line 103 now uses
LOGGER.info(...)instead of the problematicTT_LOG_I(TAG, ...), ensuring consistent logging throughout the function.
126-145: LGTM! Voltage divider calculation is mathematically correct.The voltage divider compensation formula is correctly implemented. With 85kΩ upper and 100kΩ lower resistors, the battery voltage is properly calculated as ADC voltage × 1.85. The use of
int64_tfor intermediate calculations prevents overflow, and the fallback raw scaling is appropriate.
147-165: LGTM! Proper resource cleanup with scheme-specific deletion.The cleanup logic correctly handles both calibration schemes with appropriate conditional compilation. While the nested preprocessor directives create some empty blocks in certain configurations, the logic is sound—
calSchemecan only be set within the matching compilation guards, so all paths are correct.
178-180: LGTM! Clean factory implementation.The factory function correctly returns a shared pointer to the PowerDevice implementation.
21-175: Consider thread safety if concurrent access is expected.The
JcPowerclass does not implement any synchronization mechanisms. IfgetMetric()or other methods might be called concurrently from multiple threads, race conditions could occur on the shared state (initialized,calibrated,adcHandle,caliHandle).If the device is only accessed from a single thread or access is externally synchronized, this is acceptable. Otherwise, consider adding mutex protection around the ADC operations and state variables.
Based on the architecture, please verify whether concurrent access to PowerDevice instances is possible in the system.
Tactility/Source/service/wifi/WifiEsp.cpp (1)
979-979: LGTM!The end guard comment correctly reflects the updated compilation condition.
Firmware/Kconfig (2)
44-49: LGTM!The device options now use consistent "Guition" capitalization across all three entries, addressing the previous review feedback.
100-105: TT_WIFI_ENABLED is properly defined with no issues.The option is correctly implemented. It controls whether WiFi features (apps and services) are included in Tactility when WiFi support is available. The low-level implementation (WifiEsp.cpp) is already guarded by
CONFIG_SOC_WIFI_SUPPORTEDorCONFIG_SLAVE_SOC_WIFI_SUPPORTED, which are ESP-IDF macros automatically set based on the target chip—not Tactility Kconfig options. The TT_WIFI_ENABLED flag requires no dependency chain in Kconfig because the SOC-level guards are determined by the build system, not user configuration.Devices/guition-jc1060p470ciwy/Source/devices/Display.cpp (3)
9-18: LGTM!The pin and resolution constants are well-defined using
constexpr autoas per project conventions. The touch configuration withGPIO_NUM_NCfor reset and interrupt pins is acceptable for hardware that doesn't require these signals.
35-65: LGTM!The display initialization sequence is well-structured:
- PWM backlight initialization (with graceful degradation on failure)
- Touch device creation
- Comprehensive LCD configuration
- Display driver instantiation
The use of
reinterpret_pointer_castfor the DisplayDevice conversion is appropriate here.
20-33: LGTM.The GT911 touch device configuration follows the established pattern and is building successfully. The I2C initialization is handled through the dependency chain.
Devices/guition-jc1060p470ciwy/Source/devices/Jd9165Display.cpp (7)
7-67: LGTM!The initialization command sequence is comprehensive and follows the ESP32-P4 Function EV Board reference implementation. The explicit delay values (120ms for sleep out, 20ms for display on) are appropriately specified for reliable panel initialization.
69-79: LGTM!The destructor properly releases both the MIPI DSI bus and LDO channel resources. The fix for the previously reported LDO channel leak has been successfully implemented.
The TODO comment about lifecycle management is noted but doesn't impact correctness.
81-112: LGTM!The MIPI DSI bus initialization is well-structured with proper error handling. The LDO channel is correctly acquired and stored in the member variable for later cleanup. The 2-lane configuration at 750 Mbps is appropriate for the 1024x600 display.
The TODO about the clock source macro for ESP-IDF 6.0 is noted for future updates.
114-131: LGTM!The lazy initialization pattern for the MIPI DSI bus is appropriate, and error handling is consistent throughout. The use of the
JD9165_PANEL_IO_DBI_CONFIG()macro ensures compatibility with the esp_lcd_jd9165 library.
133-144: LGTM!The panel configuration structure is properly initialized with values from the EspLcdConfiguration. The
vendor_configfield is intentionally set tonullptrhere and populated increatePanelHandle, which is a clear and explicit initialization pattern.
196-203: LGTM with documented trade-off.The decision to disable
avoid_tearingis clearly documented with a valid rationale. While this may allow some tearing artifacts, it prevents more severe issues like display stalls and blank flashes caused by blocking operations (e.g., flash writes).This is an acceptable trade-off for this hardware configuration.
146-194: Verify if ESP-IDF JD9165 driver expects persistent vendor_config references.The concern about stack-local
dpi_configandvendor_configpointers is theoretically valid—both structures are created on the stack and their addresses are stored inmutable_panel_configbefore being passed toesp_lcd_new_panel_jd9165(). However, this exact pattern appears consistently across multiple display drivers in the codebase (Jd9853Display, ST7796Display, St7701Display), all of which operate without reported issues.The safety of this code depends on whether the JD9165 driver deep-copies the vendor_config structure during the
esp_lcd_new_panel_jd9165()call, or only reads it during that call without storing pointers to it. The JD9853 driver header documentation explicitly states thatinit_cmdsmust be "declared asstatic constand positioned outside the function," but makes no such requirement for the vendor_config struct itself, suggesting the struct may only be read during initialization.Without access to the actual JD9165 driver source code or definitive ESP-IDF documentation on vendor_config lifetime guarantees, this concern cannot be fully resolved from the codebase alone.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Tactility/Source/service/wifi/WifiMock.cpp (1)
168-168: Update misleading closing comment.The comment
// ESP_PLATFORMis misleading, as this#endifcloses the WiFi support guard from line 5, not anESP_PLATFORMguard. Consider updating it to reflect the actual condition or removing it if not helpful.📝 Suggested fix
-#endif // ESP_PLATFORM +#endif // !CONFIG_SOC_WIFI_SUPPORTED && !CONFIG_SLAVE_SOC_WIFI_SUPPORTED
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Drivers/EspLcdCompat/Source/EspLcdDisplayV2.cppTactility/Source/service/wifi/WifiMock.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- Drivers/EspLcdCompat/Source/EspLcdDisplayV2.cpp
⏰ 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). (3)
- GitHub Check: Linux
- GitHub Check: macOS
- GitHub Check: run
🔇 Additional comments (1)
Tactility/Source/service/wifi/WifiMock.cpp (1)
5-5: No action required—guard logic is sound.The CONFIG_SOC_WIFI_SUPPORTED and CONFIG_SLAVE_SOC_WIFI_SUPPORTED guards are ESP-IDF level hardware capability flags (from sdkconfig.h), not Tactility level. They correctly control whether the mock or real WiFi implementation compiles: WifiMock.cpp (when no hardware) and WifiEsp.cpp (when hardware exists) use complementary guards, which is proper design. CONFIG_TT_WIFI_ENABLED is a separate Tactility-level feature flag serving a different purpose, and no inconsistency exists.
Likely an incorrect or invalid review comment.
This PR contains @josemalm32 's implementation for the Guition JC1060P470CIWY (see #427)
I've added these changes:
EspLcdDisplayV2init for when features are not supportedWifiManageloggingWifiEsp.cpp's check for wifi presenceWifiEsp's scan list logginggcc_soft_float_symbolsin TactiltyC