Skip to content

Conversation

frak0d
Copy link
Contributor

@frak0d frak0d commented Apr 14, 2025

@earlephilhower
Copy link
Owner

That poor bug has been open 2 years now. Wouldn't expect them to be doing anything to fix it at this point. :(

How is this different from the INTERNAL_DAC mode already present in the I2S output object?

if (output_mode == INTERNAL_DAC || output_mode == INTERNAL_PDM) {
#if CONFIG_IDF_TARGET_ESP32
i2s_set_pin((i2s_port_t)portNo, NULL);
i2s_set_dac_mode(I2S_DAC_CHANNEL_BOTH_EN);
#else
return false;
#endif

@frak0d
Copy link
Contributor Author

frak0d commented Apr 14, 2025

to be frank, googling "esp8266audio esp32 s2" lead me straight to Open issue #551 , which suggests that I2S and ULP implementations are broken on esp32-s2, and only option is to use I2CNoDAC.

and since i wasn't satisfied with audio quality of I2CNoDAC, i ended up making my own implementation for internal dac.

now if issue #551 is still an issue, then this PR should fix it, but if #551 is no longer an issue then you should close it and leave a comment on it so future visitors don't end up wasting their time making another AudioOutput implementation like me.

@earlephilhower
Copy link
Owner

Ah, thanks for the explanation. The built-in DAC seems to always have been an afterthought and available only on a couple of the range.

Do you think it would make sense to just remove the INTERNAL_DAC from the I2C ESP32 wrapper? The way the I2S API has evolved on the ESP32 over the past years has really turned the code into a mess of #ifdefs to get around all the changes, and getting rid of unneeded code always feels like a win...

The astyle CI failure is related to the new formatting (so I could backport the speedups from BackgroundAudio). If you can run tools/restyle.sh on the PR that would clear it up. OTW we can probably commit here and I can run it as an update. Only issue there is git blame will then list the restyle PR for every line instead of your original new commit...

@frak0d
Copy link
Contributor Author

frak0d commented Apr 17, 2025

I tested both implementations on esp32 and esp32-s2 and this is the result:

AudioOutputI2S AudioOutputInternalDAC
esp32 ✅mono ✅stereo ✅mono ⚠️stereo
esp32-s2 ❌mono ❌stereo ✅mono ⚠️stereo

⚠️= channel swap bug in ESP-IDF upstream

I think we should keep the I2S implementation for now, at least until that channel mixing bug is solved upstream. Also not sure why, but the I2S implementation seems to have better audio quality than mine. will need some investigation.

I could workaround the channel swap bug by swapping the channels in advance, but that would be unnecessary overhead. It would be better if you could make the I2S implementation work for esp32-s2. After that is done, you could replace my Implementation in AudioOutputInternalDAC.h with the your I2S INTERNAL_DAC one and keep only the External DAC code in AudioOutputI2S.h if you want the seperation.

@earlephilhower
Copy link
Owner

Okay, that makes sense. Let's merge this here and then see about the I2S on SD+INTERNALDAC setup (I don't have an ESP32-S3 on hand, but did just order a couple)...

Thx again!

@earlephilhower earlephilhower merged commit 9dee301 into earlephilhower:master Apr 18, 2025
18 checks passed
@frak0d
Copy link
Contributor Author

frak0d commented Apr 19, 2025

ESP32-S3 doesn't have an Internal DAC, i hope you mean ESP32-S2

@earlephilhower
Copy link
Owner

Yes, just ordered esp32-s2s, not s3. Too many models! 😆

@earlephilhower
Copy link
Owner

earlephilhower commented Apr 24, 2025

Just got the S2Minis and looked into this. AudioOutputI2S via DAC can never work on the S2 because it depends on the I2S0 controller being connected to an internal I2S(lite) DAC which is only present on the original ESP32 from many years ago. So as far as I can tell, your code here is the only way to get DAC output on the S2.

https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/peripherals/i2s.html#overview-of-all-modes

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.

2 participants