-
Notifications
You must be signed in to change notification settings - Fork 365
Extend support for ESP32 targets #1187
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
base: master-dev
Are you sure you want to change the base?
Conversation
|
Thanks for working on this @pera! I appreciate the thought behind making the code more flexible. Logic issue: The pin_config parameter is being overwritten inside the constructor by the #if blocks, so custom configurations passed in won't actually be used when board defines are set. Missing default: The original #else case with working default pins (bck: 33, ws: 25, data_out: 26, data_in: 27) was removed. Now DEFAULT_I2S_PIN_CONFIG is all I2S_PIN_NO_CHANGE, which won't work for boards without a specific define. Design concern: I'm not sure this flexibility is needed in practice. Pin configuration is really a compile-time concern - you set it once for your hardware and it never changes. You're not swapping codecs at runtime. The current pattern with board defines makes it clear what pins are being used and works well for the use case. |
|
Hi @thopman thanks very much for reviewing and your comments. Given that there are a few things happening in this PR I will list them here:
Maybe a better option for point 2 could be to make GPIO pins configurable through Kconfig and sdkconfig. What do you think? My intention with this PR is to make it as easy as possible for people wanting to use an esp32-s3 together with pcm5100 or pcm5102 breakout boards, DACs that don't require codec initialisation code. I think this combination is very common because you can buy them for about £10 on many shops. |
|
Thanks for clarifying @pera. Why runtime configuration doesn't work: Why Kconfig/sdkconfig isn't better: The real problem you're solving is "I have an ESP32-S3 with a PCM5102 and the current defaults don't work for me." That's legitimate, but it's better solved with: Either editing the default in your local faust install and use that to compile your faust dsp. Something like: My suggestion: |
|
Thanks very much for your thoughts @thopman, I have created a new PR for the Regarding the PIN defaults:
|
This PR extends the current support for other ESP32 targets (e.g. ESP32-S3), allows for custom I2S pin configurations and also simplifies integration with DACs that don't require a codec driver (e.g. PCM5100).