Skip to content

I2s experimental #2501

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

Closed
wants to merge 7 commits into from
Closed

I2s experimental #2501

wants to merge 7 commits into from

Conversation

Palatis
Copy link
Contributor

@Palatis Palatis commented Sep 11, 2016

various fixes to the i2s subsystem.

  1. add missing convenient function (i2s_write_lr_nb())
  2. fix memory leak in i2s_slc_begin()
  3. sample rate handling refactored

we `malloc()` memories in `i2s_slc_begin()`, but never `free()` them.
so the cpu runs out of memory after several `i2s_begin(); i2s_end();`

so we add a `i2s_init()` to do the `malloc()` and `i2s_deinit()` to
`free()` those allocated buffers.
- do a search to find the closest divider for optimal performance.
- use the current cpu clock to get the correct sample rate.
- only set the sample rate if it's changed
@codecov-io
Copy link

codecov-io commented Sep 11, 2016

Current coverage is 27.80% (diff: 100%)

Merging #2501 into master will not change coverage

@@             master      #2501   diff @@
==========================================
  Files            20         20          
  Lines          3625       3625          
  Methods         335        335          
  Messages          0          0          
  Branches        656        656          
==========================================
  Hits           1008       1008          
  Misses         2441       2441          
  Partials        176        176          

Powered by Codecov. Last update 4897e00...6a3a77c

return the underlying buffer used by I2S SLC DMA subsystem, so
client code can write data directly into it, instead of calling
i2s_write_sample() / i2s_write_sample_nb().

greatly boost the performance, and reduce memory footprint when
using an audio codec. because the codec can now decode the stream
directly into the buffer, instead of cache them and doing
i2s_write_sample{,_nb}().
@@ -194,46 +219,94 @@ bool ICACHE_FLASH_ATTR i2s_write_lr(int16_t left, int16_t right){
return i2s_write_sample(sample);
}

bool ICACHE_FLASH_ATTR i2s_write_lr_nb(int16_t left, int16_t right){
int sample = right & 0xFFFF;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why signed integer here and not uint32_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i2s_write_lr() does use int32_t, I copied the code and only changed that i2s_write_sample(sample); to i2s_write_sample_nb(sample);

@Palatis
Copy link
Contributor Author

Palatis commented Sep 13, 2016

the clock divider guessing code is copied from mp3 example.
it use 160Mhz statically.

the modifications I made are reading runtime clock, and do not allow bit mode (like sending with 19 bit or 20 bit per sample, and let the DAC output at a lower volume.)

I'm still waiting for my hw college to provide me with a PCB with a working i2s DAC and amp, tho.

i2s_write_lr() takes two int16_t, so I think the non-blocking version should take signed as well.

@Palatis
Copy link
Contributor Author

Palatis commented Sep 13, 2016

as for the i2s base clock, I confirmed that the samples are pushed out at almost correct rate, such as RATE_HZ samples pushed out around ~1015ms.

I think some rounding error and dma buffer consume speed and maybe some random weirdness are acceptable......

@me-no-dev
Copy link
Collaborator

I have also started writing this lib from the mp3 example ;) did many calculations and came up that the formula I used was the fastest and easiest option, but please prove me wrong :) we are people and make mistakes.
your left and right channels are signed 16 bit values, but both combined, it's unsigned 32 bit value ;)
do not remember if I run the tests at both 80 and 160 MHz, but I would think that I have and that is why I asked about the base clock in your calculations

@Palatis
Copy link
Contributor Author

Palatis commented Sep 14, 2016

I will provide a easy sketch to test the sample rate sometime later.
Not at home right now.

the i2s buffer was fixed to 8*64 uint32_t, this may be inefficient
for some codecs.
by allowing the user to adjust the buffer size, the codec can now
decode the whole window directly into the acquired buffer, thus
avoid a lot of memory copy.
@earlephilhower
Copy link
Collaborator

Hi @Palatis I see this has been sitting in the backlog for two years, ouch. Are you still interested in getting this patch into 2.5 or 2.6 of the Arduino SDK? There was a massive cleanup this past release and we're trying to be more methodical in getting these PRs reviewed and accepted in a reasonable process and time.

If you're still interested, please update the conflicts and I'll do a review ASAP.

Thx
-EFP3

@Palatis
Copy link
Contributor Author

Palatis commented Jan 16, 2018

i'm still willing to help, but i've shifted onto some other things since then.
I'm currently working in Shenzhen, but all my esp8266 (I'm referring to both hw and sw...) are at home in Taipei...

@earlephilhower
Copy link
Collaborator

No worries, don't want to put you out. We can shelve this for now and if you ever want to get back to it we can re-open it. Sound OK?

@Crypter
Copy link
Contributor

Crypter commented Jan 21, 2018

I'll take a look in the following days, there sure must be something we can use and clean up this pull request too :)

@earlephilhower earlephilhower added this to the 2.5.0 milestone Mar 9, 2018
@earlephilhower
Copy link
Collaborator

@Crypter and @Palatis, since the I2S subsystem has been rewritten to support bidirectional transfer and the method for choosing the proper clock setup for a given frequency has also been redone, I think this PR should be closed.

The PR is pretty much unmergeable now, and the only thing I'm seeing is missing is way of writing multiple samples-worth of data to the buffer (which is basically what the get/put buffer calls that were added let you do). That's a much smaller PR and should be started against the current head to preserve everyone's sanity. If I ever get time to get back to working on my ESP8266Audio library (which obviously uses the I2S extensively), adding that capability (uint32_t i2s_write_block/non(uint16_t samples[], int count));) is something I'll definitely look into as the overhead of calling the I2S add-sample call 44,000 times per second can't be small.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants