Skip to content

Fix redundantly setting i2s_format #18

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chmorgan merged 1 commit into from
May 17, 2025
Merged

Fix redundantly setting i2s_format #18

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 16 additions & 15 deletions audio_player.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ typedef struct audio_instance {
HMP3Decoder mp3_decoder;
mp3_instance mp3_data;
#endif

format i2s_format; // last configured i2s format
} audio_instance_t;

static audio_instance_t instance;
Expand Down Expand Up @@ -238,9 +240,6 @@ static esp_err_t aplay_file(audio_instance_t *i, FILE *fp)
{
LOGI_1("start to decode");

format i2s_format;
memset(&i2s_format, 0, sizeof(i2s_format));

esp_err_t ret = ESP_OK;
audio_player_event_t audio_event = { .type = AUDIO_PLAYER_REQUEST_NONE, .fp = NULL };

Expand Down Expand Up @@ -359,17 +358,17 @@ static esp_err_t aplay_file(audio_instance_t *i, FILE *fp)
}

/* Configure I2S clock if the output format changed */
if ((i2s_format.sample_rate != i->output.fmt.sample_rate) ||
(i2s_format.channels != i->output.fmt.channels) ||
(i2s_format.bits_per_sample != i->output.fmt.bits_per_sample)) {
i2s_format = i->output.fmt;
LOGI_1("format change: sr=%d, bit=%d, ch=%d",
i2s_format.sample_rate,
i2s_format.bits_per_sample,
i2s_format.channels);
i2s_slot_mode_t channel_setting = (i2s_format.channels == 1) ? I2S_SLOT_MODE_MONO : I2S_SLOT_MODE_STEREO;
ret = i->config.clk_set_fn(i2s_format.sample_rate,
i2s_format.bits_per_sample,
if ((instance.i2s_format.sample_rate != i->output.fmt.sample_rate) ||
(instance.i2s_format.channels != i->output.fmt.channels) ||
(instance.i2s_format.bits_per_sample != i->output.fmt.bits_per_sample)) {
instance.i2s_format = i->output.fmt;
LOGI_1("format change: sr=%d, bit=%lu, ch=%lu",
instance.i2s_format.sample_rate,
instance.i2s_format.bits_per_sample,
instance.i2s_format.channels);
i2s_slot_mode_t channel_setting = (instance.i2s_format.channels == 1) ? I2S_SLOT_MODE_MONO : I2S_SLOT_MODE_STEREO;
ret = i->config.clk_set_fn(instance.i2s_format.sample_rate,
instance.i2s_format.bits_per_sample,
Comment on lines +361 to +371

Copilot AI May 17, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] For consistency and future support of multiple instances, consider using i->i2s_format instead of the global instance inside this function.

Suggested change
if ((instance.i2s_format.sample_rate != i->output.fmt.sample_rate) ||
(instance.i2s_format.channels != i->output.fmt.channels) ||
(instance.i2s_format.bits_per_sample != i->output.fmt.bits_per_sample)) {
instance.i2s_format = i->output.fmt;
LOGI_1("format change: sr=%d, bit=%lu, ch=%lu",
instance.i2s_format.sample_rate,
instance.i2s_format.bits_per_sample,
instance.i2s_format.channels);
i2s_slot_mode_t channel_setting = (instance.i2s_format.channels == 1) ? I2S_SLOT_MODE_MONO : I2S_SLOT_MODE_STEREO;
ret = i->config.clk_set_fn(instance.i2s_format.sample_rate,
instance.i2s_format.bits_per_sample,
if ((i->i2s_format.sample_rate != i->output.fmt.sample_rate) ||
(i->i2s_format.channels != i->output.fmt.channels) ||
(i->i2s_format.bits_per_sample != i->output.fmt.bits_per_sample)) {
i->i2s_format = i->output.fmt;
LOGI_1("format change: sr=%d, bit=%lu, ch=%lu",
i->i2s_format.sample_rate,
i->i2s_format.bits_per_sample,
i->i2s_format.channels);
i2s_slot_mode_t channel_setting = (i->i2s_format.channels == 1) ? I2S_SLOT_MODE_MONO : I2S_SLOT_MODE_STEREO;
ret = i->config.clk_set_fn(i->i2s_format.sample_rate,
i->i2s_format.bits_per_sample,

Copilot uses AI. Check for mistakes.

Copilot AI May 17, 2025

Copy link

Choose a reason for hiding this comment

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

This still references the removed local i2s_format. Update it to instance.i2s_format.bits_per_sample (or i->i2s_format.bits_per_sample if following suggestion above) so the correct field is used.

Copilot uses AI. Check for mistakes.
channel_setting);
ESP_GOTO_ON_ERROR(ret, clean_up, TAG, "i2s_set_clk");
}
Expand All @@ -381,7 +380,7 @@ static esp_err_t aplay_file(audio_instance_t *i, FILE *fp)
* to ensure playback without interruption.
*/
size_t i2s_bytes_written = 0;
size_t bytes_to_write = i->output.frame_count * i->output.fmt.channels * (i2s_format.bits_per_sample / 8);
size_t bytes_to_write = i->output.frame_count * i->output.fmt.channels * (instance.i2s_format.bits_per_sample / 8);
LOGI_2("c %d, bps %d, bytes %d, frame_count %d",
i->output.fmt.channels,
i2s_format.bits_per_sample,
Expand Down Expand Up @@ -558,6 +557,8 @@ esp_err_t audio_player_new(audio_player_config_t config)
TAG, "Failed create MP3 decoder");
#endif

memset(&instance.i2s_format, 0, sizeof(instance.i2s_format));

instance.running = true;
task_val = xTaskCreatePinnedToCore(
(TaskFunction_t) audio_task,
Expand Down