Add generic screen to ESP32_S3 target #3054
Conversation
WalkthroughThe pull request introduces updates to the Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
targets/ESP32/CMakePresets.json (2)
341-346: Consider adding SPIRAM allocation for graphics supportWhile the graphics configuration is correct, consider adding
ESP32_SPIRAM_FOR_IDF_ALLOCATIONlike other boards with display support to ensure sufficient memory for graphics operations."ESP32_USB_CDC": "ON", "API_nanoFramework.Graphics": "ON", "GRAPHICS_DISPLAY": "Generic_SPI.cpp", "TOUCHPANEL_DEVICE": "XPT2046.cpp", "GRAPHICS_DISPLAY_INTERFACE": "Spi_To_Display.cpp", -"TOUCHPANEL_INTERFACE": "Spi_To_TouchPanel.cpp" +"TOUCHPANEL_INTERFACE": "Spi_To_TouchPanel.cpp", +"ESP32_SPIRAM_FOR_IDF_ALLOCATION": "1024 * 1024"
341-346: LGTM: Consistent graphics configuration across ESP32_S3 variantsThe graphics configuration is consistently applied across all ESP32_S3 variants (base, BLE, and ALL), with appropriate SPIRAM allocation in the BLE and ALL variants. This implementation aligns well with the PR objective of adding generic screen support for M5STACK devices.
The consistent use of Generic_SPI.cpp display driver across variants provides good flexibility for different screen types while maintaining a unified codebase.
Also applies to: 359-367, 385-393
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)targets/ESP32/CMakePresets.json(2 hunks)
🔇 Additional comments (3)
README.md (1)
73-73: LGTM: Board descriptions accurately reflect display capabilities
The updated descriptions for ESP32_S3 boards clearly indicate the addition of display support while maintaining consistency with existing documentation style.
targets/ESP32/CMakePresets.json (2)
Line range hint 83-87: LGTM: Graphics configuration matches established patterns
The graphics configuration follows the same pattern used in other ESP32 boards with display support, using appropriate display and touchpanel drivers.
341-346: Verify display driver implementation
The configuration references Generic_SPI.cpp and XPT2046.cpp. Let's verify these files are available in the codebase.
✅ Verification successful
All required display and touchpanel driver files are present
The verification confirms that all referenced files exist in the codebase:
- Display drivers:
src/nanoFramework.Graphics/Graphics/Displays/Generic_SPI.cppsrc/nanoFramework.Graphics/Graphics/Displays/Spi_To_Display.cpp
- Touch panel drivers:
src/nanoFramework.Graphics/TouchPanel/Devices/XPT2046.cpptargets/ESP32/ESP32/nanoCLR/nanoFramework.Graphics/Spi_To_TouchPanel.cpp(and variants for other ESP32 targets)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for display and touchpanel driver files
echo "Checking for display driver files..."
fd "Generic_SPI.cpp|XPT2046.cpp|Spi_To_Display.cpp|Spi_To_TouchPanel.cpp"
Length of output: 735
josesimoes
added
Platform: ESP32
Description
Add generic screen to ESP32_S3 target
The ESP32_S3 target can be used for ESP32_S3 targets without any PSRAM. i.e M5STACK devices.
Motivation and Context
From discord discussion
https://discord.com/channels/478725473862549535/1318854127429881866
How Has This Been Tested?
Build tested locally
Types of changes
Checklist
Summary by CodeRabbit
New Features
Documentation
Chores