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 6 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)
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.
1 participant
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