Switch to out of tree AudioReactive Usermod#5311
Switch to out of tree AudioReactive Usermod#5311
Conversation
|
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:
WalkthroughRemoved the audioreactive usermod (sources, manifest, build override, docs), added a centralized Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
netmindz
changed the title
|
aOpt is missing in upstream |
|
|
| protected: | ||
| // Shim for oappend(), which used to exist in utils.cpp | ||
| template<typename T> static inline void oappend(const T& t) { oappend_shim->print(t); }; | ||
| template<typename T> static inline void oappendi(const T& t) { oappend_shim->print(t); }; |
There was a problem hiding this comment.
@willmmiles - possibly not needed, just change the usermod
WLED-MM has
bool oappendi(int i)
{
char s[16]; // WLEDMM max 32bit integer needs 11 chars (sign + 10) not 10
snprintf(s, 15, "%d", i); // WLEDMM
return oappend(s);
}
There was a problem hiding this comment.
Ah, the usual API drift between forks ;)
Yes, in a perfect world I'd suggest a local shim in the usermod. That said, I wouldn't be opposed to a "MM usermod compatibility" PR, since we wanted to add global name support anyways.
There was a problem hiding this comment.
To be honest, I'm out of my depth with C when it comes to templates, so I have no idea if what I have written is valid, I was just trying to get it to compile. The comment suggests that oappend is kinda legacy, so is there something better the usermod should use?
There was a problem hiding this comment.
The intent was to change to a new API, passing an Arduino Print& directly to the usermod code so we could completely eliminate buffer size limitations. Updated usermods implement void appendConfigData(Print& uiScript) instead. The relevant PR was #4152 .
The quick shim would be to do something like this in audio_reactive.h:
#ifndef WLEDMM // or whatever other define is unique to MM
#define oappendi oppend
#endif
... since the current oappend shim here is guaranteed to be length safe for all integer values.
![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
🤖 Fix all issues with AI agents
In `@platformio.ini`:
- Line 395: The change sets custom_usermods to empty which removes the
AudioReactive usermod for generic builds; update the PR/release notes and add a
migration guide explaining how affected users can restore AudioReactive:
instruct them to build with the env named "env:usermods", or copy the
AudioReactive example from platformio_override.sample.ini into their
platformio.ini, or enable the feature via build flags (e.g., -D
UM_AUDIOREACTIVE_ENABLE plus required I2S pin flags); optionally add a new
example build environment demonstrating external AudioReactive library usage and
clearly mark this as a breaking change in the CHANGELOG with these migration
steps.
- Line 284: The AR_lib_deps entry currently lacks version pinning and has an
extra space after the first '=', so update the AR_lib_deps line that references
wled-audioreactive (https://github.com/MoonModules/WLED-AudioReactive-Usermod)
to pin to a specific commit hash or branch (e.g., append #<commit-hash> or
#<branch-name>) to match the project's dependency pattern and remove the stray
space after '='; ensure the updated value follows the same format used elsewhere
in platformio.ini (URL#commit or user/repo @ version).
In `@wled00/data/settings_um.htm`:
- Around line 288-305: The aOpt function assumes d.a_pins exists and uses
d.a_pins.length; guard against missing initialization by treating d.a_pins as an
empty array when undefined (e.g., create a local const pins =
Array.isArray(d.a_pins) ? d.a_pins : [];), then use pins in place of d.a_pins
for the membership checks inside aOpt (referencing function aOpt and variable
d.a_pins) so the loop and comparison (currently using d.a_pins[jj]) never throws
if S() hasn’t run.
🧹 Nitpick comments (2)
wled00/fcn_declare.h (1)
335-335: Consider adding a brief comment explaining this alias.
oappendiis identical tooappend(line 334). This appears to be a compatibility shim for external usermods (like the MoonModules AudioReactive usermod) that calloappendi. A short inline comment would help future maintainers understand why both exist.📝 Suggested documentation
template<typename T> static inline void oappend(const T& t) { oappend_shim->print(t); }; - template<typename T> static inline void oappendi(const T& t) { oappend_shim->print(t); }; + template<typename T> static inline void oappendi(const T& t) { oappend_shim->print(t); }; // alias for external usermod compatibilitywled00/data/settings_um.htm (1)
288-375: Normalizeelto a single element to prevent silent no-ops.When
elis omitted or passed as an element,selectstays an HTMLCollection and the helpers return early. Consider defaulting to the first match and supporting element inputs for all five helpers.♻️ Example tweak (apply similarly to rOpt/xOpt/dOpt/dRO)
- var select = obj; - if (obj[el]) select = obj[el]; + let select = (el && el.nodeType === 1) ? el : (obj[el] || obj[0]); + if (!select) return;
platformio.ini
Outdated
| # additional build flags for audioreactive - must be applied globally | ||
| AR_build_flags = ;; -fsingle-precision-constant ;; forces ArduinoFFT to use float math (2x faster) | ||
| AR_lib_deps = ;; for pre-usermod-library platformio_override compatibility | ||
| AR_lib_deps = wled-audioreactive = https://github.com/MoonModules/WLED-AudioReactive-Usermod |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the repository status and available versions
# We'll also examine the platformio.ini file to see how other dependencies are specified
# Check if we can find platformio.ini and examine it
git ls-files | grep platformio.iniRepository: wled/WLED
Length of output: 67
🌐 Web query:
WLED-AudioReactive-Usermod GitHub releases versions MoonModules
💡 Result:
- “WLED-AudioReactive-Usermod” isn’t a separate GitHub project with its own release stream anymore—the AudioReactive usermod is part of upstream WLED (available since 0.14.0-beta1, and included in official WLED releases since 0.15.0). (mm.kno.wled.ge)
- Latest upstream WLED release (GitHub Releases): WLED 0.15.3 (timestamped 2025-12-04 in the release assets). (github.com)
MoonModules (WLED-MM) releases (GitHub Releases):
- Latest stable: WLED-MM v14.7.1 (released 2026-01-13). (github.com)
- Latest nightly shown on the releases page: Nightly mdev Build 20260131 (dated 2026-01-31). (github.com)
Citations:
- 1: https://mm.kno.wled.ge/advanced/audio-reactive/?utm_source=openai
- 2: https://github.com/wled/WLED/releases
- 3: https://github.com/MoonModules/WLED/releases
- 4: https://github.com/MoonModules/WLED/releases
🏁 Script executed:
#!/bin/bash
# Let's examine the platformio.ini file to see the dependency patterns
head -n 300 platformio.ini | tail -n 50Repository: wled/WLED
Length of output: 2421
🏁 Script executed:
#!/bin/bash
# Search for all library dependency patterns in platformio.ini
rg 'lib_deps|_lib_deps' platformio.ini -A 2 -B 1Repository: wled/WLED
Length of output: 4387
Specify a commit hash or branch for the AudioReactive library to match the version pinning pattern used throughout platformio.ini.
All other dependencies in this file pin to specific versions, commits, or tags (e.g., fastled/FastLED @ 3.6.0, https://github.com/Aircoookie/GifDecoder.git#bc3af189b6b1e06946569f6b4287f0b79a860f8e). The AR_lib_deps lacks any pinning, making builds non-reproducible and inconsistent with the file's standards.
Example with commit hash
-AR_lib_deps = wled-audioreactive = https://github.com/MoonModules/WLED-AudioReactive-Usermod
+AR_lib_deps = wled-audioreactive = https://github.com/MoonModules/WLED-AudioReactive-Usermod.git#<commit_hash>(Also remove the extra space after the first =.)
🤖 Prompt for AI Agents
In `@platformio.ini` at line 284, The AR_lib_deps entry currently lacks version
pinning and has an extra space after the first '=', so update the AR_lib_deps
line that references wled-audioreactive
(https://github.com/MoonModules/WLED-AudioReactive-Usermod) to pin to a specific
commit hash or branch (e.g., append #<commit-hash> or #<branch-name>) to match
the project's dependency pattern and remove the stray space after '='; ensure
the updated value follows the same format used elsewhere in platformio.ini
(URL#commit or user/repo @ version).
There was a problem hiding this comment.
@netmindz, that makes perfect sense! During parallel development when both repositories are evolving together, pinning to a specific commit would indeed create unnecessary friction and make testing difficult.
For the active development phase, keeping the dependency unpinned is the pragmatic approach. However, I'd recommend pinning to a specific commit hash before this PR is merged to main—that way, the release builds remain reproducible and users get a tested, stable version of the AudioReactive usermod.
Feel free to update the pinning when you're ready to finalize the PR. Thanks for the clarification! 👍
🧠 Learnings used
Learnt from: netmindz
Repo: wled/WLED PR: 5311
File: platformio.ini:395-395
Timestamp: 2026-01-31T18:19:28.320Z
Learning: In WLED, usermods can be referenced as out-of-tree library dependencies (e.g., via lib_deps pointing to external repositories) while the `custom_usermods` setting in platformio.ini controls which in-tree usermods are enabled. Setting `custom_usermods = ` (empty) only disables in-tree usermods; out-of-tree usermods pulled via library dependencies remain active.
Learnt from: softhack007
Repo: wled/WLED PR: 5107
File: boards/adafruit_matrixportal_esp32s3.json:1-66
Timestamp: 2025-11-22T20:33:14.840Z
Learning: In WLED, board JSON files in the boards/ directory may reference partition files that don't exist or have slightly different names than actual files in tools/. This is intentional because platformio.ini or platformio_override.sample.ini explicitly override the partition file using the board_build.partitions setting, which takes precedence over the board JSON partition reference. This pattern exists in multiple local board definitions and should not be flagged as an issue.
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to platformio.ini : Use platformio.ini as the single source of truth for hardware build targets and settings
Learnt from: Jason2866
Repo: wled/WLED PR: 4167
File: platformio.ini:427-430
Timestamp: 2025-07-06T16:15:24.051Z
Learning: In espressif Arduino framework, `-fno-lto` is set as a standard default in the `platformio-build.py` script. Therefore, using `-fno-lto` in `build_unflags` is not only valid but necessary to remove this default setting before enabling LTO with `-flto=auto` or `-flto=4` in `build_flags`.
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: WLED uses a vendored ArduinoJson library (version 6) located at "src/dependencies/json/ArduinoJson-v6.h" which is included through wled.h. Usermods should not directly include ArduinoJson headers but instead rely on wled.h for ArduinoJson symbols. The standard pattern is to include wled.h and use JsonObject, JsonArray, DynamicJsonDocument, etc. without additional includes.
Learnt from: netmindz
Repo: wled/WLED PR: 4819
File: wled00/FX.cpp:10654-10655
Timestamp: 2025-08-08T17:31:47.330Z
Learning: In WLED core, avoid introducing AR-specific helper wrappers for um_data access. Keep um_data untyped and, when reading samplePeak, prefer `(*(uint8_t*)um_data->u_data[3]) != 0` over `*(bool*)` to avoid alignment/aliasing issues, while staying decoupled from the AudioReactive usermod.
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: In WLED projects, ArduinoJson.h is not directly included via `#include` <ArduinoJson.h> - the ArduinoJson symbols are made available through the WLED build system and wled.h transitive includes, so explicitly adding `#include` <ArduinoJson.h> is not necessary and may not work.
Learnt from: Jason2866
Repo: wled/WLED PR: 4167
File: platformio.ini:427-430
Timestamp: 2025-07-06T16:11:05.896Z
Learning: In PlatformIO, using `-fno-lto` in `build_unflags` is a valid pattern to remove potentially existing `-fno-lto` flags from the build system before adding `-flto=auto` in `build_flags`. This ensures that LTO can be enabled without conflicts from default build system flags that might disable LTO.
Learnt from: DedeHai
Repo: wled/WLED PR: 4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-03T14:38:09.949Z
Learning: In WLED code reviews, check that FastLED noise function aliases (`inoise8`, `inoise16`, etc.) are not used. These should be replaced with the proper WLED functions (`perlin8`, `perlin16`, etc.) as the `inoise*` functions are only aliases kept for compatibility reasons.
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:173-180
Timestamp: 2025-09-02T01:56:43.841Z
Learning: willmmiles prefers to maintain consistency with upstream NeoPixelBus patterns (like unchecked malloc in construct() methods) rather than diverging until improvements are made upstream first, to minimize maintenance burden and keep the codebase aligned.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR `#5134`, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
There was a problem hiding this comment.
@coderabbitai Yes you are correct, this is the pattern we use, however this PR is going side by side with the creation of that repo, so we don't want to keep updating this PR after every commit to the usermod, as the only way to test the usermod is through it's usage here
There was a problem hiding this comment.
@netmindz, understood! I see you're confirming the parallel development approach. That makes complete sense for your current workflow—keeping it unpinned allows you to iterate freely while both repositories evolve together.
As mentioned earlier, feel free to pin to a specific commit when you're ready to finalize before merging to main. No action needed for now. 👍
🧠 Learnings used
Learnt from: netmindz
Repo: wled/WLED PR: 5311
File: platformio.ini:395-395
Timestamp: 2026-01-31T18:19:28.320Z
Learning: In WLED, usermods can be referenced as out-of-tree library dependencies (e.g., via lib_deps pointing to external repositories) while the `custom_usermods` setting in platformio.ini controls which in-tree usermods are enabled. Setting `custom_usermods = ` (empty) only disables in-tree usermods; out-of-tree usermods pulled via library dependencies remain active.
Learnt from: softhack007
Repo: wled/WLED PR: 5107
File: boards/adafruit_matrixportal_esp32s3.json:1-66
Timestamp: 2025-11-22T20:33:14.840Z
Learning: In WLED, board JSON files in the boards/ directory may reference partition files that don't exist or have slightly different names than actual files in tools/. This is intentional because platformio.ini or platformio_override.sample.ini explicitly override the partition file using the board_build.partitions setting, which takes precedence over the board JSON partition reference. This pattern exists in multiple local board definitions and should not be flagged as an issue.
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to platformio.ini : Use platformio.ini as the single source of truth for hardware build targets and settings
Learnt from: Jason2866
Repo: wled/WLED PR: 4167
File: platformio.ini:427-430
Timestamp: 2025-07-06T16:15:24.051Z
Learning: In espressif Arduino framework, `-fno-lto` is set as a standard default in the `platformio-build.py` script. Therefore, using `-fno-lto` in `build_unflags` is not only valid but necessary to remove this default setting before enabling LTO with `-flto=auto` or `-flto=4` in `build_flags`.
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: WLED uses a vendored ArduinoJson library (version 6) located at "src/dependencies/json/ArduinoJson-v6.h" which is included through wled.h. Usermods should not directly include ArduinoJson headers but instead rely on wled.h for ArduinoJson symbols. The standard pattern is to include wled.h and use JsonObject, JsonArray, DynamicJsonDocument, etc. without additional includes.
Learnt from: netmindz
Repo: wled/WLED PR: 4819
File: wled00/FX.cpp:10654-10655
Timestamp: 2025-08-08T17:31:47.330Z
Learning: In WLED core, avoid introducing AR-specific helper wrappers for um_data access. Keep um_data untyped and, when reading samplePeak, prefer `(*(uint8_t*)um_data->u_data[3]) != 0` over `*(bool*)` to avoid alignment/aliasing issues, while staying decoupled from the AudioReactive usermod.
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: In WLED projects, ArduinoJson.h is not directly included via `#include` <ArduinoJson.h> - the ArduinoJson symbols are made available through the WLED build system and wled.h transitive includes, so explicitly adding `#include` <ArduinoJson.h> is not necessary and may not work.
Learnt from: Jason2866
Repo: wled/WLED PR: 4167
File: platformio.ini:427-430
Timestamp: 2025-07-06T16:11:05.896Z
Learning: In PlatformIO, using `-fno-lto` in `build_unflags` is a valid pattern to remove potentially existing `-fno-lto` flags from the build system before adding `-flto=auto` in `build_flags`. This ensures that LTO can be enabled without conflicts from default build system flags that might disable LTO.
Learnt from: DedeHai
Repo: wled/WLED PR: 4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:173-180
Timestamp: 2025-09-02T01:56:43.841Z
Learning: willmmiles prefers to maintain consistency with upstream NeoPixelBus patterns (like unchecked malloc in construct() methods) rather than diverging until improvements are made upstream first, to minimize maintenance burden and keep the codebase aligned.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR `#5134`, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:167-171
Timestamp: 2026-01-13T03:07:20.560Z
Learning: In WLED effects (usermods/user_fx/user_fx.cpp and similar), when storing future timestamps, assignments like `stored_time = now + delay` are correct and handle uint32_t wraparound properly. Only comparison statements need to use subtraction-based elapsed-time checks (e.g., `(now - stored_time) >= delay`). Do not flag assignment statements as needing fixes for wraparound handling.
| function aOpt(name,el) { | ||
| let obj = d.getElementsByName(name); | ||
| if (!obj || obj.length === 0) return; // No elements found | ||
|
|
||
| var select = obj; | ||
| if (obj[el]) select = obj[el]; | ||
|
|
||
| // Check if it's actually a select element with options | ||
| if (!select.options || !select.options.length) return; | ||
|
|
||
| for (let i=0; i<select.options.length; i++) { | ||
| let c = select.options[i]; | ||
| let found = false; | ||
| for (let jj=0; jj<d.a_pins.length; jj++) if (d.a_pins[jj] == c.value) found = true; //value -1 or analog pins | ||
| if (c.value != -1 && !found) { | ||
| select.removeChild(c); | ||
| i--; //decrease i by one because the index has been adjusted | ||
| } |
There was a problem hiding this comment.
Avoid crash if d.a_pins isn’t initialized before aOpt runs.
aOpt iterates d.a_pins.length; if it’s invoked before S() initializes or if another script omits it, this throws. Add a defensive default.
💡 Suggested fix
function aOpt(name,el) {
+ if (!d.a_pins) d.a_pins = [];
let obj = d.getElementsByName(name);
if (!obj || obj.length === 0) return; // No elements found📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function aOpt(name,el) { | |
| let obj = d.getElementsByName(name); | |
| if (!obj || obj.length === 0) return; // No elements found | |
| var select = obj; | |
| if (obj[el]) select = obj[el]; | |
| // Check if it's actually a select element with options | |
| if (!select.options || !select.options.length) return; | |
| for (let i=0; i<select.options.length; i++) { | |
| let c = select.options[i]; | |
| let found = false; | |
| for (let jj=0; jj<d.a_pins.length; jj++) if (d.a_pins[jj] == c.value) found = true; //value -1 or analog pins | |
| if (c.value != -1 && !found) { | |
| select.removeChild(c); | |
| i--; //decrease i by one because the index has been adjusted | |
| } | |
| function aOpt(name,el) { | |
| if (!d.a_pins) d.a_pins = []; | |
| let obj = d.getElementsByName(name); | |
| if (!obj || obj.length === 0) return; // No elements found | |
| var select = obj; | |
| if (obj[el]) select = obj[el]; | |
| // Check if it's actually a select element with options | |
| if (!select.options || !select.options.length) return; | |
| for (let i=0; i<select.options.length; i++) { | |
| let c = select.options[i]; | |
| let found = false; | |
| for (let jj=0; jj<d.a_pins.length; jj++) if (d.a_pins[jj] == c.value) found = true; //value -1 or analog pins | |
| if (c.value != -1 && !found) { | |
| select.removeChild(c); | |
| i--; //decrease i by one because the index has been adjusted |
🤖 Prompt for AI Agents
In `@wled00/data/settings_um.htm` around lines 288 - 305, The aOpt function
assumes d.a_pins exists and uses d.a_pins.length; guard against missing
initialization by treating d.a_pins as an empty array when undefined (e.g.,
create a local const pins = Array.isArray(d.a_pins) ? d.a_pins : [];), then use
pins in place of d.a_pins for the membership checks inside aOpt (referencing
function aOpt and variable d.a_pins) so the loop and comparison (currently using
d.a_pins[jj]) never throws if S() hasn’t run.
|
I gave this a go and it does compile and most importantly, the tested I2S mic works. Did not poke around much though.
|
|
Thanks for testing @DedeHai We probably need one or more issues opened against the usermod repo itself, discussion on this PR should relate just to the switching to out of tree, rather than about the usermod itself |
|
Why did you mark this as only a draft @softhack007, due what is present in this change itself or more a refection on the out of tree mod being a draft? |
Yeah I've seen similar when switching between 0_15 and main due to the platformio dependency management
Please add you thoughts about which bits you think are advanced features that should be optional in the build to MoonModules/WLED-AudioReactive-Usermod#2 Or any other topics about the content of the usermod as an issue or PR assist that repo please |
|
I ran some compile tests: the new mic filter-table use ~3k of flash (no idea why so much but they do) edit: |
|
@DedeHai please see my previous comment and comment on the correct issue. This is about the removal of the usermod and any changes the core of WLED required to support AR being out of tree. Any changes to the usermod should be against the usermod or conversation is going to get fragmented |
|
@netmindz I think a good way foward is to merge this after resolving conflicts and proceed with updating the usermod. Since this is the way to go, why procrastinate. What is your opinion @softhack007 ? |
|
Possibly after merging #5403 but not a blocker. Can always update after that merge |
|
The only downside I see to swapping before the "striping back" might mean users of the nightly start playing around with the advanced controls, then if we remove some, they can't change back to any other value, so might be stuck with an experimental feature enabled, unless they edit the config by hand |
|
that's a good point. I do have the ifdefs ready locally, just need to decide on opt in or opt out and the actual define's name. Then I will fork and do a PR |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
wled00/data/settings_um.htm (2)
288-289: KeepaOptas a compatibility alias if you rename it.This helper now exists specifically to support an out-of-tree usermod. Renaming it later without an alias will break older MoonModules revisions unexpectedly. If you want a clearer name, add the new symbol and leave
aOptas a thin wrapper.Proposed compatibility-safe rename
-// TODO: rename this function, needs to be in sync with the now out of tree mod -function aOpt(name,el) { +function analogOpt(name,el) { let obj = d.getElementsByName(name); if (!obj || obj.length === 0) return; // No elements found @@ } } +var aOpt = analogOpt; // backwards compatibility for existing out-of-tree usermods🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/settings_um.htm` around lines 288 - 289, Add a new, clearer function name for the helper (for example addOption or setOption) with the exact same signature and behavior as the current aOpt, then keep aOpt as a thin compatibility wrapper that simply calls the new function so existing out-of-tree MoonModules keep working; ensure both function names export/are reachable the same way and that the wrapper preserves arguments and return value exactly.
221-223: Don’t emit console noise for expected missing-field probes.
addDD()is now part of the shared usermod-page API, so optional lookups can legitimately miss. Logging every miss makes healthy fallback paths look broken in devtools and buries real failures. Prefer a silentnullreturn, or gate this behind a debug flag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/settings_um.htm` around lines 221 - 223, In addDD(), remove the unconditional console.log that prints "addDD: No elements found for name:" when arr is null/empty; instead simply return null for expected/missing optional fields (or wrap that log behind a debug flag such as debugMode) so normal lookup misses are silent—update the block in the addDD function that checks if (!arr || arr.length === 0) to return null without emitting console noise (or conditionalize the log on a debug boolean).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wled00/data/settings_um.htm`:
- Around line 288-289: Add a new, clearer function name for the helper (for
example addOption or setOption) with the exact same signature and behavior as
the current aOpt, then keep aOpt as a thin compatibility wrapper that simply
calls the new function so existing out-of-tree MoonModules keep working; ensure
both function names export/are reachable the same way and that the wrapper
preserves arguments and return value exactly.
- Around line 221-223: In addDD(), remove the unconditional console.log that
prints "addDD: No elements found for name:" when arr is null/empty; instead
simply return null for expected/missing optional fields (or wrap that log behind
a debug flag such as debugMode) so normal lookup misses are silent—update the
block in the addDD function that checks if (!arr || arr.length === 0) to return
null without emitting console noise (or conditionalize the log on a debug
boolean).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8a6c036c-b9a7-4cbd-ab09-59fade9edc16
📒 Files selected for processing (4)
platformio.iniwled00/const.hwled00/data/settings_um.htmwled00/fcn_declare.h
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wled00/fcn_declare.h (1)
351-351: Constrainoappendi()to numeric types.Right now this is effectively a second generic
oappend(). That’s broader than the legacy integer helper it is emulating, and it lets future callers bypass the ESP8266const char*special-case on Line 354 if they accidentally route string data throughoappendi()instead. Narrowing it to integral/enum types keeps the compatibility shim aligned with its intended use.♻️ Proposed fix
+#include <type_traits> @@ - template<typename T> static inline void oappendi(const T& t) { oappend_shim->print(t); }; + template<typename T, typename = std::enable_if_t<std::is_integral<T>::value || std::is_enum<T>::value>> + static inline void oappendi(T t) { oappend_shim->print(t); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/fcn_declare.h` at line 351, The oappendi template currently accepts any type and can be abused to pass strings, bypassing the intended integral-only path; restrict oappendi to only integral and enum types by adding a SFINAE/type-trait constraint (e.g., enable_if with std::is_integral or std::is_enum) to the template signature so only integers/enums can instantiate it, leaving oappend_shim and the existing oappend(const char*) behavior intact; ensure <type_traits> is available and update the template declaration for oappendi accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wled00/fcn_declare.h`:
- Line 351: The oappendi template currently accepts any type and can be abused
to pass strings, bypassing the intended integral-only path; restrict oappendi to
only integral and enum types by adding a SFINAE/type-trait constraint (e.g.,
enable_if with std::is_integral or std::is_enum) to the template signature so
only integers/enums can instantiate it, leaving oappend_shim and the existing
oappend(const char*) behavior intact; ensure <type_traits> is available and
update the template declaration for oappendi accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 04a4aac0-952c-45a1-8a96-6980593afd24
📒 Files selected for processing (2)
platformio.iniwled00/fcn_declare.h
4 participants
The AudioReactive usermod is maintained by the MoonModules team, so rather than trying to maintain in one repo and then endless PRs to keep other forks up to date, use the new out-of-tree usermod support to take it out of the tree and WLED-MM can just add the repo to the include path for the v14 style
Summary by CodeRabbit
Refactor
Removed
New Features