Skip to content

feat(bsp): add Waveshare ESP32-P4-WIFI6-Dev-Kit board support - #741

Open
finger563 wants to merge 8 commits into
mainfrom
feat/bsp-esp32-p4-wifi6-dev-kit
Open

feat(bsp): add Waveshare ESP32-P4-WIFI6-Dev-Kit board support#741
finger563 wants to merge 8 commits into
mainfrom
feat/bsp-esp32-p4-wifi6-dev-kit

Conversation

@finger563

Copy link
Copy Markdown
Contributor

Summary

New BSP component esp32-p4-wifi6-dev-kit (espp::Esp32P4Wifi6DevKit) for the Waveshare ESP32-P4-WIFI6-Dev-Kit (ESP32-P4NRW32 + onboard ESP32-C6FH8 WiFi 6), with example, docs, and CI wiring. Pins verified against the official schematic PDF.

Notable schematic finding: despite the name, this board also has 100M Ethernet + a reserved PoE-module header — it is pin-identical to the esp32-p4-nano for every wrapped peripheral. The BSP mirrors the nano:

  • MIPI-DSI display (JD9365 10.1" default / ILI9881C / EK79007 via Kconfig) + LVGL + polled GT911 touch, MIPI-CSI camera (esp_video/V4L2), ES8311 audio out + mic (with hardware headphone-detect PA gating on the board), 4-bit SDMMC microSD, IP101GRI Ethernet (espp::Ethernet, DHCP client/server).
  • WiFi via the onboard C6 (ESP-Hosted SDIO defaults, C6 CHIP_PU on GPIO54): documented (c6_sdio_* constants + esp_wifi_remote/ESP-Hosted enablement steps and the C6 reflash procedure in the READMEs) rather than wrapped, matching the other P4 BSPs. The C6 is 2.4 GHz-only — noted.
  • Example mirrors the nano's (Status/Audio/Camera LVGL Gui). Docs under doc/en/dev_boards/waveshare/, Doxyfile entries, build.yml matrix + upload_components.yml entries.

Needs hardware verification

Ethernet PHY address (strap resistors not legible on the schematic; copied addr 1 from the nano), DSI panel timings/backlight controller inherited from the nano, flash size taken as 16 MB (the wiki self-contradicts 16 vs 32 MB; product page says 16 MB).

Testing

  • idf.py set-target esp32p4 && idf.py build clean (1.49 MB app, 64% slot free).

🤖 Generated with Claude Code

…6-Dev-Kit

Adds a new espp BSP component for the Waveshare ESP32-P4-WIFI6-DEV-KIT
(ESP32-P4NRW32 + onboard ESP32-C6FH8 for Wi-Fi 6 / BT5 LE over SDIO via
ESP-Hosted), modeled on the pin-compatible esp32-p4-nano BSP:

- espp::Esp32P4Wifi6DevKit singleton exposing:
  - MIPI-DSI display (JD9365 10.1" default / ILI9881C / EK79007 via
    Kconfig) + GT911 touch (polled; INT/RST not routed)
  - MIPI-CSI camera (OV5647 by default) via esp_video / V4L2
  - ES8311 codec + NS4150B amp (speaker out, mic in, full duplex);
    headphone jack auto-mutes the speaker amp in hardware
  - 4-bit SDMMC uSD (CLK 43 / CMD 44 / D0-D3 39-42, LDO_VO4)
  - 10/100 Ethernet (internal EMAC + IP101GRI RMII PHY) via
    espp::Ethernet; RJ45 with optional external PoE-module header
  - documented ESP32-C6 SDIO wiring constants (CLK 18 / CMD 19 /
    D0-D3 14-17 / C6 reset 54, the ESP-Hosted P4 defaults)
- Example (mirrors the esp32-p4-nano example): LVGL GUI with Status /
  Audio / Camera tabs exercising display, touch, audio record/playback,
  camera feed, uSD, and Ethernet DHCP; README documents enabling Wi-Fi
  through esp_wifi_remote + esp_hosted (BSP intentionally does not wrap
  the radio)
- Docs: dev_boards/waveshare rst + example include, Doxyfile entries
- CI: build matrix entry (esp32p4) and component-registry upload entry

All pin assignments verified against the official board schematic
(ESP32-P4-WIFI6-DEV-KIT-datasheet.pdf) and the Waveshare wiki; the board
is pin-identical to the ESP32-P4-NANO for every peripheral this BSP
touches. Example builds clean against ESP-IDF (esp32p4).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 28, 2026 16:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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.

Adds a new BSP component for the Waveshare ESP32-P4-WIFI6-DEV-KIT, including documentation, an LVGL-based example, and CI/component-registry wiring.

Changes:

  • Introduces components/esp32-p4-wifi6-dev-kit BSP (display/touch/camera/audio/SD/Ethernet bring-up + headers/Kconfig/manifest).
  • Adds a full example project (partition table, sdkconfig defaults, GUI, embedded audio asset).
  • Wires documentation/Doxygen inputs and CI build/upload matrices for the new component.

Reviewed changes

Copilot reviewed 26 out of 27 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
doc/en/dev_boards/waveshare/index.rst Adds the new board page to the Waveshare docs index.
doc/en/dev_boards/waveshare/esp32_p4_wifi6_dev_kit_example.md Includes the example README into the docs build.
doc/en/dev_boards/waveshare/esp32_p4_wifi6_dev_kit.rst Adds the board documentation and API reference include.
doc/Doxyfile Adds the BSP header and example to Doxygen inputs/examples.
components/esp32-p4-wifi6-dev-kit/src/video.cpp Implements MIPI-DSI LCD init, LVGL display integration, brightness, flush/rotation.
components/esp32-p4-wifi6-dev-kit/src/touchpad.cpp Implements GT911 bring-up and polling/interrupt touch feeding.
components/esp32-p4-wifi6-dev-kit/src/sdcard.cpp Implements SDMMC FAT mount and space querying.
components/esp32-p4-wifi6-dev-kit/src/esp32-p4-wifi6-dev-kit.cpp Implements Ethernet init wrapper using espp::Ethernet.
components/esp32-p4-wifi6-dev-kit/src/camera.cpp Implements esp_video/V4L2 camera init, capture task, and teardown.
components/esp32-p4-wifi6-dev-kit/src/audio.cpp Implements ES8311 audio out + microphone capture tasks and buffering.
components/esp32-p4-wifi6-dev-kit/include/esp32-p4-wifi6-dev-kit.hpp Declares BSP public API, pin mapping constants, and internal state.
components/esp32-p4-wifi6-dev-kit/idf_component.yml Defines BSP manifest metadata and managed-component dependencies.
components/esp32-p4-wifi6-dev-kit/example/sdkconfig.defaults Enables PSRAM, camera pipeline, Ethernet, and flash/partition settings for the example.
components/esp32-p4-wifi6-dev-kit/example/partitions.csv Provides a larger app partition for the example.
components/esp32-p4-wifi6-dev-kit/example/main/idf_component.yml Overrides dependencies for in-repo espp/ethernet during example builds.
components/esp32-p4-wifi6-dev-kit/example/main/gui.hpp Declares the example’s LVGL GUI wrapper and thread-safety model.
components/esp32-p4-wifi6-dev-kit/example/main/gui.cpp Implements the tabbed GUI, camera canvas, and touch-trail overlay.
components/esp32-p4-wifi6-dev-kit/example/main/esp32_p4_wifi6_dev_kit_example.cpp Implements the example app_main wiring all peripherals + GUI.
components/esp32-p4-wifi6-dev-kit/example/main/CMakeLists.txt Embeds the click.wav asset and registers the example component.
components/esp32-p4-wifi6-dev-kit/example/README.md Documents what the example does and Wi-Fi/ESP-Hosted steps.
components/esp32-p4-wifi6-dev-kit/example/CMakeLists.txt Sets up the example project and extra component dirs list.
components/esp32-p4-wifi6-dev-kit/README.md Documents the BSP and supported peripherals.
components/esp32-p4-wifi6-dev-kit/Kconfig.projbuild Adds Kconfig options for panel choice and touch/interrupt settings.
components/esp32-p4-wifi6-dev-kit/CMakeLists.txt Registers the BSP component and its IDF component dependencies.
.github/workflows/upload_components.yml Adds the new BSP to the registry upload list.
.github/workflows/build.yml Adds the new example to the CI build matrix.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread components/esp32-p4-wifi6-dev-kit/src/video.cpp Outdated
Comment thread components/esp32-p4-wifi6-dev-kit/src/video.cpp Outdated
Comment thread components/esp32-p4-wifi6-dev-kit/src/video.cpp
Comment thread components/esp32-p4-wifi6-dev-kit/src/video.cpp Outdated
Comment thread components/esp32-p4-wifi6-dev-kit/include/esp32-p4-wifi6-dev-kit.hpp Outdated
Comment thread components/esp32-p4-wifi6-dev-kit/src/camera.cpp Outdated
@github-actions

Copy link
Copy Markdown

✅Static analysis result - no issues found! ✅

- video: propagate JD9365 panel power/backlight I2C write failures and abort
  initialize_lcd() instead of proceeding with an unpowered/in-reset panel
- video: drop misleading IRAM_ATTR from the LVGL flush_cb (task-context only)
  and document why the DPI on_color_trans_done callback keeps IRAM_ATTR and is
  ISR-safe (runs from the DMA2D transfer-done ISR when DMA2D is enabled)
- camera/microphone: make initialize_camera()/initialize_microphone()
  idempotent (warn + return true), matching the other initialize_* methods
- sdcard: store the SD power-control handle as sd_pwr_ctrl_handle_t (with
  sd_pwr_ctrl.h include) instead of void*, and document its ownership/cleanup
- example: cache the parsed WAV sample rate so the cached load_audio() path
  also sets out_sample_rate

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@finger563
finger563 requested a balanced review from Copilot August 28, 2026 21:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 27 changed files in this pull request and generated 7 comments.

Comment thread components/esp32-p4-wifi6-dev-kit/src/video.cpp Outdated
Comment thread components/esp32-p4-wifi6-dev-kit/include/esp32-p4-wifi6-dev-kit.hpp Outdated
Comment thread components/esp32-p4-wifi6-dev-kit/src/audio.cpp
Comment thread components/esp32-p4-wifi6-dev-kit/src/audio.cpp
Comment thread components/esp32-p4-wifi6-dev-kit/src/touchpad.cpp Outdated
Comment thread components/esp32-p4-wifi6-dev-kit/src/video.cpp Outdated
Comment thread components/esp32-p4-wifi6-dev-kit/src/video.cpp Outdated
…on PR #741

- video: check esp_lcd_panel_draw_bitmap() return in flush() and call
  lv_display_flush_ready() on failure (with an error log) so LVGL cannot
  stall waiting on a flush whose on_color_trans_done callback will never
  fire; also log draw_bitmap failures in write_lcd_lines()
- video: eliminate the flush()-vs-initialize_display() rotation-buffer
  free/realloc race: the scratch buffer is now an instance member,
  allocated exactly once before the LVGL display (and thus any GUI task
  that can call flush()) exists, and re-initialization is refused (warn +
  return true) once display_ exists; contract documented in the header
- audio: calc_audio_buffer_size() now takes the channel count of the path
  being sized (TX is mono, RX captures both slots as stereo) instead of
  hard-coding stereo, so the TX buffer represents one real update period
- audio: silence unused `cv` parameters in the audio/microphone task
  callbacks with (void)cv and a brief comment
- touchpad: fail initialize_touch() cleanly (error + return false) when
  the GT911 answers at neither I2C address, instead of creating a
  device/driver and a polling task that would error forever

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 27 changed files in this pull request and generated 1 comment.

Suppressed comments (11)

Previously missed (10) — in code that hasn't changed since the last review.

components/esp32-p4-wifi6-dev-kit/idf_component.yml:24

  • This component calls I2c::native_bus_handle() for camera SCCB, but that API is only available with the new I2C implementation, whose Kconfig contract requires ESP-IDF >= 5.4 (components/i2c/Kconfig:21-22). Advertising 5.3 allows Component Manager to select the BSP for a version where it cannot compile. Raise the manifest floor to 5.4.
    version: ">=5.3"

components/esp32-p4-wifi6-dev-kit/src/touchpad.cpp:92

  • The polling task uses the legacy two-argument callback and treats every condition-variable wake as a stop request. Since condition variables may wake spuriously, touch polling can terminate permanently without Task::stop() being called. Use the notified-aware callback, wait with a predicate, and check/clear the flag while holding the mutex as required by the Task contract.
        .callback = [this](std::mutex &m, std::condition_variable &cv) -> bool {

components/esp32-p4-wifi6-dev-kit/src/touchpad.cpp:95

  • Task::start() can fail, but this return value is ignored and initialize_touch() still reports success. Because touch_driver_ is already set, a later retry also takes the “already initialized” path even though no polling task exists. Check the result and clear the partially initialized touch state before returning false.
    touch_task_->start();

components/esp32-p4-wifi6-dev-kit/example/main/gui.cpp:295

  • A spurious condition-variable wake makes this return true, permanently stopping the GUI update task even though no stop was requested. Convert Gui::update, its declaration, and the forwarding lambda to the notified-aware Task callback; use the flag as the wait predicate and clear it under m before returning.
  std::unique_lock<std::mutex> lock(m);
  // stop promptly if Task::stop() notified us (otherwise ~Gui can hang joining
  // the update thread); a plain timeout means keep running
  return cv.wait_for(lock, std::chrono::milliseconds(16)) == std::cv_status::no_timeout;

components/esp32-p4-wifi6-dev-kit/example/main/gui.hpp:105

  • The GUI task can already process button events while app_main assigns this callback. on_clicked() reads record_callback_ under mutex_, but this setter writes the same std::function without that lock, which is a C++ data race if the user taps Record during initialization. Acquire the GUI mutex in the setter.

This issue also appears on line 109 of the same file.

  void set_record_callback(audio_button_callback_t callback) {
    record_callback_ = std::move(callback);
  }

components/esp32-p4-wifi6-dev-kit/src/audio.cpp:257

  • Once i2s_channel_init_std_mode() succeeds, either an enable failure or a task-start failure leaves the RX channel initialized (the latter only disables it). A subsequent initialize_microphone() calls i2s_channel_init_std_mode() again on a READY channel and fails, so the advertised retry path cannot recover. Track the configured state and skip reinitialization, or fully return/recreate the RX channel on these failure paths.
  if (!microphone_task_->start()) {
    i2s_channel_disable(audio_rx_handle);
    microphone_task_.reset();
    return false;

components/esp32-p4-wifi6-dev-kit/src/audio.cpp:104

  • es8311_codec_set_sample_rate() returns ESP_FAIL for unsupported rates, but the result is ignored and initialization continues with the codec still configured for a different rate. Since sample_rate is a public caller-supplied value, this can make initialize_audio() report success while playback is unusable. Propagate the codec configuration failure through the existing cleanup path.
  es8311_codec_set_sample_rate(sample_rate);

components/esp32-p4-wifi6-dev-kit/src/camera.cpp:237

  • This fatal dequeue path has the same stale-state problem as the requeue failure: the task stops while camera_initialized_ stays true and resources remain allocated, so later initialization falsely reports an active stream. Record the failure and make the owner-side retry path tear down the dead stream before reinitializing.
    // A real capture error (device/stream/driver): surface it and stop the task
    // instead of spinning forever with no diagnostics.
    logger_.error("VIDIOC_DQBUF failed (errno {}); stopping the camera task", errno);
    return true;

components/esp32-p4-wifi6-dev-kit/src/camera.cpp:228

  • On a fatal requeue error the task exits, but camera_initialized_, dimensions, fd, stream, and mapped buffers all remain live. Subsequent initialize_camera() therefore returns true from its idempotence guard even though no capture task is running, contradicting the API's “initialized and streaming” result. Transition to an error state and arrange teardown before a retry (without joining the task from itself).
    if (ioctl(camera_fd_, VIDIOC_QBUF, &buf) != 0) {
      logger_.error("VIDIOC_QBUF failed (errno {}); stopping the camera task", errno);
      return true; // stop the task; the owner can stop_camera() / re-init

components/esp32-p4-wifi6-dev-kit/include/esp32-p4-wifi6-dev-kit.hpp:748

  • The configured controller is known from Kconfig at construction, but this member starts as UNKNOWN until initialize_lcd() calls apply_panel_params(). Consequently get_display_controller()/get_display_controller_name() violate their “configured panel” contract before LCD initialization, and the example's pre-init panel log always prints Unknown. Initialize this from default_controller_.
  DisplayController display_controller_{DisplayController::UNKNOWN};

components/esp32-p4-wifi6-dev-kit/example/main/gui.hpp:109

  • As with the record callback, the running GUI task reads play_callback_ under mutex_ while this setter writes it unlocked. A Play tap during the assignment is a data race on std::function; use the same GUI mutex here.
  void set_play_callback(audio_button_callback_t callback) { play_callback_ = std::move(callback); }

…ack on PR #741

- example: close the record/playback race on recording_buffer - the
  microphone callback now checks the recording flag and copies under a
  recording_mutex, and the GUI button callbacks flip the state under the
  same mutex, so a state flip drains any in-flight buffer write before
  playback/reset touches the buffer
- manifest: raise the IDF floor to >=5.4 (the camera hands esp_video the
  I2c::native_bus_handle(), which needs the new I2C master API)
- touchpad/example gui: convert the polling/update task callbacks to the
  notified-aware Task signature and wait with the notified flag as the
  predicate, so spurious condition-variable wakes no longer stop the
  tasks; clear the flag under the mutex per the Task contract
- touchpad: check Task::start() and roll back the partial touch init on
  failure instead of reporting success with no polling task
- audio: derive the ES8311 init-time samples enum from the requested
  sample rate (rejecting unsupported rates) and propagate
  es8311_codec_set_sample_rate() failures through the cleanup path
- audio: make a failed initialize_microphone() recoverable - track that
  the RX channel is std-mode configured (init may only run once) and
  reconfigure just the clock on retry
- camera: tear down the capture pipeline on fatal capture errors via an
  idempotent teardown_camera_pipeline() helper (no self-join), called
  from the camera task and stop_camera(), so a later initialize_camera()
  can recover; set camera_initialized_ before starting the task so an
  early in-task teardown is not overwritten; make the frame dimensions
  atomic (read cross-thread, cleared by the in-task teardown)
- video: on IDF < 6.0 condition DMA2D on the JD9365 panel like the
  IDF >= 6.0 path (DMA2D corrupts RGB565 on ILI9881C/EK79007)
- bsp: initialize display_controller_ from the Kconfig-selected default
  so get_display_controller()/name() are correct before initialize_lcd()
- example gui: lock the GUI mutex in set_record_callback()/
  set_play_callback() (the GUI task reads them under that mutex) and
  bound the pending touch-point queue (drop-oldest, rate-limited warn)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@finger563

Copy link
Copy Markdown
Contributor Author

Addressed all 11 suppressed review comments in fdd1af0 (example builds clean):

  1. idf_component.yml:24 — IDF floor vs I2c::native_bus_handle() — Fixed. The camera does hand esp_video the native bus handle, which only exists with the new I2C master API (ESP-IDF >= 5.4 per components/i2c/Kconfig). Bumped the manifest to idf: ">=5.4" with a comment explaining why.

  2. src/touchpad.cpp:92 — spurious CV wake stops the polling task — Fixed. The polling task now uses the notified-aware callback (std::mutex &, std::condition_variable &, bool &task_notified) and waits with the notified flag as the predicate, clearing it under the mutex per the Task contract. A spurious wake now just re-waits; only a real Task::stop() stops the task.

  3. src/touchpad.cpp:95Task::start() result ignored — Fixed. A start failure now logs, rolls back the partial init (touch_task_, touchpad_input_, touch_driver_, touch_i2c_device_, touch_callback_) and returns false, so a later initialize_touch() retry does not take the "already initialized" path with no polling task.

  4. example/main/gui.cpp:295 — spurious CV wake stops the GUI task — Fixed. Gui::update, its declaration, and the forwarding lambda now use the notified-aware signature; the wait uses task_notified as the predicate and clears it under m before returning true.

  5. example/main/gui.hpp:105set_record_callback data race — Fixed. The setter now takes the GUI mutex_ (the same lock on_clicked() reads the callback under), so a Record tap during app_main initialization can no longer race the std::function assignment.

  6. src/audio.cpp:257 — failed initialize_microphone() unrecoverable — Fixed. Added audio_rx_std_configured_: i2s_channel_init_std_mode() (which may only run once per channel) is skipped on retry after an enable/task-start failure, and only the RX clock is reconfigured instead (in case audio_sample_rate() changed in between). The task-start failure path disables the channel back to READY so a retry can re-enable it.

  7. src/audio.cpp:104es8311_codec_set_sample_rate() result ignored — Fixed, and went one step further (matching the sibling P4 BSP): the requested rate is validated up front and mapped onto the ES8311 HAL samples enum (rejecting unsupported rates with a clear error), so the codec's init-time clock config no longer hard-codes 48 k; a set_sample_rate failure is also propagated through the existing fail_audio_init cleanup path.

  8. src/camera.cpp:237 — fatal DQBUF leaves stale state — Fixed together with (9): added an idempotent teardown_camera_pipeline() (STREAMOFF, munmap, close, esp_video_deinit, state/dimension reset including camera_initialized_ = false). Both fatal paths call it from the camera task itself (no self-join — only the pipeline is torn down, the task exits via return true), so the driver is not left wedged and initialize_camera() can be called again to recover. stop_camera() now stops/joins the task first and then calls the same teardown (safe if the task already tore down: the teardown is idempotent and stop() on an exited task just joins it).

  9. src/camera.cpp:228 — fatal QBUF leaves stale state — Fixed, same teardown_camera_pipeline() mechanism as (8). Also ordered camera_initialized_ = true before camera_task_->start() so a very early in-task teardown cannot be overwritten by the owner-side assignment (a start failure still goes through stop_camera(), which clears it).

  10. include/esp32-p4-wifi6-dev-kit.hpp:748display_controller_ starts UNKNOWN — Fixed. It is now initialized from the Kconfig-selected default_controller_, so get_display_controller() / get_display_controller_name() honor their "configured panel" contract before initialize_lcd() (the example's pre-init panel log now prints the real controller).

  11. example/main/gui.hpp:109set_play_callback data race — Fixed, same as (5): the setter takes the GUI mutex_.

Related hardening in the same commit (same defect classes, found while applying the above): the camera frame dimensions are now std::atomic<uint16_t> (they are cleared by the in-task teardown while other threads read them via camera_width()/camera_height()); the IDF < 6.0 DPI config now conditions flags.use_dma2d on the JD9365 panel exactly like the IDF >= 6.0 path (DMA2D corrupts RGB565 on the ILI9881C/EK79007); and the example GUI's pending touch-point queue is bounded (drop-oldest at a small cap with a rate-limited warning) so it cannot grow without limit if the GUI task stalls.

@finger563
finger563 requested a balanced review from Copilot August 29, 2026 02:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 27 changed files in this pull request and generated 4 comments.

Comment thread components/esp32-p4-wifi6-dev-kit/src/video.cpp Outdated
Comment thread components/esp32-p4-wifi6-dev-kit/src/touchpad.cpp Outdated
Comment thread components/esp32-p4-wifi6-dev-kit/src/camera.cpp Outdated
Comment thread components/esp32-p4-wifi6-dev-kit/src/audio.cpp Outdated
…on PR #741

- video: flush() reads rotation from the disp argument; rotated_display_width/
  height and touchpad_convert query the BSP-managed display via a new
  current_display_rotation() helper instead of lv_display_get_default()
- camera: clamp the frame length handed to the callback to the mmap'd buffer
  size, with a rate-limited warning on mismatch
- audio: capture i2s_channel_write()'s return and bytes_written and warn
  (rate-limited) on failure or partial write

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 27 changed files in this pull request and generated 4 comments.

Suppressed comments (7)

Previously missed (5) — in code that hasn't changed since the last review.

components/esp32-p4-wifi6-dev-kit/src/audio.cpp:439

  • The stored clock configuration is changed before reconfiguration succeeds. If i2s_channel_reconfig_std_clock() rejects the rate or otherwise fails, audio_sample_rate() reports the new value even though hardware remains at the old rate, and execution still clears the stream. Reconfigure from a temporary config and commit it only on success; on failure, re-enable the old channel and return.
  audio_std_cfg.clk_cfg.sample_rate_hz = sample_rate;
  err = i2s_channel_reconfig_std_clock(audio_tx_handle, &audio_std_cfg.clk_cfg);
  if (err != ESP_OK) {
    logger_.error("Failed to reconfigure I2S clock: {}", esp_err_to_name(err));
  }

components/esp32-p4-wifi6-dev-kit/src/camera.cpp:135

  • VIDIOC_S_FMT may return success after adjusting the requested format, but only width and height are checked here. If RGB565 is unavailable, subsequent code still labels and interprets the returned payload as RGB565. Verify the returned pixelformat before publishing the frame dimensions and tear down if the driver selected another format.
  camera_width_ = static_cast<uint16_t>(format.fmt.pix.width);
  camera_height_ = static_cast<uint16_t>(format.fmt.pix.height);
  logger_.info("Camera format: {}x{} RGB565", camera_width_.load(), camera_height_.load());

components/esp32-p4-wifi6-dev-kit/example/main/esp32_p4_wifi6_dev_kit_example.cpp:379

  • The embedded click.wav is stereo 16-bit PCM, while play_audio() accepts mono samples. This loader strips the WAV payload without parsing the channel count or downmixing, so interleaved left/right samples are emitted as sequential mono samples, stretching/distorting the click. Convert the asset to mono or parse the fmt chunk and downmix before storing audio_bytes.
  uint32_t sample_rate = 0;
  std::memcpy(&sample_rate, &audio_bytes[24], sizeof(sample_rate));
  // Walk the RIFF chunks to find the 'data' chunk and keep exactly its payload.

components/esp32-p4-wifi6-dev-kit/example/main/esp32_p4_wifi6_dev_kit_example.cpp:265

  • Starting a recording only stops the playback producer; previously queued speaker samples continue playing and can be captured by the full-duplex microphone after the UI switches to “Recording...”. Clear the audio queue when transitioning from playback to recording.

This issue also appears on line 278 of the same file.

      playing = false;
      gui.set_play_active(false);

components/esp32-p4-wifi6-dev-kit/example/main/esp32_p4_wifi6_dev_kit_example.cpp:168

  • This click path clears the same stream used for recorded playback. A Status-tab touch during playback discards bytes for which the main loop has already advanced play_offset, then writes to the stream concurrently with the playback producer, causing skipped/interleaved audio. Suppress clicks until recorded playback and its queued tail have drained, or serialize/mix the two producers explicitly.
        if (new_touch && click_due && !audio_bytes.empty()) {
          board.clear_audio();           // drop any queued tail (restart)
          board.play_audio(audio_bytes); // non-blocking
          last_click_time = now;

components/esp32-p4-wifi6-dev-kit/src/audio.cpp:200

  • mute() has the same pre-initialization crash path: es8311_set_voice_mute() reads/writes through callbacks that are not installed until initialize_audio(). Guard the codec call when audio is not initialized, and apply the cached mute_ during initialization instead of unconditionally applying false, so software and hardware state remain consistent.
void Esp32P4Wifi6DevKit::mute(bool mute) {
  mute_ = mute;
  es8311_set_voice_mute(mute_);
}

components/esp32-p4-wifi6-dev-kit/example/main/esp32_p4_wifi6_dev_kit_example.cpp:281

  • Setting playing false only stops future enqueues; audio already queued in the BSP stream continues playing, so the UI reports “Playback stopped” while sound remains audible. Clear the queue when handling the stop action.
    if (playing) {
      playing = false;
      gui.set_play_active(false);
      gui.set_audio_status("Playback stopped");

Comment thread components/esp32-p4-wifi6-dev-kit/src/audio.cpp
Comment thread components/esp32-p4-wifi6-dev-kit/src/video.cpp
Comment thread components/esp32-p4-wifi6-dev-kit/src/camera.cpp
Comment thread components/esp32-p4-wifi6-dev-kit/example/main/esp32_p4_wifi6_dev_kit_example.cpp Outdated
…n PR #741

- audio: guard volume()/mute() before initialize_audio() (the ES8311 driver
  callbacks are not installed yet; calling through them would terminate) -
  cache the values and apply them to the codec during initialization
- audio: commit the cached I2S clock config only after
  i2s_channel_reconfig_std_clock() succeeds; on failure re-enable the channel
  at the old rate so audio_sample_rate() never reports a rate the hardware
  rejected
- video: fail initialize_display() outright when the rotation scratch buffer
  cannot be allocated (a null buffer would make flush() send unrotated pixels
  at rotated coordinates), and make flush() drop - not misdraw - a rotated
  area that cannot be rotated (defense-in-depth)
- camera: make stop_camera() safe to call from the frame callback: detect the
  in-task context and do a teardown-only + deferred task exit instead of a
  self-join; document the contract
- camera: verify VIDIOC_S_FMT actually kept RGB565 (it may adjust the pixel
  format and still return success) and fail initialization otherwise
- example: honor the BSP-validated frame length in the camera callback and
  clamp the Gui::set_camera_frame() copy to min(length, w*h*2)
- example: parse the WAV fmt chunk (PCM/16-bit/channels) and downmix the
  stereo click.wav to mono instead of playing interleaved words as mono
- example: clear the queued audio stream when starting a recording and when
  stopping playback, and suppress touch clicks while recorded playback is
  streaming (single producer for the shared audio stream)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@finger563

Copy link
Copy Markdown
Contributor Author

Dispositions for the 7 suppressed comments from Copilot's latest review, addressed in 6426652:

  1. src/audio.cpp:439 — cached clock config mutated before i2s_channel_reconfig_std_clock() succeeds — Fixed. audio_sample_rate(uint32_t) now reconfigures from a local copy of the clock config and commits it to audio_std_cfg only on success; on failure it leaves the cached rate and the queued stream untouched and re-enables the channel at the old (still-configured) rate, so the getter never reports a rate the hardware rejected.

  2. src/audio.cpp:200mute() has the same pre-init crash path as volume() — Fixed (together with the inline thread on volume()): both now cache-only before initialize_audio(), and initialization applies the cached volume and mute (previously it forced mute(false), which could disagree with a cached mute_).

  3. src/camera.cpp:135VIDIOC_S_FMT may adjust the pixel format and still return success — Fixed. The negotiated pixelformat is now verified to be V4L2_PIX_FMT_RGB565 before the frame dimensions are published; anything else fails initialize_camera() through stop_camera() (full pipeline teardown) instead of delivering mislabeled frames.

  4. example/main:379 — embedded click.wav is stereo but play_audio() takes mono — Fixed; confirmed the asset really is stereo (fmt chunk: PCM, 2ch, 44.1 kHz, 16-bit). load_audio() now parses the fmt chunk (format/channels/rate/bits) while walking the RIFF chunks, rejects anything that isn't 16-bit PCM mono/stereo, and downmixes stereo in place by averaging each interleaved L/R pair before caching audio_bytes.

  5. example/main:265 (and 278) — starting a recording leaves queued speaker samples playing into the mic — Fixed. The record-start path now calls board.clear_audio() after stopping the playback producer, so the queued tail is dropped before the UI switches to "Recording..." (the ES8311 is full duplex, so that tail was capturable).

  6. example/main:281 — stopping playback only stops future enqueues; the queued tail keeps sounding — Fixed. The play-button stop path also calls board.clear_audio(), so "Playback stopped" is true immediately rather than when the stream drains.

  7. example/main:168 — Status-tab click during recorded playback clears/interleaves the shared stream — Fixed by serializing the producers: the click path is suppressed while playing is set (best-effort atomic check, documented as such — it's feedback audio, not a hard mutual exclusion), so it can no longer discard queued playback bytes that play_offset already accounted for or interleave with the playback producer.

All builds verified with idf.py build for the example on ESP-IDF 6.0.

🤖 Generated with Claude Code

@finger563
finger563 requested a balanced review from Copilot August 29, 2026 20:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.

Comment thread components/esp32-p4-wifi6-dev-kit/idf_component.yml Outdated
Comment thread components/esp32-p4-wifi6-dev-kit/src/camera.cpp Outdated
…n PR #741

- example: convert the status task to the 3-arg notified-aware Task
  callback with a predicate wait, matching the pattern used everywhere
  else in the PR
- idf_component.yml: use the https:// repository URL instead of git://
- camera: validate the dequeued V4L2 buffer index against the runtime
  camera_buffer_count_ (VIDIOC_REQBUFS may grant fewer buffers than the
  compile-time max) before indexing the mmap arrays, with a fatal error
  path

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@finger563
finger563 requested a balanced review from Copilot August 30, 2026 01:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.

Comment thread components/esp32-p4-wifi6-dev-kit/src/sdcard.cpp Outdated
Comment thread components/esp32-p4-wifi6-dev-kit/example/CMakeLists.txt Outdated
Comment thread components/esp32-p4-wifi6-dev-kit/src/camera.cpp Outdated
… on PR #741

- sdcard: make initialize_sdcard() idempotent (warn + return true when
  already initialized), matching the other initialize_* methods
- camera: replace the file-scope 'using namespace espp' with a
  'namespace espp { }' block, matching the BSP's other source files
- example: set CMAKE_CXX_STANDARD to 23 and move it before
  project()/project.cmake so the IDF build system picks it up
- example: embed click.wav with EMBED_FILES instead of EMBED_TXTFILES
  (binary asset; TXTFILES appends a NUL terminator)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.

Comment on lines +167 to +170
audio_tx_stream = xStreamBufferCreate(std::max<size_t>(tx_buf_size * 4, 64 * 1024), 0);
if (audio_tx_stream == nullptr) {
return fail_audio_init("Failed to allocate the audio TX stream buffer");
}
Comment on lines +381 to +389
void Gui::set_status_text(std::string_view text) {
std::lock_guard<std::recursive_mutex> lock(mutex_);
lv_label_set_text(status_label_, std::string(text).c_str());
}

void Gui::set_audio_status(std::string_view text) {
std::lock_guard<std::recursive_mutex> lock(mutex_);
lv_label_set_text(audio_status_label_, std::string(text).c_str());
}
Comment on lines +43 to +44
/// The ESP32-P4-WIFI6-DEV-KIT is an ESP32-P4NRW32 (32 MB stacked PSRAM, 16 MB
/// NOR flash) multimedia development board with an onboard ESP32-C6
Copilot stopped reviewing on behalf of finger563 due to an error August 30, 2026 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants