From bd6826496cfb7e5fdcf48591c463f89243d5d0fb Mon Sep 17 00:00:00 2001 From: Dan Halbert Date: Thu, 8 Jul 2021 08:14:37 -0400 Subject: [PATCH 1/2] Fix various RP2040 audio dma things: (see detailed commit message) 1. Check for correct error values from dma_claim_unused_channel. 2. Introduce a .stereo flag for simplicity. 3. Clarify PWM carrier frequency choice. 4. Start introducing quiescent audio value. Still need to ramp up/down. 5. Redo audio stop logic a bit. 6. Fix (unrelated) displayio dependency things. There is still an interference problem between other DMA users and audio. Still debugging this. --- ports/raspberrypi/audio_dma.c | 17 +++-- .../common-hal/audiobusio/I2SOut.c | 7 ++ .../common-hal/audiopwmio/PWMAudioOut.c | 74 ++++++++++++------- .../common-hal/audiopwmio/PWMAudioOut.h | 1 + ports/raspberrypi/mpconfigport.mk | 4 +- 5 files changed, 68 insertions(+), 35 deletions(-) diff --git a/ports/raspberrypi/audio_dma.c b/ports/raspberrypi/audio_dma.c index 91805e0a4d..52b1c84b91 100644 --- a/ports/raspberrypi/audio_dma.c +++ b/ports/raspberrypi/audio_dma.c @@ -169,15 +169,20 @@ audio_dma_result audio_dma_setup_playback(audio_dma_t *dma, uint8_t dma_trigger_source) { // Use two DMA channels to play because the DMA can't wrap to itself without the // buffer being power of two aligned. - dma->channel[0] = dma_claim_unused_channel(false); - dma->channel[1] = dma_claim_unused_channel(false); - if (dma->channel[0] == NUM_DMA_CHANNELS || dma->channel[1] == NUM_DMA_CHANNELS) { - if (dma->channel[0] < NUM_DMA_CHANNELS) { - dma_channel_unclaim(dma->channel[0]); - } + int dma_channel_0_maybe = dma_claim_unused_channel(false); + if (dma_channel_0_maybe < 0) { return AUDIO_DMA_DMA_BUSY; } + int dma_channel_1_maybe = dma_claim_unused_channel(false); + if (dma_channel_1_maybe < 0) { + dma_channel_unclaim((uint)dma_channel_0_maybe); + return AUDIO_DMA_DMA_BUSY; + } + + dma->channel[0] = (uint8_t)dma_channel_0_maybe; + dma->channel[1] = (uint8_t)dma_channel_1_maybe; + dma->sample = sample; dma->loop = loop; dma->single_channel_output = single_channel_output; diff --git a/ports/raspberrypi/common-hal/audiobusio/I2SOut.c b/ports/raspberrypi/common-hal/audiobusio/I2SOut.c index 0ad44c2527..0958de6883 100644 --- a/ports/raspberrypi/common-hal/audiobusio/I2SOut.c +++ b/ports/raspberrypi/common-hal/audiobusio/I2SOut.c @@ -145,7 +145,13 @@ void common_hal_audiobusio_i2sout_deinit(audiobusio_i2sout_obj_t *self) { return; } + if (common_hal_audiobusio_i2sout_get_playing(self)) { + common_hal_audiobusio_i2sout_stop(self); + } + common_hal_rp2pio_statemachine_deinit(&self->state_machine); + + audio_dma_deinit(&self->dma); } void common_hal_audiobusio_i2sout_play(audiobusio_i2sout_obj_t *self, @@ -153,6 +159,7 @@ void common_hal_audiobusio_i2sout_play(audiobusio_i2sout_obj_t *self, if (common_hal_audiobusio_i2sout_get_playing(self)) { common_hal_audiobusio_i2sout_stop(self); } + uint8_t bits_per_sample = audiosample_bits_per_sample(sample); // Make sure we transmit a minimum of 16 bits. // TODO: Maybe we need an intermediate object to upsample instead. This is diff --git a/ports/raspberrypi/common-hal/audiopwmio/PWMAudioOut.c b/ports/raspberrypi/common-hal/audiopwmio/PWMAudioOut.c index bf9d04bb65..3a8a7ef226 100644 --- a/ports/raspberrypi/common-hal/audiopwmio/PWMAudioOut.c +++ b/ports/raspberrypi/common-hal/audiopwmio/PWMAudioOut.c @@ -46,6 +46,12 @@ #define NUM_DMA_TIMERS 4 +// The PWM clock frequency is base_clock_rate / PWM_TOP, typically 125_000_000 / PWM_TOP. +// We pick BITS_PER_SAMPLE so we get a clock frequency that is above what would cause aliasing. +#define BITS_PER_SAMPLE 10 +#define SAMPLE_BITS_TO_DISCARD (16 - BITS_PER_SAMPLE) +#define PWM_TOP ((1 << BITS_PER_SAMPLE) - 1) + void audiopwmout_reset() { for (size_t i = 0; i < NUM_DMA_TIMERS; i++) { dma_hw->timer[i] = 0; @@ -55,7 +61,10 @@ void audiopwmout_reset() { // Caller validates that pins are free. void common_hal_audiopwmio_pwmaudioout_construct(audiopwmio_pwmaudioout_obj_t *self, const mcu_pin_obj_t *left_channel, const mcu_pin_obj_t *right_channel, uint16_t quiescent_value) { - if (left_channel != NULL && right_channel != NULL) { + + self->stereo = right_channel != NULL; + + if (self->stereo) { if (pwm_gpio_to_slice_num(left_channel->number) != pwm_gpio_to_slice_num(right_channel->number)) { mp_raise_ValueError(translate("Pins must share PWM slice")); } @@ -72,22 +81,20 @@ void common_hal_audiopwmio_pwmaudioout_construct(audiopwmio_pwmaudioout_obj_t *s // we want. ;-) We mark ourselves variable only if we're a mono output to // prevent other PWM use on the other channel. If stereo, we say fixed // frequency so we can allocate with ourselves. - bool mono = right_channel == NULL; - // We don't actually know our frequency yet so just pick one that shouldn't - // match anyone else. (We'll only know the frequency once we play something - // back.) - uint32_t frequency = 12500; + // We don't actually know our frequency yet. It is set when + // pwmio_pwmout_set_top() is called. This value is unimportant; it just needs to be valid. + const uint32_t frequency = 12000000; // Make sure the PWMOut's are "deinited" by default. self->left_pwm.pin = NULL; self->right_pwm.pin = NULL; - pwmout_result_t result = common_hal_pwmio_pwmout_construct(&self->left_pwm, - left_channel, 0, frequency, mono); + pwmout_result_t result = + common_hal_pwmio_pwmout_construct(&self->left_pwm, left_channel, 0, frequency, !self->stereo); if (result == PWMOUT_OK && right_channel != NULL) { - result = common_hal_pwmio_pwmout_construct(&self->right_pwm, - right_channel, 0, frequency, false); + result = + common_hal_pwmio_pwmout_construct(&self->right_pwm, right_channel, 0, frequency, false); if (result != PWMOUT_OK) { common_hal_pwmio_pwmout_deinit(&self->left_pwm); } @@ -96,10 +103,16 @@ void common_hal_audiopwmio_pwmaudioout_construct(audiopwmio_pwmaudioout_obj_t *s mp_raise_RuntimeError(translate("All timers in use")); } + self->quiescent_value = quiescent_value >> SAMPLE_BITS_TO_DISCARD; + common_hal_pwmio_pwmout_set_duty_cycle(&self->left_pwm, self->quiescent_value); + pwmio_pwmout_set_top(&self->left_pwm, PWM_TOP); + if (self->stereo) { + common_hal_pwmio_pwmout_set_duty_cycle(&self->right_pwm, self->quiescent_value); + pwmio_pwmout_set_top(&self->right_pwm, PWM_TOP); + } + audio_dma_init(&self->dma); self->pacing_timer = NUM_DMA_TIMERS; - - self->quiescent_value = quiescent_value; } bool common_hal_audiopwmio_pwmaudioout_deinited(audiopwmio_pwmaudioout_obj_t *self) { @@ -110,6 +123,7 @@ void common_hal_audiopwmio_pwmaudioout_deinit(audiopwmio_pwmaudioout_obj_t *self if (common_hal_audiopwmio_pwmaudioout_deinited(self)) { return; } + if (common_hal_audiopwmio_pwmaudioout_get_playing(self)) { common_hal_audiopwmio_pwmaudioout_stop(self); } @@ -139,13 +153,11 @@ void common_hal_audiopwmio_pwmaudioout_play(audiopwmio_pwmaudioout_obj_t *self, mp_raise_RuntimeError(translate("No DMA pacing timer found")); } uint32_t tx_register = (uint32_t)&pwm_hw->slice[self->left_pwm.slice].cc; - if (common_hal_pwmio_pwmout_deinited(&self->right_pwm)) { - // Shift the destination if we are outputting to the second PWM channel. + if (self->stereo) { + // Shift the destination if we are outputting to both PWM channels. tx_register += self->left_pwm.channel * sizeof(uint16_t); } - pwmio_pwmout_set_top(&self->left_pwm, 1023); - audio_dma_result result = audio_dma_setup_playback( &self->dma, sample, @@ -153,15 +165,16 @@ void common_hal_audiopwmio_pwmaudioout_play(audiopwmio_pwmaudioout_obj_t *self, false, // single channel 0, // audio channel false, // output signed - 10, - (uint32_t)tx_register, // output register + BITS_PER_SAMPLE, + (uint32_t)tx_register, // output register: PWM cc register 0x3b + pacing_timer); // data request line if (result == AUDIO_DMA_DMA_BUSY) { - // common_hal_audiobusio_i2sout_stop(self); + common_hal_audiopwmio_pwmaudioout_stop(self); mp_raise_RuntimeError(translate("No DMA channel found")); - } else if (result == AUDIO_DMA_MEMORY_ERROR) { - // common_hal_audiobusio_i2sout_stop(self); + } + if (result == AUDIO_DMA_MEMORY_ERROR) { + common_hal_audiopwmio_pwmaudioout_stop(self); mp_raise_RuntimeError(translate("Unable to allocate buffers for signed conversion")); } @@ -177,6 +190,7 @@ void common_hal_audiopwmio_pwmaudioout_play(audiopwmio_pwmaudioout_obj_t *self, // are 16-bit. uint32_t sample_rate = audiosample_sample_rate(sample); + uint32_t system_clock = common_hal_mcu_processor_get_frequency(); uint32_t best_numerator = 0; uint32_t best_denominator = 0; @@ -204,19 +218,25 @@ void common_hal_audiopwmio_pwmaudioout_play(audiopwmio_pwmaudioout_obj_t *self, } void common_hal_audiopwmio_pwmaudioout_stop(audiopwmio_pwmaudioout_obj_t *self) { - dma_hw->timer[self->pacing_timer] = 0; - self->pacing_timer = NUM_DMA_TIMERS; + if (self->pacing_timer < NUM_DMA_TIMERS) { + dma_hw->timer[self->pacing_timer] = 0; + self->pacing_timer = NUM_DMA_TIMERS; + } audio_dma_stop(&self->dma); + + // Set to quiescent level. + pwm_hw->slice[self->left_pwm.slice].cc = self->quiescent_value; + if (self->stereo) { + pwm_hw->slice[self->right_pwm.slice].cc = self->quiescent_value; + } } bool common_hal_audiopwmio_pwmaudioout_get_playing(audiopwmio_pwmaudioout_obj_t *self) { bool playing = audio_dma_get_playing(&self->dma); - if (!playing && self->pacing_timer < NUM_DMA_TIMERS) { - dma_hw->timer[self->pacing_timer] = 0; - self->pacing_timer = NUM_DMA_TIMERS; - audio_dma_stop(&self->dma); + if (!playing && self->pacing_timer < NUM_DMA_TIMERS) { + common_hal_audiopwmio_pwmaudioout_stop(self); } return playing; diff --git a/ports/raspberrypi/common-hal/audiopwmio/PWMAudioOut.h b/ports/raspberrypi/common-hal/audiopwmio/PWMAudioOut.h index 2b43b8354d..e9f0f68d06 100644 --- a/ports/raspberrypi/common-hal/audiopwmio/PWMAudioOut.h +++ b/ports/raspberrypi/common-hal/audiopwmio/PWMAudioOut.h @@ -38,6 +38,7 @@ typedef struct { audio_dma_t dma; uint16_t quiescent_value; uint8_t pacing_timer; + bool stereo; // if false, only using left_pwm. } audiopwmio_pwmaudioout_obj_t; void audiopwmout_reset(void); diff --git a/ports/raspberrypi/mpconfigport.mk b/ports/raspberrypi/mpconfigport.mk index cdb8769bfc..bc8a26de7b 100644 --- a/ports/raspberrypi/mpconfigport.mk +++ b/ports/raspberrypi/mpconfigport.mk @@ -21,13 +21,13 @@ CIRCUITPY_ALARM ?= 1 CIRCUITPY_RP2PIO ?= 1 CIRCUITPY_NEOPIXEL_WRITE ?= $(CIRCUITPY_RP2PIO) -CIRCUITPY_FRAMEBUFFERIO ?= 1 +CIRCUITPY_FRAMEBUFFERIO ?= $(CIRCUITPY_DISPLAYIO) CIRCUITPY_FULL_BUILD ?= 1 CIRCUITPY_AUDIOMP3 ?= 1 CIRCUITPY_BITOPS ?= 1 CIRCUITPY_IMAGECAPTURE ?= 1 CIRCUITPY_PWMIO ?= 1 -CIRCUITPY_RGBMATRIX ?= 1 +CIRCUITPY_RGBMATRIX ?= $(CIRCUITPY_DISPLAYIO) CIRCUITPY_ROTARYIO ?= 1 CIRCUITPY_ROTARYIO_SOFTENCODER = 1 From ab52a92704fc7ecb52deb7fcb82337f0abca306f Mon Sep 17 00:00:00 2001 From: Dan Halbert Date: Thu, 8 Jul 2021 13:42:24 -0400 Subject: [PATCH 2/2] Disallow ctrl-C interrupts of RP2040 SPI and PIO --- ports/raspberrypi/common-hal/busio/SPI.c | 13 ++----------- .../common-hal/rp2pio/StateMachine.c | 17 +---------------- 2 files changed, 3 insertions(+), 27 deletions(-) diff --git a/ports/raspberrypi/common-hal/busio/SPI.c b/ports/raspberrypi/common-hal/busio/SPI.c index 23030cbd0d..ab295c4ff1 100644 --- a/ports/raspberrypi/common-hal/busio/SPI.c +++ b/ports/raspberrypi/common-hal/busio/SPI.c @@ -212,15 +212,6 @@ static bool _transfer(busio_spi_obj_t *self, while (dma_channel_is_busy(chan_rx) || dma_channel_is_busy(chan_tx)) { // TODO: We should idle here until we get a DMA interrupt or something else. RUN_BACKGROUND_TASKS; - if (mp_hal_is_interrupted()) { - if (dma_channel_is_busy(chan_rx)) { - dma_channel_abort(chan_rx); - } - if (dma_channel_is_busy(chan_tx)) { - dma_channel_abort(chan_tx); - } - break; - } } } @@ -233,7 +224,7 @@ static bool _transfer(busio_spi_obj_t *self, dma_channel_unclaim(chan_tx); } - if (!use_dma && !mp_hal_is_interrupted()) { + if (!use_dma) { // Use software for small transfers, or if couldn't claim two DMA channels // Never have more transfers in flight than will fit into the RX FIFO, // else FIFO will overflow if this code is heavily interrupted. @@ -241,7 +232,7 @@ static bool _transfer(busio_spi_obj_t *self, size_t rx_remaining = len; size_t tx_remaining = len; - while (!mp_hal_is_interrupted() && (rx_remaining || tx_remaining)) { + while (rx_remaining || tx_remaining) { if (tx_remaining && spi_is_writable(self->peripheral) && rx_remaining - tx_remaining < fifo_depth) { spi_get_hw(self->peripheral)->dr = (uint32_t)*data_out; // Increment only if the buffer is the transfer length. It's 1 otherwise. diff --git a/ports/raspberrypi/common-hal/rp2pio/StateMachine.c b/ports/raspberrypi/common-hal/rp2pio/StateMachine.c index ab70a27fcd..887ea8e644 100644 --- a/ports/raspberrypi/common-hal/rp2pio/StateMachine.c +++ b/ports/raspberrypi/common-hal/rp2pio/StateMachine.c @@ -654,15 +654,6 @@ static bool _transfer(rp2pio_statemachine_obj_t *self, (tx && dma_channel_is_busy(chan_tx))) { // TODO: We should idle here until we get a DMA interrupt or something else. RUN_BACKGROUND_TASKS; - if (mp_hal_is_interrupted()) { - if (rx && dma_channel_is_busy(chan_rx)) { - dma_channel_abort(chan_rx); - } - if (tx && dma_channel_is_busy(chan_tx)) { - dma_channel_abort(chan_tx); - } - break; - } } // Clear the stall bit so we can detect when the state machine is done transmitting. self->pio->fdebug = stall_mask; @@ -677,7 +668,7 @@ static bool _transfer(rp2pio_statemachine_obj_t *self, dma_channel_unclaim(chan_tx); } - if (!use_dma && !mp_hal_is_interrupted()) { + if (!use_dma) { // Use software for small transfers, or if couldn't claim two DMA channels size_t rx_remaining = in_len / in_stride_in_bytes; size_t tx_remaining = out_len / out_stride_in_bytes; @@ -706,9 +697,6 @@ static bool _transfer(rp2pio_statemachine_obj_t *self, --rx_remaining; } RUN_BACKGROUND_TASKS; - if (mp_hal_is_interrupted()) { - break; - } } // Clear the stall bit so we can detect when the state machine is done transmitting. self->pio->fdebug = stall_mask; @@ -719,9 +707,6 @@ static bool _transfer(rp2pio_statemachine_obj_t *self, while (!pio_sm_is_tx_fifo_empty(self->pio, self->state_machine) || (self->wait_for_txstall && (self->pio->fdebug & stall_mask) == 0)) { RUN_BACKGROUND_TASKS; - if (mp_hal_is_interrupted()) { - break; - } } } return true;