MoonModules Audio driver node#178
Open
netmindz wants to merge 2 commits into
Open
Conversation
Specifies D_MoonModulesAudio.h — a local I2S audio capture driver that runs the WLEDMM audio pipeline (AudioFilters/AGC/FFT) and writes to sharedData.bands/volume/majorPeak/magnitude, as an alternative to the UDP-based WLEDAudioDriver. Covers: library integration (OO-split branch), WLED compatibility shim, build system changes (FT_MOONLIGHT_AUDIO / S3+P4 only), all 11 mic types, pin management, sharedData mapping, and node registration.
|
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)
WalkthroughAdds a new implementation spec and a gated-in driver header for FT_MOONLIGHT_AUDIO that define build wiring, WLED compatibility shims, node controls and pin management, mic-type→AudioSource mapping, audio processing pipeline/FFT task, runtime loop/teardown, and integration constraints. ChangesMoonModules Audio Driver Specification & Header
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 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. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@misc/specs/D_MoonModulesAudio.md`:
- Around line 142-147: The fenced code block containing the lines with name(),
dim(), tags(), and category() lacks a language identifier; update the markdown
fence to include a language tag (for example "text") so the block reads ```text
... ``` to satisfy markdownlint MD040 and keep the docs-lint clean.
- Around line 201-211: The documentation mismatches the implementation: either
include an explicit check for the I2C readiness flag or remove the claim that
"first checks that I2CReady is true." Update the snippet around moduleIO->read
and initialize() to explicitly read the I2CReady flag from ModuleState (e.g.,
state.data["I2CReady"].as<bool>() or similar) and guard the path by returning
with updateControl("status", "Error: I2C not configured") if I2CReady is false,
or alternatively remove the phrase mentioning I2CReady so the text matches the
current sda/scl negative-value 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: 95a391dd-41c8-436d-a83a-39036b90b154
📒 Files selected for processing (1)
misc/specs/D_MoonModulesAudio.md
Comment on lines
+142
to
+147
| ``` | ||
| name() → "MoonModules Audio" | ||
| dim() → _NoD | ||
| tags() → "☸️♫" | ||
| category() → "Driver" | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to this fenced code block.
This fence currently has no language tag (markdownlint MD040). Use a language (e.g., text) to keep docs-lint clean.
Proposed fix
-```
+```text
name() → "MoonModules Audio"
dim() → _NoD
tags() → "☸️♫"
category() → "Driver"
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 142-142: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@misc/specs/D_MoonModulesAudio.md` around lines 142 - 147, The fenced code
block containing the lines with name(), dim(), tags(), and category() lacks a
language identifier; update the markdown fence to include a language tag (for
example "text") so the block reads ```text ... ``` to satisfy markdownlint MD040
and keep the docs-lint clean.
Source: Linters/SAST tools
Comment on lines
+201
to
+211
| **I2C codec pins:** For mic types that need I2C (ES7243=2, ES8388=6, WM8978=7, AC101=8, ES8311=9), the driver reads the global I2C SDA/SCL from ModuleIO state before calling `initialize()`. It first checks that `I2CReady` is true (same pattern as `D_IMU.h`): | ||
| ```cpp | ||
| moduleIO->read([&](ModuleState& state) { | ||
| i2c_sda = (int8_t)state.data["sdaPin"].as<int>(); | ||
| i2c_scl = (int8_t)state.data["sclPin"].as<int>(); | ||
| }, name()); | ||
| if (i2c_sda < 0 || i2c_scl < 0) { | ||
| updateControl("status", "Error: I2C not configured"); | ||
| return; | ||
| } | ||
| ``` |
There was a problem hiding this comment.
I2C readiness contract is inconsistent in this section.
Line 201 says initialization “first checks that I2CReady is true,” but the provided snippet only reads sdaPin/sclPin and checks for negative values. Please either add the explicit I2CReady check to the documented flow/snippet or remove the claim, otherwise implementers may ship a mismatched guard path.
🤖 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 `@misc/specs/D_MoonModulesAudio.md` around lines 201 - 211, The documentation
mismatches the implementation: either include an explicit check for the I2C
readiness flag or remove the claim that "first checks that I2CReady is true."
Update the snippet around moduleIO->read and initialize() to explicitly read the
I2CReady flag from ModuleState (e.g., state.data["I2CReady"].as<bool>() or
similar) and guard the path by returning with updateControl("status", "Error:
I2C not configured") if I2CReady is false, or alternatively remove the phrase
mentioning I2CReady so the text matches the current sda/scl negative-value
check.
… switch Replace the inline switch(micType) in startService() with an AudioSourceConfig fill + createAudioSource() call from audio_source_factory.h. The manual PDM auto-detect block is replaced by normalizeDmType(). All 10 mic types continue to work identically; the factory handles the dmType-10 alias for legacy PDM internally.
Collaborator
Author
|
Currently crashing due to IS2 legacy issue, need to dig out the IDF V5 fix for that |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a new Node Driver that allows for using MoonModules's Audio Reactive library
Summary by CodeRabbit
New Features
Documentation