Fix redundantly setting i2s_format#18
Merged
chmorgan merged 1 commit intochmorgan:mainfrom May 17, 2025
Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR prevents unnecessary reconfiguration of the I2S format on each audio playback by storing the last-used format in the audio_instance_t context instead of a temporary variable.
- Added
i2s_formatfield toaudio_instance_t - Removed local
i2s_formatand replaced its uses with the context field - Zero-initialized
instance.i2s_formatinaudio_player_new
Comments suppressed due to low confidence (1)
audio_player.cpp:365
- The format specifiers
%lumay not match the type ofbits_per_sampleandchannels. Verify their types and use%dor%zuaccordingly to avoid logging mismatches.
LOGI_1("format change: sr=%d, bit=%lu, ch=%lu",
Comment on lines
+361
to
+371
| 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, |
There was a problem hiding this comment.
[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, |
| 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, |
There was a problem hiding this comment.
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.
Owner
|
Thank you for your contribution! |
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
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.
The i2s format configuration was being set upon every call to play audio regardless of an actual format change because the variable being used to track and compare against was always new.
By moving the tracking variable to the audio instance context, we can safely compare and set when necessary.