Swap to using external repo for AudioReactive#331
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:
WalkthroughAdds a pre-build script to patch wled-audioreactive/library.json to exclude sources, expands and propagates Audioreactive build flags and lib_deps in platformio.ini, removes local audioreactive header/README and switches the include, and moves mapf implementation into a header. ChangesAudioreactive integration
Sequence Diagram(s)sequenceDiagram
participant P as PlatformIO
participant S as patch_audioreactive.py
participant FS as Filesystem
participant L as wled-audioreactive/library.json
participant B as Build
P->>S: run pre-build script
S->>FS: resolve path using PROJECT_LIBDEPS_DIR and PIOENV
alt library.json exists & readable
S->>L: read file
S->>S: validate/repair JSON if malformed
S->>L: ensure `build.srcFilter` and set to ["-<*>"]
S->>L: write updated file
S-->>P: print success
else missing or error
S-->>P: print warning / no-op
end
P->>B: continue build with updated deps/includes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🤖 Fix all issues with AI agents
In `@pio-scripts/patch_audioreactive.py`:
- Around line 21-31: The code currently only sets
library_config["build"]["srcFilter"] when the key is missing; instead, check the
existing value and enforce ["-<*>"] whenever it differs: ensure library_config
has a "build" dict (create if missing), read current =
library_config["build"].get("srcFilter"), and if current != ["-<*>"] then set
library_config["build"]["srcFilter"] = ["-<*>"], write back to library_json_path
with json.dump, and print the success message; reference the symbols
library_config, "build", "srcFilter", and library_json_path to locate the
change.
| # Add srcFilter to exclude all source files from library compilation | ||
| if "build" not in library_config: | ||
| library_config["build"] = {} | ||
|
|
||
| if "srcFilter" not in library_config["build"]: | ||
| library_config["build"]["srcFilter"] = ["-<*>"] | ||
|
|
||
| with open(library_json_path, 'w') as f: | ||
| json.dump(library_config, f, indent=2) | ||
|
|
||
| print("✓ Patched wled-audioreactive library.json to exclude .cpp from compilation") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n pio-scripts/patch_audioreactive.py | head -40Repository: MoonModules/WLED-MM
Length of output: 1866
Enforce srcFilter even when it already exists
The patch only applies when srcFilter is missing. If the upstream library is updated with a different srcFilter value that still includes sources, the patch won't override it and builds will unintentionally compile those .cpp files. Update the logic to compare the current value with the desired one and enforce ["-<*>"] whenever it differs.
🛠️ Suggested fix
- # Add srcFilter to exclude all source files from library compilation
- if "build" not in library_config:
- library_config["build"] = {}
-
- if "srcFilter" not in library_config["build"]:
- library_config["build"]["srcFilter"] = ["-<*>"]
-
- with open(library_json_path, 'w') as f:
- json.dump(library_config, f, indent=2)
-
- print("✓ Patched wled-audioreactive library.json to exclude .cpp from compilation")
+ # Add srcFilter to exclude all source files from library compilation
+ build_cfg = library_config.get("build")
+ if not isinstance(build_cfg, dict):
+ build_cfg = {}
+ library_config["build"] = build_cfg
+
+ desired_filter = ["-<*>"]
+ if build_cfg.get("srcFilter") != desired_filter:
+ build_cfg["srcFilter"] = desired_filter
+
+ with open(library_json_path, 'w') as f:
+ json.dump(library_config, f, indent=2)
+
+ print("✓ Patched wled-audioreactive library.json to exclude .cpp from compilation")🤖 Prompt for AI Agents
In `@pio-scripts/patch_audioreactive.py` around lines 21 - 31, The code currently
only sets library_config["build"]["srcFilter"] when the key is missing; instead,
check the existing value and enforce ["-<*>"] whenever it differs: ensure
library_config has a "build" dict (create if missing), read current =
library_config["build"].get("srcFilter"), and if current != ["-<*>"] then set
library_config["build"]["srcFilter"] = ["-<*>"], write back to library_json_path
with json.dump, and print the success message; reference the symbols
library_config, "build", "srcFilter", and library_json_path to locate the
change.
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)
platformio.ini (1)
197-197:⚠️ Potential issue | 🟠 MajorFix invalid PlatformIO dependency specification syntax.
The specification
https://github.com/softhack007/arduinoFFT.git#develop @ 1.9.2violates PlatformIO format. You cannot mix#(VCS reference) and@(Registry version) together. PlatformIO uses@only for registry packages and#only for git references—never both.For a Git URL, use one of:
https://github.com/softhack007/arduinoFFT.git#develop(pin to develop branch)https://github.com/softhack007/arduinoFFT.git#v1.9.2(pin to version tag if it exists)
🤖 Fix all issues with AI agents
In `@platformio.ini`:
- Line 231: The pre-build script pre:pio-scripts/patch_audioreactive.py
currently swallows failures and merely prints a warning; change it to verify the
patch succeeded and fail the build on error by either (a) validating that
library.json contains the expected modifications after applying the patch (e.g.,
checking for the expected key/value or version) and exiting non-zero when
validation fails, or (b) rethrowing the caught exception / calling sys.exit(1)
from the main entry point instead of only logging a warning so the PlatformIO
build is aborted; update the script’s exception handlers to perform this
verification and ensure any file-not-found, JSON parse, or write errors become
fatal.
| pre:pio-scripts/build_ui.py | ||
| pre:pio-scripts/conditional_usb_mode.py | ||
| pre:pio-scripts/set_repo.py | ||
| pre:pio-scripts/patch_audioreactive.py |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for the patch script and verify its existence
find . -name "patch_audioreactive.py" -o -name "*patch*" -type f | head -20Repository: MoonModules/WLED-MM
Length of output: 99
🏁 Script executed:
# Check the pio-scripts directory structure
ls -la pio-scripts/ 2>/dev/null || echo "pio-scripts directory check"
git ls-files | grep -E "pio-scripts|patch" | head -20Repository: MoonModules/WLED-MM
Length of output: 1426
🏁 Script executed:
# Examine the platformio.ini file around line 231 to see the context
sed -n '225,235p' platformio.iniRepository: MoonModules/WLED-MM
Length of output: 524
🏁 Script executed:
cat pio-scripts/patch_audioreactive.pyRepository: MoonModules/WLED-MM
Length of output: 1607
Script exists and has error handling, but failures are non-critical.
The patch_audioreactive.py script is properly registered and implemented with exception handling. However, the error handling is permissive—if the patch fails, the script only prints a warning and allows the build to continue. This means if the library.json patch is not applied due to an error (file not found, JSON parse failure, etc.), the build will proceed unpatched, potentially causing compilation errors downstream that are harder to trace back to the patch failure.
Consider adding explicit verification that the patch was successfully applied, or make patch failures fatal to the build if the patched library is required for compilation.
🤖 Prompt for AI Agents
In `@platformio.ini` at line 231, The pre-build script
pre:pio-scripts/patch_audioreactive.py currently swallows failures and merely
prints a warning; change it to verify the patch succeeded and fail the build on
error by either (a) validating that library.json contains the expected
modifications after applying the patch (e.g., checking for the expected
key/value or version) and exiting non-zero when validation fails, or (b)
rethrowing the caught exception / calling sys.exit(1) from the main entry point
instead of only logging a warning so the PlatformIO build is aborted; update the
script’s exception handlers to perform this verification and ensure any
file-not-found, JSON parse, or write errors become fatal.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@platformio.ini`:
- Around line 1189-1198: The AR_lib_deps entry for arduinoFFT is an invalid
mixed-format spec; update the AR_lib_deps line referencing arduinoFFT (currently
"https://github.com/softhack007/arduinoFFT.git#develop @ 1.9.2") to a valid
VCS-only spec by removing the "@ 1.9.2" suffix (or alternatively pin a specific
git tag/commit after the "#" instead of using "@version"); ensure AR_lib_deps
contains a clean VCS URL for arduinoFFT so PlatformIO can resolve it.
- Around line 1612-1614: The esp8266 AR-enabled env is pulling
${common_mm.AR_lib_deps} but missing the matching build flags; update the
esp8266_2MB_S environment so its build_flags include ${common_mm.AR_build_flags}
(in the same place where build_flags = ${common.build_flags_esp8266} is
declared) so the -D USERMOD_AUDIOREACTIVE define and include paths from
common_mm.AR_build_flags are applied; ensure the change mirrors how
esp8266pro_16MB_S and ESP32 AR environments include both
${common_mm.AR_lib_deps} and ${common_mm.AR_build_flags}.
| lib_deps = ${esp8266.lib_deps} | ||
| ${common_mm.AR_lib_deps} | ||
| build_flags = ${common.build_flags_esp8266} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the esp8266_2MB_S environment section
rg -n "esp8266_2MB_S" platformio.ini -A 10 -B 2Repository: MoonModules/WLED-MM
Length of output: 1430
🏁 Script executed:
# Find where common_mm.AR_lib_deps and common_mm.AR_build_flags are defined
rg -n "AR_lib_deps|AR_build_flags" platformio.ini -B 2 -A 2Repository: MoonModules/WLED-MM
Length of output: 5568
🏁 Script executed:
# Check the structure of the file to understand the context better
wc -l platformio.iniRepository: MoonModules/WLED-MM
Length of output: 82
🏁 Script executed:
# Check for comments around the 2MB, 4MB, and 16MB environments
sed -n '1608,1625p' platformio.ini
sed -n '1638,1650p' platformio.ini
sed -n '1736,1745p' platformio.iniRepository: MoonModules/WLED-MM
Length of output: 1839
🏁 Script executed:
# Verify that common_mm.AR_build_flags is properly defined
sed -n '1189,1200p' platformio.iniRepository: MoonModules/WLED-MM
Length of output: 872
Add AR_build_flags to esp8266_2MB_S build configuration.
The environment pulls ${common_mm.AR_lib_deps} but omits ${common_mm.AR_build_flags}, which includes the -D USERMOD_AUDIOREACTIVE define and required include paths. Without these flags, the AudioReactive library will fail to compile. This is inconsistent with esp8266pro_16MB_S and all ESP32 AR-enabled environments, which include both.
✅ Suggested fix:
build_flags = ${common.build_flags_esp8266}
+ ${common_mm.AR_build_flags}
-D WLED_RELEASE_NAME=esp8266_2MB_S🤖 Prompt for AI Agents
In `@platformio.ini` around lines 1612 - 1614, The esp8266 AR-enabled env is
pulling ${common_mm.AR_lib_deps} but missing the matching build flags; update
the esp8266_2MB_S environment so its build_flags include
${common_mm.AR_build_flags} (in the same place where build_flags =
${common.build_flags_esp8266} is declared) so the -D USERMOD_AUDIOREACTIVE
define and include paths from common_mm.AR_build_flags are applied; ensure the
change mirrors how esp8266pro_16MB_S and ESP32 AR environments include both
${common_mm.AR_lib_deps} and ${common_mm.AR_build_flags}.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platformio.ini (1)
1636-1642:⚠️ Potential issue | 🟡 MinorAdd AR_build_flags to esp8266_4MB_S build configuration.
The environment pulls
${common_mm.AR_lib_deps}(lines 1640-1641) but omits${common_mm.AR_build_flags}from build_flags (line 1642). This mirrors the issue inesp8266_2MB_Sand contrasts withesp8266pro_16MB_S(line 1741), which correctly includes both. Without these flags, the AudioReactive library will fail to compile due to missing defines and include paths.✅ Suggested fix:
build_flags = ${common.build_flags_esp8266} + ${common_mm.AR_build_flags} -D WLED_RELEASE_NAME=esp8266_4MB_S
| ;; WLEDMM audioreactive usermod, licensed under EUPL-1.2 | ||
| ;; NOTE: External repo needs library.json updated with "srcFilter": ["-<*>"] to prevent .cpp compilation | ||
| AR_lib_deps = | ||
| https://github.com/netmindz/WLED-AudioReactive-Usermod.git#compat-MM ;; MM version 14 |
There was a problem hiding this comment.
This should be swapped to MoonModules once we are good to merge the compat-MM PR
|
I've just done a basic test and usermod can be enabled and the settings all look as I would expect |
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)
platformio.ini (1)
1776-1800:⚠️ Potential issue | 🟠 MajorAdd AR_build_flags to esp8266pro_16MB_M build configuration.
The environment includes
${common_mm.AR_lib_deps}(line 1800) but is missing${common_mm.AR_build_flags}from the build_flags section. Without these flags, the AudioReactive library will not compile correctly because the-D USERMOD_AUDIOREACTIVEdefine and include paths are missing.Compare with
esp8266pro_16MB_S(lines 1739-1742) which correctly includes both.✅ Suggested fix:
build_flags = ${common.build_flags_esp8266} + ${common_mm.AR_build_flags} -D WLED_RELEASE_NAME=esp8266pro_16MB_M
🤖 Fix all issues with AI agents
In `@platformio.ini`:
- Around line 1641-1643: The esp8266_4MB_S environment imports
${common_mm.AR_lib_deps} but misses ${common_mm.AR_build_flags}, so
AudioReactive-specific defines and include paths (notably -D
USERMOD_AUDIOREACTIVE) are not applied and compilation fails; update the
esp8266_4MB_S env to add ${common_mm.AR_build_flags} alongside
${common.build_flags_esp8266} (mirror the fix used in esp8266_2MB_S) so the AR
build flags are passed into build_flags for that environment.
🧹 Nitpick comments (1)
platformio.ini (1)
1199-1199: Consider pinning AudioReactive to a specific commit hash instead of a branch.The dependency uses
#compat-MMwhich points to a moving branch reference. If the branch is updated, builds will pull different code without explicit version changes in this file, potentially causing unexpected breakage or behavior changes.Consider using a commit hash (e.g.,
#39fee0aebc4f7581f23723acc710ea9cacc199d1) instead of a branch name for reproducible builds. You can update the hash when you explicitly want to pull in new changes.📌 Suggested change to pin to commit hash
AR_lib_deps = - https://github.com/netmindz/WLED-AudioReactive-Usermod.git#compat-MM ;; MM version 14 + https://github.com/netmindz/WLED-AudioReactive-Usermod.git#<commit-hash> ;; MM version 14 - replace <commit-hash> with actual SHA https://github.com/softhack007/arduinoFFT.git#develop @ 1.9.2 ;; used for USERMOD_AUDIOREACTIVE - optimized version, 10% faster on -S2/-C3
| lib_deps = ${esp8266.lib_deps} | ||
| ${common_mm.AR_lib_deps} | ||
| build_flags = ${common.build_flags_esp8266} |
There was a problem hiding this comment.
Add AR_build_flags to esp8266_4MB_S build configuration.
The environment pulls ${common_mm.AR_lib_deps} but omits ${common_mm.AR_build_flags}, which includes the -D USERMOD_AUDIOREACTIVE define and required include paths. Without these flags, the AudioReactive library will fail to compile. This is the same issue flagged in esp8266_2MB_S and should be fixed consistently across all AudioReactive-enabled ESP8266 environments.
✅ Suggested fix:
+ ${common_mm.AR_build_flags}
-D WLED_RELEASE_NAME=esp8266_4MB_S🤖 Prompt for AI Agents
In `@platformio.ini` around lines 1641 - 1643, The esp8266_4MB_S environment
imports ${common_mm.AR_lib_deps} but misses ${common_mm.AR_build_flags}, so
AudioReactive-specific defines and include paths (notably -D
USERMOD_AUDIOREACTIVE) are not applied and compilation fails; update the
esp8266_4MB_S env to add ${common_mm.AR_build_flags} alongside
${common.build_flags_esp8266} (mirror the fix used in esp8266_2MB_S) so the AR
build flags are passed into build_flags for that environment.
|
Given the library is based on what was our "local" copy, I'm keen we merge this soon so we have a single source of truth for AR |
|
Before we merge this we just need to do a final diff between mdev and the out of tree version to ensure we haven't lost anything while we had these two different copies floating about |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platformio.ini (1)
1818-1823:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the AudioReactive build flags here too.
esp8266pro_16MB_Mstill inheritsUSERMOD_AUDIOREACTIVEfrom the shared 8266 flags, so pulling in${common_mm.AR_lib_deps}alone is not enough. Without${common_mm.AR_build_flags}, this env will miss the include path and FFT/pin-manager defines the external repo needs.Suggested fix
build_flags = ${common.build_flags_esp8266} + ${common_mm.AR_build_flags} -D WLED_RELEASE_NAME=esp8266pro_16MB_M🤖 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 `@platformio.ini` around lines 1818 - 1823, The esp8266pro_16MB_M environment is pulling in USERMOD_AUDIOREACTIVE via shared flags but only includes ${common_mm.AR_lib_deps}, so it lacks the required include paths and FFT/pin-manager defines; update the platformio.ini stanza that currently lists lib_deps (the block containing OneWire, U8g2, MPU6050 and ${common_mm.AR_lib_deps}) to also add ${common_mm.AR_build_flags} to the environment so the AudioReactive build flags (include paths and defines) are applied alongside the AR libraries.
🤖 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.
Outside diff comments:
In `@platformio.ini`:
- Around line 1818-1823: The esp8266pro_16MB_M environment is pulling in
USERMOD_AUDIOREACTIVE via shared flags but only includes
${common_mm.AR_lib_deps}, so it lacks the required include paths and
FFT/pin-manager defines; update the platformio.ini stanza that currently lists
lib_deps (the block containing OneWire, U8g2, MPU6050 and
${common_mm.AR_lib_deps}) to also add ${common_mm.AR_build_flags} to the
environment so the AudioReactive build flags (include paths and defines) are
applied alongside the AR libraries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 53d49fe5-4690-408b-a723-d99a64d7f271
📒 Files selected for processing (3)
platformio.iniwled00/FX.cppwled00/fcn_declare.h
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/FX.cpp
2 participants
Summary by CodeRabbit
Refactor
Documentation
Bug Fix