fix(audioreactive): skip i2s_set_clk() for PDM mode on IDF 5.x by aenertia · Pull Request #5807 · wled/WLED

aenertia · 2026-08-18T22:00:32Z

i2s_set_clk() after i2s_driver_install() kills PDM mode on ESP-IDF 5.x, producing all-zero samples from the microphone. The PDM clock is configured during driver install; calling i2s_set_clk() afterwards resets it to I2S standard mode.

Fix: guard i2s_set_clk() with if (!(_config.mode & I2S_MODE_PDM)) so PDM microphones (SPM1423, INMP441 in PDM mode) work correctly on IDF 5.x.

Also fixes PDM channel format: uses I2S_CHANNEL_FMT_ALL_LEFT/ALL_RIGHT instead of ONLY_LEFT/ONLY_RIGHT, which is required for PDM compatibility on IDF 5.x (ONLY_* variants produce silence in PDM mode).

Tested on M5StickC with SPM1423 PDM microphone.

Summary by CodeRabbit

Bug Fixes
- Improved PDM microphone channel configuration for left and right channel modes.
- Prevented unnecessary clock configuration during PDM audio initialization.
- Preserved error handling when clock setup fails for non-PDM audio modes.

i2s_set_clk() after i2s_driver_install() kills PDM mode on ESP-IDF 5.x, producing all-zero samples from the microphone. The PDM clock is configured during driver install; calling i2s_set_clk() afterwards resets it to I2S standard mode. Fix: guard i2s_set_clk() with if (!(_config.mode & I2S_MODE_PDM)) so PDM microphones (SPM1423, INMP441 in PDM mode) work correctly on IDF 5.x. Also fixes PDM channel format: uses I2S_CHANNEL_FMT_ALL_LEFT/ALL_RIGHT instead of ONLY_LEFT/ONLY_RIGHT, which is required for PDM compatibility on IDF 5.x (ONLY_* variants produce silence in PDM mode). Tested on M5StickC with SPM1423 PDM microphone.

coderabbitai · 2026-08-18T22:01:00Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e9bcaeea-097f-4fd1-9efe-8a4ea4b0ccc7

📥 Commits

Reviewing files that changed from the base of the PR and between 1021759 and da6a62c.

📒 Files selected for processing (1)

usermods/audioreactive/audio_source.h

🚧 Files skipped from review as they are similar to previous changes (1)

usermods/audioreactive/audio_source.h

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.

Walkthrough

PDM channel selection now uses side-specific ALL_RIGHT or ALL_LEFT formats. PDM initialization skips i2s_set_clk(), while non-PDM initialization retains clock setup and failure handling.

Changes

PDM I2S configuration

Layer / File(s)	Summary
PDM channel format configuration `usermods/audioreactive/audio_source.h`	PDM microphone selection now uses `I2S_CHANNEL_FMT_ALL_RIGHT` or `I2S_CHANNEL_FMT_ALL_LEFT`. The related text describes the selected format.
Conditional I2S clock setup `usermods/audioreactive/audio_source.h`	Non-PDM sources retain `i2s_set_clk()` and cleanup on failure. PDM sources skip the call.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to da6a6

The change is limited to PDM microphone clock and channel-format handling; no actionable merge-blocking risk remains beyond normal checks and review.

Possibly related PRs

wled/WLED#5357: Modifies overlapping I2S/PDM channel configuration in audio_source.h.
wled/WLED#5764: Modifies overlapping PDM channel configuration and clock initialization in audio_source.h.

Suggested labels: bug, usermod

Suggested reviewers: softhack007

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: skipping i2s_set_clk() for PDM mode on ESP-IDF 5.x.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

- pr/audioreactive-pdm-fix → wled#5807 - pr/chunked-json-fix → wled#5808 - pr/effects-fade-snap → wled#5809

softhack007 · 2026-08-19T16:53:04Z

@coderabbitai is this PR compatible with upcoming changes in #5764 and #5773 ?

coderabbitai · 2026-08-19T16:54:03Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="wled/WLED"

for pr in 5807 5764 5773; do
  echo "===== PR #${pr}: metadata ====="
  gh pr view "$pr" --repo "$repo" --json number,title,state,isDraft,baseRefName,headRefName,mergeable,mergeStateStatus,url,body \
    --jq '{number,title,state,isDraft,baseRefName,headRefName,mergeable,mergeStateStatus,url,body}'
  echo "===== PR #${pr}: changed files ====="
  gh pr view "$pr" --repo "$repo" --json files \
    --jq '.files[] | {path, additions, deletions}'
  echo "===== PR #${pr}: diff ====="
  gh pr diff "$pr" --repo "$repo" --patch
done

echo "===== Local source outline ====="
ast-grep outline usermods/audioreactive/audio_source.h --items all --view expanded

echo "===== Relevant local I2S configuration ====="
rg -n -C 8 'I2S_PDM_MIC_CHANNEL|i2s_set_clk\s*\(|I2S_MODE_PDM|I2S_CHANNEL_FMT_(ONLY|ALL)_(LEFT|RIGHT)' \
  usermods/audioreactive/audio_source.h

Length of output: 169114

@softhack007 No. PR #5807 is not directly merge-compatible with #5764.

Both PRs modify usermods/audioreactive/audio_source.h.
PR #5764 replaces the ESP-IDF 5.x legacy I2S driver with i2s_std.h and i2s_pdm.h.
Therefore, #5807’s i2s_set_clk() guard does not apply after #5764. The new I2S API does not use i2s_set_clk().
The PDM channel-format change also needs a separate review or port. #5764 uses i2s_pdm_rx_config_t, not I2S_CHANNEL_FMT_ALL_LEFT or I2S_CHANNEL_FMT_ALL_RIGHT.

PR #5807 remains relevant for the legacy IDF 4.x path in #5764. It must be manually rebased and resolved after #5764. The PDM microphone must then be tested on the new IDF 5.x driver path.

PR #5773 does not overlap with #5807. It adds the ADC manager and does not change the I2S or PDM code. It is compatible with #5807 in isolation.

_{You are interacting with an AI system.}

softhack007 · 2026-08-19T17:07:18Z

@coderabbitai your .coderabbit.yaml explicitly asks to check for deleted comments in AI generated source code. Why did you miss the two deleted comments in this PR?

coderabbitai · 2026-08-19T17:08:00Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '===== Configuration files ====='
fd -HI '^\.coderabbit\.(ya?ml)$|^AGENTS\.md$' . -d 3 -x sh -c 'echo "--- $1"; cat -n "$1"' sh {}

echo '===== PR `#5807` deleted lines and context ====='
git diff --find-renames --unified=12 origin/"$(git remote show origin | sed -n '/HEAD branch/s/.*: //p')"...HEAD -- usermods/audioreactive/audio_source.h \
  || true

echo '===== Relevant markers and comments in PR file ====='
rg -n -C 5 'AI:|I2S_PDM_MIC_CHANNEL|i2s_set_clk|//|/\*|\*/' usermods/audioreactive/audio_source.h

Length of output: 75534

@softhack007 You are correct. I missed a required review check.

.coderabbit.yaml Line 58 applies extra checks when source appears AI-generated. Line 65 requires a finding for deleted comments that are unrelated to the PR description.

PR #5807 deletes these two comments from the i2s_set_clk() error path:

// set bit clocks. Also takes care of MCLK routing if needed.
// uninstall already-installed driver

I focused on the functional PDM change and did not perform the required removed-line review as a separate step. That omission caused the missed finding. The deleted comments should have been reviewed and either retained, moved to the new guarded path, or justified in the PR description.

🐇‍⬛

🧠 Learnings used

Learnt from: CR
Repo: wled/WLED PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-26T17:47:14.996Z
Learning: Applies to wled00/**/*.{cpp,h} : AI-generated C++ source blocks must be marked with `// AI: below section was generated by an AI` and `// AI: end`.

_{You are interacting with an AI system.}

…dback

aenertia · 2026-08-19T21:50:10Z

Just some comments before I push fixup commit :

IDF 4.x compatibility:
Safe on all 4.x. i2s_driver_install() configures PDM clocks internally on every version -- the extra i2s_set_clk() is always redundant for PDM. On 4.4 it can actually crash if called with rate=0 (ESP-IDF #8850). Tested on IDF 5.x with SPM1423.

#5764 relationship:
This fixes the current legacy path. #5764 keeps that path for IDF 4.x, so the guard stays relevant. If the legacy path goes away entirely, the guard becomes dead code. PDM is broken on IDF 5.x today and #5764 is a bigger piece of work

aenertia added a commit to aenertia/WLED that referenced this pull request Aug 18, 2026

docs: record upstream PR submissions wled#5807, wled#5808, wled#5809

086e189

- pr/audioreactive-pdm-fix → wled#5807 - pr/chunked-json-fix → wled#5808 - pr/effects-fade-snap → wled#5809

softhack007 added the AI Partly generated by an AI. Make sure that the contributor fully understands the code! label Aug 19, 2026