Repository navigation
Conversation
…MA2D Two opt-outs for applications that are short of internal DMA-capable RAM (the SD card driver needs some of it for unaligned transfers): - CONFIG_M5STACK_TAB5_AUDIO_RECORDING (default y): when disabled, initialize_audio() creates the I2S TX channel alone, with no RX channel (whose DMA ring matches the 12 x 240-frame playback ring), no ES7210 setup and no microphone task (whose stack is in internal RAM). start_audio_recording() then fails. - CONFIG_M5STACK_TAB5_DPI_DMA2D (default y): when disabled, the DPI panel keeps CPU draw_bitmap() copies instead of enabling DMA2D. Rotated LVGL flushes go through the PPA straight into the frame buffer, so an application that always rotates never uses it. Both defaults keep the current behavior. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SFCMzomCZ5wRHJN4sxwkvY
|
✅Static analysis result - no issues found! ✅ |
finger563
left a comment
finger563
left a comment
There was a problem hiding this comment.
Reviewed the diff against main (70d1474). Both opt-outs keep the default paths byte-for-byte, and every recording-dependent call is behind kAudioRecordingEnabled (I checked the whole of audio.cpp on the branch, not just the hunks: audio_sample_rate(uint32_t) only touches TX, stop_audio_recording() is harmless without a task, the microphone callback can only run from the task that is not created). Using constexpr bool + plain if rather than #if around the bodies keeps the disabled code compiling, which is the right call. Nothing blocking; small points:
- The DMA2D saving is quoted as two different numbers. The Kconfig help and the README say "about 5 KB"; the test table in the description says "~3 KB". Worth settling on the measured one in all three places.
microphone_volume(float)doc. The description says it only stores the value when recording is off, but the header comment onmicrophone_volume()was not updated (onlyinitialize_audio()andstart_audio_recording()got a\note). One line there would complete the picture.- Startup log with recording off still reads "Initializing dual audio system (ES8388 + ES7210)"; the later "Recording disabled" line corrects it, so purely cosmetic.
- Heads-up on an upcoming merge conflict, not on this PR: the new README section is inserted at the same spot (just before "## Example") as the section the tear-free-flush PR I am preparing adds. Whichever lands second needs a one-line conflict resolution.
- Aside, pre-existing and out of scope:
audio_sample_rate(uint32_t)reconfigures only the TX clock while the RX channel (when enabled) shares BCLK/WS and stays enabled through the change. Not something this PR touches, just noting it while I was in there.
🤖 Generated with Claude Code
There was a problem hiding this comment.
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
4 open issues
CONFIG_M5STACK_TAB5_AUDIO_RECORDINGis disabled, ES7210 is intentionally not initialized,… · New#if CONFIG_M5STACK_TAB5_DPI_DMA2Dis typically more robust than… · New
What changed in this PR
Adds Kconfig switches to let Tab5 applications opt out of (1) audio recording setup and (2) DPI DMA2D, reducing internal DMA-capable RAM usage while keeping defaults unchanged.
Changes:
- Add
CONFIG_M5STACK_TAB5_AUDIO_RECORDINGto optionally skip ES7210 + I2S RX + microphone task setup. - Add
CONFIG_M5STACK_TAB5_DPI_DMA2Dto optionally avoid DMA2D usage for DPI paneldraw_bitmap()copies. - Document the RAM-saving options in the component README.
| File | Description |
|---|---|
| components/m5stack-tab5/src/video.cpp | Gate pre-IDF6 DPI use_dma2d config and log DMA2D-disabled behavior. |
| components/m5stack-tab5/src/audio.cpp | Add build-time opt-out for recording setup; guard RX channel, ES7210 init, mic task, and recording API. |
| components/m5stack-tab5/include/m5stack-tab5.hpp | Document behavior changes when audio recording is disabled. |
| components/m5stack-tab5/README.md | Add “Saving internal DMA-capable RAM” section describing the new options. |
| components/m5stack-tab5/Kconfig | Introduce the two new Kconfig options and their help text. |
Review effort: Lite
…tions Replace the two Kconfig options with parameters, following the DisplayConfig / initialize_display(config) pattern: - initialize_audio(const AudioConfig &): sample_rate, playback_enabled, recording_enabled and task_config. Each direction is set up only when enabled (its I2S channel and DMA ring, its codec and its task); alone, either channel runs as a simplex master. initialize_audio(sample_rate, task_config) keeps its signature and enables both. - initialize_lcd(const LcdConfig &): dma2d_enabled. initialize_lcd() keeps the current behavior. play_audio(), volume() and mute() do nothing for the ES8388 without playback, start_audio_recording() fails without recording, and the sample rate setter reconfigures whichever channel drives the clocks. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SFCMzomCZ5wRHJN4sxwkvY
finger563
changed the title
- audio_sample_rate(rate) also disables the RX channel while it reconfigures the shared clock in full duplex - volume(), mute() and microphone_volume() note that they only store the value when their direction is not set up - quote the measured DMA2D saving (about 3 KB) everywhere Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SFCMzomCZ5wRHJN4sxwkvY
|
Thanks for the review. Going through the points against the current head (abe3da8):
Since your review, 1929957 replaced the Kconfig options with runtime parameters: 🤖 Generated with Claude Code |
There was a problem hiding this comment.
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
7 open issues
playbackis disabled… · Newconfig.dma2d_enabledis wired intodpi_cfg.flags.use_dma2donly under `#if ESP_IDF_VERSION <… · Newconfig.dma2d_enabledis wired intodpi_cfg.flags.use_dma2donly under `#if ESP_IDF_VERSION <… · Newconfig.dma2d_enabledis wired intodpi_cfg.flags.use_dma2donly under `#if ESP_IDF_VERSION <… · Newconfig.dma2d_enabledis wired intodpi_cfg.flags.use_dma2donly under `#if ESP_IDF_VERSION <… · New
4 resolved since last review
CONFIG_M5STACK_TAB5_AUDIO_RECORDINGis disabled, ES7210 is intentionally not initialized,…#if CONFIG_M5STACK_TAB5_DPI_DMA2Dis typically more robust than…
Review effort: Lite
| if (!es8388_i2c_device_) { | ||
| logger_.error("Could not initialize ES8388 I2C device: {}", ec.message()); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Agreed it would be better, but this predates the PR. On main, initialize_audio() also returns false after creating and enabling both channels and the stream buffer, and its I2S calls are ESP_ERROR_CHECKs that abort rather than return. This PR keeps the same failure semantics and only makes each step conditional. A proper unwind (disable and delete the channels, delete the stream, stop the tasks, release the I2C devices, reset the codec state) touches every step, so it belongs in its own PR. Leaving this open as a follow-up.
| if (es7210_adc_init(&es7210_cfg) != ESP_OK) { | ||
| logger_.error("ES7210 init failed"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Same as the other thread on this function: pre-existing failure semantics, left for a separate cleanup PR.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SFCMzomCZ5wRHJN4sxwkvY
Description
Lets applications skip parts of the m5stack-tab5 BSP they don't use, to save internal DMA-capable RAM. Both are runtime parameters that follow the existing
DisplayConfig/initialize_display(config)pattern. The existing calls keep their behavior.initialize_audio(const AudioConfig &)takessample_rate,playback_enabled,recording_enabledandtask_config. Each direction is set up only when it's enabled: its I2S channel and DMA ring, its codec (ES8388 / ES7210) and its task.i2s_new_channel()call (needed for full duplex) and share itsdma_desc_num/dma_frame_num.play_audio()queues nothing, andvolume()/mute()just store their value.start_audio_recording()fails.audio_sample_rate(rate)reconfigures whichever channel drives the clocks.initialize_audio(sample_rate, task_config)keeps its signature and enables both directions.initialize_lcd(const LcdConfig &)takesdma2d_enabled. When it's false, the panel keeps CPUdraw_bitmap()copies instead of callingesp_lcd_dpi_panel_enable_dma2d()(or settingflags.use_dma2dbefore IDF 6.0).initialize_lcd()is unchanged.The README has a new "Saving internal DMA-capable RAM" section.
Motivation and Context
In 1.4.0 the I2S DMA ring doubled to 12 x 240 frames, for RX as well as TX, and DMA2D is now enabled on the ST71xx panels. Together that cut free internal DMA-capable RAM roughly in half in tab5-emu. The ESP-IDF SD card driver needs some of that RAM as a bounce buffer for unaligned transfers. With the reduced headroom, tab5-emu's TPM save states failed with
sdmmc_cmd: allocate_dma_buf: not enough mem. tab5-emu never records and always runs in landscape.How has this been tested?
initialize_lcd({.dma2d_enabled = false})andinitialize_audio({.sample_rate = 48000, .recording_enabled = false})ran the same 8-session soak on the Tab5. It was clean, with the same memory as the first run (83.1 KB internal and 43.7 KB DMA-capable free after GUI init), stable across launches, and no audio drops. The m5stack-tab5 example builds against it with the defaults. 74df8e1 changes only two log lines.initialize_audio()initialize_lcd()(DMA2D)Types of changes
Checklist:
🤖 Generated with Claude Code
https://claude.ai/code/session_01SFCMzomCZ5wRHJN4sxwkvY