embedded_esp32s3: add pluggable audio sources and internal test tone#37
embedded_esp32s3: add pluggable audio sources and internal test tone#37ovelhaaa wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe pull request adds audio source type configuration to the parameter bridge system, allowing runtime audio source switching. Changes include extending Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
🚥 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 docstrings
🧪 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 resolve merge conflicts |
|
✅ Merge conflicts resolved successfully! Resolved 1 conflict file(s). Commit: 3 file operation(s)
View agent analysis |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a flexible audio source system for the ESP32-S3 target, enabling runtime switching between external I2S input and an internal triangle wave generator. Key changes include the addition of I2sInputSource and InternalTestTriangleSource classes, and updates to the audio callback and parameter bridge to support source selection. Review feedback identifies a configuration issue where disabling RX prevents external audio input and suggests optimizing the triangle wave generator by pre-calculating the phase increment to avoid per-sample floating-point division.
| AudioEngineEsp32::Config audioCfg; | ||
| audioCfg.sampleRate = board::audio::kSampleRate; | ||
| audioCfg.enableRx = true; | ||
| audioCfg.enableRx = false; |
There was a problem hiding this comment.
Setting enableRx to false prevents the I2S driver from being initialized with receive capabilities. This contradicts the PR's stated goal of allowing runtime switching to an external I2S source without reconfiguration. If enableRx is false, the I2sInputSource will receive a null pointer and produce silence when selected. Consider enabling RX by default to support seamless switching.
| audioCfg.enableRx = false; | |
| audioCfg.enableRx = true; |
| outL = sample; | ||
| outR = sample; | ||
|
|
||
| phase_ += frequencyHz_ / sampleRateHz_; |
There was a problem hiding this comment.
Performing a floating-point division (frequencyHz_ / sampleRateHz_) inside the renderFrame method is inefficient as it executes for every audio sample. It is recommended to pre-calculate this value (e.g., as a phaseIncrement_ member) whenever frequencyHz_ or sampleRateHz_ is updated via their respective setters.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4f18c35ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| AudioEngineEsp32::Config audioCfg; | ||
| audioCfg.sampleRate = board::audio::kSampleRate; | ||
| audioCfg.enableRx = true; | ||
| audioCfg.enableRx = false; |
There was a problem hiding this comment.
Setting audioCfg.enableRx = false disables I2S capture for the lifetime of the audio engine, so when sourceType is switched to ExternalI2s the callback still receives inInterleaved == nullptr (see AudioEngineEsp32::audioTaskLoop), and I2sInputSource therefore outputs silence. This means the new runtime source-selection path cannot actually produce external input unless the engine is reinitialized, which this change does not do.
Useful? React with 👍 / 👎.
Resolved conflicts in: - targets/embedded_esp32s3/main/app_main.cpp (content) Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
![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)
targets/embedded_esp32s3/main/app_main.cpp (1)
138-143:⚠️ Potential issue | 🟡 Minor
uiTickoverwritessourceTypeto default on every publish.The local
AudioParams pis default-initialized withsourceType = InternalTest. SinceuiTickpublishes partial updates without preserving the currentsourceType, any runtime source change would be reverted on the next UI tick.If
uiTickis only meant to cycle visual/demo parameters, consider preservingsourceType(and other "stable" params) from the current state, or refactor to publish incremental updates.Suggested approach: preserve current sourceType
void uiTick(void* userData) { auto* app = static_cast<AppContext*>(userData); static uint32_t tick = 0; ++tick; AudioParams p; + // Preserve current source type + p.sourceType = app->sourceType; p.mix = 0.30f + 0.15f * ((tick / 120) % 2); p.feedback = 0.40f; p.toneHz = 6500.0f; p.smearAmount = 0.20f; app->params.publish(p); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@targets/embedded_esp32s3/main/app_main.cpp` around lines 138 - 143, uiTick is creating a fresh AudioParams p which default-initializes sourceType (InternalTest) and then app->params.publish(p), unintentionally overwriting any runtime source selection; fix by first reading the current params from the params store (e.g., via app->params.get()/read()/value() or whatever accessor exists), copy over stable fields like sourceType (and any other non-UI fields) into the local AudioParams p before mutating mix/feedback/toneHz/smearAmount, then call app->params.publish(p) so only the intended UI/demo values are updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@targets/embedded_esp32s3/main/app_main.cpp`:
- Line 183: The runtime crash occurs because audioCfg.enableRx is false so the
audio engine passes nullptr as inInterleaved and
I2sInputSource::prepare(nullptr) stores a null pointer; fix by either enabling
RX at init or adding a guard to prevent switching to ExternalI2s when RX was
disabled: set audioCfg.enableRx = true in the initialization to always provide
valid input buffers, or in updateActiveSource()/audioCallback check
audioCfg.enableRx (or the saved rx-enabled flag) and ignore/rollback any request
to set AudioSourceType::ExternalI2s; ensure I2sInputSource::prepare never stores
a nullptr (add a null-check) so renderFrame() cannot dereference it.
---
Outside diff comments:
In `@targets/embedded_esp32s3/main/app_main.cpp`:
- Around line 138-143: uiTick is creating a fresh AudioParams p which
default-initializes sourceType (InternalTest) and then app->params.publish(p),
unintentionally overwriting any runtime source selection; fix by first reading
the current params from the params store (e.g., via
app->params.get()/read()/value() or whatever accessor exists), copy over stable
fields like sourceType (and any other non-UI fields) into the local AudioParams
p before mutating mix/feedback/toneHz/smearAmount, then call
app->params.publish(p) so only the intended UI/demo values are updated.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e42ff3b-4557-4f7e-8a4e-8865b176a523
📒 Files selected for processing (3)
targets/embedded_esp32s3/main/app_main.cpptargets/embedded_esp32s3/main/parameter_bridge.cpptargets/embedded_esp32s3/main/parameter_bridge.h
| AudioEngineEsp32::Config audioCfg; | ||
| audioCfg.sampleRate = board::audio::kSampleRate; | ||
| audioCfg.enableRx = true; | ||
| audioCfg.enableRx = false; |
There was a problem hiding this comment.
Runtime switch to ExternalI2s will fail when RX is disabled at init.
With enableRx = false, the audio engine passes nullptr as inInterleaved to the callback (per audio_engine_esp32.cpp:168). If a user switches to AudioSourceType::ExternalI2s via the parameter bridge at runtime, I2sInputSource::prepare(nullptr) stores a null pointer, and subsequent renderFrame() calls will dereference it—causing a crash or undefined behavior.
Options to resolve:
- Prevent runtime switching to ExternalI2s when RX is disabled – add a guard in
updateActiveSource()oraudioCallbackthat ignores the source change request if RX was not enabled at init. - Enable RX unconditionally – set
enableRx = trueso I2S input is always available, even if unused. - Document the constraint – if internal-only is the intended permanent mode, remove or stub out the ExternalI2s path entirely.
Option 1: Guard against invalid source switch
+// Store RX capability at init
+struct AppContext {
+ ...
+ bool rxEnabled = false;
+ ...
+};
// In app_main, after audioCfg setup:
+app.rxEnabled = audioCfg.enableRx;
// In audioCallback or updateActiveSource:
void updateActiveSource(AppContext* app) {
if (app->activeSourceType == app->sourceType) {
return;
}
+ // Prevent switch to ExternalI2s if RX not available
+ if (app->sourceType == AudioSourceType::ExternalI2s && !app->rxEnabled) {
+ return;
+ }
app->activeSourceType = app->sourceType;
...
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@targets/embedded_esp32s3/main/app_main.cpp` at line 183, The runtime crash
occurs because audioCfg.enableRx is false so the audio engine passes nullptr as
inInterleaved and I2sInputSource::prepare(nullptr) stores a null pointer; fix by
either enabling RX at init or adding a guard to prevent switching to ExternalI2s
when RX was disabled: set audioCfg.enableRx = true in the initialization to
always provide valid input buffers, or in updateActiveSource()/audioCallback
check audioCfg.enableRx (or the saved rx-enabled flag) and ignore/rollback any
request to set AudioSourceType::ExternalI2s; ensure I2sInputSource::prepare
never stores a nullptr (add a null-check) so renderFrame() cannot dereference
it.
1 participant
Motivation
Description
audio_source.handaudio_source.cppimplementingI2sInputSource,InternalTestTriangleSource, and theAudioSourceTypeenum.ParameterBridgeto carry asourceTypesetting via a new atomicsourceType_and to snapshotAudioParams::sourceTypeinpublish/consumeIfUpdated.app_main.cppto use a templatedprocessWithSourceandupdateActiveSource, switched the audio callback to prepare and render from the selected source, and initialized the internal test source parameters.CMakeLists.txtto include the newmain/audio_source.cppand setAudioEngineEsp32::Config.enableRxtofalsewhen using the internal source by default.Testing
embedded_esp32s3target withidf.py build, which completed successfully.cmake --build/ninjabuild of the target tree, which completed successfully.Codex Task
Summary by CodeRabbit
New Features
Configuration Changes