-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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.
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
[I2S] removes code forcing two slots in PCM Short Slot (IDFGH-14064) #14879
Conversation
👋 Hello gl-agnx, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
Thanks for looking into this issue! And you've caught the exact buggy line. But removing the limitation directly will affect the Philips and MSB format, and might cause some breaking changes, so I suggest to add one more conditions so that we can only allow one slot in PCM SHORT format. (See suggestions as follows) |
@@ -104,8 +104,7 @@ static esp_err_t i2s_tdm_set_slot(i2s_chan_handle_t handle, const i2s_tdm_slot_c | |||
handle->active_slot = slot_cfg->slot_mode == I2S_SLOT_MODE_MONO ? 1 : __builtin_popcount(slot_cfg->slot_mask); | |||
uint32_t max_slot_num = 32 - __builtin_clz(slot_cfg->slot_mask); | |||
handle->total_slot = slot_cfg->total_slot < max_slot_num ? max_slot_num : slot_cfg->total_slot; | |||
handle->total_slot = handle->total_slot < 2 ? 2 : handle->total_slot; // At least two slots in a frame |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handle->total_slot = (handle->total_slot < 2) && (slot_cfg->ws_width != 1) ? 2 : handle->total_slot;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar here with adding ()
around the conditions.
@@ -318,8 +318,7 @@ void i2s_hal_tdm_set_tx_slot(i2s_hal_context_t *hal, bool is_slave, const i2s_ha | |||
uint32_t msk = slot_cfg->tdm.slot_mask; | |||
/* Get the maximum slot number */ | |||
cnt = 32 - __builtin_clz(msk); | |||
/* There should be at least 2 slots in total even for mono mode */ | |||
cnt = cnt < 2 ? 2 : cnt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cnt = (cnt < 2) && (slot_cfg->tdm.ws_width != 1) ? 2 : cnt;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest:
cnt = ((cnt < 2) && (slot_cfg->tdm.ws_width != 1)) ? 2 : cnt;
as depending on the compiler optimization it may end up assigning zero to cnt
if the first piece (cnt < 2)
is evaluated first and shortcircuits the remaining operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you help to update the commit?
sha=3e0e2b1e75934d74c7b30574e57033ffc99a672e |
fix(i2s): fixed the issue in PR 14879 Closes #14879 [Kevin: Update to only remove the limitation for PCM short format]
Closes #14879 [Kevin: Update to only remove the limitation for PCM short format]
Description
Changes the i2s_hal and i2s_tdm configuration to not force two slots when only one slot is required.
This issue prevents a configuration of I2S_STD_SLOT_LEFT and I2S_SLOT_MODE_MONO.
If two slots are required, then I2S_STD_SLOT_BOTH could be specified by the use
This change was made because it was not possible to generate a 16-bit PCM output that's mono with only one slot, which is the case in some configurations
Explanation:
I2S_SLOT_MODE_MONO forces data to repeat across all slots. By definition, I2S_STD_SLOT_LEFT, I2S_STD_SLOT_RIGHT, or I2S_STD_SLOT_BOTH define whether we enable the left, right or both slots with 1 bit encoding.
However, requiring always at least 2 slots removes the ability to send out a single slot and makes I2S_STD_SLOT_BOTH useless.
The current code was enforcing the requirement of 2 slots at a low level for some reason in mono.
Related
None
Testing
Tested on ESP32-S3-DevKitC with a logic analyzer and can see properly that only a single slot is sent when configuring I2S_STD_SLOT_LEFT. Tested with I2S_STD_SLOT_BOTH and can see that both slots are being sent, as was the case before this cahnge.
Configuration example:
Checklist
Before submitting a Pull Request, please ensure the following: