Skip to content

Free memory we allocated and actually stop i2s. #3191

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

Merged
merged 2 commits into from
May 8, 2017

Conversation

pablo-mendoza
Copy link
Contributor

i2s_end was leaking memory and wasn't actually stopping i2s.

@codecov-io
Copy link

codecov-io commented May 2, 2017

Codecov Report

Merging #3191 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3191   +/-   ##
======================================
  Coverage    27.6%   27.6%           
======================================
  Files          20      20           
  Lines        3655    3655           
  Branches      678     678           
======================================
  Hits         1009    1009           
  Misses       2468    2468           
  Partials      178     178

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca3a172...c8cba5f. Read the comment docs.

@pablo-mendoza
Copy link
Contributor Author

pablo-mendoza commented May 3, 2017

I put in this patch, but at some point the i2s support should probably change a lot. There are actually for modes that can be made to work:

i2s output master mode (this is what the current code supports only).
i2s output slave mode.
i2s input master mode.
i2s input slave mode (I have this working on my machine with a teensy audio board (SGTL500)).

  • I2s input and output can be working at the same time.
  • You can set the clock rate in master mode, but it doesn't make sense in slave mode.
  • It would be very nice to be able to set the size of the DMA buffers and the amount of buffers depending on your usage, for example if you are reading/saving to SD cards 512 bytes makes sense as the buffer size. But if you are pushing to the network you may want bigger packages (about MTU size I guess would maximize throughput).

The fact that ESP8266 can't output a master clock that is a multiple of the sample rate is unfortunate, that's why I had to put the SGTL5000 in master mode (ESP8266 in slave mode) so that it can generate its own clocks (I'm feeding the 26Mhz XTAL_CLOCK from pin0 to it just to make it go).

I haven't looked into much detail into the ESP32 API, but if it already supports all of this, it would be something nice to emulate.

Anyway, I should probably open an issue with all of this, instead of piggy backing this into this simple fix.

@igrr igrr merged commit db4d3e0 into esp8266:master May 8, 2017
@igrr
Copy link
Member

igrr commented May 8, 2017

Thanks! Suggest opening a new issue for discussion of further I2S enhancements. CC @me-no-dev there regarding the ESP32 library.

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

Successfully merging this pull request may close these issues.

3 participants