Skip to content

Conversation

@pera
Copy link

@pera pera commented Nov 22, 2025

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).

@sletz
Copy link
Member

sletz commented Nov 22, 2025

Thanks @pera. @thopman can you have a look ?

@thopman
Copy link
Contributor

thopman commented Nov 23, 2025

Thanks for working on this @pera! I appreciate the thought behind making the code more flexible.
I have some concerns about this approach though:

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.
If someone has a custom board, they're already going to be editing the code to add their codec initialization. Adding a #elif for their board or tweaking the default is straightforward and keeps everything explicit.
The added runtime parameter creates ambiguity about where pins are actually configured and adds complexity for a scenario that doesn't really come up.
What do you think? I'm inclined to stick with the current implementation, but I'm open to discussing if there's a use case I'm missing.

@pera
Copy link
Author

pera commented Nov 24, 2025

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:

  1. Put FUNC_GPIO0_CLK_OUT1 (soc/esp32/register/soc/io_mux_reg.h) behind a flag: Without this change faust2esp32 output can only be compiled for ESP32 target chip (the name is confusing so just to be clear, this is not referring to the family but the original ESP32). For example, my target ESP32S3 doesn't compile because this define doesn't exist in soc/esp32s3/register/soc/io_mux_reg.h. Maybe this could be configurable but I don't believe it's strictly necessary.
  2. Regarding the possibility of pin configuration being overwritten, this is a fair concern and I do agree that maybe the proposed change is not the best option but I do believe this should be configurable and also that the current behaviour is confusing.
  3. I think part of the issue is that faust2esp32 defaults to WM8978 / TTGO_TAUDIO hardcoding its pinout parameters. As a new user this was a bit confusing for me (my guess is that the faust2esp32 started with this platform as the only target and then added a few other ones but wanted to keep backward compatibility, is this correct?).

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.

@thopman
Copy link
Contributor

thopman commented Nov 28, 2025

Thanks for clarifying @pera.
Point 1 (ESP32-S3 compatibility): The FUNC_GPIO0_CLK_OUT1 flag fix is solid and needed. This should definitely be merged.
Point 2 & 3 (Pin configuration approach): I don't think the runtime configuration approach works here, and I don't think Kconfig/sdkconfig is the right solution either.

Why runtime configuration doesn't work:
Hardware pin assignments are fundamentally a compile-time concern. When you build firmware for an ESP32 board, the physical connections between the ESP32 and your codec are fixed - they're literally soldered or wired on the PCB. These don't change during program execution. Runtime parameters are for things that actually vary at runtime (sample rates, buffer sizes, user settings, etc.), not for hardware topology.
The current implementation also has a logic bug where the pin_config parameter gets immediately overwritten by the #if blocks, making it non-functional. But even if that were fixed, you'd have ambiguity: are pins set by the parameter, or by the preprocessor defines? This creates confusion about what's actually configured.

Why Kconfig/sdkconfig isn't better:
Moving the configuration to Kconfig/sdkconfig doesn't solve the fundamental issue - it just relocates it. You'd still have the same problem of managing multiple board configurations, but now split across different files and systems. Preprocessor defines keep hardware configuration explicit and localized in the code where it's used. When someone reads esp32-dsp.h, they can immediately see what pins are configured for what boards. With Kconfig, that information is scattered.
Additionally, Kconfig adds build system complexity. For projects using Faust, the simpler the integration the better. Preprocessor defines are straightforward - set a flag, get the right pins. No additional build configuration needed.

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.
or:
A PCM510X board define that works across ESP32 variants (ESP32, ESP32-S3, etc.)

Something like:

    #elif PCM510X
        pin_config = {
            .mck_io_num = I2S_PIN_NO_CHANGE,
            .bck_io_num = 27,
            .ws_io_num = 26,
            .data_out_num = 25,
            .data_in_num = 35
        };

My suggestion:
If you want to submit just the ESP32-S3 flag fix as a separate PR, that's immediately mergeable. Adding a PCM510X board define would also be a clean addition. Setting aside the implementation bugs, I think the runtime configuration pattern itself adds complexity without solving the actual problem.

@pera
Copy link
Author

pera commented Nov 30, 2025

Thanks very much for your thoughts @thopman, I have created a new PR for the FUNC_GPIO0_CLK_OUT1: #1188

Regarding the PIN defaults:

  • The reason I suggested Kconfig + sdkconfig.defaults was because I wrongly believed Faust already used it. A similar alternative is having those values as macro definitions and then allow users to use CMake's add_compile_definitions, for example. What do you think?
  • I have the feeling we are using Faust in different ways: I installed Faust through my Linux distribution official package manager repository. Faust is officially available in Arch Linux, Debian, Ubuntu and provably other distros. I am not aware of a nice way to configure GPIO PINs for such install (i.e. without having to either modify system wide Faust files or having to manually patch output code). I imagine maybe you are working on a fork and making changes for your specific targets, so it's not a big deal for you to hardcode a few values, but I think this can create significant friction for users who installed Faust through their Linux distro package manager, as it force them to be familiar with Faust's codebase and also having to maintain these changes each time the package is updated.
  • If you still prefer to use hardcoded PIN configuration the only thing I would ask if I may is to use lower GPIO numbers given that ESP32-S3 is common in very small form factors (e.g. Seeed Studio XIAOs have only 14 PINs).

@pera pera marked this pull request as draft November 30, 2025 17:47
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