From aabb56c840456dfdd065c8ca2e11854f32c5a63c Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Fri, 22 Nov 2019 13:54:37 -0600 Subject: nrf: i2sout: Only update hold_value when appropriate If we put no samples into the buffer, then there is no last sample to fill out hold_value with. (and, in fact, the expression such as *(uint32_t*)(buffer-4) is outside an allocated region) Detect this condition, and leave the prior value in place. This improves clicks heard when pausing and resuming a waveform. --- ports/nrf/common-hal/audiobusio/I2SOut.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/ports/nrf/common-hal/audiobusio/I2SOut.c b/ports/nrf/common-hal/audiobusio/I2SOut.c index ac369277f..5a53f66d2 100644 --- a/ports/nrf/common-hal/audiobusio/I2SOut.c +++ b/ports/nrf/common-hal/audiobusio/I2SOut.c @@ -108,6 +108,7 @@ void choose_i2s_clocking(audiobusio_i2sout_obj_t *self, uint32_t sample_rate) { static void i2s_buffer_fill(audiobusio_i2sout_obj_t* self) { void *buffer = self->buffers[self->next_buffer]; + void *buffer_start = buffer; NRF_I2S->TXD.PTR = (uintptr_t)buffer; self->next_buffer = !self->next_buffer; size_t bytesleft = self->buffer_length; @@ -157,15 +158,17 @@ static void i2s_buffer_fill(audiobusio_i2sout_obj_t* self) { // Find the last frame of real audio data and replicate its samples until // you have 32 bits worth, which is the fundamental unit of nRF I2S DMA - if (self->bytes_per_sample == 1 && self->channel_count == 1) { - // For 8-bit mono, 4 copies of the final sample are required - self->hold_value = 0x01010101 * *(uint8_t*)(buffer-1); - } else if (self->bytes_per_sample == 2 && self->channel_count == 2) { - // For 16-bit stereo, 1 copy of the final sample is required - self->hold_value = *(uint32_t*)(buffer-4); - } else { - // For 8-bit stereo and 16-bit mono, 2 copies of the final sample are required - self->hold_value = 0x00010001 * *(uint16_t*)(buffer-2); + if(buffer != buffer_start) { + if (self->bytes_per_sample == 1 && self->channel_count == 1) { + // For 8-bit mono, 4 copies of the final sample are required + self->hold_value = 0x01010101 * *(uint8_t*)(buffer-1); + } else if (self->bytes_per_sample == 2 && self->channel_count == 2) { + // For 16-bit stereo, 1 copy of the final sample is required + self->hold_value = *(uint32_t*)(buffer-4); + } else { + // For 8-bit stereo and 16-bit mono, 2 copies of the final sample are required + self->hold_value = 0x00010001 * *(uint16_t*)(buffer-2); + } } // Emulate pausing and stopping by filling the DMA buffer with copies of -- cgit v1.2.3 From 1c6c9a3e1f041670c008e719be635f4205add6e9 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Fri, 22 Nov 2019 14:15:07 -0600 Subject: nrf: i2sout: Ensure hardware I2S is stopped at deinit .. otherwise, it may be possible under some scenario, for the background task to continue and overwrite unrelated memory. --- ports/nrf/common-hal/audiobusio/I2SOut.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ports/nrf/common-hal/audiobusio/I2SOut.c b/ports/nrf/common-hal/audiobusio/I2SOut.c index 5a53f66d2..93e6d485d 100644 --- a/ports/nrf/common-hal/audiobusio/I2SOut.c +++ b/ports/nrf/common-hal/audiobusio/I2SOut.c @@ -221,6 +221,8 @@ void common_hal_audiobusio_i2sout_deinit(audiobusio_i2sout_obj_t* self) { if (common_hal_audiobusio_i2sout_deinited(self)) { return; } + NRF_I2S->TASKS_STOP = 1; + NRF_I2S->ENABLE = I2S_ENABLE_ENABLE_Disabled; reset_pin_number(self->bit_clock_pin_number); self->bit_clock_pin_number = 0xff; reset_pin_number(self->word_select_pin_number); -- cgit v1.2.3 From 5115fdaa6f651ae4e1236e5cf7194438fe1c0533 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Mon, 25 Nov 2019 09:53:55 -0600 Subject: shared-bindings: I2SOut: Ensure object is deinitialised (or deinitialized, for those of us on this side of the pond) Otherwise, a sequence like ``` audio = audiobusio.I2SOut(bit_clock=board.D6, word_select=board.D9, data=board.D10) sine_wave_sample = audiocore.RawSample(sine_wave) audio.play(sine_wave_sample, loop=True) del audio ``` could free the memory associated with audio without stopping the related background task. Later, when fresh objects are allocated within a now-freed memory region, they can get overwritten in the background task, leading to a hard crash. This presumably can affect multiple I2S implementations, but it was reported against the nRF one. --- shared-bindings/audiobusio/I2SOut.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/shared-bindings/audiobusio/I2SOut.c b/shared-bindings/audiobusio/I2SOut.c index 81383c777..8f7382fde 100644 --- a/shared-bindings/audiobusio/I2SOut.c +++ b/shared-bindings/audiobusio/I2SOut.c @@ -117,7 +117,7 @@ STATIC mp_obj_t audiobusio_i2sout_make_new(const mp_obj_type_t *type, size_t n_a assert_pin(data_obj, false); const mcu_pin_obj_t *data = MP_OBJ_TO_PTR(data_obj); - audiobusio_i2sout_obj_t *self = m_new_obj(audiobusio_i2sout_obj_t); + audiobusio_i2sout_obj_t *self = m_new_obj_with_finaliser(audiobusio_i2sout_obj_t); self->base.type = &audiobusio_i2sout_type; common_hal_audiobusio_i2sout_construct(self, bit_clock, word_select, data, args[ARG_left_justified].u_bool); @@ -268,6 +268,7 @@ const mp_obj_property_t audiobusio_i2sout_paused_obj = { STATIC const mp_rom_map_elem_t audiobusio_i2sout_locals_dict_table[] = { // Methods + { MP_ROM_QSTR(MP_QSTR___del__), MP_ROM_PTR(&audiobusio_i2sout_deinit_obj) }, { MP_ROM_QSTR(MP_QSTR_deinit), MP_ROM_PTR(&audiobusio_i2sout_deinit_obj) }, { MP_ROM_QSTR(MP_QSTR___enter__), MP_ROM_PTR(&default___enter___obj) }, { MP_ROM_QSTR(MP_QSTR___exit__), MP_ROM_PTR(&audiobusio_i2sout___exit___obj) }, -- cgit v1.2.3