Fix incomplete support for Audio Source - Network only - 8266 Audio Reactive #5514
Fix incomplete support for Audio Source - Network only - 8266 Audio Reactive #5514
Conversation
…network receive only" option.
…platformio.ini now that we support network only option for 8266
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughEnable the audioreactive usermod for several ESP8266 build environments and refine audioreactive input logic: introduce a named network-only dmType, make FFT-task disablement conditional on local audioSource, improve ESP32 init messaging for network-only mode, and add a UI option for "None - network receive only" (254). Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
added
external
|
sorry, tried to clean the code slightly before commit, clearly missed something, standby ... |
![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
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)
378-458:⚠️ Potential issue | 🟡 MinorAlign
audioreactiveenablement across all three ESP01 variantsThe
custom_usermods = audioreactiveline is present inenv:esp01_1m_full_160but absent fromenv:esp01_1m_fullandenv:esp01_1m_full_compat. Since the_160variant only changes CPU frequency and extends the base, all three should either include or exclude audioreactive consistently. If 1MB flash cannot fit audioreactive, remove it fromesp01_1m_full_160; if it can, add it to the other two.This change to
platformio.inirequires explicit maintainer approval per coding guidelines, as addingcustom_usermodsto the default ESP8266 release environments will increase firmware size for all 8266 release binaries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` around lines 378 - 458, The three ESP01 envs are inconsistent: add or remove the audioreactive usermod so env:esp01_1m_full, env:esp01_1m_full_compat and env:esp01_1m_full_160 are aligned; decide whether 1M flash can include audioreactive—if it cannot, remove custom_usermods = audioreactive from env:esp01_1m_full_160; if it can, add custom_usermods = audioreactive to env:esp01_1m_full and env:esp01_1m_full_compat; if you choose to add it to the base release envs, obtain explicit maintainer approval before committing because this affects firmware size for all ESP8266 release builds.
🧹 Nitpick comments (2)
usermods/audioreactive/audio_reactive.cpp (2)
1442-1475: Replace the bare254literal with a named constant.
254now appears as a sentinel in multiple places (case label at 1442, guard at 1459, branches at 1467/1473, and the UI dropdown at 2130). Introduce a named constant (e.g.constexpr uint8_t SR_DMTYPE_NETWORK_ONLY = 254;) and use it everywhere, including theaddOption(dd, 'None - network receive only', 254)call — this keeps the magic value and its meaning in sync if it ever changes.Minor secondary nit on the new branch: the "failed to initialize" case uses
DEBUG_PRINTLNunderWLED_DEBUGandDEBUGSR_PRINTLNotherwise, but the newelsebranch unconditionally usesDEBUG_PRINTLN— this will silently emit nothing on builds withoutWLED_DEBUG. Consider using the same#ifdef WLED_DEBUG/DEBUGSR_PRINTLNpattern, or just useDEBUG_PRINTLNconsistently in both branches.As per coding guidelines: "Avoid 'magic numbers' where meaning is repeated; if 254 represents a configured 'no local driver / network receive only' mode, consider naming it (e.g., constexpr) in the edited code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/audioreactive/audio_reactive.cpp` around lines 1442 - 1475, Replace the magic literal 254 with a named constant (e.g. constexpr uint8_t SR_DMTYPE_NETWORK_ONLY = 254) and use that constant everywhere the sentinel appears: the switch case label in audio_reactive.cpp (case 254), the guard check (dmType != 254), the branch conditions (dmType < 254 / dmType == 254) and the UI dropdown call addOption(..., 254). Also make the debug logging consistent in the new else branch by using the same `#ifdef` WLED_DEBUG / DEBUGSR_PRINTLN pattern used in the preceding failure branch so logs are emitted correctly on non-WLED_DEBUG builds.
1442-1456: Minor: misleading single-linedelete+ unconditionalnullptrassignment.if (audioSource) delete audioSource; audioSource = nullptr;Reads as if both statements are guarded, but only
deleteis; theaudioSource = nullptr;runs unconditionally. Behaviorally this is what you want (and safe sinceaudioSourceis null here anyway), but splitting onto two lines with braces avoids the trap for future readers and matches the intent. Same comment applies to line 1452.♻️ Proposed tweak
- case 254: // dummy "network receive only" mode - if (audioSource) delete audioSource; audioSource = nullptr; + case 254: // dummy "network receive only" mode + if (audioSource) { delete audioSource; audioSource = nullptr; } disableSoundProcessing = true; audioSyncEnabled = 2; // force udp sound receive mode enabled = true; break; case 255: // 255 = -1 = no audio source // falls through to default default: - if (audioSource) delete audioSource; audioSource = nullptr; + if (audioSource) { delete audioSource; audioSource = nullptr; } disableSoundProcessing = true; enabled = false; break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/audioreactive/audio_reactive.cpp` around lines 1442 - 1456, The one-line pattern "if (audioSource) delete audioSource; audioSource = nullptr;" is misleading because only the delete is guarded; update both occurrences in the switch (the case 254 branch and the default/-1 branch) to use a braced if and set the pointer to nullptr inside the guard (e.g. if (audioSource) { delete audioSource; audioSource = nullptr; }) so both delete and nulling are clearly tied to audioSource and future readers won't be confused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/audioreactive/audio_reactive.cpp`:
- Line 166: The premature "#endif // ARDUINO_ARCH_ESP32" closes the ESP32 guard
too early causing ESP8266 builds to see ESP32-only types from audio_source.h
(e.g., static AudioSource *audioSource, FFTsampleType/FFTmathType/SRate_t),
FFTcode(), and AudioReactive::setup() as undefined; fix by restoring the
original guard scope so audio_source.h and all dependent declarations (including
static AudioSource *audioSource, FFTcode(), FFT* type usages, and
AudioReactive::setup()) remain inside the ARDUINO_ARCH_ESP32 conditional, or
alternatively re-wrap each ESP32-only block that was exposed after the stray
"#endif" with its own `#ifdef` ARDUINO_ARCH_ESP32; also audit and correct the
`#ifdef/`#endif pairing to remove the lone "#endif" left around line 740 and
ensure every `#ifdef` has a matching `#endif`.
---
Outside diff comments:
In `@platformio.ini`:
- Around line 378-458: The three ESP01 envs are inconsistent: add or remove the
audioreactive usermod so env:esp01_1m_full, env:esp01_1m_full_compat and
env:esp01_1m_full_160 are aligned; decide whether 1M flash can include
audioreactive—if it cannot, remove custom_usermods = audioreactive from
env:esp01_1m_full_160; if it can, add custom_usermods = audioreactive to
env:esp01_1m_full and env:esp01_1m_full_compat; if you choose to add it to the
base release envs, obtain explicit maintainer approval before committing because
this affects firmware size for all ESP8266 release builds.
---
Nitpick comments:
In `@usermods/audioreactive/audio_reactive.cpp`:
- Around line 1442-1475: Replace the magic literal 254 with a named constant
(e.g. constexpr uint8_t SR_DMTYPE_NETWORK_ONLY = 254) and use that constant
everywhere the sentinel appears: the switch case label in audio_reactive.cpp
(case 254), the guard check (dmType != 254), the branch conditions (dmType < 254
/ dmType == 254) and the UI dropdown call addOption(..., 254). Also make the
debug logging consistent in the new else branch by using the same `#ifdef`
WLED_DEBUG / DEBUGSR_PRINTLN pattern used in the preceding failure branch so
logs are emitted correctly on non-WLED_DEBUG builds.
- Around line 1442-1456: The one-line pattern "if (audioSource) delete
audioSource; audioSource = nullptr;" is misleading because only the delete is
guarded; update both occurrences in the switch (the case 254 branch and the
default/-1 branch) to use a braced if and set the pointer to nullptr inside the
guard (e.g. if (audioSource) { delete audioSource; audioSource = nullptr; }) so
both delete and nulling are clearly tied to audioSource and future readers won't
be confused.
🪄 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: 021cfb0a-a518-49e4-ad48-08c206413b65
📒 Files selected for processing (2)
platformio.iniusermods/audioreactive/audio_reactive.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/audioreactive/audio_reactive.cpp`:
- Around line 1467-1475: The dmType check and debug printing are incorrect:
change the condition that treats network-only sentinel to use an explicit named
constant (e.g., add constexpr uint8_t SR_DMTYPE_NETWORK_ONLY = 254 near the
dmType defaults) and replace the faulty dmType < 254 with dmType ==
SR_DMTYPE_NETWORK_ONLY (or use comparisons against that constant where the code
currently uses 254 in case labels, dmType != 254, and addOption), and normalize
diagnostics so both branches use the same debug macro pattern (use
DEBUG_PRINTF/DEBUG_PRINTLN or the existing DEBUGSR fallback consistently instead
of mixing DEBUG_PRINTLN and DEBUGSR_PRINTLN) when checking audioSource/dmType to
ensure SR-only builds still emit the network-only message; update references to
dmType, audioSource, DEBUG_PRINTLN, DEBUGSR_PRINTLN, and addOption accordingly.
🪄 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: 572d17bd-c261-4d8c-ac5c-3093eb8a3116
📒 Files selected for processing (1)
usermods/audioreactive/audio_reactive.cpp
|
@copilot please apply these recommendations #5514 (comment) |
netmindz
changed the title
Agent-Logs-Url: https://github.com/wled/WLED/sessions/85b2a158-f6ba-45cf-88af-0d51b9202cab Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
Agent-Logs-Url: https://github.com/wled/WLED/sessions/85b2a158-f6ba-45cf-88af-0d51b9202cab Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
Applied in 8aa4bee (with the main logic fix in 012fbdd). I implemented the discussion_r3104832410 recommendations: introduced |
This pull request primarily enables the
audioreactiveusermod for several ESP8266 build environments and improves the robustness and user feedback of the audio reactive module, especially for network-only sound input configurations. It also adds a new option in the UI for network-only audio input and makes some minor code organization improvements.PlatformIO build configuration:
audioreactiveusermod by addingcustom_usermods = audioreactiveto multiple ESP8266 build environments inplatformio.ini, ensuring the audio reactive features are available in these builds. [1] [2] [3] [4]Audio reactive usermod enhancements:
audio_reactive.cppto distinguish between sound input driver initialization failures and intentional network-only (UDP Sound Sync) configurations. Now, if no audio input driver is configured (dmType 254), a specific debug message is shown, and sound processing is disabled accordingly.Code organization and minor fixes:
audio_reactive.cppfor ESP32 architectures. [1] [2]Summary by CodeRabbit
New Features
Bug Fixes
Chores