Add support for Waveshare ESP32-C6-LCD-1.47 board #394
Conversation
📝 WalkthroughWalkthroughAdds support for a Waveshare ESP32-C6 LCD 1.47 board: new board files (CMakeLists, Configuration.cpp, Init.cpp, display and SD card device headers/impl), a sdkconfig for the board, and a Kconfig choice. CI build matrix gains a waveshare-esp32-c6-lcd-147 entry. IDF component dependencies in idf_component.yml are converted to mapping form with target-based rules. CMakeLists now conditionally wraps the esp_panic_handler link option for Xtensa. Code paths were made architecture-aware: crash diagnostics and panic handling add Xtensa-specific processing, and CPU affinity functions are gated for dual-core FreeRTOS. Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tactility/Source/hal/sdcard/SdmmcDevice.cpp (1)
100-103: Fix misleading comment about SPI bus.This comment references "SPI bus" sharing between SD card and screen, but
SdmmcDeviceuses the SDMMC peripheral with dedicated pins, not a shared SPI bus. The comment appears to be copied from an SPI-based SD card implementation and doesn't apply to SDMMC.Apply this diff to clarify the locking rationale:
- /** - * The SD card and the screen are on the same SPI bus. - * Writing and reading to the bus from 2 devices at the same time causes crashes. - * This work-around ensures that this check is only happening when LVGL isn't rendering. - */ + /** + * Acquire lock to ensure thread-safe access when checking card status. + * This prevents concurrent access issues when LVGL or other components may be active. + */
🧹 Nitpick comments (11)
Tactility/Source/app/crashdiagnostics/QrUrl.cpp (2)
18-33: Avoid raw malloc; prefer std::vector and handle OOM.Safer and simpler, and avoids null‑checks and manual free.
- auto* stack_buffer = (uint32_t*) malloc(crash_data.callstackLength * 2 * sizeof(uint32_t)); + std::vector<uint32_t> stack_buffer(crash_data.callstackLength * 2, 0); ... - stack_buffer[i * 2] = pc; + stack_buffer[i * 2] = pc; #if CRASH_DATA_INCLUDES_SP - stack_buffer[(i * 2) + 1] = sp; + stack_buffer[(i * 2) + 1] = sp; #endif ... - free(stack_buffer); + // vector frees itself
43-50: Consider delimiters or zero‑padding for hex fields.Current concatenation without separators and padding can be ambiguous. If protocol allows, add a separator (e.g., '.') and zero‑pad to 8 hex chars with i/o manipulators.
Verify server parser expectations before changing the URL format.
TactilityCore/Source/kernel/PanicHandler.cpp (2)
40-49: Validate subsequent frames too.Only the first frame’s PC/SP is sanity‑checked. If later frames are bogus, they’ll be recorded. Recommend validating each frame with esp_cpu_process_stack_pc + esp_ptr_executable and esp_stack_ptr_is_sane, and marking corruption when any check fails (don’t ignore post‑first EXCCAUSE).
while ( frame.next_pc != 0 && !crashData.callstackCorrupted && crashData.callstackLength < CRASH_DATA_CALLSTACK_LIMIT ) { if (esp_backtrace_get_next_frame(&frame)) { + uint32_t processed = esp_cpu_process_stack_pc(frame.pc); + if (!esp_ptr_executable((void*)processed) || !esp_stack_ptr_is_sane(frame.sp)) { + crashData.callstackCorrupted = true; + break; + } crashData.callstack[crashData.callstackLength].pc = frame.pc; #if CRASH_DATA_INCLUDES_SP crashData.callstack[crashData.callstackLength].sp = frame.sp; #endif crashData.callstackLength++;
76-86: RISC‑V stub: acceptable stopgap; document and track.Stub returns empty data; fine for now. Please open a follow‑up to implement RISC‑V collection (e.g., using backtrace helpers or frame pointers).
I can draft a minimal RISC‑V backtrace using esp_backtrace and EH frames when available.
Boards/WaveshareEsp32C6Lcd147/Source/devices/Display.h (1)
5-5: Namespace the factory to avoid ODR collisions.Global createDisplay may collide if multiple boards compile together. Consider a board‑scoped namespace (e.g., boards::waveshare::esp32c6_lcd147::createDisplay).
Boards/WaveshareEsp32C6Lcd147/Source/devices/SdCard.cpp (2)
3-3: Remove unused include.Tactility/lvgl/LvglSync.h isn’t used in this file.
-#include <Tactility/lvgl/LvglSync.h>
9-21: Nit: include gpio header explicitly.createSdCard uses GPIO_NUM_*; include driver/gpio.h for clarity and robustness against transitive changes.
+#include <driver/gpio.h>Boards/WaveshareEsp32C6Lcd147/Source/devices/Display.cpp (1)
29-35: Consider bumping SPI clock if stable on C6.12 MHz is safe; some panels tolerate 20–40 MHz on SPI2 with proper wiring. Optional later tuning.
Boards/WaveshareEsp32C6Lcd147/Source/Init.cpp (1)
51-76: Simplify backlight mapping (optional).Current “81*(100−percentage)” mapping is non‑linear and opaque. A linear 0–255 to 0–LEDC_MAX map is predictable and avoids magic numbers.
- // Calculate duty with inverted logic (higher level = higher duty) - uint32_t duty; - if (percentage == 0) { - duty = 0; - } else { - // Map to full range: 0% -> 0, 100% -> LCD_BL_LEDC_DUTY - duty = LCD_BL_LEDC_DUTY - ((81 * (100 - percentage))); - } + // Linear map: 0 -> 0, 255 -> LCD_BL_LEDC_DUTY + uint32_t duty = (static_cast<uint32_t>(level) * LCD_BL_LEDC_DUTY) / 255;If the non‑linear curve is intentional, add a brief comment with the source/rationale.
Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp (2)
4-4: Remove unused include.PwmBacklight.h isn’t referenced here.
-#include <PwmBacklight.h>
42-43: max_transfer_sz: explicit parentheses and type.Make arithmetic intent clear and avoid integer promotion surprises.
- .max_transfer_sz = ((172 * (320 / 10)) * LV_COLOR_DEPTH / 8), + .max_transfer_sz = static_cast<int>((172 * (320 / 10)) * (LV_COLOR_DEPTH / 8)),Or compute once with constexprs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.github/workflows/build-firmware.yml(1 hunks)Boards/WaveshareEsp32C6Lcd147/CMakeLists.txt(1 hunks)Boards/WaveshareEsp32C6Lcd147/Source/Init.cpp(1 hunks)Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp(1 hunks)Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.h(1 hunks)Boards/WaveshareEsp32C6Lcd147/Source/devices/Display.cpp(1 hunks)Boards/WaveshareEsp32C6Lcd147/Source/devices/Display.h(1 hunks)Boards/WaveshareEsp32C6Lcd147/Source/devices/SdCard.cpp(1 hunks)Boards/WaveshareEsp32C6Lcd147/Source/devices/SdCard.h(1 hunks)Buildscripts/board.cmake(1 hunks)CMakeLists.txt(1 hunks)Firmware/Kconfig(1 hunks)Firmware/idf_component.yml(2 hunks)Tactility/Source/app/crashdiagnostics/QrUrl.cpp(2 hunks)Tactility/Source/hal/sdcard/SdmmcDevice.cpp(1 hunks)TactilityCore/Source/CpuAffinity.cpp(2 hunks)TactilityCore/Source/kernel/PanicHandler.cpp(3 hunks)sdkconfig.board.waveshare-esp32-c6-lcd-147(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.h (1)
Tactility/Include/Tactility/Tactility.h (1)
hal(38-43)
Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp (3)
Boards/WaveshareEsp32C6Lcd147/Source/devices/Display.cpp (2)
createDisplay(6-38)createDisplay(6-6)Boards/WaveshareEsp32C6Lcd147/Source/devices/SdCard.cpp (2)
createSdCard(9-26)createSdCard(9-9)Boards/WaveshareEsp32C6Lcd147/Source/Init.cpp (2)
initBoot(80-82)initBoot(80-80)
Boards/WaveshareEsp32C6Lcd147/Source/devices/Display.cpp (1)
Boards/WaveshareEsp32C6Lcd147/Source/Init.cpp (2)
setBacklightDuty(51-76)setBacklightDuty(51-51)
TactilityCore/Source/kernel/PanicHandler.cpp (1)
Tactility/Source/app/boot/Boot.cpp (1)
ESP_PLATFORM(121-139)
Boards/WaveshareEsp32C6Lcd147/Source/devices/Display.h (2)
Tactility/Include/Tactility/hal/display/DisplayDevice.h (1)
DisplayDevice(15-47)Boards/WaveshareEsp32C6Lcd147/Source/devices/Display.cpp (2)
createDisplay(6-38)createDisplay(6-6)
Boards/WaveshareEsp32C6Lcd147/Source/devices/SdCard.h (2)
Tactility/Include/Tactility/hal/sdcard/SdCardDevice.h (1)
SdCardDevice(14-67)Boards/WaveshareEsp32C6Lcd147/Source/devices/SdCard.cpp (2)
createSdCard(9-26)createSdCard(9-9)
🪛 Clang (14.0.6)
Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.h
[error] 3-3: 'Tactility/hal/Configuration.h' file not found
(clang-diagnostic-error)
Boards/WaveshareEsp32C6Lcd147/Source/Init.cpp
[error] 1-1: 'driver/ledc.h' file not found
(clang-diagnostic-error)
Boards/WaveshareEsp32C6Lcd147/Source/devices/Display.h
[error] 3-3: 'Tactility/hal/display/DisplayDevice.h' file not found
(clang-diagnostic-error)
Boards/WaveshareEsp32C6Lcd147/Source/devices/SdCard.h
[error] 3-3: 'Tactility/hal/sdcard/SdCardDevice.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (20)
Tactility/Source/hal/sdcard/SdmmcDevice.cpp (1)
1-1: LGTM: Appropriate guard for SDMMC host availability.The additional
SOC_SDMMC_HOST_SUPPORTEDcheck correctly restricts compilation to platforms with SDMMC host controller support. This is essential for ESP32-C6 compatibility, which lacks the SDMMC peripheral and uses SPI for SD card access instead.TactilityCore/Source/CpuAffinity.cpp (2)
44-62: LGTM! Well-structured single-core and dual-core configurations.The configuration correctly handles both single-core and dual-core systems:
- Single-core (ESP32-C6): All affinities hardcoded to 0, which is appropriate since there's only one core available
- Dual-core: Uses the helper functions to determine WiFi and main dispatcher affinities based on the system configuration
The separation ensures that the helper functions (which are only compiled for dual-core systems) are only called in the dual-core configuration, preventing compilation errors for single-core targets.
9-30: LGTM! Core 0 default is correct.The conditional compilation properly restricts these helper functions to dual-core systems. The addition of default return values (core 0) in the
#elseclauses is both necessary and correct—it prevents compilation errors when neitherCONFIG_ESP32_WIFI_TASK_PINNED_TO_CORE_0norCONFIG_ESP32_WIFI_TASK_PINNED_TO_CORE_1is defined. The assertion on line 82 (assert(esp.wifi == esp.mainDispatcher)) validates that both functions return the same value, confirming that core 0 is the intended default and maintains the design invariant.Tactility/Source/app/crashdiagnostics/QrUrl.cpp (1)
21-25: PC handling: LGTM for Xtensa vs RISC‑V.Using esp_cpu_process_stack_pc on Xtensa and raw PC on RISC‑V aligns with IDF expectations and the producer in PanicHandler.
Confirm IDF version guarantees esp_cpu.h availability on RISC‑V targets in your CI matrix.
TactilityCore/Source/kernel/PanicHandler.cpp (2)
8-11: Includes: OK after gating fix.esp_cpu.h/esp_cpu_utils.h and xtensa/xtruntime.h are appropriate for this block. No further changes once sdkconfig.h precedes the gate.
1-86: Repo-wide CONFIG_ gating issue confirmed across 21 files.*Verification reveals the problem is systematic: PanicHandler.cpp is one of 21 files using CONFIG_* macros in preprocessor conditions before including sdkconfig.h. Affected files span TactilityCore, Tactility, Libraries, Boards, and Drivers. At file-level conditionals like line 1 of PanicHandler.cpp, standard include-ordering solutions may not apply if CONFIG_* values come from compiler flags. Consider whether these configurations should derive from sdkconfig.h or if they're correctly provided via build system defines.
.github/workflows/build-firmware.yml (1)
44-45: LGTM! Build matrix entry correctly added.The new board entry follows the existing pattern and properly specifies the esp32c6 architecture for the RISC-V-based ESP32-C6 chip.
CMakeLists.txt (1)
51-54: LGTM! Architecture-specific panic handler wrapping.Correctly guards the panic handler wrapping for Xtensa-only targets, enabling RISC-V (ESP32-C6) support. This aligns with the PR objectives for architecture compatibility.
Boards/WaveshareEsp32C6Lcd147/Source/devices/SdCard.h (1)
1-7: LGTM! Clean factory header.The header follows the established pattern for device factories. The static analysis error about the missing include is a false positive—the file exists in the Tactility component, which is properly declared as a dependency in the board's CMakeLists.txt.
Boards/WaveshareEsp32C6Lcd147/CMakeLists.txt (1)
1-7: LGTM! Standard board component registration.The CMakeLists.txt follows the established pattern for board integration, with appropriate dependencies for the ST7789 display, PWM backlight, and core Tactility framework.
Firmware/Kconfig (1)
72-73: LGTM! Kconfig entry correctly added.The board configuration option follows the established naming convention and is positioned logically with other Waveshare boards.
sdkconfig.board.waveshare-esp32-c6-lcd-147 (1)
1-55: LGTM! Comprehensive board configuration.The sdkconfig properly configures the ESP32-C6 target with appropriate settings for Octa PSRAM, LVGL display parameters (~600 DPI for the 1.47" screen), and necessary stack size adjustments. All settings align with the hardware specifications described in the PR.
Firmware/idf_component.yml (1)
2-31: LGTM! Target-based dependency rules added.Converting dependencies to structured format with target restrictions correctly prevents incompatible LCD/touch drivers from being included on the ESP32-C6 (RISC-V) target. This enables the multi-architecture support goals of the PR while maintaining existing version pins.
Buildscripts/board.cmake (1)
82-83: LGTM! Board mapping correctly added.The board ID mapping follows the established pattern and correctly maps "waveshare-esp32-c6-lcd-147" to the WaveshareEsp32C6Lcd147 component directory.
Boards/WaveshareEsp32C6Lcd147/Source/devices/Display.h (1)
3-3: Include path check.Analyzer flagged Tactility/hal/display/DisplayDevice.h not found. Confirm include dirs propagate Tactility headers here as well.
Boards/WaveshareEsp32C6Lcd147/Source/devices/Display.cpp (1)
14-27: Panel config looks correct for 1.47" ST7789.Offsets, mirror flags, invert color, and reset pin align with common Waveshare mappings.
If you have the hardware handy, please confirm the 34‑pixel gapX and invertColor=true visually (no tearing/inversion).
Boards/WaveshareEsp32C6Lcd147/Source/Init.cpp (1)
1-1: Compile environment note.Clang static analysis reported driver/ledc.h missing. Ensure ESP‑IDF includes are available for this TU (component depends on driver).
Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp (1)
11-17: Device order LGTM; keep TODO for RGB LED.Display then SD on shared bus is sensible; placeholder for RGB LED is fine.
Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.h (1)
3-3: No action required—include path is properly configured.The include path for
<Tactility/hal/Configuration.h>is correctly configured:
- Tactility is declared in the
REQUIRESclause ofWaveshareEsp32C6Lcd147/CMakeLists.txt, ensuring Tactility's include directories are available- Tactility's
CMakeLists.txtexposesInclude/viaidf_component_register(INCLUDE_DIRS "Include/")- The file
Tactility/Include/Tactility/hal/Configuration.hexistsESP-IDF's build system will correctly resolve the include path through the REQUIRES mechanism.
Boards/WaveshareEsp32C6Lcd147/Source/devices/SdCard.cpp (1)
18-20: The review comment is incorrect. Both the display and SD card already use the same lock.The code is working as intended. When the SPI configuration is initialized,
tt::lvgl::getSyncLock()is passed via the.lockfield. Thespi::init()function stores this directly indataArray[SPI2_HOST].lock. Subsequently, whenspi::getLock(SPI2_HOST)is called from SdCard.cpp, it returns this same lock instance. The developer correctly understood this relationship when they wrote the comment "Use same lock as display"—both code paths reference the identical lock object. No unification or changes are required.Likely an incorrect or invalid review comment.
|
Thanks, I'll review this tomorrow. I love seeing C6 support! I'll update the app projects soon so they can make C6 builds too. I'm mainly dropping in to tell you to take the review bot's comments with a grain of salt. I'm not a big fan of AI for this purpose, but it's the only form of reviews I get on my own PRs, hence why it's here. |
KenVanHoeylandt
left a comment
KenVanHoeylandt
left a comment
There was a problem hiding this comment.
Looks good! Meanwhile I resolved some of the AI code review responses.
| ledc_update_duty(LCD_BL_LEDC_MODE, LCD_BL_LEDC_CHANNEL); | ||
| } | ||
|
|
||
| } // namespace driver::pwmbacklight |
There was a problem hiding this comment.
Did the driver from Drivers/PwmBacklight/ not work?
If it didn't work then I'm fine with the custom code. Otherwise:
You should be able to use the things from <PwmBacklight.h> (although in rare cases it might not work)
You use it by calling this from initBoot(), just like your function:
if (!driver::pwmbacklight::init(LCD_BL_PIN, 256)) {
TT_LOG_E(TAG, "Backlight init failed");
return false;
}And then the display config can simply set:
.backlightDutyFunction = driver::pwmbacklight::setBacklightDuty,| .data6_io_num = GPIO_NUM_NC, | ||
| .data7_io_num = GPIO_NUM_NC, | ||
| .data_io_default_level = false, | ||
| .max_transfer_sz = ((172 * (320 / 10)) * LV_COLOR_DEPTH / 8), |
There was a problem hiding this comment.
Can we make this a constexpr auto .. in Display.h?
A common setup (from another board) looks like this:
constexpr auto LCD_SPI_HOST = SPIX_HOST;
constexpr auto LCD_PIN_CS = GPIO_NUM_XX;
constexpr auto LCD_PIN_DC = GPIO_NUM_XX; // RS
constexpr auto LCD_HORIZONTAL_RESOLUTION = ...;
constexpr auto LCD_VERTICAL_RESOLUTION = ...;
constexpr auto LCD_BUFFER_HEIGHT = (LCD_VERTICAL_RESOLUTION / 3);
constexpr auto LCD_BUFFER_SIZE = (LCD_HORIZONTAL_RESOLUTION * LCD_BUFFER_HEIGHT);
constexpr auto LCD_SPI_TRANSFER_SIZE_LIMIT = LCD_BUFFER_SIZE * LV_COLOR_DEPTH / 8;
Yesterday I refactored the remaining boards to use constexpr in this manner
| @@ -0,0 +1,5 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
This file can be removed.
(it's recently changed, likely before you started the board development)
|
Considering it's the first C6 implementation, I ordered the device so it can get continued support with each release. |
|
FYI: I'm about to merge a change where |
This commit adds full support for the Waveshare ESP32-C6-LCD-1.47 development board, featuring a 172x320 ST7789 display with 1.47" diagonal and SD card support. This is the first ESP32-C6 (RISC-V) board supported by the project. Key changes: - New board implementation with ST7789 display (172x320, 12MHz SPI) - SD card support via shared SPI bus with proper CS coordination - PWM backlight control using LEDC peripheral - Build system integration (GitHub Actions, CMake, Kconfig) - Architecture compatibility fixes for ESP32-C6/RISC-V: - Conditional panic handler wrapping (Xtensa-only feature) - Updated CPU affinity handling for single-core chips - Fixed stack PC processing for RISC-V - SDMMC host check for unsupported peripherals - Component dependency rules for multi-architecture support - Development environment setup with .envrc for ESP-IDF Hardware specifications: - MCU: ESP32-C6 (RISC-V, single-core, 160MHz) - Display: 1.47" ST7789 LCD (172x320, ~600 DPI) - RAM: 4MB Octal PSRAM (80MHz) - Flash: 4MB QSPI (80MHz) - Storage: microSD card slot (shared SPI with display) - Backlight: PWM-controlled via GPIO22 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Rename WaveshareEsp32C6Lcd147.cpp to Configuration.cpp - Remove unused WaveshareEsp32C6Lcd147.h header file This aligns the board implementation with other boards in the codebase, which use Configuration.cpp as the standard name for the board configuration file. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
marciogranzotto
force-pushed
the
waveshare-esp32-c6-lcd-147
branch
from
b12be69 to
6481292
Compare
Thanks! I've rebased the branch and made these changes. I'll now check out the other comments |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
TactilityCore/Source/kernel/PanicHandler.cpp (1)
1-1: Include sdkconfig.h before checking CONFIG_IDF_TARGET_ARCH_XTENSA.The preprocessor checks
CONFIG_IDF_TARGET_ARCH_XTENSAat line 1 beforesdkconfig.his included, which can cause the Xtensa-specific code to be skipped even on Xtensa targets if the macro is undefined at that point.Boards/waveshare-esp32-c6-lcd-147/Source/Init.cpp (1)
16-49: Parameters unused; already flagged in prior review.The
init()function parameters are not consumed—hardcoded constants are used instead. This issue and a suggestion to use the existingDrivers/PwmBacklight/driver have already been raised in previous review comments.
🧹 Nitpick comments (2)
Boards/waveshare-esp32-c6-lcd-147/CMakeLists.txt (1)
1-7: Consider explicit source listing instead of GLOB_RECURSE.
GLOB_RECURSEis convenient but won't trigger CMake reconfiguration when source files are added or removed, potentially causing stale builds. If this pattern is already established project-wide, you can keep it; otherwise, consider listing sources explicitly for more reliable incremental builds.Boards/waveshare-esp32-c6-lcd-147/Source/Configuration.cpp (1)
11-17: Device creation looks good; RGB LED can be added later.The display and SD card device initialization is properly structured.
For the RGB LED TODO on line 15: Do you want me to open a new issue to track adding WS2812 RGB LED support on GPIO8 via RMT peripheral?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/build-firmware.yml(1 hunks)Boards/waveshare-esp32-c6-lcd-147/CMakeLists.txt(1 hunks)Boards/waveshare-esp32-c6-lcd-147/Source/Configuration.cpp(1 hunks)Boards/waveshare-esp32-c6-lcd-147/Source/Init.cpp(1 hunks)Boards/waveshare-esp32-c6-lcd-147/Source/devices/Display.cpp(1 hunks)Boards/waveshare-esp32-c6-lcd-147/Source/devices/Display.h(1 hunks)Boards/waveshare-esp32-c6-lcd-147/Source/devices/SdCard.cpp(1 hunks)Boards/waveshare-esp32-c6-lcd-147/Source/devices/SdCard.h(1 hunks)CMakeLists.txt(1 hunks)Firmware/Kconfig(1 hunks)Firmware/idf_component.yml(2 hunks)Tactility/Source/app/crashdiagnostics/QrUrl.cpp(2 hunks)Tactility/Source/hal/sdcard/SdmmcDevice.cpp(1 hunks)TactilityCore/Source/CpuAffinity.cpp(2 hunks)TactilityCore/Source/kernel/PanicHandler.cpp(3 hunks)sdkconfig.board.waveshare-esp32-c6-lcd-147(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/build-firmware.yml
- Tactility/Source/hal/sdcard/SdmmcDevice.cpp
- CMakeLists.txt
- sdkconfig.board.waveshare-esp32-c6-lcd-147
- TactilityCore/Source/CpuAffinity.cpp
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-27T22:33:23.792Z
Learnt from: KenVanHoeylandt
PR: ByteWelder/Tactility#394
File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50
Timestamp: 2025-10-27T22:33:23.792Z
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:
Boards/waveshare-esp32-c6-lcd-147/Source/Configuration.cpp
📚 Learning: 2025-10-26T12:44:21.833Z
Learnt from: KenVanHoeylandt
PR: ByteWelder/Tactility#391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.833Z
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:
Boards/waveshare-esp32-c6-lcd-147/Source/Configuration.cpp
🧬 Code graph analysis (5)
Boards/waveshare-esp32-c6-lcd-147/Source/devices/Display.h (2)
Tactility/Include/Tactility/hal/display/DisplayDevice.h (1)
DisplayDevice(15-47)Boards/waveshare-esp32-c6-lcd-147/Source/devices/Display.cpp (2)
createDisplay(6-38)createDisplay(6-6)
Boards/waveshare-esp32-c6-lcd-147/Source/devices/SdCard.h (3)
Tactility/Source/service/sdcard/Sdcard.cpp (1)
sdcard(29-52)Tactility/Include/Tactility/hal/sdcard/SdCardDevice.h (1)
SdCardDevice(14-67)Boards/waveshare-esp32-c6-lcd-147/Source/devices/SdCard.cpp (2)
createSdCard(9-26)createSdCard(9-9)
Boards/waveshare-esp32-c6-lcd-147/Source/devices/Display.cpp (1)
Boards/waveshare-esp32-c6-lcd-147/Source/Init.cpp (2)
setBacklightDuty(51-76)setBacklightDuty(51-51)
TactilityCore/Source/kernel/PanicHandler.cpp (1)
Tactility/Source/app/boot/Boot.cpp (1)
ESP_PLATFORM(121-139)
Boards/waveshare-esp32-c6-lcd-147/Source/Configuration.cpp (1)
Tactility/Include/Tactility/Tactility.h (1)
hal(38-43)
🪛 Clang (14.0.6)
Boards/waveshare-esp32-c6-lcd-147/Source/Init.cpp
[error] 1-1: 'driver/ledc.h' file not found
(clang-diagnostic-error)
Boards/waveshare-esp32-c6-lcd-147/Source/devices/Display.h
[error] 3-3: 'Tactility/hal/display/DisplayDevice.h' file not found
(clang-diagnostic-error)
Boards/waveshare-esp32-c6-lcd-147/Source/devices/SdCard.h
[error] 3-3: 'Tactility/hal/sdcard/SdCardDevice.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (13)
Tactility/Source/app/crashdiagnostics/QrUrl.cpp (1)
21-25: LGTM! Architecture-specific PC handling is correct.The conditional PC processing appropriately handles the architectural differences between XTENSA and RISC-V. Using
esp_cpu_process_stack_pc()for XTENSA and the rawframe.pcfor RISC-V aligns with ESP-IDF conventions and the PR's stated objective to fix stack PC processing for RISC-V.TactilityCore/Source/kernel/PanicHandler.cpp (1)
8-10: LGTM! Robust PC processing for Xtensa.The PC processing logic correctly:
- Uses
esp_cpu_process_stack_pcto handle stack PC transformations- Validates executable memory via
esp_ptr_executable- Handles the
InstrFetchProhibitededge case where the faulting PC is itself non-executable but still valid for backtrace purposesThe updated corruption check properly combines stack pointer sanity with PC validity.
Also applies to: 40-48
Firmware/Kconfig (1)
74-75: LGTM!The new board configuration entry follows the established pattern and integrates cleanly into the existing choice menu.
Boards/waveshare-esp32-c6-lcd-147/Source/Init.cpp (1)
51-76: Verify the backlight duty mapping logic.The duty calculation on line 66 uses a hardcoded formula (
LCD_BL_LEDC_DUTY - (81 * (100 - percentage))) that may produce unexpected results. For example, at 100% brightness (percentage=100), duty = 8191 - 0 = 8191 (correct). But at lower percentages, the subtraction of a fixed81 * (100 - percentage)may not scale linearly across the full duty range (0–8191).Confirm this formula matches the vendor's demo code and produces the intended brightness curve.
Boards/waveshare-esp32-c6-lcd-147/Source/devices/Display.h (1)
1-5: LGTM!Clean factory function declaration following established patterns.
Boards/waveshare-esp32-c6-lcd-147/Source/devices/SdCard.h (1)
1-7: LGTM!Clean factory function declaration with a convenience alias. The pattern is consistent with the display device setup.
Boards/waveshare-esp32-c6-lcd-147/Source/devices/Display.cpp (1)
6-38: LGTM!The display configuration is well-documented and appropriate for the ST7789 1.47" panel. The 12 MHz SPI clock and 34-pixel X offset match the hardware specs, and the shared SPI2_HOST bus is correctly set up for coordination with the SD card.
Boards/waveshare-esp32-c6-lcd-147/Source/devices/SdCard.cpp (1)
9-26: LGTM! Well-coordinated shared SPI bus configuration.The SD card configuration correctly shares SPI2_HOST with the display and uses the same lock for mutual exclusion. The
csProtectionPinsparameter with GPIO_NUM_14 (display CS) ensures the display CS line stays high during SD operations, preventing bus contention.Firmware/idf_component.yml (1)
2-18: The component registry API calls failed. Let me search for documentation on ESP32-C6 LCD driver compatibility.Let me search for official Espressif component documentation on these drivers.
Verification shows LCD drivers are likely ESP32-C6 compatible despite conservative target gating.
The official Espressif esp_lcd_ili9341 v2.0.1 component supports "all targets", which includes ESP32-C6. Working examples of ESP32-C6 with ILI9341 TFT displays exist in repositories, and the nopnop2002/esp-idf-ili9340 driver explicitly lists esp32c6 as a supported target. For the XPT2046 touch controller, component documentation states "testing is limited to ESP32 and ESP32-S3, other ESP32 variants should work", suggesting ESP32-C6 compatibility is expected.
All four drivers (ILI9341, ILI9488, ST7735, XPT2046) use SPI or parallel interfaces with no inherent ESP32-C6 incompatibility. The conservative target gating to
[esp32, esp32s3]likely reflects testing limitations rather than genuine incompatibility. Addesp32c6to the target lists to enable future ESP32-C6 board ports, or document the specific incompatibility reason if one exists beyond untested status.Boards/waveshare-esp32-c6-lcd-147/Source/Configuration.cpp (4)
1-10: LGTM: Includes and namespace usage are clean.The includes are well-organized and all dependencies are necessary for the configuration.
21-25: Core configuration fields are appropriate.The boot initialization, UI scale setting (
UiScale::Smallestfor the 1.47" display), and device creation setup are correctly configured.
47-50: SPI initialization mode and lock usage are correct.The
ByTactilityinit mode andtt::lvgl::getSyncLock()lock are properly configured for esp_lvgl_port integration.Based on learnings
52-53: Empty UART configuration is acceptable.No UART devices are needed for this board configuration.
| espressif/esp_lcd_st7796: | ||
| version: "1.3.4" | ||
| rules: | ||
| - if: "target in [esp32, esp32s3]" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify ST7796 driver compatibility with ESP32-C6.
The ST7796 driver is now restricted to [esp32, esp32s3]. Verify whether it supports ESP32-C6, and if so, add esp32c6 to enable future board support.
🏁 Script executed:
#!/bin/bash
# Check if ST7796 supports ESP32-C6
curl -s "https://components.espressif.com/api/v1/components/espressif/esp_lcd_st7796/versions/1.3.4" | jq -r '.targets[]' 2>/dev/null || echo "Failed to fetch target info"Length of output: 27
I'll search for documentation on the ST7796 driver's ESP32 compatibility to verify ESP32-C6 support.
Add esp32c6 to ST7796 driver target list.
ESP32-C6 support is available for ST7796, requiring ESP-IDF V5.1. Update the rules condition in Firmware/idf_component.yml at lines 28-31 to include esp32c6:
espressif/esp_lcd_st7796:
version: "1.3.4"
rules:
- if: "target in [esp32, esp32s3, esp32c6]"
🤖 Prompt for AI Agents
Firmware/idf_component.yml around lines 28-31: the ST7796 component rule only
allows esp32 and esp32s3 targets but needs esp32c6 support; modify the rules
condition to include esp32c6 so the line becomes a target list containing esp32,
esp32s3, esp32c6, and ensure the project uses ESP-IDF v5.1 or later as required
by the driver.
Board-specific improvements (waveshare-esp32-c6-lcd-147): - Replace custom backlight implementation with existing PwmBacklight driver (Init.cpp) - Add constexpr constants for all pins and configuration values (Display.h) - Use constexpr constants instead of magic numbers throughout board code - Clean up includes: remove unused LvglSync.h, add explicit driver/gpio.h - Simplify board initialization code (78 lines → 4 lines in Init.cpp) Framework-level improvements: - Fix misleading comment in SdmmcDevice.cpp about locking rationale - Replace malloc/free with std::vector in QrUrl.cpp for safer memory management - Add stack frame validation for all frames in PanicHandler.cpp (not just first) Build verified: waveshare-esp32-c6-lcd-147 builds successfully 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Boards/waveshare-esp32-c6-lcd-147/Source/devices/Display.h (1)
1-30: LGTM!The header structure and constant definitions are well-organized and documented. The use of modern C++ features (digit separators,
autowithconstexpr) is appropriate.Optional note: Line 30 uses
LV_COLOR_DEPTHwhich is an LVGL macro. While this is likely available via transitive includes fromDisplayDevice.h, consider explicitly including the LVGL header if you encounter any compilation issues in different include orders.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Boards/waveshare-esp32-c6-lcd-147/Source/Configuration.cpp(1 hunks)Boards/waveshare-esp32-c6-lcd-147/Source/Init.cpp(1 hunks)Boards/waveshare-esp32-c6-lcd-147/Source/devices/Display.cpp(1 hunks)Boards/waveshare-esp32-c6-lcd-147/Source/devices/Display.h(1 hunks)Boards/waveshare-esp32-c6-lcd-147/Source/devices/SdCard.cpp(1 hunks)Boards/waveshare-esp32-c6-lcd-147/Source/devices/SdCard.h(1 hunks)Tactility/Source/app/crashdiagnostics/QrUrl.cpp(1 hunks)Tactility/Source/hal/sdcard/SdmmcDevice.cpp(2 hunks)TactilityCore/Source/kernel/PanicHandler.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- TactilityCore/Source/kernel/PanicHandler.cpp
- Tactility/Source/hal/sdcard/SdmmcDevice.cpp
- Boards/waveshare-esp32-c6-lcd-147/Source/Configuration.cpp
- Boards/waveshare-esp32-c6-lcd-147/Source/devices/SdCard.cpp
- Boards/waveshare-esp32-c6-lcd-147/Source/Init.cpp
🧰 Additional context used
🧬 Code graph analysis (3)
Boards/waveshare-esp32-c6-lcd-147/Source/devices/SdCard.h (2)
Tactility/Include/Tactility/hal/sdcard/SdCardDevice.h (1)
SdCardDevice(14-67)Boards/waveshare-esp32-c6-lcd-147/Source/devices/SdCard.cpp (2)
createSdCard(9-26)createSdCard(9-9)
Tactility/Source/app/crashdiagnostics/QrUrl.cpp (1)
TactilityCore/Source/kernel/PanicHandler.cpp (4)
getRtcCrashData(83-83)getRtcCrashData(83-83)getRtcCrashData(94-94)getRtcCrashData(94-94)
Boards/waveshare-esp32-c6-lcd-147/Source/devices/Display.h (2)
Tactility/Include/Tactility/hal/display/DisplayDevice.h (1)
DisplayDevice(15-47)Boards/waveshare-esp32-c6-lcd-147/Source/devices/Display.cpp (2)
createDisplay(6-38)createDisplay(6-6)
🪛 Clang (14.0.6)
Boards/waveshare-esp32-c6-lcd-147/Source/devices/SdCard.h
[error] 3-3: 'Tactility/hal/sdcard/SdCardDevice.h' file not found
(clang-diagnostic-error)
Boards/waveshare-esp32-c6-lcd-147/Source/devices/Display.h
[error] 3-3: 'Tactility/hal/display/DisplayDevice.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (8)
Boards/waveshare-esp32-c6-lcd-147/Source/devices/SdCard.h (1)
1-13: LGTM! Clean SD card header with proper shared SPI bus design.The header correctly declares the SD card configuration for the shared SPI bus setup. The factory pattern is appropriate, the CS pin is well-documented, and the implementation properly coordinates with the display's CS line to prevent conflicts.
Note: The static analysis error about
SdCardDevice.hnot being found is a false positive—the build system provides the correct include paths.Tactility/Source/app/crashdiagnostics/QrUrl.cpp (3)
8-13: LGTM – Architecture-aware includes are correct.The conditional inclusion of
esp_cpu_utils.hfor Xtensa is appropriate sinceesp_cpu_process_stack_pcis Xtensa-specific. Note that the maintainer previously reviewed and approved the include order (CONFIG_* usage beforesdkconfig.h), so the current structure is intentional.
19-19: Excellent refactor to std::vector.Replacing dynamic allocation with
std::vectorprovides automatic memory management, exception safety, and eliminates the manualfree()call. The sizing (callstackLength * 2) correctly accommodates the PC and optional SP for each frame.
23-27: LGTM – Correct architecture-specific PC handling.The conditional PC processing appropriately handles the architectural difference: Xtensa requires
esp_cpu_process_stack_pcto decode the stack PC, while RISC-V uses the raw value. The explanatory comment is helpful.Boards/waveshare-esp32-c6-lcd-147/Source/devices/Display.cpp (3)
1-4: LGTM!The includes are appropriate for the display factory implementation.
29-37: LGTM!The SPI configuration and display instantiation are correct. The 12 MHz clock speed and queue depth of 10 are appropriate for this hardware.
14-27: Initialization order verified correctly.The backlight initialization happens before display creation. The
Hal.cppflow confirms thatinitBoot()is called first (which callsdriver::pwmbacklight::init()), thenregisterDevices()immediately follows, which triggerscreateDisplay(). The panel configuration correctly referencesdriver::pwmbacklight::setBacklightDutyafter the PWM module is already initialized. No changes needed.Boards/waveshare-esp32-c6-lcd-147/Source/devices/Display.h (1)
32-32: LGTM!The factory function declaration is correct and follows the expected pattern.
|
How does Tactility look on your board (+ how is the scaling)? Just curious! |
|
Regarding the new merge conflicts: the device-specific In the very near future, I'll also be renaming the |
|
I created a separate PR for P4 and C6 support, based on your code changes related to the new SOC. That should resolve the pipeline issues you're having. As soon as it's merged, you should be able to update this PR so it's only about the new board. edit: It's merged. |
- Implement generic ESP32 devices - Updated GitHub Actions to first build the SDKs. These are now based on the generic device implementations and the build act as a filter before compiling the dozens of other devices. It should save on resources when boards fail to compile. - Adapted code to C6 and P4 differences, heavily borrowed from from #394 written by @marciogranzotto, with some changes of my own - Updated `device.py` to make the `[display]` section optional
|
Any chance for getting this PR updated to fix the build? If not, then I can probably pick up the work in the coming months. |
3 participants
This PR adds full support for the Waveshare ESP32-C6-LCD-1.47 development board, featuring a 172x320 ST7789 display with 1.47" diagonal and SD card support. This is the first ESP32-C6 (RISC-V) board supported by the project.
Key changes:
Hardware specifications: