[finger563]

Description

Follow-up to the cleanup-on-failure review comments on #878.

Before this change, a failure part way through initialize_audio() returned false and left everything set up so far in place:

• the I2S channels, still enabled and holding their DMA rings in internal DMA-capable RAM
• the stream buffer
• the tasks
• the I2C devices
• the started codecs

The non-null channel handles also let audio_sample_rate() operate on the half-initialized system. The I2S calls used ESP_ERROR_CHECK, so a failure there aborted the device instead of returning.

All failure paths in initialize_audio() now go through one cleanup lambda that undoes what was set up before the failure:

• stops the playback and microphone tasks; for playback it first detaches the feeder's task handle and afterwards resumes charging
• stops the ES7210 and stops and deinitializes the ES8388, if they were started
• releases the I2C device members. The codec register functions stay wired, because volume()/mute() reach the ES8388 even without audio.
• disables the I2S channels it enabled and deletes every channel that exists. If i2s_new_channel() registers TX and then fails on RX, it returns an error with TX still registered and its handle set, so that channel gets deleted too.
• deletes the stream buffer, frees the TX/RX buffers, and restores the direction flags to their defaults

After the cleanup the call can be retried. The I2S calls (i2s_new_channel, i2s_channel_init_std_mode, i2s_channel_enable) and xStreamBufferCreate are now checked, log an error and return false. The existing handling of the two task starts uses the same cleanup path. audio_tx_stream now defaults to nullptr.

There is no change when initialization succeeds.

Testing

On a Tab5 running the BSP example (ESP-IDF 6.1), I temporarily injected a failure after both tasks had started, so every cleanup step ran, and logged internal DMA-capable free memory:

Point	Free
Before initialize_audio()	247,875
After setup, before the injected failure	183,587
After cleanup	246,827
After the retry succeeded	183,583

• The cleanup returned 63,240 of the 64,288 bytes.
• The retry used the same amount as the first try, less about 1 KB, so the ~1 KB not returned is allocated only on the first try. It doesn't grow with retries.
• After the retry, the example ran normally: the audio loaded, the 44.1 kHz sample-rate change worked, and the camera started. There were no errors or aborts.

I removed the injection before committing. The final code builds cleanly in the example.

🤖 Generated with Claude Code

https://claude.ai/code/session_01SFCMzomCZ5wRHJN4sxwkvY

[github-actions]

✅Static analysis result - no issues found! ✅

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

1 open issue

• fail() unconditionally sets audio_playback_enabled_ / audio_recording_enabled_ to true,… · New

What changed in this PR

Ensures initialize_audio() on M5Stack Tab5 cleans up any partially-initialized audio resources on failure so callers can safely retry without leaked DMA/I2S/task resources.

Changes:

• Added a centralized failure/cleanup path to undo partial initialization (tasks, codecs, I2C devices, I2S channels, stream buffer, buffers).
• Replaced ESP_ERROR_CHECK abort-style handling with logged errors + graceful failure returns for I2S init/enable and stream buffer creation.
• Documented retry/cleanup behavior and default-initialized audio_tx_stream to nullptr.

File	Description
components/​m5stack-tab5/​src/​audio.cpp	Add unified cleanup-on-failure logic and convert hard-fail ESP_ERROR_CHECK paths to recoverable errors.
components/​m5stack-tab5/​include/​m5stack-tab5.hpp	Document failure cleanup/retry semantics and default audio_tx_stream to null.

_{Review effort: Lite}

[Copilot]

🟡 Changes recommended

A playback shutdown race can cause notification of a deleted FreeRTOS task, while failed codec initialization can leave partially configured hardware powered without cleanup.

1 open issue

• Detach playback task again after stop completes · New

1 previously missed issue

In code that hasn't changed since last review

Mark codec for cleanup before initialization

components/​m5stack-tab5/​src/​audio.cpp:304

es8388_init() accumulates I2C write errors but continues through the remaining writes, including powering up the DAC and ADC, before returning an error. In that case this flag remains false, so fail() skips both stopping and deinitializing the partially configured codec and leaves it powered. Mark it as requiring cleanup before calling es8388_init(), once its register-access callbacks are wired.

1 resolved since last review

• fail() unconditionally sets audio_playback_enabled_ / audio_recording_enabled_ to true,…

_{Review effort: Balanced}

[finger563]

Re the review's "previously missed issue" (mark the codec for cleanup before es8388_init()): also fixed in 4b29297. es8388_init() and es7210_adc_init() both make every register write, powering the codec up, even after one fails, so fail() now stops (and for the ES8388 deinitializes) a codec once its init has begun, rather than only after it succeeded.