M5Stack Core S3 Audio Display#364
Open
troyhacks wants to merge 8 commits into
Open
Conversation
WalkthroughAdds a new M5Stack Core S3 display usermod (ILI9342 over SPI3) with docs, a PlatformIO helper script to enable LTO for ESP32‑S3 toolchains, and gates octal-PSRAM GPIO reservations on octal-mode build flags. ChangesM5Stack Core S3 Display Usermod
LTO Build Optimization for ESP32-S3
ESP32-S3 PSRAM GPIO Reservation
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 4
🧹 Nitpick comments (2)
pio-scripts/enable_lto_s3.py (1)
47-50: ⚡ Quick winDerive toolchain prefix from compiler basename, not global string replacement.
Current replacement is applied to the entire
CCvalue and can mangle paths, causinggcc-ar/gcc-ranliblookup to fail silently.Suggested fix
- toolchain_prefix = cc.replace("gcc", "").replace("g++", "") - # toolchain_prefix is something like "xtensa-esp32s3-elf-" - new_ar = toolchain_prefix + "gcc-ar" - new_ranlib = toolchain_prefix + "gcc-ranlib" + cc_name = os.path.basename(cc) + # toolchain_prefix is something like "xtensa-esp32s3-elf-" + if cc_name.endswith("gcc"): + toolchain_prefix = cc_name[:-3] + elif cc_name.endswith("g++"): + toolchain_prefix = cc_name[:-3] + else: + toolchain_prefix = "" + + new_ar = f"{toolchain_prefix}gcc-ar" + new_ranlib = f"{toolchain_prefix}gcc-ranlib"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pio-scripts/enable_lto_s3.py` around lines 47 - 50, The current code mangles CC when replacing "gcc"/"g++" across the whole path; instead derive toolchain_prefix from the compiler basename: get cc_basename = os.path.basename(cc), strip a trailing "gcc" or "g++" only from cc_basename (handle possible suffixes like ".exe" first if needed) to produce toolchain_prefix, then build new_ar and new_ranlib from that prefix (use the variables cc, toolchain_prefix, new_ar, new_ranlib to locate the change). This ensures you only remove the compiler name, not parts of the path, preventing broken paths when constructing gcc-ar/gcc-ranlib.usermods/M5Stack_CoreS3_Display/usermod_m5stack_s3_display.h (1)
127-251: ⚡ Quick winMove repeated literals to
static const char[] PROGMEM.This usermod repeatedly uses identical string literals (
"M5StackS3Display...","M5StackS3Display","M5S3_enabled"). Centralizing them in PROGMEM reduces RAM churn and keeps usermod style consistent.As per coding guidelines,
usermods/**/*.{cpp,h,hpp}: Store repeated strings in usermods as static const char[] PROGMEM.Also applies to: 356-401
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/M5Stack_CoreS3_Display/usermod_m5stack_s3_display.h` around lines 127 - 251, The code repeatedly emits identical string literals (e.g. "M5StackS3Display: starting setup", "M5StackS3Display: setup complete", "M5StackS3Display: init TFT", "M5StackS3Display", "Loading...", "WLED", "M5S3_enabled", and the various Serial.printf messages) which should be moved into static const char[] stored in PROGMEM and referenced instead to reduce RAM usage; add a small block of static const char[] PROGMEM declarations at the top of usermod_m5stack_s3_display.h and replace all literal usages in the setup routine (the Serial.println/printf calls, tft.drawString calls, and any configuration keys) with references to those PROGMEM constants (using FPSTR or appropriate pgm functions/macros) so the strings are read from flash rather than duplicated in RAM.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@usermods/M5Stack_CoreS3_Display/readme.md`:
- Around line 20-34: The Markdown code fence is malformed: replace the opening
line that reads "```build_flags =" with a plain triple-backtick line so the
block starts with ``` followed by the existing lines (including the build_flags
= and USERMOD_M5STACK_CORE_S3_DISPLAY entries) and ensure the block is closed
with a matching triple-backtick after the LovyanGFX URL in the lib_deps section
so the entire build_flags / lib_deps snippet (including keys like build_flags,
lib_deps, USERMOD_M5STACK_CORE_S3_DISPLAY and the LovyanGFX dependency) is
inside a single valid fenced code block.
- Around line 47-50: The README claims "BGR color order" but the usermod sets
cfg.rgb_order = false (commented as RGB); fix the mismatch by either updating
the README to state "RGB color order" to match cfg.rgb_order = false and adjust
the README comment about inversion/backlight if needed, or change cfg.rgb_order
to true (and update the inline comment) if BGR was intended—ensure consistency
between the README entry and the cfg.rgb_order setting and its inline comment.
In `@usermods/M5Stack_CoreS3_Display/usermod_m5stack_s3_display.h`:
- Around line 142-143: The I2C reads call Wire.requestFrom(...) and immediately
use Wire.read() without verifying data was received; update each occurrence that
reads from AXP2101_ADDR (where variables like pwr_orig and similar bytes are
assigned) to first check the return value or Wire.available() >= 1 and only call
Wire.read() when a byte is available, otherwise handle the error path (skip the
read, return/error out, or set a safe default and log the failure) to avoid
using invalid register data from failed I2C transactions; apply this change to
all occurrences that read from AXP2101_ADDR in usermod_m5stack_s3_display.h.
- Around line 225-229: If pinManager.allocateMultiplePins(pins, 4,
PinOwner::UM_Unspecified) fails, abort further initialization so the usermod
does not drive TFT/SPI pins; specifically, inside the same setup/init function
(where the Serial prints occur) stop any subsequent pinMode/digitalWrite/SPI/TFT
initialization, set the module state to disabled (e.g., a flag like _enabled =
false or return false from setupUsermod), and optionally release any partial
allocations via pinManager.releaseMultiplePins if available; ensure no further
calls to functions that drive the pins (TFT.begin, SPI.begin, pinMode for pins
in pins[]) execute when allocation failed.
---
Nitpick comments:
In `@pio-scripts/enable_lto_s3.py`:
- Around line 47-50: The current code mangles CC when replacing "gcc"/"g++"
across the whole path; instead derive toolchain_prefix from the compiler
basename: get cc_basename = os.path.basename(cc), strip a trailing "gcc" or
"g++" only from cc_basename (handle possible suffixes like ".exe" first if
needed) to produce toolchain_prefix, then build new_ar and new_ranlib from that
prefix (use the variables cc, toolchain_prefix, new_ar, new_ranlib to locate the
change). This ensures you only remove the compiler name, not parts of the path,
preventing broken paths when constructing gcc-ar/gcc-ranlib.
In `@usermods/M5Stack_CoreS3_Display/usermod_m5stack_s3_display.h`:
- Around line 127-251: The code repeatedly emits identical string literals (e.g.
"M5StackS3Display: starting setup", "M5StackS3Display: setup complete",
"M5StackS3Display: init TFT", "M5StackS3Display", "Loading...", "WLED",
"M5S3_enabled", and the various Serial.printf messages) which should be moved
into static const char[] stored in PROGMEM and referenced instead to reduce RAM
usage; add a small block of static const char[] PROGMEM declarations at the top
of usermod_m5stack_s3_display.h and replace all literal usages in the setup
routine (the Serial.println/printf calls, tft.drawString calls, and any
configuration keys) with references to those PROGMEM constants (using FPSTR or
appropriate pgm functions/macros) so the strings are read from flash rather than
duplicated in RAM.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8f180cd-573c-4944-8c1c-a6936cf49480
📒 Files selected for processing (6)
pio-scripts/enable_lto_s3.pyusermods/M5Stack_CoreS3_Display/readme.mdusermods/M5Stack_CoreS3_Display/usermod_m5stack_s3_display.hwled00/const.hwled00/usermods_list.cppwled00/xml.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/M5Stack_CoreS3_Display/usermod_m5stack_s3_display.h (1)
316-318:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
um_data->u_data[2]before memcpy.
getUMData()returning true confirms the usermod exists, but doesn't guaranteeu_data[2]is a valid pointer. If AudioReactive's data layout changes or initialization is incomplete, this memcpy could dereference null.Suggested fix
um_data_t *um_data = nullptr; if (usermods.getUMData(&um_data, USERMOD_ID_AUDIOREACTIVE)) { // Real audio data - memcpy(geq, um_data->u_data[2], NUM_GEQ_BANDS); + if (um_data && um_data->u_data[2]) { + memcpy(geq, um_data->u_data[2], NUM_GEQ_BANDS); + } else { + memset(geq, 0, NUM_GEQ_BANDS); // fallback to silence + } } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/M5Stack_CoreS3_Display/usermod_m5stack_s3_display.h` around lines 316 - 318, The memcpy from um_data->u_data[2] into geq can dereference a null pointer even when usermods.getUMData(...) returns true; before calling memcpy in the block that checks USERMOD_ID_AUDIOREACTIVE, verify that um_data is non-null and that um_data->u_data[2] is a valid pointer (and ideally that it points to at least NUM_GEQ_BANDS bytes) — if the pointer is null or size is insufficient, skip the memcpy (or zero/clear geq) and emit a warning/error via the same logging mechanism so the code does not crash. Ensure you reference usermods.getUMData, um_data, um_data->u_data[2], memcpy, geq, and NUM_GEQ_BANDS when implementing the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pio-scripts/enable_lto_s3.py`:
- Around line 47-57: The cc_basename stripping logic leaves a trailing dash and
yields names like "xtensa-...-elf--gcc-ar"; update the removal to drop the
preceding dash as well (e.g., remove "-gcc" or "-g++" rather than just
"gcc"/"g++") when computing cc_basename so new_ar and new_ranlib produce
"xtensa-...-elf-gcc-ar" and "xtensa-...-elf-gcc-ranlib"; locate the code around
the cc_basename manipulation (the block that checks
cc_basename.endswith(".exe"), endswith("gcc") and endswith("g++")) and change
the slice or use a safer approach (e.g., rsplit with "-" or strip the suffix
including the dash) so the generated tool names are correct.
---
Outside diff comments:
In `@usermods/M5Stack_CoreS3_Display/usermod_m5stack_s3_display.h`:
- Around line 316-318: The memcpy from um_data->u_data[2] into geq can
dereference a null pointer even when usermods.getUMData(...) returns true;
before calling memcpy in the block that checks USERMOD_ID_AUDIOREACTIVE, verify
that um_data is non-null and that um_data->u_data[2] is a valid pointer (and
ideally that it points to at least NUM_GEQ_BANDS bytes) — if the pointer is null
or size is insufficient, skip the memcpy (or zero/clear geq) and emit a
warning/error via the same logging mechanism so the code does not crash. Ensure
you reference usermods.getUMData, um_data, um_data->u_data[2], memcpy, geq, and
NUM_GEQ_BANDS when implementing the check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0af1bb63-cfb8-4901-970d-114090c625fe
📒 Files selected for processing (3)
pio-scripts/enable_lto_s3.pyusermods/M5Stack_CoreS3_Display/readme.mdusermods/M5Stack_CoreS3_Display/usermod_m5stack_s3_display.h
✅ Files skipped from review due to trivial changes (1)
- usermods/M5Stack_CoreS3_Display/readme.md
Collaborator
Author
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
usermods/M5Stack_CoreS3_Display/usermod_m5stack_s3_display.h (1)
362-398: 💤 Low valueStore repeated string literals as
static const char[] PROGMEM.The strings
"M5StackS3Display"and"M5S3_enabled"are used multiple times. Per usermod guidelines, these should be stored as PROGMEM constants to reduce RAM usage.♻️ Suggested refactor
Add near the top of the class (private section):
static const char _name[] PROGMEM; static const char _enabled[] PROGMEM;And after the class definition:
const char M5StackCoreS3DisplayUsermod::_name[] PROGMEM = "M5StackS3Display"; const char M5StackCoreS3DisplayUsermod::_enabled[] PROGMEM = "M5S3_enabled";Then replace string literals with
FPSTR(_name)andFPSTR(_enabled)in the JSON methods.As per coding guidelines: "Store repeated strings in usermods as
static const char[] PROGMEM"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/M5Stack_CoreS3_Display/usermod_m5stack_s3_display.h` around lines 362 - 398, Declare two PROGMEM string constants for the repeated literals (e.g., static const char _name[] PROGMEM; static const char _enabled[] PROGMEM) in the M5StackCoreS3DisplayUsermod class (private section), provide their definitions after the class (const char M5StackCoreS3DisplayUsermod::_name[] PROGMEM = "M5StackS3Display"; const char M5StackCoreS3DisplayUsermod::_enabled[] PROGMEM = "M5S3_enabled";), and replace all occurrences of the raw literals in addToJsonInfo, addToJsonState, readFromJsonState, addToConfig, readFromConfig and appendConfigData with FPSTR(_name) and FPSTR(_enabled) (and format the addHB call to use FPSTR(_name) appropriately) so the strings are stored in PROGMEM and RAM usage is reduced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@usermods/M5Stack_CoreS3_Display/usermod_m5stack_s3_display.h`:
- Around line 362-398: Declare two PROGMEM string constants for the repeated
literals (e.g., static const char _name[] PROGMEM; static const char _enabled[]
PROGMEM) in the M5StackCoreS3DisplayUsermod class (private section), provide
their definitions after the class (const char
M5StackCoreS3DisplayUsermod::_name[] PROGMEM = "M5StackS3Display"; const char
M5StackCoreS3DisplayUsermod::_enabled[] PROGMEM = "M5S3_enabled";), and replace
all occurrences of the raw literals in addToJsonInfo, addToJsonState,
readFromJsonState, addToConfig, readFromConfig and appendConfigData with
FPSTR(_name) and FPSTR(_enabled) (and format the addHB call to use FPSTR(_name)
appropriately) so the strings are stored in PROGMEM and RAM usage is reduced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 57efb5a6-660a-44af-811e-54a8dd9fe389
📒 Files selected for processing (1)
usermods/M5Stack_CoreS3_Display/usermod_m5stack_s3_display.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The goal was to add a visualizer to the display on an M5Stack Core S3 board with their ModuleAudio add-on device so you can see it's working. It's intended to be used as a standalone line-in device for AudioReactive network sync.
This was an on-site hack as a gift to HexSauna camp at SideBurn 2026 to give their lighting lead something that didn't exist in WLED or WLED-MM... and looked fancy AF.
Because of the WLED-MM FPS limiter being able to be set to "unlimited", the code keeps a minimum of 5 FPS on the display but otherwise stays out of the way as best it can. It also pushes a maximum of 100 FPS to the display match AudioReactive. The minimum was established because if you're using this display usermod, you likely want to see it do something.
Drawing the display uses a differential system so only changes are painted. This decreases the display overhead by 80% or more on average.
This PR also fixes a bug in xml.cpp:
if (psramFound()) oappend(SET_F(",33,34,35,36,37"));This was unguarded on the S3 with PSRAM detected, so any S3 board blocked off these pins even if it's QSPI, as the Core S3 board uses. This is set now only when octal modes are used. The Core S3 has 8MB of PSRAM but it's QSPI, likely a design tradeoff to keep more pins usable for their addon boards but still ha e lots of PSRAM.
Intention was to combine the M5Stack Ethernet module with their Audio module, but they are incompatible due to overlapping pins.
This uses LovyanGFX as that sidesteps a TON of issues with TFT_eSPI in "keeping it simple".
@softhack007 This is absolutely "vibe coded" but with sane ideas and multiple rounds of code audits. My personal LTO script for the S3 is in here - it's not required, but could be added to the codebase.
Summary by CodeRabbit
New Features
Documentation
Chores
Bug Fixes