update AR usermod with proper V5 support (new API) - #5764
Open
DedeHai wants to merge 3 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAudioReactive now supports ESP-IDF v5 I2S channel APIs and continuous ADC analog microphones. It preserves ESP-IDF v4 paths, uses capability-based analog-microphone guards, and centralizes audioreactive usermod selection across ESP32 environments. ChangesAudioReactive ESP-IDF 5 compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AudioReactive
participant AdcContinuousSource
participant ESPIDF_ADC
AudioReactive->>AdcContinuousSource: initialize analog microphone
AdcContinuousSource->>ESPIDF_ADC: configure continuous ADC
AudioReactive->>AdcContinuousSource: read microphone samples
AdcContinuousSource->>ESPIDF_ADC: acquire ADC frames
ESPIDF_ADC-->>AdcContinuousSource: ADC samples
AdcContinuousSource-->>AudioReactive: scaled FFT samples
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
usermods/audioreactive/audio_source.h (2)
526-530: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
%dformat specifier forsize_targuments.
bytes_wanted/bytes_readaresize_t; use%u/%zuto avoid mismatched varargs in debug builds.🤖 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/audioreactive/audio_source.h` around lines 526 - 530, Update the DEBUGSR_PRINTF call in the audio sample read check to use a format specifier compatible with the size_t variables bytes_wanted and bytes_read, preferably %zu for both arguments, while preserving the existing failure handling.
331-344: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
erris left at its previous value if the PDM#ifbody is compiled out.Practically unreachable (the unsupported case returns earlier at line 300), but the branch is fragile if the guards ever diverge; a
#else err = ESP_FAIL;makes the intent explicit.🤖 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/audioreactive/audio_source.h` around lines 331 - 344, Update the usePDM branch around i2s_channel_init_pdm_rx_mode so the SOC_I2S_SUPPORTS_PDM_RX-disabled path explicitly assigns ESP_FAIL to err, while preserving the existing initialization and _bytesPerSample behavior when PDM RX is supported.platformio.ini (1)
423-423: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTypo in comment: "audiorective" → "audioreactive".
✏️ Proposed fix
-custom_usermods = ${common.default_usermods} ;; ToDO: audiorective compiles, but not tested on C5 yet +custom_usermods = ${common.default_usermods} ;; ToDO: audioreactive compiles, but not tested on C5 yetAlso applies to: 478-478
🤖 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` at line 423, Correct the comment typo from “audiorective” to “audioreactive” in both occurrences of the custom_usermods configuration comments, including the additional occurrence noted by the review.
🤖 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/audioreactive/audio_source.h`:
- Around line 516-518: The sampling hot path uses runtime-sized stack VLAs for
rawBuf, risking FFT-task stack exhaustion. Replace both rawBuf declarations in
the relevant sampling method with a fixed BLOCK_SIZE-derived buffer or a member
buffer allocated once during initialize(), using d_malloc()/p_malloc() if
dynamic storage is required; preserve the existing sample-processing behavior
without VLAs or String allocations.
- Around line 899-906: Remove the duplicated return statement in the ADC driver
error branch, leaving a single return after cleanup in the initialization flow.
- Around line 941-962: In the incomplete-frame branch after the ADC restart
logic, replace the leftover Serial.print call with the function-style
DEBUGSR_PRINTLN debug macro and fix the indentation of that branch, preserving
the existing early return and old-sample behavior.
- Around line 1020-1024: Initialize _audioPin to an invalid-pin sentinel and
update deinitialize() to call PinManager::deallocatePin only when _audioPin
contains a valid allocated pin; preserve normal cleanup after successful
allocation and leave the sentinel unchanged when allocation fails.
---
Nitpick comments:
In `@platformio.ini`:
- Line 423: Correct the comment typo from “audiorective” to “audioreactive” in
both occurrences of the custom_usermods configuration comments, including the
additional occurrence noted by the review.
In `@usermods/audioreactive/audio_source.h`:
- Around line 526-530: Update the DEBUGSR_PRINTF call in the audio sample read
check to use a format specifier compatible with the size_t variables
bytes_wanted and bytes_read, preferably %zu for both arguments, while preserving
the existing failure handling.
- Around line 331-344: Update the usePDM branch around
i2s_channel_init_pdm_rx_mode so the SOC_I2S_SUPPORTS_PDM_RX-disabled path
explicitly assigns ESP_FAIL to err, while preserving the existing initialization
and _bytesPerSample behavior when PDM RX is supported.
🪄 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 Plus
Run ID: 6abff830-78e1-445b-812a-7ce6ceb66d96
📒 Files selected for processing (3)
platformio.iniusermods/audioreactive/audio_reactive.cppusermods/audioreactive/audio_source.h
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 support for all ESPs on V5.
I2S is updated to the new API, tested and working on ESP32 and C3
The I2S "ADC" interface is no longer supported, instead the normal "continuous sampling" ADC interface is used. As a bonus of using this, all ESP32 variants now support analog mics as the API is the same. ESP32 still uses I2S DMA sampling as before, just the interface changed.
Tested analog input on C3, ESP32 and C6 - fully working both with and without I2S_GRAB_ADC1_COMPLETELY.
There is one issue though: even if not using I2S_GRAB_ADC1_COMPLETELY, analogRead() wont work. In order to make it work, the continuous ADC would have to be deinitialized and re-initialized each loop. Doable but not a pretty solution - and not the one we should use. What would be much better is so actually use continuous sampling in the background and only stop that if something wants to use the ADC - i.e. pin manager or battery usermod (or some other usermod) which read at intervals of 250ms or even more. In order to do this, we need an ADC manager (or a general hardware manager) that handles it properly instead of having the AR usermod set up the ADC and having to free it just in case something else wants to use it.
edit:
this is an intermediate solution and this code should be migrated to the out-of-tree AR usermod in a next step.
edit2:
forgot to mention: I did not check if the V4 path still works or if there are any misplaced ifdefs for that
Summary by CodeRabbit