Add initial support for LilyGO T-HMI S3 with device configuration and…#509
Add initial support for LilyGO T-HMI S3 with device configuration and…#509Revers-BR wants to merge 8 commits intoTactilityProject:mainfrom
Conversation
… driver implementations
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds support for the LilyGO T‑HMI S3 (ESP32‑S3): an ESP‑IDF CMake component registration with a recursive Source/.c glob; boot initialization function 🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 3
🧹 Nitpick comments (5)
Devices/lilygo-thmi-s3/Source/devices/SdCard.h (1)
7-7: Avoidusingdeclarations at file scope in headers.
using tt::hal::sdcard::SdCardDevice;in a header pollutes the global namespace of every includer. Consider using the fully-qualified name in the declaration on line 14 instead, or moving theusinginside a namespace or function scope.♻️ Suggested fix
-using tt::hal::sdcard::SdCardDevice; - constexpr auto SD_DIO_CMD = GPIO_NUM_11; constexpr auto SD_DIO_SCLK = GPIO_NUM_12; constexpr auto SD_DIO_DATA0 = GPIO_NUM_13; constexpr auto SD_DIO_NC = GPIO_NUM_NC; -std::shared_ptr<SdCardDevice> createSdCard(); +std::shared_ptr<tt::hal::sdcard::SdCardDevice> createSdCard();Devices/lilygo-thmi-s3/lilygo,thmi-s3.dts (1)
5-6: Unused includes:esp32_i2c.handesp32_spi.h.No I2C or SPI nodes are defined in this DTS. If these are placeholders for future expansion, consider adding a brief comment; otherwise, they can be removed to keep the file minimal.
Devices/lilygo-thmi-s3/Source/Init.cpp (1)
5-5:SystemEvents.happears unused — remove or verify.No
SystemEventssymbols are referenced anywhere in this translation unit. This looks like a stale include. Also note the inconsistency: line 5 uses"..."quoting while line 6 uses<...>for a sibling Tactility header.🔧 Proposed change
-#include "Tactility/kernel/SystemEvents.h" `#include` <Tactility/TactilityCore.h>Devices/lilygo-thmi-s3/Source/devices/Display.cpp (1)
8-8:touchSpiInitializedis declared but never read or written — dead code.This static flag is initialized to
falseand never referenced again anywhere in the file. Remove it.🔧 Proposed change
-static bool touchSpiInitialized = false; - static std::shared_ptr<tt::hal::touch::TouchDevice> createTouch() {Devices/lilygo-thmi-s3/Source/devices/Display.h (1)
5-5:#include "driver/spi_common.h"appears to be an unused include.The display bus is i8080 (parallel); touch uses soft-SPI, not the hardware SPI bus. No types from
spi_common.hare referenced in this header or inDisplay.cpp. Consider removing it to keep the include surface minimal.🔧 Proposed change
`#include` <driver/gpio.h> `#include` <Tactility/hal/display/DisplayDevice.h> - -#include "driver/spi_common.h"
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Devices/lilygo-thmi-s3/Source/devices/Power.h (1)
7-10: Consider wrapping constants and factory declaration in a dedicated namespace.
THMI_S3_POWEREN_GPIO,THMI_S3_POWERON_GPIO, andcreatePower()all live in the global namespace. The device-prefix mitigates collision risk, but a namespace (e.g.,lilygo::thmi_s3) would make the ownership explicit and consistent with how the rest of the HAL is structured undertt::hal::*.♻️ Suggested namespace wrapping
+namespace lilygo::thmi_s3 { + constexpr auto THMI_S3_POWEREN_GPIO = GPIO_NUM_10; constexpr auto THMI_S3_POWERON_GPIO = GPIO_NUM_14; std::shared_ptr<tt::hal::power::PowerDevice> createPower(); + +} // namespace lilygo::thmi_s3
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Devices/lilygo-thmi-s3/Source/Configuration.cpp (1)
10-12: Redundanttt::hal::prefix withusing namespace tt::halin scope.Line 12 still qualifies
Devicewith the full namespace despite theusing namespace tt::haldirective on line 10. TheConfigurationon line 21 already drops the prefix. Consider being consistent.♻️ Proposed fix
-static std::vector<std::shared_ptr<tt::hal::Device>> createDevices() { +static std::vector<std::shared_ptr<Device>> createDevices() {
…ssary backlight setting
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Devices/lilygo-thmi-s3/Source/devices/SdCard.h (1)
7-7: Avoidusingdeclarations at global scope in headers.
using tt::hal::sdcard::SdCardDevice;in a header injectsSdCardDeviceinto the global namespace of every translation unit that includesSdCard.h. Prefer the fully-qualified name in the declaration and drop theusing.♻️ Proposed fix
-using tt::hal::sdcard::SdCardDevice; - constexpr auto SD_DIO_CMD = GPIO_NUM_11; constexpr auto SD_DIO_SCLK = GPIO_NUM_12; constexpr auto SD_DIO_DATA0 = GPIO_NUM_13; constexpr auto SD_DIO_NC = GPIO_NUM_NC; constexpr auto SD_DIO_BUS_WIDTH = 1; -std::shared_ptr<SdCardDevice> createSdCard(); +std::shared_ptr<tt::hal::sdcard::SdCardDevice> createSdCard();The
usinginSdCard.cpp(.cppscope) is fine and can stay as-is.
KenVanHoeylandt
left a comment
KenVanHoeylandt
left a comment
There was a problem hiding this comment.
Thanks for the contribution! I've reviewed it and left some comments.
Don't forget to update .github/workflows/build.yml, otherwise the build won't pick it up.
There was a problem hiding this comment.
This file can be removed
| gpio0 { | ||
| compatible = "espressif,esp32-gpio"; | ||
| gpio-count = <49>; | ||
| }; |
There was a problem hiding this comment.
A SPI device is missing here.
| #pragma once | ||
| #include <driver/gpio.h> | ||
| #include <Tactility/hal/display/DisplayDevice.h> | ||
|
|
||
| #include "driver/spi_common.h" | ||
|
|
||
| class St7789i8080Display; | ||
|
|
||
| constexpr auto DISPLAY_CS = GPIO_NUM_6; | ||
| constexpr auto DISPLAY_DC = GPIO_NUM_7; | ||
| constexpr auto DISPLAY_WR = GPIO_NUM_8; | ||
| constexpr auto DISPLAY_RD = GPIO_NUM_NC; | ||
| constexpr auto DISPLAY_RST = GPIO_NUM_NC; | ||
| constexpr auto DISPLAY_BL = GPIO_NUM_38; | ||
| constexpr auto DISPLAY_I80_D0 = GPIO_NUM_48; | ||
| constexpr auto DISPLAY_I80_D1 = GPIO_NUM_47; | ||
| constexpr auto DISPLAY_I80_D2 = GPIO_NUM_39; | ||
| constexpr auto DISPLAY_I80_D3 = GPIO_NUM_40; | ||
| constexpr auto DISPLAY_I80_D4 = GPIO_NUM_41; | ||
| constexpr auto DISPLAY_I80_D5 = GPIO_NUM_42; | ||
| constexpr auto DISPLAY_I80_D6 = GPIO_NUM_45; | ||
| constexpr auto DISPLAY_I80_D7 = GPIO_NUM_46; | ||
| constexpr auto DISPLAY_HORIZONTAL_RESOLUTION = 240; | ||
| constexpr auto DISPLAY_VERTICAL_RESOLUTION = 320; | ||
|
|
||
| // Touch (XPT2046, resistive, shared SPI with display) | ||
| constexpr auto TOUCH_MISO_PIN = GPIO_NUM_4; | ||
| constexpr auto TOUCH_MOSI_PIN = GPIO_NUM_3; | ||
| constexpr auto TOUCH_SCK_PIN = GPIO_NUM_1; | ||
| constexpr auto TOUCH_CS_PIN = GPIO_NUM_2; | ||
| constexpr auto TOUCH_IRQ_PIN = GPIO_NUM_9; | ||
|
|
||
| // Factory function for registration | ||
| std::shared_ptr<tt::hal::display::DisplayDevice> createDisplay(); |
There was a problem hiding this comment.
This comment doesn't seem to be correct. The display isn't a SPI one.
| DISPLAY_VERTICAL_RESOLUTION | ||
| ); | ||
|
|
||
| return std::make_shared<Xpt2046SoftSpi>(std::move(config)); |
There was a problem hiding this comment.
There doesn't seem to be a need to use SoftSPI here. SoftSPI is only for when you need a third SPI device, but a hardware one is not available.
Xpt2046Touch should be used instead, unless there's a really good reason to not use it (and then it should be documented here)
There was a problem hiding this comment.
I tried using Xpt2046 Touch
However, there was no touch response.
Only Xpt2046SoftSpi worked without problems, passing all the necessary pins.
I don't have much knowledge about this; I practically only adapted it from lilygo-tdisplay to thmi.
But after publication, if you want to make the adjustments, I can test it and get back to you.
| return ERROR_NONE; | ||
| } | ||
|
|
||
| /** @warning The variable name must be exactly "device_module" */ |
There was a problem hiding this comment.
This comment can be removed as it's not correct anymore.
| } | ||
|
|
||
| /** @warning The variable name must be exactly "device_module" */ | ||
| struct Module device_module = { |
There was a problem hiding this comment.
The naming recently changed: it should now be "lilygo_thmi_s3_module".
The documentation wasn't updated for this yet, sorry!
it's basically the device id, replacing "-" with "_" and post-fixed with "_module".
2 participants
Add initial support for LilyGO T-HMI S3 with device configuration and driver implementations
📝 Description
This commit adds complete support for the LilyGO T-HMI S3, an ESP32-S3 based display device, including all the necessary hardware configurations, drivers, and initialization routines.
🎯 What's Added
Configuration Files
device.properties– Hardware specifications (16MB flash, SPI RAM in OCT mode, 2.8" 240×320 display, 125 DPI)devicetree.yaml– Device tree dependencies and bindingslilygo,thmi-s3.dts– Device tree source with GPIO configuration (49 GPIO pins)Core Implementation
Configuration.cpp– Hardware initialization factory, registering power, SD card, display, and single-button deviceInit.cpp– Boot initialization including power signal setup (GPIO 10 & 14) and PWM backlight initialization (GPIO 38)CMakeLists.txt– Build configuration with required dependenciesmodule.cpp– Module registration structureDriver Components
Display.cpp/h) – ST7789 display controller via i8080 interface with XPT2046 resistive touch supportPower.cpp/h) – Estimated power monitoring via ADC with 2.11x multiplierSdCard.cpp/h) – SDMMC interface (1-bit mode on GPIO 11-13)✨ Key Features
🔧 Dependencies
📋 Hardware Details
Display Interface
Power Management
SD Card Interface
Button
🚀 Type of Change
✅ Checklist
https://lilygo.cc/en-us/products/t-hmi?srsltid=AfmBOormg1UM8XAeQ7_zp3xhivIllgu3_fREdxg7SxNG8-itKqPkpL2D