Skip to content
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

Added audioio to espressif #9995

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

MarshallMiller
Copy link

@MarshallMiller MarshallMiller commented Jan 25, 2025

Added audioio support for the one esp32 board that I have (adafruit_feather_esp32_v2) but I believe it will work with other espressif boards. This is my first contribution to CircuitPython so I'm sure there will be lots of little things that I did wrong. Resolves #3898.

Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thank you! This looks amazing for a first submission. I took a look at the code, but didn't test anything. I have a few small comments.

ports/espressif/common-hal/audioio/AudioOut.c Outdated Show resolved Hide resolved
ports/espressif/common-hal/audioio/AudioOut.c Show resolved Hide resolved
Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the changes I requested.

@MarshallMiller MarshallMiller force-pushed the espressif-audioio-support branch from 751f316 to 4bbc390 Compare January 26, 2025 17:26
@MarshallMiller MarshallMiller force-pushed the espressif-audioio-support branch from 4bbc390 to 6f4f57e Compare January 26, 2025 17:27
@MarshallMiller
Copy link
Author

MarshallMiller commented Jan 26, 2025

rebased onto main and squashed the commits

Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thank you! This looks close, but I have a few more changes to request.

ports/espressif/boards/adafruit_feather_esp32_v2/pins.c Outdated Show resolved Hide resolved
ports/espressif/common-hal/audioio/AudioOut.c Outdated Show resolved Hide resolved
ports/espressif/common-hal/audioio/AudioOut.c Show resolved Hide resolved
jepler
jepler previously approved these changes Jan 27, 2025
Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good now. I still haven't tested it.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Thank you for doing this! I tested on a Metro ESP32-S2, with some 16k and 22050 wav files. They worked fine. One file had pretty low audio level, and I heard significant noise. I'm not sure if that's leakage or the relatively poor performance of the DAC. I tried the same on a Metro M4 with AudioOut, and it was quieter.

I also tried with an audiomp3.MP3Decoder source. That caused a Hard Fault into safe mode. The audio file is attached (zipped up)
16000.mp3.zip.
Here is the test program:

import time
import board
import digitalio
from audiomp3 import MP3Decoder

from audioio import AudioOut

mp3 = MP3Decoder("16000.mp3")
audio = AudioOut(left_channel=board.A0, right_channel=board.A1)

while True:
    audio.play(mp3)
    while audio.playing:
        pass
    time.sleep(1)

By contrast, if I changed the above program to use audiobusio, I did not get a crash:

from audiobusio import I2SOut

mp3 = MP3Decoder("16000.mp3")
audio = I2SOut(bit_clock=board.A0, word_select=board.A1, data=board.A2)

So there some issue about taking an MP3 stream as input to the AudioOut. I did not test on an plain ESP32, only on ESP32-S2.

@jepler
Copy link
Member

jepler commented Jan 28, 2025

This may be hitting assertions about audio buffer sizes. It looks like there's a limitation of 512 bytes of audio data (#define DMA_BUFFER_SIZE 512 ... uint8_t scratch_buffer[DMA_BUFFER_SIZE];) but mp3 decoding produces audio data in larger amounts.

If that's the case, the problem might also reproduce with, say, a RawSample with 4096 samples of data.

@MarshallMiller
Copy link
Author

Interesting. I'll take a look.

@MarshallMiller MarshallMiller force-pushed the espressif-audioio-support branch from 8a31465 to 90a040b Compare February 2, 2025 04:07
@MarshallMiller
Copy link
Author

This latest patch switches to dynamically allocating the scratch buffer and also corrects some other problems that I found. One was that the audio quality is very bad in stereo mode with the APLL clock so I switch to using the DEFAULT clock for sample rates over 19600. The DEFAULT clock cannot be used for sample rates lower than 19600. I considered adding logic to scale up sample rates below 19600 by just duplicating the samples 2^n times but I'm not sure if that is necessary. There was also some ordering logic flaws with dequeuing the dma buffer from the ring buffer and with deciding when to reset the sample buffer or stop playing when the samples were finished being read.

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.

Implement DAC-based audio (audioio) on Espressif
4 participants