Merge pull request #10182 from dhalbert/audioout-fixes

dhalbert · web-flow · commit e172e15cf34f · 2025-03-26T12:06:08.000-04:00
Disallow identical AudioOut channel pins. Work around ESP-IDF ESP32-S2 bug that swaps DAC channels randomly.
diff --git 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
@@ -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,24 +517,15 @@ 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 &&
          right_channel_pin == &pin_CHANNEL_0)) {
         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
@@ -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);
