From 725f8f1e68d09fc956aca6381537d04cfd502f02 Mon Sep 17 00:00:00 2001 From: Dan Halbert Date: Tue, 25 Mar 2025 21:43:20 -0400 Subject: [PATCH] Disallow identical AudioOut channel pins. Work around ESP-IDF ESP32-S2 bug that swaps DAC channels randomly. - Fixes #10003. Stumbled on https://github.com/espressif/esp-idf/issues/11425 bug while testing. It causes the left and right channels to be swapped randomly on play, which was very confusing. Worked around by forcing a deinit/init on each play. Also got confused because board.A0 and board.A1 are not consistently assigned to left/right channels. Added a documentation warning about this. --- .../atmel-samd/common-hal/audioio/AudioOut.c | 7 +- ports/espressif/common-hal/audioio/AudioOut.c | 76 +++++++++---------- shared-bindings/audioio/AudioOut.c | 15 +++- 3 files changed, 51 insertions(+), 47 deletions(-) diff --git a/ports/atmel-samd/common-hal/audioio/AudioOut.c b/ports/atmel-samd/common-hal/audioio/AudioOut.c index d9078a7470902..f7af5e292a719 100644 --- a/ports/atmel-samd/common-hal/audioio/AudioOut.c +++ b/ports/atmel-samd/common-hal/audioio/AudioOut.c @@ -79,6 +79,9 @@ static void ramp_value(uint16_t start, uint16_t end) { // Caller validates that pins are free. void common_hal_audioio_audioout_construct(audioio_audioout_obj_t *self, const mcu_pin_obj_t *left_channel, const mcu_pin_obj_t *right_channel, uint16_t quiescent_value) { + + // The case of left_channel == right_channel is already disallowed in shared-bindings. + #ifdef SAM_D5X_E5X bool dac_clock_enabled = hri_mclk_get_APBDMASK_DAC_bit(MCLK); #endif @@ -107,10 +110,6 @@ void common_hal_audioio_audioout_construct(audioio_audioout_obj_t *self, if (right_channel != NULL && right_channel != &pin_PA02 && right_channel != &pin_PA05) { raise_ValueError_invalid_pin_name(MP_QSTR_right_channel); } - if (right_channel == left_channel) { - mp_raise_ValueError_varg(MP_ERROR_TEXT("%q and %q must be different"), - MP_QSTR_left_channel, MP_QSTR_right_channel); - } claim_pin(left_channel); if (right_channel != NULL) { claim_pin(right_channel); diff --git a/ports/espressif/common-hal/audioio/AudioOut.c b/ports/espressif/common-hal/audioio/AudioOut.c index 8322bc3799850..6d829228e1555 100644 --- a/ports/espressif/common-hal/audioio/AudioOut.c +++ b/ports/espressif/common-hal/audioio/AudioOut.c @@ -11,7 +11,6 @@ #include "driver/dac_continuous.h" - #if defined(CONFIG_IDF_TARGET_ESP32) #define pin_CHANNEL_0 pin_GPIO25 #define pin_CHANNEL_1 pin_GPIO26 @@ -304,6 +303,32 @@ static audioout_sample_convert_func_t audioout_get_samples_convert_func( } } +static void audioio_audioout_start(audioio_audioout_obj_t *self) { + esp_err_t ret; + + self->playing = true; + self->paused = false; + + ret = dac_continuous_start_async_writing(self->handle); + if (ret != ESP_OK) { + mp_raise_RuntimeError(MP_ERROR_TEXT("Failed to start async audio")); + } +} + +static void audioio_audioout_stop(audioio_audioout_obj_t *self, bool full_stop) { + dac_continuous_stop_async_writing(self->handle); + if (full_stop) { + self->get_buffer_index = 0; + self->put_buffer_index = 0; + self->sample_buffer = NULL; + self->sample = NULL; + self->playing = false; + self->paused = false; + } else { + self->paused = true; + } +} + static bool audioout_fill_buffer(audioio_audioout_obj_t *self) { if (!self->playing) { return false; @@ -342,7 +367,7 @@ static bool audioout_fill_buffer(audioio_audioout_obj_t *self) { &raw_sample_buf, &raw_sample_buf_size); if (get_buffer_result == GET_BUFFER_ERROR) { - common_hal_audioio_audioout_stop(self); + audioio_audioout_stop(self, true); return false; } @@ -390,7 +415,7 @@ static bool audioout_fill_buffer(audioio_audioout_obj_t *self) { } else { // TODO: figure out if it is ok to call this here or do we need // to somehow wait for all of the samples to be flushed - common_hal_audioio_audioout_stop(self); + audioio_audioout_stop(self, true); return false; } } @@ -492,11 +517,8 @@ void common_hal_audioio_audioout_construct(audioio_audioout_obj_t *self, self->paused = false; self->freq_hz = DEFAULT_SAMPLE_RATE; - /* espressif has two dac channels and it can support true stereo or - * outputting the same signal to both channels (dual mono). - * if different pins are supplied for left and right then use true stereo. - * if the same pin is supplied for left and right then use dual mono. - */ + // The case of left_channel == right_channel is already disallowed in shared-bindings. + if ((left_channel_pin == &pin_CHANNEL_0 && right_channel_pin == &pin_CHANNEL_1) || (left_channel_pin == &pin_CHANNEL_1 && @@ -504,12 +526,6 @@ void common_hal_audioio_audioout_construct(audioio_audioout_obj_t *self, self->channel_mask = DAC_CHANNEL_MASK_ALL; self->num_channels = 2; self->channel_mode = DAC_CHANNEL_MODE_ALTER; - } else if ((left_channel_pin == &pin_CHANNEL_0 || - left_channel_pin == &pin_CHANNEL_1) && - right_channel_pin == left_channel_pin) { - self->channel_mask = DAC_CHANNEL_MASK_ALL; - self->num_channels = 1; - self->channel_mode = DAC_CHANNEL_MODE_SIMUL; } else if (left_channel_pin == &pin_CHANNEL_0 && right_channel_pin == NULL) { self->channel_mask = DAC_CHANNEL_MASK_CH0; @@ -550,32 +566,6 @@ void common_hal_audioio_audioout_deinit(audioio_audioout_obj_t *self) { _active_handle = NULL; } -static void audioio_audioout_start(audioio_audioout_obj_t *self) { - esp_err_t ret; - - self->playing = true; - self->paused = false; - - ret = dac_continuous_start_async_writing(self->handle); - if (ret != ESP_OK) { - mp_raise_RuntimeError(MP_ERROR_TEXT("Failed to start async audio")); - } -} - -static void audioio_audioout_stop(audioio_audioout_obj_t *self, bool full_stop) { - dac_continuous_stop_async_writing(self->handle); - if (full_stop) { - self->get_buffer_index = 0; - self->put_buffer_index = 0; - self->sample_buffer = NULL; - self->sample = NULL; - self->playing = false; - self->paused = false; - } else { - self->paused = true; - } -} - void common_hal_audioio_audioout_play(audioio_audioout_obj_t *self, mp_obj_t sample, bool loop) { @@ -597,7 +587,11 @@ void common_hal_audioio_audioout_play(audioio_audioout_obj_t *self, self->looping = loop; freq_hz = audiosample_get_sample_rate(self->sample); - if (freq_hz != self->freq_hz) { + // Workaround: always reset the DAC completely between plays, + // due to a bug that causes the left and right channels to be swapped randomly. + // See https://github.com/espressif/esp-idf/issues/11425 + // TODO: Remove the `true` when this issue is fixed. + if (true || freq_hz != self->freq_hz) { common_hal_audioio_audioout_deinit(self); self->freq_hz = freq_hz; audioout_init(self); diff --git a/shared-bindings/audioio/AudioOut.c b/shared-bindings/audioio/AudioOut.c index 1bfc51cb9ab27..82aecefa370ba 100644 --- a/shared-bindings/audioio/AudioOut.c +++ b/shared-bindings/audioio/AudioOut.c @@ -28,11 +28,16 @@ //| """Create a AudioOut object associated with the given pin(s). This allows you to //| play audio signals out on the given pin(s). //| -//| :param ~microcontroller.Pin left_channel: The pin to output the left channel to -//| :param ~microcontroller.Pin right_channel: The pin to output the right channel to +//| :param ~microcontroller.Pin left_channel: Output left channel data to this pin +//| :param ~microcontroller.Pin right_channel: Output right channel data to this pin. May be ``None``. //| :param int quiescent_value: The output value when no signal is present. Samples should start //| and end with this value to prevent audible popping. //| +//| .. note:: On ESP32 and ESP32-S2, the DAC channels are usually designated +//| as ``DAC_1`` (right stereo channel) and DAC_2 (left stereo channel). +//| These pins are sometimes labelled as ``A0`` and ``A1``, but they may be assigned +//| in either order. Check your board's pinout to verify which pin is which channel. +//| //| Simple 8ksps 440 Hz sin wave:: //| //| import audiocore @@ -90,6 +95,12 @@ static mp_obj_t audioio_audioout_make_new(const mp_obj_type_t *type, size_t n_ar const mcu_pin_obj_t *right_channel_pin = validate_obj_is_free_pin_or_none(args[ARG_right_channel].u_obj, MP_QSTR_right_channel); + // Can't use the same pin for both left and right channels. + if (left_channel_pin == right_channel_pin) { + mp_raise_ValueError_varg(MP_ERROR_TEXT("%q and %q must be different"), + MP_QSTR_left_channel, MP_QSTR_right_channel); + } + // create AudioOut object from the given pin audioio_audioout_obj_t *self = mp_obj_malloc_with_finaliser(audioio_audioout_obj_t, &audioio_audioout_type); common_hal_audioio_audioout_construct(self, left_channel_pin, right_channel_pin, args[ARG_quiescent_value].u_int);