From c209165d4357708c5d9ed08f150206cad36e6281 Mon Sep 17 00:00:00 2001 From: Scott Shawcroft Date: Tue, 9 Oct 2018 13:56:02 -0700 Subject: [PATCH 1/2] Ramp values to and from a default value while active. This reduces the popping sound on initial playback of an audio sample. The M4 DAC has a pop on startup that cannot be prevented. It also does not allow readback so current values of the DAC are ignored. Fixes #1090 --- .../common-hal/analogio/AnalogOut.c | 12 --- .../atmel-samd/common-hal/audioio/AudioOut.c | 87 ++++++++++++++++++- .../atmel-samd/common-hal/audioio/AudioOut.h | 1 + ports/atmel-samd/supervisor/port.c | 2 +- shared-bindings/audioio/AudioOut.c | 9 +- shared-bindings/audioio/AudioOut.h | 2 +- 6 files changed, 93 insertions(+), 20 deletions(-) diff --git a/ports/atmel-samd/common-hal/analogio/AnalogOut.c b/ports/atmel-samd/common-hal/analogio/AnalogOut.c index 2e42abdd45..8419927fe7 100644 --- a/ports/atmel-samd/common-hal/analogio/AnalogOut.c +++ b/ports/atmel-samd/common-hal/analogio/AnalogOut.c @@ -138,16 +138,4 @@ void common_hal_analogio_analogout_set_value(analogio_analogout_obj_t *self, } void analogout_reset(void) { - #if defined(SAMD21) && !defined(PIN_PA02) - return; - #endif - #ifdef SAMD21 - while (DAC->STATUS.reg & DAC_STATUS_SYNCBUSY) {} - #endif - #ifdef SAMD51 - while (DAC->SYNCBUSY.reg & DAC_SYNCBUSY_SWRST) {} - #endif - DAC->CTRLA.reg |= DAC_CTRLA_SWRST; - - // TODO(tannewt): Turn off the DAC clocks to save power. } diff --git a/ports/atmel-samd/common-hal/audioio/AudioOut.c b/ports/atmel-samd/common-hal/audioio/AudioOut.c index 0592775a56..b15c642b53 100644 --- a/ports/atmel-samd/common-hal/audioio/AudioOut.c +++ b/ports/atmel-samd/common-hal/audioio/AudioOut.c @@ -33,6 +33,7 @@ #include "py/runtime.h" #include "common-hal/audioio/AudioOut.h" #include "shared-bindings/audioio/AudioOut.h" +#include "shared-bindings/microcontroller/__init__.h" #include "shared-bindings/microcontroller/Pin.h" #include "supervisor/shared/translate.h" @@ -52,11 +53,73 @@ #include "samd/pins.h" #include "samd/timers.h" +#ifdef SAMD21 +static void ramp_value(uint16_t start, uint16_t end) { + start = DAC->DATA.reg; + int32_t diff = (int32_t) end - start; + int32_t step = 49; + int32_t steps = diff / step; + if (diff < 0) { + steps *= -1; + step *= -1; + } + for (int32_t i = 0; i < steps; i++) { + uint32_t value = start + step * i; + DAC->DATA.reg = value; + DAC->DATABUF.reg = value; + common_hal_mcu_delay_us(50); + #ifdef MICROPY_VM_HOOK_LOOP + MICROPY_VM_HOOK_LOOP + #endif + } +} +#endif + +#ifdef SAMD51 +static void ramp_value(uint16_t start, uint16_t end) { + int32_t diff = (int32_t) end - start; + int32_t step = 49; + int32_t steps = diff / step; + if (diff < 0) { + steps *= -1; + step *= -1; + } + + for (int32_t i = 0; i < steps; i++) { + uint16_t value = start + step * i; + DAC->DATA[0].reg = value; + DAC->DATABUF[0].reg = value; + DAC->DATA[1].reg = value; + DAC->DATABUF[1].reg = value; + + common_hal_mcu_delay_us(50); + #ifdef MICROPY_VM_HOOK_LOOP + MICROPY_VM_HOOK_LOOP + #endif + } +} +#endif + void audioout_reset(void) { + #if defined(SAMD21) && !defined(PIN_PA02) + return; + #endif + #ifdef SAMD21 + while (DAC->STATUS.reg & DAC_STATUS_SYNCBUSY) {} + #endif + #ifdef SAMD51 + while (DAC->SYNCBUSY.reg & DAC_SYNCBUSY_SWRST) {} + #endif + if (DAC->CTRLA.bit.ENABLE) { + ramp_value(0x8000, 0); + } + DAC->CTRLA.reg |= DAC_CTRLA_SWRST; + + // TODO(tannewt): Turn off the DAC clocks to save power. } 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) { + const mcu_pin_obj_t* left_channel, const mcu_pin_obj_t* right_channel, uint16_t default_value) { #ifdef SAMD51 bool dac_clock_enabled = hri_mclk_get_APBDMASK_DAC_bit(MCLK); #endif @@ -94,12 +157,10 @@ void common_hal_audioio_audioout_construct(audioio_audioout_obj_t* self, if (right_channel != NULL) { claim_pin(right_channel); self->right_channel = right_channel; - gpio_set_pin_function(self->right_channel->number, GPIO_PIN_FUNCTION_B); audio_dma_init(&self->right_dma); } #endif self->left_channel = left_channel; - gpio_set_pin_function(self->left_channel->number, GPIO_PIN_FUNCTION_B); audio_dma_init(&self->left_dma); #ifdef SAMD51 @@ -118,6 +179,10 @@ void common_hal_audioio_audioout_construct(audioio_audioout_obj_t* self, DAC->CTRLA.bit.SWRST = 1; while (DAC->CTRLA.bit.SWRST == 1) {} + // Make sure there are no outstanding access errors. (Reading DATA can cause this.) + #ifdef SAMD51 + PAC->INTFLAGD.reg = PAC_INTFLAGD_DAC; + #endif bool channel0_enabled = true; #ifdef SAMD51 @@ -159,6 +224,8 @@ void common_hal_audioio_audioout_construct(audioio_audioout_obj_t* self, #endif #ifdef SAMD51 while (DAC->SYNCBUSY.bit.ENABLE == 1) {} + while (channel0_enabled && DAC->STATUS.bit.READY0 == 0) {} + while (channel1_enabled && DAC->STATUS.bit.READY1 == 0) {} #endif // Use a timer to coordinate when DAC conversions occur. @@ -220,13 +287,21 @@ void common_hal_audioio_audioout_construct(audioio_audioout_obj_t* self, #ifdef SAMD51 connect_event_user_to_channel(EVSYS_ID_USER_DAC_START_1, channel); + if (right_channel != NULL) { + gpio_set_pin_function(self->right_channel->number, GPIO_PIN_FUNCTION_B); + } #define EVSYS_ID_USER_DAC_START EVSYS_ID_USER_DAC_START_0 #endif connect_event_user_to_channel(EVSYS_ID_USER_DAC_START, channel); + gpio_set_pin_function(self->left_channel->number, GPIO_PIN_FUNCTION_B); init_async_event_channel(channel, tc_gen_id); self->tc_to_dac_event_channel = channel; + // Ramp the DAC up. + self->default_value = default_value; + ramp_value(0, default_value); + // Leave the DMA setup to playback. } @@ -239,6 +314,9 @@ void common_hal_audioio_audioout_deinit(audioio_audioout_obj_t* self) { return; } + // Ramp the DAC down. + ramp_value(self->default_value, 0); + DAC->CTRLA.bit.ENABLE = 0; #ifdef SAMD21 while (DAC->STATUS.bit.SYNCBUSY == 1) {} @@ -381,6 +459,9 @@ void common_hal_audioio_audioout_stop(audioio_audioout_obj_t* self) { #ifdef SAMD51 audio_dma_stop(&self->right_dma); #endif + // Ramp the DAC to default. The start is ignored when the current value can be readback. + // Otherwise, we just set it immediately. + ramp_value(self->default_value, self->default_value); } bool common_hal_audioio_audioout_get_playing(audioio_audioout_obj_t* self) { diff --git a/ports/atmel-samd/common-hal/audioio/AudioOut.h b/ports/atmel-samd/common-hal/audioio/AudioOut.h index efbfe2a1d6..9e5a7390d5 100644 --- a/ports/atmel-samd/common-hal/audioio/AudioOut.h +++ b/ports/atmel-samd/common-hal/audioio/AudioOut.h @@ -44,6 +44,7 @@ typedef struct { uint8_t tc_to_dac_event_channel; bool playing; + uint16_t default_value; } audioio_audioout_obj_t; void audioout_reset(void); diff --git a/ports/atmel-samd/supervisor/port.c b/ports/atmel-samd/supervisor/port.c index 2349a74182..45b8ed1a4c 100644 --- a/ports/atmel-samd/supervisor/port.c +++ b/ports/atmel-samd/supervisor/port.c @@ -249,11 +249,11 @@ void reset_port(void) { } #if defined(EXPRESS_BOARD) && !defined(__SAMR21G18A__) + audio_dma_reset(); audioout_reset(); #if !defined(__SAMD51G19A__) && !defined(__SAMD51G18A__) i2sout_reset(); #endif - audio_dma_reset(); //pdmin_reset(); #endif #ifdef SAMD21 diff --git a/shared-bindings/audioio/AudioOut.c b/shared-bindings/audioio/AudioOut.c index db5b371e22..8c4b927855 100644 --- a/shared-bindings/audioio/AudioOut.c +++ b/shared-bindings/audioio/AudioOut.c @@ -43,13 +43,15 @@ //| //| AudioOut can be used to output an analog audio signal on a given pin. //| -//| .. class:: AudioOut(left_channel, right_channel=None) +//| .. class:: AudioOut(left_channel, *, right_channel=None, default_value=0x8000) //| //| 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 int default_value: The default output value. Samples should start and end with this +//| value to prevent popping. //| //| Simple 8ksps 440 Hz sin wave:: //| @@ -95,10 +97,11 @@ STATIC mp_obj_t audioio_audioout_make_new(const mp_obj_type_t *type, size_t n_ar mp_arg_check_num(n_args, n_kw, 1, 2, true); mp_map_t kw_args; mp_map_init_fixed_table(&kw_args, n_kw, pos_args + n_args); - enum { ARG_left_channel, ARG_right_channel }; + enum { ARG_left_channel, ARG_right_channel, ARG_default_value }; static const mp_arg_t allowed_args[] = { { MP_QSTR_left_channel, MP_ARG_OBJ | MP_ARG_REQUIRED }, { MP_QSTR_right_channel, MP_ARG_OBJ | MP_ARG_KW_ONLY, {.u_rom_obj = mp_const_none} }, + { MP_QSTR_default_value, MP_ARG_OBJ | MP_ARG_KW_ONLY, {.u_int = 0x8000} }, }; mp_arg_val_t args[MP_ARRAY_SIZE(allowed_args)]; mp_arg_parse_all(n_args, pos_args, &kw_args, MP_ARRAY_SIZE(allowed_args), allowed_args, args); @@ -117,7 +120,7 @@ STATIC mp_obj_t audioio_audioout_make_new(const mp_obj_type_t *type, size_t n_ar // create AudioOut object from the given pin audioio_audioout_obj_t *self = m_new_obj(audioio_audioout_obj_t); self->base.type = &audioio_audioout_type; - common_hal_audioio_audioout_construct(self, left_channel_pin, right_channel_pin); + common_hal_audioio_audioout_construct(self, left_channel_pin, right_channel_pin, args[ARG_default_value].u_int); return MP_OBJ_FROM_PTR(self); } diff --git a/shared-bindings/audioio/AudioOut.h b/shared-bindings/audioio/AudioOut.h index 751473605d..d09a5c9caa 100644 --- a/shared-bindings/audioio/AudioOut.h +++ b/shared-bindings/audioio/AudioOut.h @@ -35,7 +35,7 @@ extern const mp_obj_type_t audioio_audioout_type; // left_channel will always be non-NULL but right_channel may be for mono output. 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); + const mcu_pin_obj_t* left_channel, const mcu_pin_obj_t* right_channel, uint16_t default_value); void common_hal_audioio_audioout_deinit(audioio_audioout_obj_t* self); bool common_hal_audioio_audioout_deinited(audioio_audioout_obj_t* self); From 4eb1fe18e5dfa4df03625b7d009ffbea75ea8f63 Mon Sep 17 00:00:00 2001 From: Scott Shawcroft Date: Wed, 17 Oct 2018 11:31:08 -0700 Subject: [PATCH 2/2] Tweaks from feedback: * default_value is now quiescent_value * Use step = -step format for sign switch * Add note about analogout_reset being empty --- .../atmel-samd/common-hal/analogio/AnalogOut.c | 1 + ports/atmel-samd/common-hal/audioio/AudioOut.c | 18 +++++++++--------- ports/atmel-samd/common-hal/audioio/AudioOut.h | 2 +- shared-bindings/audioio/AudioOut.c | 12 ++++++------ 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/ports/atmel-samd/common-hal/analogio/AnalogOut.c b/ports/atmel-samd/common-hal/analogio/AnalogOut.c index 8419927fe7..19475e00bf 100644 --- a/ports/atmel-samd/common-hal/analogio/AnalogOut.c +++ b/ports/atmel-samd/common-hal/analogio/AnalogOut.c @@ -138,4 +138,5 @@ void common_hal_analogio_analogout_set_value(analogio_analogout_obj_t *self, } void analogout_reset(void) { + // AudioOut resets the DAC in case its been used for audio which requires special handling. } diff --git a/ports/atmel-samd/common-hal/audioio/AudioOut.c b/ports/atmel-samd/common-hal/audioio/AudioOut.c index b15c642b53..596f3214af 100644 --- a/ports/atmel-samd/common-hal/audioio/AudioOut.c +++ b/ports/atmel-samd/common-hal/audioio/AudioOut.c @@ -60,8 +60,8 @@ static void ramp_value(uint16_t start, uint16_t end) { int32_t step = 49; int32_t steps = diff / step; if (diff < 0) { - steps *= -1; - step *= -1; + steps = -steps; + step = -step; } for (int32_t i = 0; i < steps; i++) { uint32_t value = start + step * i; @@ -81,8 +81,8 @@ static void ramp_value(uint16_t start, uint16_t end) { int32_t step = 49; int32_t steps = diff / step; if (diff < 0) { - steps *= -1; - step *= -1; + steps = -steps; + step = -step; } for (int32_t i = 0; i < steps; i++) { @@ -119,7 +119,7 @@ void audioout_reset(void) { } 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 default_value) { + const mcu_pin_obj_t* left_channel, const mcu_pin_obj_t* right_channel, uint16_t quiescent_value) { #ifdef SAMD51 bool dac_clock_enabled = hri_mclk_get_APBDMASK_DAC_bit(MCLK); #endif @@ -299,8 +299,8 @@ void common_hal_audioio_audioout_construct(audioio_audioout_obj_t* self, self->tc_to_dac_event_channel = channel; // Ramp the DAC up. - self->default_value = default_value; - ramp_value(0, default_value); + self->quiescent_value = quiescent_value; + ramp_value(0, quiescent_value); // Leave the DMA setup to playback. } @@ -315,7 +315,7 @@ void common_hal_audioio_audioout_deinit(audioio_audioout_obj_t* self) { } // Ramp the DAC down. - ramp_value(self->default_value, 0); + ramp_value(self->quiescent_value, 0); DAC->CTRLA.bit.ENABLE = 0; #ifdef SAMD21 @@ -461,7 +461,7 @@ void common_hal_audioio_audioout_stop(audioio_audioout_obj_t* self) { #endif // Ramp the DAC to default. The start is ignored when the current value can be readback. // Otherwise, we just set it immediately. - ramp_value(self->default_value, self->default_value); + ramp_value(self->quiescent_value, self->quiescent_value); } bool common_hal_audioio_audioout_get_playing(audioio_audioout_obj_t* self) { diff --git a/ports/atmel-samd/common-hal/audioio/AudioOut.h b/ports/atmel-samd/common-hal/audioio/AudioOut.h index 9e5a7390d5..56b6b75c89 100644 --- a/ports/atmel-samd/common-hal/audioio/AudioOut.h +++ b/ports/atmel-samd/common-hal/audioio/AudioOut.h @@ -44,7 +44,7 @@ typedef struct { uint8_t tc_to_dac_event_channel; bool playing; - uint16_t default_value; + uint16_t quiescent_value; } audioio_audioout_obj_t; void audioout_reset(void); diff --git a/shared-bindings/audioio/AudioOut.c b/shared-bindings/audioio/AudioOut.c index 8c4b927855..18d908eef8 100644 --- a/shared-bindings/audioio/AudioOut.c +++ b/shared-bindings/audioio/AudioOut.c @@ -43,15 +43,15 @@ //| //| AudioOut can be used to output an analog audio signal on a given pin. //| -//| .. class:: AudioOut(left_channel, *, right_channel=None, default_value=0x8000) +//| .. class:: AudioOut(left_channel, *, right_channel=None, quiescent_value=0x8000) //| //| 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 int default_value: The default output value. Samples should start and end with this -//| value to prevent popping. +//| :param int quiescent_value: The output value when no signal is present. Samples should start +//| and end with this value to prevent audible popping. //| //| Simple 8ksps 440 Hz sin wave:: //| @@ -97,11 +97,11 @@ STATIC mp_obj_t audioio_audioout_make_new(const mp_obj_type_t *type, size_t n_ar mp_arg_check_num(n_args, n_kw, 1, 2, true); mp_map_t kw_args; mp_map_init_fixed_table(&kw_args, n_kw, pos_args + n_args); - enum { ARG_left_channel, ARG_right_channel, ARG_default_value }; + enum { ARG_left_channel, ARG_right_channel, ARG_quiescent_value }; static const mp_arg_t allowed_args[] = { { MP_QSTR_left_channel, MP_ARG_OBJ | MP_ARG_REQUIRED }, { MP_QSTR_right_channel, MP_ARG_OBJ | MP_ARG_KW_ONLY, {.u_rom_obj = mp_const_none} }, - { MP_QSTR_default_value, MP_ARG_OBJ | MP_ARG_KW_ONLY, {.u_int = 0x8000} }, + { MP_QSTR_quiescent_value, MP_ARG_OBJ | MP_ARG_KW_ONLY, {.u_int = 0x8000} }, }; mp_arg_val_t args[MP_ARRAY_SIZE(allowed_args)]; mp_arg_parse_all(n_args, pos_args, &kw_args, MP_ARRAY_SIZE(allowed_args), allowed_args, args); @@ -120,7 +120,7 @@ STATIC mp_obj_t audioio_audioout_make_new(const mp_obj_type_t *type, size_t n_ar // create AudioOut object from the given pin audioio_audioout_obj_t *self = m_new_obj(audioio_audioout_obj_t); self->base.type = &audioio_audioout_type; - common_hal_audioio_audioout_construct(self, left_channel_pin, right_channel_pin, args[ARG_default_value].u_int); + common_hal_audioio_audioout_construct(self, left_channel_pin, right_channel_pin, args[ARG_quiescent_value].u_int); return MP_OBJ_FROM_PTR(self); }