Skip to content
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

Fix for mclk/bclk divisors #8326 (IDFGH-6698) #8327

Closed
wants to merge 1 commit into from

Conversation

s-hadinger
Copy link
Contributor

This is a first tentative to #8326

bclk_div is set to 32 which is the highest power of 2 possible for this hardware register (6 bits).

bclk target is sample_rate * I2S_LL_AD_BCK_FACTOR. I removed the additional factor 2 which looks erroneous with audio output.

@s-hadinger
Copy link
Contributor Author

Tentative fix for espressif/arduino-esp32#5938

@espressif-bot espressif-bot added the Status: Opened Issue is new label Jan 31, 2022
@github-actions github-actions bot changed the title Fix for mclk/bclk divisors #8326 Fix for mclk/bclk divisors #8326 (IDFGH-6698) Jan 31, 2022
@s-hadinger
Copy link
Contributor Author

Applying this patch locally makes Audio DAC working again.

Here are the new computed values:
sclk=160000000 mclk=1024000 bclk=32000 mclk_div=156 bclk_div=32

@L-KAYA
Copy link
Collaborator

L-KAYA commented Feb 18, 2022

Many thanks for fixing the DAC clock, I have checked it and it can work. But have you test this clock setting with ADC mode? It seems can't work for ADC mode

@s-hadinger
Copy link
Contributor Author

No I haven't tested ADC mode.

@L-KAYA
Copy link
Collaborator

L-KAYA commented Feb 18, 2022

Oh sorry, don't mind ADC, it's my fault.

So now the bclk_div is the real bit_per_chan, for sclk=160000000 mclk=1024000 bclk=32000 mclk_div=156 bclk_div=32, we got 32 bit * 2 channel = 64 mclk ticks in one sample period. Do we need to fix the bclk_div to 32? Or set bclk_div equal to bit_per_chan dynamically, so that users can control this field.

FYI, while we are not using APLL, and set a low sample rate for ADC/DAC, the mclk_div will exceed its maximum value 255 (it has only 8 bit in reg), which can lead the clock go wrong.

@s-hadinger
Copy link
Contributor Author

Thanks for the feedback. I suppose that mclk_div=156 is the maximum we can set to not override the 8 bits counter, then bclk_div can indeed be used to adjust.

@L-KAYA
Copy link
Collaborator

L-KAYA commented Feb 21, 2022

Thanks for the feedback. I suppose that mclk_div=156 is the maximum we can set to not override the 8 bits counter, then bclk_div can indeed be used to adjust.

Considering sometimes DAC will work together with ADC, 8 bits is not the best solution, because ADC needs 16 bits channel width to receive 12 bits width data. I think we need to use bit_per_chan as bclk_div and add mclk_div check for it.

This issue will be fixed recently, the commit in the PR will be involved, thanks for the contribution!

espressif-bot pushed a commit that referenced this pull request Mar 3, 2022
espressif-bot pushed a commit that referenced this pull request Mar 3, 2022
@Alvin1Zhang
Copy link
Collaborator

Thanks for your contribution, changes merged with 2f0e7e9

@Alvin1Zhang Alvin1Zhang closed this Mar 8, 2022
@espressif-bot espressif-bot added Resolution: Done Issue is done internally Status: Done Issue is done internally and removed Status: Opened Issue is new labels Mar 8, 2022
@dzanis
Copy link

dzanis commented Jun 2, 2022

This fix broke the I2S ADC, had to revert the old code back

@s-hadinger
Copy link
Contributor Author

Can you give more details about the parameters you have been using?

@dzanis
Copy link

dzanis commented Jun 6, 2022

My board is ESP32-WROOM-32.
Current version IDF 4.4.1

I didn't use a DAC so I don't know how it behaves, but the ADC works with the wrong frequency to read samples. I recorded from a microphone and the sound turned out to be very slow.
My parameters for one I2S ADC channel

int _sampleRate = 16000;

i2s_config_t i2s_config = {
    .mode = (i2s_mode_t)(I2S_MODE_MASTER | I2S_MODE_RX | I2S_MODE_ADC_BUILT_IN),
    .sample_rate = _sampleRate,
    .bits_per_sample = I2S_BITS_PER_SAMPLE_16BIT,
    .channel_format = I2S_CHANNEL_FMT_ONLY_LEFT,
    .communication_format = I2S_COMM_FORMAT_STAND_I2S,
    .intr_alloc_flags = ESP_INTR_FLAG_LEVEL2,
    .dma_buf_count = 128,
    .dma_buf_len = 2,
    .use_apll = false
  };

I fixed it by returning the code with this condition to the compiler

#if ESP_IDF_VERSION > ESP_IDF_VERSION_VAL(3, 3, 5)
  #define I2S_LL_AD_BCK_FACTOR           (2)
  int bclk = _sampleRate * I2S_LL_AD_BCK_FACTOR * 2;
  int bclk_div = I2S_LL_AD_BCK_FACTOR;
  i2s_config.use_apll = true;
  i2s_config.fixed_mclk = bclk * bclk_div;
#endif

And the problem was solved, the samples are normal.
But I hope that this will be fixed, such a crutch is not very beautiful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants