-
Notifications
You must be signed in to change notification settings - Fork 14
feat(ws-s3-touch): Add espp::WsS3Touch
Board Support Package (BSP) component for the Waveshare ESP32-S3 TouchLCD hardware.
#475
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…component for the Waveshare ESP32-S3 TouchLCD hardware.
✅Static analysis result - no issues found! ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
A new board support package component for the Waveshare ESP32-S3 TouchLCD is introduced, including source code, documentation, example, and CI updates.
- Add the
espp::WsS3Touch
singleton API and its implementation (display, touch, IMU, audio, buttons, board control) - Provide documentation (RST, Markdown) and Doxygen entries for the new component and example
- Update GitHub workflows,
idf_component.yml
, and CMake files to include the new BSP and its example
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
components/ws-s3-touch/include/ws-s3-touch.hpp | Public API definition for the WsS3Touch component |
components/ws-s3-touch/src/*.cpp | Implementation of display, touchpad, IMU, audio, button, and board methods |
doc/en/ws_s3_touch.rst, index.rst, ws_s3_touch_example.md | Documentation additions and toctree entries |
doc/Doxyfile | Added example and header inputs for Doxygen |
components/ws-s3-touch/idf_component.yml | Component manifest for ESP-IDF Component Manager |
.github/workflows/build.yml, upload_components.yml | CI updated to build and publish the new component and example |
Comments suppressed due to low confidence (2)
components/ws-s3-touch/src/touchpad.cpp:55
- [nitpick] Remove the trailing newline (
\n
) in the log format since the logger automatically appends a newline, to avoid a blank line in the output.
logger_.error("could not update touch driver: {}\n", ec.message());
components/ws-s3-touch/src/video.cpp:1
- [nitpick] Consider adding unit tests for SPI callbacks (
lcd_spi_pre_transfer_callback
,lcd_spi_post_transfer_callback
) andwrite_lcd_*
methods to ensure correct flag setting and queue behavior.
#include "ws-s3-touch.hpp"
// | ||
// cppcheck-suppress constParameterCallback | ||
static void lcd_spi_post_transfer_callback(spi_transaction_t *t) { | ||
uint16_t user_flags = (uint32_t)(t->user); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Casting a 32-bit user field to a 16-bit variable may truncate flags. Consider using a 32-bit or pointer-sized type (e.g. uintptr_t user_flags = reinterpret_cast<uintptr_t>(t->user);
).
uint16_t user_flags = (uint32_t)(t->user); | |
uint32_t user_flags = (uint32_t)(t->user); |
Copilot uses AI. Check for mistakes.
esp_err_t ret; | ||
size_t length = (xe - xs + 1) * (ye - ys + 1) * 2; | ||
if (length == 0) { | ||
logger_.error("lcd_send_lines: Bad length: ({},{}) to ({},{})", xs, ys, xe, ye); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After logging a bad frame length, the code continues and may queue zero-length SPI transactions. Add an early return;
inside this block to prevent invalid operations.
logger_.error("lcd_send_lines: Bad length: ({},{}) to ({},{})", xs, ys, xe, ye); | |
logger_.error("lcd_send_lines: Bad length: ({},{}) to ({},{})", xs, ys, xe, ye); | |
return; |
Copilot uses AI. Check for mistakes.
bool WsS3Touch::initialize_touch(const WsS3Touch::touch_callback_t &callback) { | ||
if (touchpad_input_) { | ||
logger_.warn("Touchpad already initialized, not initializing again!"); | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] initialize_touch
returns true
on a redundant call, whereas other initialize_*
methods return false
if already initialized. Consider unifying the return semantics for consistency across the API.
return true; | |
return false; |
Copilot uses AI. Check for mistakes.
Description
Motivation and Context
How has this been tested?
Building and running
ws-s3-touch/example
on a Waveshare ESP32-S3 TouchLCD and playing with it :)Screenshots (if appropriate, e.g. schematic, board, console logs, lab pictures):
ws-s3-touch-compressed.mp4
Types of changes
Checklist:
Software
.github/workflows/build.yml
file to add my new test to the automated cloud build github action.