M5Stack Core S3 Audio Display#364
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
Editor & gitignore tweaks
🎯 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. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)wled00/usermods_list.cppIn file included from wled00/usermods_list.cpp:1: ... [truncated 1094 characters] ... ng/install/lib/clang/18/include" wled00/xml.cppIn file included from wled00/xml.cpp:1: ... [truncated 1060 characters] ... lugins/clang/install/lib/clang/18/include" 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
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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
|
Actionable comments posted: 0 |
|
|
||
| // Ensure Wire is initialized (safe to call even if WLED already started it) | ||
| if (i2c_sda >= 0 && i2c_scl >= 0) { | ||
| Wire.begin(i2c_sda, i2c_scl); |
There was a problem hiding this comment.
please use pinManager.joinWire(), instead of starting a new I2C bus instance with Wire.begin().
| } else { | ||
| Wire.begin(); | ||
| } | ||
| Wire.setTimeout(50); |
There was a problem hiding this comment.
@coderabbitai please check if this timeout is needed.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for Wire.setTimeout usage in the codebase
rg -n "Wire.setTimeout\|Wire\.setTimeout" --type=cpp -C3
# Also check for any Wire.begin calls in usermods for comparison
rg -n "Wire\.begin\|Wire\.setTimeout\|joinWire" usermods/ -C2Length of output: 489
🏁 Script executed:
#!/bin/bash
# Search broadly for Wire.setTimeout
rg -rn "setTimeout" usermods/ wled00/ -C2
# Search for Wire.begin and joinWire in usermods
rg -rn "Wire\.begin\|joinWire" usermods/ wled00/ -C2
# Check how Wire is used in the new usermod specifically
grep -n "Wire\." usermods/M5Stack_CoreS3_Display/usermod_m5stack_s3_display.hLength of output: 49479
🌐 Web query:
ESP32 Arduino Wire.setTimeout default value milliseconds
Length of output: 11098
@softhack007 — The Wire.setTimeout(50) call is not needed and can be removed, for two reasons:
-
Wrong method for I2C transaction timeout:
Wire.setTimeout()(lowercaseo) is theStream-class method that controls how long streaming read functions likereadBytes()block. It does not affect I2C transaction-level timeouts — that isWire.setTimeOut()(capitalO), which already defaults to 50 ms in arduino-esp32. -
Not used by this code's I2C reads: All I2C reads in this usermod use
Wire.available() ? Wire.read() : 0x00, which are direct byte reads and are unaffected by theStreamtimeout.Wire.setTimeout(50)would only matter if the code calledWire.readBytes()orWire.readBytesUntil(), which it does not.
So the call sets the Stream read timeout from 1000 ms to 50 ms but has no observable effect since none of the stream read methods are used. It can be dropped without any behavioral change.
🧠 Learnings used
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-14T23:40:02.031Z
Learning: Add usermod IDs to `wled00/const.h` when creating new usermods
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-14T23:40:02.031Z
Learning: Applies to wled00/**/*.{cpp,h,hpp,c} usermods/**/*.{cpp,h,hpp,c} : Wrap platform-conditional C++ includes in `#ifdef ARDUINO_ARCH_ESP32` / `#ifdef ESP8266`
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR `#270`, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 329
File: usermods/usermod_v2_voice_control/usermod_v2_voice_control.h:192-192
Timestamp: 2026-06-08T16:03:51.733Z
Learning: In MoonModules/WLED-MM, the ESP8266 platform is deprecated. `appendConfigData()` buffer overflows that would only affect ESP8266 (i.e., where the oappend content exceeds the ~2,048 B ESP8266 SETTINGS_STACK_BUF_SIZE but fits comfortably within the ESP32 WLEDMM buffer of 3,834–4,000 B) are acceptable and should not be flagged as blocking issues. The DF2301Q voice control usermod's appendConfigData() (~2,290 bytes) is a confirmed example of this acceptable overflow.
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-14T23:40:02.031Z
Learning: Applies to wled00/**/*.{cpp,h,hpp,c} usermods/**/*.{cpp,h,hpp,c} : Use `WLED_DISABLE_*` and `WLED_ENABLE_*` feature flags for preprocessor-based feature toggling in C++
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2026-04-17T19:40:08.064Z
Learning: In MoonModules/WLED-MM, defining `static const char[] PROGMEM` members for usermod config-key strings in the header file (rather than a companion `.cpp`) is the intentional and repo-normal pattern. Each usermod header is included exactly once via `#ifdef USERMOD_<NAME>` in `usermods_list.cpp`, so there is no ODR risk. Do NOT suggest marking these `inline` (C++17), as that risks duplicating the strings and defeats the flash-saving purpose of `PROGMEM`, especially on ESP8266. Do not flag this pattern as an issue in code reviews.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR `#248` merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-14T23:40:02.031Z
Learning: Applies to wled00/**/*.{cpp,h,hpp,c} usermods/**/*.{cpp,h,hpp,c} : Use `DEBUG_PRINTF()` / `DEBUG_PRINTLN()` for debug output in C++ (compiled out unless `-D WLED_DEBUG`)
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2026-04-04T00:29:49.127Z
Learning: In MoonModules/WLED-MM, function-level doc comment blocks (e.g. /* ***** */ style headers) are NOT required during code reviews, especially not for previously existing functions. Do not flag missing function-level comments as review issues in this repository.
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-14T23:40:02.031Z
Learning: Applies to wled00/**/*.cpp usermods/**/*.cpp : Include `wled.h` as the primary project header in C++ files
| #endif | ||
|
|
||
| #ifdef USERMOD_M5STACK_CORE_S3_DISPLAY | ||
| usermods.add(new M5StackCoreS3DisplayUsermod()); |
There was a problem hiding this comment.
please use the WLED-MM style constructor : usermods.add(new M5StackCoreS3DisplayUsermod("M5StackS3Display", false));
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/xml.cpp (1)
234-244:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign ESP32-S3 GPIO 33–37 reporting guard with runtime reservation contract.
At Line 234, the outer
defined(BOARD_HAS_PSRAM)gate prevents this block from compiling for OPI-flash-only S3 builds, even though Line 242 includesCONFIG_ESPTOOLPY_FLASHMODE_OPI. That can maked.rsvdomit 33–37 while runtime reservation logic still treats them as reserved in OPI mode.Suggested fix
- `#if` defined(ARDUINO_ARCH_ESP32) && defined(BOARD_HAS_PSRAM) + `#if` defined(ARDUINO_ARCH_ESP32) && (defined(BOARD_HAS_PSRAM) || defined(CONFIG_ESPTOOLPY_FLASHMODE_OPI)) `#if` defined(CONFIG_IDF_TARGET_ESP32) // classic esp32 if (strncmp_P(PSTR("ESP32-D0WDR2-V3"), ESP.getChipModel(), 15) == 0) { if (psramFound()) oappend(SET_F(",16")); // GPIO16 reserved for SPI RAM on ESP32-D0WDR2-V3 @@ `#elif` defined(CONFIG_IDF_TARGET_ESP32S3) `#if` CONFIG_SPIRAM_MODE_OCT || CONFIG_ESPTOOLPY_FLASHMODE_OPI if (psramFound()) oappend(SET_F(",33,34,35,36,37")); // in use for "octal" PSRAM or "octal" FLASH `#endif` `#endif` `#endif`🤖 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 `@wled00/xml.cpp` around lines 234 - 244, The GPIO 33–37 reservation branch is currently guarded by defined(BOARD_HAS_PSRAM) so it won't compile into ESP32-S3 OPI-flash-only builds even though CONFIG_ESPTOOLPY_FLASHMODE_OPI should mark those pins reserved at runtime; update the preprocessor gating around the whole block that contains CONFIG_IDF_TARGET_ESP32S3 / CONFIG_SPIRAM_MODE_OCT / CONFIG_ESPTOOLPY_FLASHMODE_OPI so that the S3 OPI branch is compiled even when BOARD_HAS_PSRAM is not defined (e.g. change the outer condition to also allow the case where CONFIG_IDF_TARGET_ESP32S3 && CONFIG_ESPTOOLPY_FLASHMODE_OPI), locating the change around the macros and the oappend(SET_F(",33,34,35,36,37")) call and psramFound() references to ensure d.rsvd includes 33–37 for OPI-flash S3 builds.
🤖 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 @.vscode/extensions.json:
- Line 5: The extension ID strings are inconsistent: replace the
"pioarduino.pioarduino-ide" entry so it matches the other devcontainer
configuration; specifically, pick a canonical ID (e.g.,
"platformio.platformio-ide") and update the mismatched
"pioarduino.pioarduino-ide" occurrence to that same ID, ensuring both extension
entries are identical across the repository.
In `@usermods/M5Stack_CoreS3_Display/usermod_m5stack_s3_display.h`:
- Around line 216-231: The TFT is being initialized (tft.init()) before
reserving SPI pins, so move the pin reservation block (the pins[] array and the
call to pinManager.allocateMultiplePins(...)) to run before calling tft.init();
if allocation fails keep the existing failure path (set enabled = false and
_initOk = false and return) so the display never touches the SPI lines,
referencing the existing pins array, pinManager.allocateMultiplePins,
tft.init(), enabled and _initOk symbols.
---
Outside diff comments:
In `@wled00/xml.cpp`:
- Around line 234-244: The GPIO 33–37 reservation branch is currently guarded by
defined(BOARD_HAS_PSRAM) so it won't compile into ESP32-S3 OPI-flash-only builds
even though CONFIG_ESPTOOLPY_FLASHMODE_OPI should mark those pins reserved at
runtime; update the preprocessor gating around the whole block that contains
CONFIG_IDF_TARGET_ESP32S3 / CONFIG_SPIRAM_MODE_OCT /
CONFIG_ESPTOOLPY_FLASHMODE_OPI so that the S3 OPI branch is compiled even when
BOARD_HAS_PSRAM is not defined (e.g. change the outer condition to also allow
the case where CONFIG_IDF_TARGET_ESP32S3 && CONFIG_ESPTOOLPY_FLASHMODE_OPI),
locating the change around the macros and the oappend(SET_F(",33,34,35,36,37"))
call and psramFound() references to ensure d.rsvd includes 33–37 for OPI-flash
S3 builds.
🪄 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: af04929c-57e0-4f0a-b410-466331ee2f64
📒 Files selected for processing (8)
.gitignore.vscode/extensions.jsonpio-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
| Serial.println("M5StackS3Display: init TFT"); | ||
| tft.init(); | ||
|
|
||
| // Lock SPI pins so they can't be reassigned to other usermods | ||
| PinManagerPinType pins[] = { | ||
| { (gpio_num_t)TFT_MOSI, true }, | ||
| { (gpio_num_t)TFT_SCLK, true }, | ||
| { (gpio_num_t)TFT_CS, true }, | ||
| { (gpio_num_t)TFT_DC, true } | ||
| }; | ||
| if (!pinManager.allocateMultiplePins(pins, 4, PinOwner::UM_Unspecified)) { | ||
| Serial.println("M5StackS3Display: SPI pin allocation FAILED — disabling usermod"); | ||
| enabled = false; | ||
| _initOk = false; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Reserve SPI pins before first TFT bus activity.
Line 217 calls tft.init() before Line 226 validates pin ownership. If allocation fails, the usermod has already driven those pins once, which can conflict with the active owner.
Suggested minimal reorder
- Serial.println("M5StackS3Display: init TFT");
- tft.init();
-
// Lock SPI pins so they can't be reassigned to other usermods
PinManagerPinType pins[] = {
{ (gpio_num_t)TFT_MOSI, true },
{ (gpio_num_t)TFT_SCLK, true },
{ (gpio_num_t)TFT_CS, true },
{ (gpio_num_t)TFT_DC, true }
};
if (!pinManager.allocateMultiplePins(pins, 4, PinOwner::UM_Unspecified)) {
Serial.println("M5StackS3Display: SPI pin allocation FAILED — disabling usermod");
enabled = false;
_initOk = false;
return;
}
Serial.println("M5StackS3Display: SPI pins allocated");
+ Serial.println("M5StackS3Display: init TFT");
+ tft.init();🤖 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
216 - 231, The TFT is being initialized (tft.init()) before reserving SPI pins,
so move the pin reservation block (the pins[] array and the call to
pinManager.allocateMultiplePins(...)) to run before calling tft.init(); if
allocation fails keep the existing failure path (set enabled = false and _initOk
= false and return) so the display never touches the SPI lines, referencing the
existing pins array, pinManager.allocateMultiplePins, tft.init(), enabled and
_initOk symbols.
There was a problem hiding this comment.
I agree, "tft.init();" should come after pinManager.allocateMultiplePins(), not before.
There was a problem hiding this comment.
The PR description image shows an M5Stack Core S3 device with a 320×240 TFT display rendering a 16-band audio visualizer — colored vertical bars with a SSID/IP header — exactly as the usermod describes.
@softhack007 — glad we're aligned. The fix is straightforward: move the pins[] array declaration and the pinManager.allocateMultiplePins() call (with its failure-return block) to just before tft.init(). That way, if pin ownership can't be secured, the SPI lines are never driven by LovyanGFX at all.
🧠 Learnings used
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 308
File: platformio.ini:560-560
Timestamp: 2026-01-07T23:30:25.779Z
Learning: In platformio.ini, ESP32-S3 environments intentionally have WLED_USE_SHARED_RMT commented out (keeping RMTHI driver enabled) for testing purposes, while ESP32-S2 has it enabled (using shared RMT driver) to avoid boot loops. This inconsistency is temporary while investigating why RMTHI causes interrupt watchdog resets on S3. Once the S3 issue is understood, both S2 and S3 will use the same driver flags.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 152
File: wled00/wled.h:827-829
Timestamp: 2026-04-06T21:23:57.593Z
Learning: In MoonModules/WLED-MM on ESP32 with FreeRTOS (wled00/wled.h), calling `xSemaphoreCreateRecursiveMutex()` during C++ static initialization (before `main()`/`setup()`) works in practice because it only requires `malloc()`, which is available before any static initializers run and does not interact with FreeRTOS internal data structures. However, the preferred pattern is to pre-allocate a `StaticSemaphore_t` buffer and call `xSemaphoreCreateRecursiveMutexStatic()` in `setup()` instead — this avoids any heap allocation failure at init, eliminates the need to handle a NULL return at every usage site, and saves 8 bytes of heap overhead compared to the dynamic variant. Insight attributed to willmmiles.
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-14T23:40:02.031Z
Learning: Add usermod IDs to `wled00/const.h` when creating new usermods
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-14T23:40:02.031Z
Learning: Applies to wled00/**/*.{cpp,h,hpp,c} usermods/**/*.{cpp,h,hpp,c} : Wrap platform-conditional C++ includes in `#ifdef ARDUINO_ARCH_ESP32` / `#ifdef ESP8266`
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 329
File: usermods/usermod_v2_voice_control/usermod_v2_voice_control.h:192-192
Timestamp: 2026-06-08T16:03:51.733Z
Learning: In MoonModules/WLED-MM, the ESP8266 platform is deprecated. `appendConfigData()` buffer overflows that would only affect ESP8266 (i.e., where the oappend content exceeds the ~2,048 B ESP8266 SETTINGS_STACK_BUF_SIZE but fits comfortably within the ESP32 WLEDMM buffer of 3,834–4,000 B) are acceptable and should not be flagged as blocking issues. The DF2301Q voice control usermod's appendConfigData() (~2,290 bytes) is a confirmed example of this acceptable overflow.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR `#270`, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-14T23:40:02.031Z
Learning: Applies to wled00/**/*.{cpp,h,hpp,c} usermods/**/*.{cpp,h,hpp,c} : Use `WLED_DISABLE_*` and `WLED_ENABLE_*` feature flags for preprocessor-based feature toggling in C++
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-14T23:40:02.031Z
Learning: Applies to wled00/**/*.cpp usermods/**/*.cpp : Include `wled.h` as the primary project header in C++ files
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2026-04-17T19:40:08.064Z
Learning: In MoonModules/WLED-MM, defining `static const char[] PROGMEM` members for usermod config-key strings in the header file (rather than a companion `.cpp`) is the intentional and repo-normal pattern. Each usermod header is included exactly once via `#ifdef USERMOD_<NAME>` in `usermods_list.cpp`, so there is no ODR risk. Do NOT suggest marking these `inline` (C++17), as that risks duplicating the strings and defeats the flash-saving purpose of `PROGMEM`, especially on ESP8266. Do not flag this pattern as an issue in code reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2026-04-04T00:29:49.127Z
Learning: In MoonModules/WLED-MM, function-level doc comment blocks (e.g. /* ***** */ style headers) are NOT required during code reviews, especially not for previously existing functions. Do not flag missing function-level comments as review issues in this repository.
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-14T23:40:02.031Z
Learning: Applies to wled00/**/*.{cpp,h,hpp,c} usermods/**/*.{cpp,h,hpp,c} : Use `DEBUG_PRINTF()` / `DEBUG_PRINTLN()` for debug output in C++ (compiled out unless `-D WLED_DEBUG`)
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-14T23:40:02.031Z
Learning: Applies to wled00/**/*.{cpp,h,hpp,c} usermods/**/*.{cpp,h,hpp,c} : Do not use `delay()` in effects or hot pixel path in C++
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR `#248` merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
@troyhacks thanks, cool addon 😃. @coderabbitai I'm not sure if LovyanGFX can have conflicts with the HUB75 driver on -S3, please check. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check if HUB75 driver is used anywhere in this repo
rg -rn "HUB75\|MatrixPanel\|ESP32-HUB75" --type=cpp --type=h -l
# Check how LovyanGFX is included/initialized in the new usermod
grep -n "SPI\|HSPI\|VSPI\|SPI3_HOST\|SPI2_HOST\|DMA\|gdma\|dma_chan\|spi_host" usermods/M5Stack_CoreS3_Display/usermod_m5stack_s3_display.h
# Check if there's existing HUB75 usermod
fd -e h -e cpp "hub75\|HUB75\|matrixpanel" --case-sensitive=falseLength of output: 1431 🌐 Web query:
Length of output: 16133
Short answer: No direct hardware conflict, but there is a potential GDMA channel pressure issue. Different hardware peripherals — no bus collision
The two libraries use entirely different hardware peripherals on ESP32-S3, so there is no SPI bus conflict and no peripheral-level collision. Potential GDMA channel exhaustionThe ESP32-S3 has a shared pool of ~5 AHB-GDMA channels. In a WLED-MM context that simultaneously runs:
…you could potentially run into GDMA channel exhaustion, though in practice Practical verdict for this PRThe M5Stack Core S3 + ModuleAudio combination is designed as a standalone audio display device, not a HUB75 controller. The M5Stack Core S3 doesn't have a HUB75 connector and the PR note already calls out that even the Ethernet and Audio modules conflict on pins. Running HUB75 panels from a Core S3 simultaneously would be an unusual edge case — and pin conflicts would likely be the bigger obstacle. So for the purpose of this PR, no action needed. If someone eventually tries to combine both, GDMA channel allocation failure would be the thing to watch for, but that's a separate concern outside this PR's scope. 🧠 Learnings used |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain⚙️ CodeRabbit configuration inspected Length of output: 48126 The PR description image shows the M5Stack Core S3 display running the new usermod — a 16-band equalizer with rainbow-colored bars, SSID "cherrytree" and an IP address in the header bar. Looks great in action!
tools:
fbinfer:
enabled: false
enable_java: falseThe Infer warning you saw was from a review run before that setting was in place (or before my earlier reply prompted the change). From here on, fbinfer will be skipped entirely on all future PRs — |
softhack007
added
the
AI
2 participants
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