feat: add ns4150 audio player#28
Merged
Merged
Conversation
📝 WalkthroughWalkthroughIntroduces a new NS4150 audio player example for ESP32-C5, including hardware initialization (I2S/PDM, PA control GPIO), SPIFFS-based audio storage, sequential playback orchestration, an audio codec abstraction layer, and a board support package for the ESP Sensair Shuttle with touchpad and display integration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 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 unit tests (beta)
📝 Coding Plan
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 Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
examples/audio/ns4150_player/test/esp-sensairshuttle.cc (1)
163-174: Rewrite the nested designator in standard C++ form.This
.ccfile uses a nested designated initializer fori2c_bus_cfg.flags. ESP-IDF's C++ guide calls out nested designated initialization as unsupported in the default C++ mode, so this is brittle across toolchains and editor tooling even if one compiler accepts it as an extension. Initializeflags.enable_internal_pullupafter constructing the aggregate instead. (docs.espressif.com)Minimal rewrite
- i2c_master_bus_config_t i2c_bus_cfg = { - .i2c_port = I2C_NUM_0, - .sda_io_num = LCD_TP_SDA, - .scl_io_num = LCD_TP_SCL, - .clk_source = I2C_CLK_SRC_DEFAULT, - .glitch_ignore_cnt = 7, - .intr_priority = 0, - .trans_queue_depth = 0, - .flags = { - .enable_internal_pullup = 1, - }, - }; + i2c_master_bus_config_t i2c_bus_cfg = { + .i2c_port = I2C_NUM_0, + .sda_io_num = LCD_TP_SDA, + .scl_io_num = LCD_TP_SCL, + .clk_source = I2C_CLK_SRC_DEFAULT, + .glitch_ignore_cnt = 7, + .intr_priority = 0, + .trans_queue_depth = 0, + .flags = {}, + }; + i2c_bus_cfg.flags.enable_internal_pullup = 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/audio/ns4150_player/test/esp-sensairshuttle.cc` around lines 163 - 174, Replace the nested designated initializer for i2c_bus_cfg.flags with a standard C++-compatible assignment: construct i2c_master_bus_config_t i2c_bus_cfg using only top-level designated initializers (or a plain aggregate initializer) and then set i2c_bus_cfg.flags.enable_internal_pullup = 1 on the next line; keep other fields (i2c_port, sda_io_num, scl_io_num, clk_source, glitch_ignore_cnt, intr_priority, trans_queue_depth) initialized as before and only mutate the flags member after construction to avoid nested designated initializers for the flags field.examples/audio/ns4150_player/main/idf_component.yml (1)
4-5: Advertise the real minimum ESP-IDF version here.This example is registered against
release-v5.5inbuild_config.yml, but the manifest still claims compatibility with>=4.1.0. That gives component-manager users a misleading floor and pushes any incompatibility to compile time. Please tighten this to the oldest IDF version the example actually supports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/audio/ns4150_player/main/idf_component.yml` around lines 4 - 5, The idf manifest currently declares "idf.version" as ">=4.1.0" but the example is registered against release-v5.5 in build_config.yml, so update the idf.version field in idf_component.yml to the actual minimum ESP-IDF release this example supports (e.g., bump "idf.version" to the oldest 5.x release you validated, such as ">=5.5.0"), ensuring the idf.version entry matches the release-v5.5 registration to avoid misleading component-manager users.examples/audio/ns4150_player/test/adc_pdm_audio_codec.h (1)
10-35: Make this handle-owning type non-copyable.The destructor plus raw
esp_codec_dev_handle_t/esp_timer_handle_tmembers suggest unique ownership, but the declaration still permits accidental copies. A future pass-by-value would duplicate those handles and make cleanup unsafe.Suggested guardrails
class AdcPdmAudioCodec : public AudioCodec { private: esp_codec_dev_handle_t output_dev_ = nullptr; esp_codec_dev_handle_t input_dev_ = nullptr; gpio_num_t pa_ctrl_pin_ = GPIO_NUM_NC; @@ public: + AdcPdmAudioCodec(const AdcPdmAudioCodec&) = delete; + AdcPdmAudioCodec& operator=(const AdcPdmAudioCodec&) = delete; + AdcPdmAudioCodec(AdcPdmAudioCodec&&) = delete; + AdcPdmAudioCodec& operator=(AdcPdmAudioCodec&&) = delete; + AdcPdmAudioCodec(int input_sample_rate, int output_sample_rate, uint32_t adc_mic_channel, gpio_num_t pdm_speak_p, gpio_num_t pdm_speak_n, gpio_num_t pa_ctl); virtual ~AdcPdmAudioCodec();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/audio/ns4150_player/test/adc_pdm_audio_codec.h` around lines 10 - 35, The AdcPdmAudioCodec class currently owns raw handles (output_dev_, input_dev_, output_timer_) but is copyable; to prevent accidental handle duplication, make the type non-copyable by deleting the copy constructor and copy assignment: add AdcPdmAudioCodec(const AdcPdmAudioCodec&) = delete; and AdcPdmAudioCodec& operator=(const AdcPdmAudioCodec&) = delete; (optionally also delete or explicitly define move operations) next to the public constructors/destructor so copies are prohibited while locateable via the AdcPdmAudioCodec symbol and its members.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/audio/ns4150_player/main/ns4150_player.c`:
- Around line 198-203: The code hardcodes IO_MUX_GPIO10_REG when calling
PIN_FUNC_SELECT which ignores the actual Kconfig-controlled
AUDIO_PDM_SPEAK_N_GPIO; change the IOMUX selection to use the pad register for
AUDIO_PDM_SPEAK_N_GPIO instead of IO_MUX_GPIO10_REG (e.g. use the
GPIO_PIN_MUX_REG/GPIO_IOMUX macros or resolve the pad register via a helper for
AUDIO_PDM_SPEAK_N_GPIO) so PIN_FUNC_SELECT targets the configured pin, keep the
following calls (gpio_set_direction(AUDIO_PDM_SPEAK_N_GPIO,...),
esp_rom_gpio_connect_out_signal(AUDIO_PDM_SPEAK_N_GPIO, I2SO_SD_OUT_IDX,...),
gpio_set_drive_capability(...)) intact but referencing the correct mux/pad for
AUDIO_PDM_SPEAK_N_GPIO.
- Around line 250-265: The timeout can cause a late completion byte to be
consumed by the next file and desynchronize playback; modify the timeout branch
inside the loop so that when xQueueReceive on s_play_done_queue times out you
explicitly stop the current playback (call the project's stop function such as
audio_player_stop() or equivalent after audio_player_play()), close the file
handle (fclose(fp)), and purge/reset the s_play_done_queue (use
xQueueReset(s_play_done_queue) or drain remaining bytes with non-blocking
xQueueReceive) before continuing to the next item to ensure no stale completion
token is carried over.
- Around line 80-85: In bsp_audio_write, do not unconditionally set
*bytes_written = len; instead check the result of
esp_codec_dev_write(play_dev_handle, audio_buffer, len) and only set
*bytes_written to len when that call returns ESP_OK (on error set *bytes_written
= 0 or an appropriate partial count), and return the esp_err_t from
esp_codec_dev_write; update the function body around the esp_codec_dev_write
call and the *bytes_written assignment to reflect this conditional behavior.
In `@examples/audio/ns4150_player/test/adc_pdm_audio_codec.cc`:
- Around line 219-228: AdcPdmAudioCodec::Write currently returns success and
drops samples when output_enabled_ is false (set by OutputTimerCallback via
EnableOutput(false)), which causes playback to never recover; change Write so
that when output_enabled_ is false it re-enables the output path before
accepting data: call EnableOutput(true) (and restart/arm output_timer_ as
needed) when new data arrives, then perform the esp_codec_dev_write and timer
reset logic; alternatively return 0/error to force caller to re-enable, but
prefer automatic re-enable in Write to restore playback automatically;
reference: AdcPdmAudioCodec::Write, OutputTimerCallback, EnableOutput,
output_enabled_, output_timer_.
- Around line 94-97: The creation result of output_dev_ from
esp_codec_dev_new(&codec_dev_cfg) is never checked; add a null-check immediately
after that call and handle failure (log error and return/fail initialization) to
avoid dereferencing a null in EnableOutput(), SetOutputVolume(), and the
destructor. Specifically, after output_dev_ = esp_codec_dev_new(&codec_dev_cfg);
verify output_dev_ != NULL, call process/log an informative error, and abort or
set an initialization-failed state so subsequent methods (EnableOutput,
SetOutputVolume, and the class destructor) do not assume a valid output_dev_
handle.
- Around line 112-115: The code incorrectly hard-codes
PIN_FUNC_SELECT(IO_MUX_GPIO10_REG) while pdm_speak_n is runtime-configurable;
remove the static IOMUX call and instead configure the pad and route the I2S
SD_OUT through the GPIO matrix for the actual pin. Replace the PIN_FUNC_SELECT
call with a generic pad setup (e.g., gpio_pad_select_gpio(pdm_speak_n) or
equivalent) and use gpio_matrix_out (or esp_rom_gpio_connect_out_signal with the
pin's matrix mapping) to connect I2SO_SD_OUT_IDX to pdm_speak_n, then keep
gpio_set_direction(pdm_speak_n, GPIO_MODE_OUTPUT) and the
esp_rom_gpio_connect_out_signal/gpio_matrix_out call using pdm_speak_n so no
IO_MUX_GPIO10_REG is mutated. Ensure you update the code paths referencing
PIN_FUNC_SELECT, IO_MUX_GPIO10_REG, esp_rom_gpio_connect_out_signal, and
I2SO_SD_OUT_IDX accordingly.
- Around line 131-143: Destructor AdcPdmAudioCodec::~AdcPdmAudioCodec currently
never releases the I2S transmit channel (tx_handle_), causing a resource leak;
update the destructor to check tx_handle_ and call the proper I2S cleanup
sequence (disable then delete) — e.g., call i2s_channel_disable(tx_handle_)
followed by i2s_del_channel(tx_handle_) (or the equivalent platform APIs used in
this project), then set tx_handle_ = nullptr — perform this before/around the
existing codec device close/delete calls to ensure the channel is fully released
when tearing down the codec.
In `@examples/audio/ns4150_player/test/esp-sensairshuttle.cc`:
- Around line 206-210: The InitializeCst816dTouchPad function calls
xTaskCreate(touch_event_task, ...) during construction (creating Cst816d via
cst816d_) and doesn't check the return value or defer startup; change this by
moving xTaskCreate out of the constructor/InitializeCst816dTouchPad so the task
is started only after board construction (after
InitializeButtons/InitializeSpi/InitializeLcdDisplay) completes, and when you do
create the task check xTaskCreate's return (or use
xTaskCreateStatic/xTaskCreatePinnedToCore as appropriate) and handle failure
(log error/cleanup/retry) to avoid silent failures; ensure touch_event_task only
accesses fully-initialized Cst816d/Application/Board singletons or pass a safe
initialized context pointer to the task.
- Line 18: The code includes and instantiates ILI9341-specific types and
functions (esp_lcd_ili9341.h, ili9341_lcd_init_cmd_t,
esp_lcd_new_panel_ili9341()) while config.h sets LCD_TYPE_ST7789_SERIAL; make
these consistent by either switching this file to use the ST7789 driver (replace
esp_lcd_ili9341.h and ILI9341-specific init commands with the ST7789 equivalents
and call esp_lcd_new_panel_st7789()) or update config.h to LCD_TYPE_ILI9341 and
keep the current ILI9341 setup; also ensure the panel init command array and any
driver-specific calls match the chosen driver (ILI9341 vs ST7789) including any
init struct/type names and panel creation function.
---
Nitpick comments:
In `@examples/audio/ns4150_player/main/idf_component.yml`:
- Around line 4-5: The idf manifest currently declares "idf.version" as
">=4.1.0" but the example is registered against release-v5.5 in
build_config.yml, so update the idf.version field in idf_component.yml to the
actual minimum ESP-IDF release this example supports (e.g., bump "idf.version"
to the oldest 5.x release you validated, such as ">=5.5.0"), ensuring the
idf.version entry matches the release-v5.5 registration to avoid misleading
component-manager users.
In `@examples/audio/ns4150_player/test/adc_pdm_audio_codec.h`:
- Around line 10-35: The AdcPdmAudioCodec class currently owns raw handles
(output_dev_, input_dev_, output_timer_) but is copyable; to prevent accidental
handle duplication, make the type non-copyable by deleting the copy constructor
and copy assignment: add AdcPdmAudioCodec(const AdcPdmAudioCodec&) = delete; and
AdcPdmAudioCodec& operator=(const AdcPdmAudioCodec&) = delete; (optionally also
delete or explicitly define move operations) next to the public
constructors/destructor so copies are prohibited while locateable via the
AdcPdmAudioCodec symbol and its members.
In `@examples/audio/ns4150_player/test/esp-sensairshuttle.cc`:
- Around line 163-174: Replace the nested designated initializer for
i2c_bus_cfg.flags with a standard C++-compatible assignment: construct
i2c_master_bus_config_t i2c_bus_cfg using only top-level designated initializers
(or a plain aggregate initializer) and then set
i2c_bus_cfg.flags.enable_internal_pullup = 1 on the next line; keep other fields
(i2c_port, sda_io_num, scl_io_num, clk_source, glitch_ignore_cnt, intr_priority,
trans_queue_depth) initialized as before and only mutate the flags member after
construction to avoid nested designated initializers for the flags field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c352d07-4463-42ab-9e38-8de48ec89193
⛔ Files ignored due to path filters (5)
examples/audio/ns4150_player/partitions.csvis excluded by!**/*.csvexamples/audio/ns4150_player/spiffs/fail.wavis excluded by!**/*.wavexamples/audio/ns4150_player/spiffs/need_approve.wavis excluded by!**/*.wavexamples/audio/ns4150_player/spiffs/start_work.wavis excluded by!**/*.wavexamples/audio/ns4150_player/spiffs/success.wavis excluded by!**/*.wav
📒 Files selected for processing (12)
build_config.ymlexamples/audio/ns4150_player/CMakeLists.txtexamples/audio/ns4150_player/main/CMakeLists.txtexamples/audio/ns4150_player/main/Kconfig.projbuildexamples/audio/ns4150_player/main/idf_component.ymlexamples/audio/ns4150_player/main/ns4150_player.cexamples/audio/ns4150_player/sdkconfig.defaultsexamples/audio/ns4150_player/test/adc_pdm_audio_codec.ccexamples/audio/ns4150_player/test/adc_pdm_audio_codec.hexamples/audio/ns4150_player/test/config.hexamples/audio/ns4150_player/test/config.jsonexamples/audio/ns4150_player/test/esp-sensairshuttle.cc
Comment on lines
+80
to
+85
| static esp_err_t bsp_audio_write(void *audio_buffer, size_t len, size_t *bytes_written, uint32_t timeout_ms) | ||
| { | ||
| esp_err_t ret = ESP_OK; | ||
| ret = esp_codec_dev_write(play_dev_handle, audio_buffer, len); | ||
| *bytes_written = len; | ||
| return ret; |
There was a problem hiding this comment.
Don't report a full write on error.
*bytes_written is set to len unconditionally. If esp_codec_dev_write() fails, the player is told the entire buffer was consumed even though nothing reached the sink.
🛠️ Minimal fix
static esp_err_t bsp_audio_write(void *audio_buffer, size_t len, size_t *bytes_written, uint32_t timeout_ms)
{
esp_err_t ret = ESP_OK;
ret = esp_codec_dev_write(play_dev_handle, audio_buffer, len);
- *bytes_written = len;
+ *bytes_written = (ret == ESP_OK) ? len : 0;
return ret;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static esp_err_t bsp_audio_write(void *audio_buffer, size_t len, size_t *bytes_written, uint32_t timeout_ms) | |
| { | |
| esp_err_t ret = ESP_OK; | |
| ret = esp_codec_dev_write(play_dev_handle, audio_buffer, len); | |
| *bytes_written = len; | |
| return ret; | |
| static esp_err_t bsp_audio_write(void *audio_buffer, size_t len, size_t *bytes_written, uint32_t timeout_ms) | |
| { | |
| esp_err_t ret = ESP_OK; | |
| ret = esp_codec_dev_write(play_dev_handle, audio_buffer, len); | |
| *bytes_written = (ret == ESP_OK) ? len : 0; | |
| return ret; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/audio/ns4150_player/main/ns4150_player.c` around lines 80 - 85, In
bsp_audio_write, do not unconditionally set *bytes_written = len; instead check
the result of esp_codec_dev_write(play_dev_handle, audio_buffer, len) and only
set *bytes_written to len when that call returns ESP_OK (on error set
*bytes_written = 0 or an appropriate partial count), and return the esp_err_t
from esp_codec_dev_write; update the function body around the
esp_codec_dev_write call and the *bytes_written assignment to reflect this
conditional behavior.
Comment on lines
+198
to
+203
| #if CONFIG_NS4150_USE_PDM_N | ||
| /* PDM N: inverted SD OUT for differential output */ | ||
| PIN_FUNC_SELECT(IO_MUX_GPIO10_REG, PIN_FUNC_GPIO); | ||
| gpio_set_direction(AUDIO_PDM_SPEAK_N_GPIO, GPIO_MODE_OUTPUT); | ||
| esp_rom_gpio_connect_out_signal(AUDIO_PDM_SPEAK_N_GPIO, I2SO_SD_OUT_IDX, 1, 0); | ||
| gpio_set_drive_capability(AUDIO_PDM_SPEAK_N_GPIO, GPIO_DRIVE_CAP_0); |
There was a problem hiding this comment.
The PDM N mux ignores the configured GPIO.
AUDIO_PDM_SPEAK_N_GPIO comes from Kconfig, and examples/audio/ns4150_player/main/Kconfig.projbuild Line 21 even defaults it to 8, but this code still programs IO_MUX_GPIO10_REG. At minimum that touches the wrong pad, and on targets where explicit IOMUX selection matters it breaks non-10 mappings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/audio/ns4150_player/main/ns4150_player.c` around lines 198 - 203,
The code hardcodes IO_MUX_GPIO10_REG when calling PIN_FUNC_SELECT which ignores
the actual Kconfig-controlled AUDIO_PDM_SPEAK_N_GPIO; change the IOMUX selection
to use the pad register for AUDIO_PDM_SPEAK_N_GPIO instead of IO_MUX_GPIO10_REG
(e.g. use the GPIO_PIN_MUX_REG/GPIO_IOMUX macros or resolve the pad register via
a helper for AUDIO_PDM_SPEAK_N_GPIO) so PIN_FUNC_SELECT targets the configured
pin, keep the following calls (gpio_set_direction(AUDIO_PDM_SPEAK_N_GPIO,...),
esp_rom_gpio_connect_out_signal(AUDIO_PDM_SPEAK_N_GPIO, I2SO_SD_OUT_IDX,...),
gpio_set_drive_capability(...)) intact but referencing the correct mux/pad for
AUDIO_PDM_SPEAK_N_GPIO.
Comment on lines
+250
to
+265
| for (size_t i = 0; i < play_count; i++) { | ||
| FILE *fp = fopen(play_list[i], "rb"); | ||
| if (fp == NULL) { | ||
| ESP_LOGE(TAG, "Failed to open: %s", play_list[i]); | ||
| continue; | ||
| } | ||
| esp_err_t ret = audio_player_play(fp); | ||
| if (ret != ESP_OK) { | ||
| ESP_LOGE(TAG, "Failed to play %s (%s)", play_list[i], esp_err_to_name(ret)); | ||
| fclose(fp); | ||
| continue; | ||
| } | ||
| uint8_t done = 0; | ||
| if (xQueueReceive(s_play_done_queue, &done, pdMS_TO_TICKS(30000)) != pdTRUE) { | ||
| ESP_LOGW(TAG, "Play timeout: %s", play_list[i]); | ||
| } |
There was a problem hiding this comment.
A timeout can desynchronize the rest of the playlist.
When the 30-second wait expires, the loop immediately starts the next file without stopping playback or invalidating the old completion token. If the previous track finishes later, that late queue byte is consumed by the next iteration and the sequence gets out of sync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/audio/ns4150_player/main/ns4150_player.c` around lines 250 - 265,
The timeout can cause a late completion byte to be consumed by the next file and
desynchronize playback; modify the timeout branch inside the loop so that when
xQueueReceive on s_play_done_queue times out you explicitly stop the current
playback (call the project's stop function such as audio_player_stop() or
equivalent after audio_player_play()), close the file handle (fclose(fp)), and
purge/reset the s_play_done_queue (use xQueueReset(s_play_done_queue) or drain
remaining bytes with non-blocking xQueueReceive) before continuing to the next
item to ensure no stale completion token is carried over.
Comment on lines
+94
to
+97
| codec_dev_cfg.dev_type = ESP_CODEC_DEV_TYPE_OUT; | ||
| codec_dev_cfg.codec_if = NULL; | ||
| codec_dev_cfg.data_if = i2s_data_if; | ||
| output_dev_ = esp_codec_dev_new(&codec_dev_cfg); |
There was a problem hiding this comment.
Check output_dev_ before continuing.
Unlike input_dev_, this esp_codec_dev_new() result is never validated. If output-device creation fails, later calls to EnableOutput(), SetOutputVolume(), and the destructor all dereference a null handle and turn an init error into a crash.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/audio/ns4150_player/test/adc_pdm_audio_codec.cc` around lines 94 -
97, The creation result of output_dev_ from esp_codec_dev_new(&codec_dev_cfg) is
never checked; add a null-check immediately after that call and handle failure
(log error and return/fail initialization) to avoid dereferencing a null in
EnableOutput(), SetOutputVolume(), and the destructor. Specifically, after
output_dev_ = esp_codec_dev_new(&codec_dev_cfg); verify output_dev_ != NULL,
call process/log an informative error, and abort or set an initialization-failed
state so subsequent methods (EnableOutput, SetOutputVolume, and the class
destructor) do not assume a valid output_dev_ handle.
Comment on lines
+112
to
+115
| if(pdm_speak_n != GPIO_NUM_NC){ | ||
| PIN_FUNC_SELECT(IO_MUX_GPIO10_REG, PIN_FUNC_GPIO); | ||
| gpio_set_direction(pdm_speak_n, GPIO_MODE_OUTPUT); | ||
| esp_rom_gpio_connect_out_signal(pdm_speak_n, I2SO_SD_OUT_IDX, 1, 0); //反转输出 SD OUT 信号 |
There was a problem hiding this comment.
pdm_speak_n is configurable, but this still programs GPIO10.
pdm_speak_n is passed in at runtime, yet the mux select is hard-coded to IO_MUX_GPIO10_REG. At minimum this mutates the wrong pad, and on targets where explicit IOMUX selection matters it breaks any non-10 mapping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/audio/ns4150_player/test/adc_pdm_audio_codec.cc` around lines 112 -
115, The code incorrectly hard-codes PIN_FUNC_SELECT(IO_MUX_GPIO10_REG) while
pdm_speak_n is runtime-configurable; remove the static IOMUX call and instead
configure the pad and route the I2S SD_OUT through the GPIO matrix for the
actual pin. Replace the PIN_FUNC_SELECT call with a generic pad setup (e.g.,
gpio_pad_select_gpio(pdm_speak_n) or equivalent) and use gpio_matrix_out (or
esp_rom_gpio_connect_out_signal with the pin's matrix mapping) to connect
I2SO_SD_OUT_IDX to pdm_speak_n, then keep gpio_set_direction(pdm_speak_n,
GPIO_MODE_OUTPUT) and the esp_rom_gpio_connect_out_signal/gpio_matrix_out call
using pdm_speak_n so no IO_MUX_GPIO10_REG is mutated. Ensure you update the code
paths referencing PIN_FUNC_SELECT, IO_MUX_GPIO10_REG,
esp_rom_gpio_connect_out_signal, and I2SO_SD_OUT_IDX accordingly.
Comment on lines
+131
to
+143
| AdcPdmAudioCodec::~AdcPdmAudioCodec() { | ||
| // 删除定时器 | ||
| if (output_timer_) { | ||
| esp_timer_stop(output_timer_); | ||
| esp_timer_delete(output_timer_); | ||
| output_timer_ = nullptr; | ||
| } | ||
|
|
||
| ESP_ERROR_CHECK(esp_codec_dev_close(output_dev_)); | ||
| esp_codec_dev_delete(output_dev_); | ||
| ESP_ERROR_CHECK(esp_codec_dev_close(input_dev_)); | ||
| esp_codec_dev_delete(input_dev_); | ||
| } |
There was a problem hiding this comment.
Release tx_handle_ during teardown.
The constructor allocates and enables an I2S channel, but the destructor never disables or deletes it. Recreating this codec leaks the channel and can make the next init fail.
♻️ Minimal cleanup
AdcPdmAudioCodec::~AdcPdmAudioCodec() {
// 删除定时器
if (output_timer_) {
esp_timer_stop(output_timer_);
esp_timer_delete(output_timer_);
output_timer_ = nullptr;
}
+
+ if (tx_handle_) {
+ ESP_ERROR_CHECK_WITHOUT_ABORT(i2s_channel_disable(tx_handle_));
+ ESP_ERROR_CHECK_WITHOUT_ABORT(i2s_del_channel(tx_handle_));
+ tx_handle_ = nullptr;
+ }
ESP_ERROR_CHECK(esp_codec_dev_close(output_dev_));
esp_codec_dev_delete(output_dev_);
ESP_ERROR_CHECK(esp_codec_dev_close(input_dev_));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| AdcPdmAudioCodec::~AdcPdmAudioCodec() { | |
| // 删除定时器 | |
| if (output_timer_) { | |
| esp_timer_stop(output_timer_); | |
| esp_timer_delete(output_timer_); | |
| output_timer_ = nullptr; | |
| } | |
| ESP_ERROR_CHECK(esp_codec_dev_close(output_dev_)); | |
| esp_codec_dev_delete(output_dev_); | |
| ESP_ERROR_CHECK(esp_codec_dev_close(input_dev_)); | |
| esp_codec_dev_delete(input_dev_); | |
| } | |
| AdcPdmAudioCodec::~AdcPdmAudioCodec() { | |
| // 删除定时器 | |
| if (output_timer_) { | |
| esp_timer_stop(output_timer_); | |
| esp_timer_delete(output_timer_); | |
| output_timer_ = nullptr; | |
| } | |
| if (tx_handle_) { | |
| ESP_ERROR_CHECK_WITHOUT_ABORT(i2s_channel_disable(tx_handle_)); | |
| ESP_ERROR_CHECK_WITHOUT_ABORT(i2s_del_channel(tx_handle_)); | |
| tx_handle_ = nullptr; | |
| } | |
| ESP_ERROR_CHECK(esp_codec_dev_close(output_dev_)); | |
| esp_codec_dev_delete(output_dev_); | |
| ESP_ERROR_CHECK(esp_codec_dev_close(input_dev_)); | |
| esp_codec_dev_delete(input_dev_); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/audio/ns4150_player/test/adc_pdm_audio_codec.cc` around lines 131 -
143, Destructor AdcPdmAudioCodec::~AdcPdmAudioCodec currently never releases the
I2S transmit channel (tx_handle_), causing a resource leak; update the
destructor to check tx_handle_ and call the proper I2S cleanup sequence (disable
then delete) — e.g., call i2s_channel_disable(tx_handle_) followed by
i2s_del_channel(tx_handle_) (or the equivalent platform APIs used in this
project), then set tx_handle_ = nullptr — perform this before/around the
existing codec device close/delete calls to ensure the channel is fully released
when tearing down the codec.
Comment on lines
+219
to
+228
| int AdcPdmAudioCodec::Write(const int16_t* data, int samples) { | ||
| if (output_enabled_) { | ||
| ESP_ERROR_CHECK_WITHOUT_ABORT(esp_codec_dev_write(output_dev_, (void*)data, samples * sizeof(int16_t))); | ||
| // 重置输出定时器 | ||
| if (output_timer_) { | ||
| esp_timer_stop(output_timer_); | ||
| esp_timer_start_once(output_timer_, TIMER_TIMEOUT_US); | ||
| } | ||
| } | ||
| return samples; |
There was a problem hiding this comment.
The idle shutdown path never recovers.
After OutputTimerCallback() calls EnableOutput(false), Write() skips the sink whenever output_enabled_ is false and still returns success. Unless some external caller explicitly re-enables output first, the first idle timeout drops all subsequent playback.
Also applies to: 245-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/audio/ns4150_player/test/adc_pdm_audio_codec.cc` around lines 219 -
228, AdcPdmAudioCodec::Write currently returns success and drops samples when
output_enabled_ is false (set by OutputTimerCallback via EnableOutput(false)),
which causes playback to never recover; change Write so that when
output_enabled_ is false it re-enables the output path before accepting data:
call EnableOutput(true) (and restart/arm output_timer_ as needed) when new data
arrives, then perform the esp_codec_dev_write and timer reset logic;
alternatively return 0/error to force caller to re-enable, but prefer automatic
re-enable in Write to restore playback automatically; reference:
AdcPdmAudioCodec::Write, OutputTimerCallback, EnableOutput, output_enabled_,
output_timer_.
| #include <esp_lcd_panel_vendor.h> | ||
| #include <esp_lcd_panel_io.h> | ||
| #include <esp_lcd_panel_ops.h> | ||
| #include "esp_lcd_ili9341.h" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find examples/audio/ns4150_player -type f \( -name "*.h" -o -name "*.cc" -o -name "*.cpp" \) | head -20Repository: YanKE01/esp_demos
Length of output: 272
🏁 Script executed:
cat -n examples/audio/ns4150_player/test/config.hRepository: YanKE01/esp_demos
Length of output: 1552
🏁 Script executed:
wc -l examples/audio/ns4150_player/test/esp-sensairshuttle.ccRepository: YanKE01/esp_demos
Length of output: 120
🏁 Script executed:
cat -n examples/audio/ns4150_player/test/esp-sensairshuttle.ccRepository: YanKE01/esp_demos
Length of output: 12503
🏁 Script executed:
rg "LCD_TYPE_ST7789_SERIAL|LCD_TYPE_ILI9341" --type-list | head -5Repository: YanKE01/esp_demos
Length of output: 450
🏁 Script executed:
rg "LCD_TYPE_ST7789_SERIAL|LCD_TYPE_ILI9341" examples/audio/ns4150_player/Repository: YanKE01/esp_demos
Length of output: 134
🏁 Script executed:
find examples/audio/ns4150_player -name "lcd_display.h" -o -name "lcd_display.cc" | xargs ls -la 2>/dev/nullRepository: YanKE01/esp_demos
Length of output: 848
🏁 Script executed:
find examples/audio/ns4150_player -type d -name displayRepository: YanKE01/esp_demos
Length of output: 43
🏁 Script executed:
ls -la examples/audio/ns4150_player/test/Repository: YanKE01/esp_demos
Length of output: 512
🏁 Script executed:
rg "#include \"display/lcd_display.h\"" examples/audio/ns4150_player/Repository: YanKE01/esp_demos
Length of output: 149
🏁 Script executed:
find examples/audio/ns4150_player -name "lcd_display.h"Repository: YanKE01/esp_demos
Length of output: 43
🏁 Script executed:
find . -name "lcd_display.h" 2>/dev/nullRepository: YanKE01/esp_demos
Length of output: 43
🏁 Script executed:
find components -type f -name "*.h" | grep -i display | head -20Repository: YanKE01/esp_demos
Length of output: 43
🏁 Script executed:
ls -la components/Repository: YanKE01/esp_demos
Length of output: 280
🏁 Script executed:
ls -la components/st7789/ && echo "---" && ls -la components/ssd1306/Repository: YanKE01/esp_demos
Length of output: 646
🏁 Script executed:
ls -la components/st7789/include/ && echo "---" && cat components/st7789/include/*.hRepository: YanKE01/esp_demos
Length of output: 1302
The LCD driver selection disagrees with the board definition.
config.h defines LCD_TYPE_ST7789_SERIAL, but this file unconditionally includes and uses the ILI9341 driver (esp_lcd_ili9341.h, ili9341_lcd_init_cmd_t, esp_lcd_new_panel_ili9341()). The init commands at lines 35–68 are ILI9341-specific. Either the hardware is ST7789 and the driver selection is wrong, or the hardware is ILI9341 and the config definition should be corrected.
Also applies to: 35-35, 254-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/audio/ns4150_player/test/esp-sensairshuttle.cc` at line 18, The code
includes and instantiates ILI9341-specific types and functions
(esp_lcd_ili9341.h, ili9341_lcd_init_cmd_t, esp_lcd_new_panel_ili9341()) while
config.h sets LCD_TYPE_ST7789_SERIAL; make these consistent by either switching
this file to use the ST7789 driver (replace esp_lcd_ili9341.h and
ILI9341-specific init commands with the ST7789 equivalents and call
esp_lcd_new_panel_st7789()) or update config.h to LCD_TYPE_ILI9341 and keep the
current ILI9341 setup; also ensure the panel init command array and any
driver-specific calls match the chosen driver (ILI9341 vs ST7789) including any
init struct/type names and panel creation function.
Comment on lines
+206
to
+210
| void InitializeCst816dTouchPad() | ||
| { | ||
| cst816d_ = new Cst816d(i2c_bus_, 0x15); | ||
| xTaskCreate(touch_event_task, "touch_task", 2 * 1024, cst816d_, 5, NULL); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the file to understand the code context
find . -name "esp-sensairshuttle.cc" -path "*/ns4150_player/*"Repository: YanKE01/esp_demos
Length of output: 118
🏁 Script executed:
# Read the file to see the context around the mentioned lines
cat -n ./examples/audio/ns4150_player/test/esp-sensairshuttle.cc | head -300Repository: YanKE01/esp_demos
Length of output: 12179
Check return value of xTaskCreate() and defer task creation until board construction completes.
xTaskCreate() is called during constructor at line 286 before InitializeButtons(), InitializeSpi(), and InitializeLcdDisplay() finish. Without checking the return value, task creation failures are silently ignored, which can cause touch handling to not start on low-memory boot paths. Additionally, if the task accesses any board state during startup (via Application or Board singletons), it may observe an incompletely initialized board.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/audio/ns4150_player/test/esp-sensairshuttle.cc` around lines 206 -
210, The InitializeCst816dTouchPad function calls xTaskCreate(touch_event_task,
...) during construction (creating Cst816d via cst816d_) and doesn't check the
return value or defer startup; change this by moving xTaskCreate out of the
constructor/InitializeCst816dTouchPad so the task is started only after board
construction (after InitializeButtons/InitializeSpi/InitializeLcdDisplay)
completes, and when you do create the task check xTaskCreate's return (or use
xTaskCreateStatic/xTaskCreatePinnedToCore as appropriate) and handle failure
(log error/cleanup/retry) to avoid silent failures; ensure touch_event_task only
accesses fully-initialized Cst816d/Application/Board singletons or pass a safe
initialized context pointer to the task.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes