From 51b9a1aeca51c34e0894a08f88888eb4173af32a Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Mon, 13 Jul 2020 10:32:52 -0500 Subject: [PATCH 01/14] tick.c: adjust whitespace --- supervisor/shared/tick.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/supervisor/shared/tick.c b/supervisor/shared/tick.c index dd7dba8f3e..01a554e156 100644 --- a/supervisor/shared/tick.c +++ b/supervisor/shared/tick.c @@ -104,13 +104,13 @@ void mp_hal_delay_ms(mp_uint_t delay) { // Check to see if we've been CTRL-Ced by autoreload or the user. if(MP_STATE_VM(mp_pending_exception) == MP_OBJ_FROM_PTR(&MP_STATE_VM(mp_kbd_exception))) { - // clear exception and generate stacktrace + // clear exception and generate stacktrace MP_STATE_VM(mp_pending_exception) = MP_OBJ_NULL; nlr_raise(&MP_STATE_VM(mp_kbd_exception)); } if( MP_STATE_VM(mp_pending_exception) == MP_OBJ_FROM_PTR(&MP_STATE_VM(mp_reload_exception)) || WATCHDOG_EXCEPTION_CHECK()) { - // stop sleeping immediately + // stop sleeping immediately break; } remaining = end_tick - port_get_raw_ticks(NULL); From dc74ae83da6f519a36d75388b5be9923a10bf063 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Tue, 14 Jul 2020 17:34:45 -0500 Subject: [PATCH 02/14] nRF: Always use sd_nvic_critical_region calls The motivation for doing this is so that we can allow common_hal_mcu_disable_interrupts in IRQ context, something that works on other ports, but not on nRF with SD enabled. This is because when SD is enabled, calling sd_softdevice_is_enabled in the context of an interrupt with priority 2 or 3 causes a HardFault. We have chosen to give the USB interrupt priority 2 on nRF, the highest priority that is compatible with SD. Since at least SoftDevice s130 v2.0.1, sd_nvic_critical_region_enter/exit have been implemented as inline functions and are safe to call even if softdevice is not enabled. Reference kindly provided by danh: https://devzone.nordicsemi.com/f/nordic-q-a/29553/sd_nvic_critical_region_enter-exit-missing-in-s130-v2 Switching to these as the default/only way to enable/disable interrupts simplifies things, and fixes several problems and potential problems: * Interrupts at priority 2 or 3 could not call common_hal_mcu_disable_interrupts because the call to sd_softdevice_is_enabled would HardFault * Hypothetically, the state of sd_softdevice_is_enabled could change from the disable to the enable call, meaning the calls would not match (__disable_irq() could be balanced with sd_nvic_critical_region_exit). This also fixes a problem I believe would exist if disable() were called twice when SD is enabled. There is a single "is_nested_critical_region" flag, and the second call would set it to 1. Both of the enable() calls that followed would call critical_region_exit(1), and interrupts would not properly be reenabled. In the new version of the code, we use our own nesting_count value to track the intended state, so now nested disable()s only call critical_region_enter() once, only updating is_nested_critical_region once; and only the second enable() call will call critical_region_exit, with the right value of i_n_c_r. Finally, in port_sleep_until_interrupt, if !sd_enabled, we really do need to __disable_irq, rather than using the common_hal_mcu routines; the reason why is documented in a comment. --- .../nrf/common-hal/microcontroller/__init__.c | 44 +++++++++---------- ports/nrf/supervisor/port.c | 13 +++++- 2 files changed, 32 insertions(+), 25 deletions(-) diff --git a/ports/nrf/common-hal/microcontroller/__init__.c b/ports/nrf/common-hal/microcontroller/__init__.c index f5caf68ef8..06aac9409d 100644 --- a/ports/nrf/common-hal/microcontroller/__init__.c +++ b/ports/nrf/common-hal/microcontroller/__init__.c @@ -50,36 +50,34 @@ void common_hal_mcu_delay_us(uint32_t delay) { static volatile uint32_t nesting_count = 0; static uint8_t is_nested_critical_region; -static uint8_t sd_is_enabled = false; void common_hal_mcu_disable_interrupts() { - sd_softdevice_is_enabled(&sd_is_enabled); - if (sd_is_enabled) { + if (nesting_count == 0) { + // Unlike __disable_irq(), this should only be called the first time + // "is_nested_critical_region" is sd's equivalent of our nesting count + // so a nested call would store 0 in the global and make the later + // exit call not actually reenable interrupts + // + // This only disables interrupts of priority 2 through 7; levels 0, 1, + // and 4, are exclusive to softdevice and should never be used, so + // this limitation is not important. sd_nvic_critical_region_enter(&is_nested_critical_region); - } else { - __disable_irq(); - __DMB(); - nesting_count++; } + __DMB(); + nesting_count++; } void common_hal_mcu_enable_interrupts() { - // Don't check here if SD is enabled, because we'll crash if interrupts - // were turned off and sd_softdevice_is_enabled is called. - if (sd_is_enabled) { - sd_nvic_critical_region_exit(is_nested_critical_region); - } else { - if (nesting_count == 0) { - // This is very very bad because it means there was mismatched disable/enables so we - // crash. - reset_into_safe_mode(HARD_CRASH); - } - nesting_count--; - if (nesting_count > 0) { - return; - } - __DMB(); - __enable_irq(); + if (nesting_count == 0) { + // This is very very bad because it means there was mismatched disable/enables so we + // crash. + reset_into_safe_mode(HARD_CRASH); } + nesting_count--; + if (nesting_count > 0) { + return; + } + __DMB(); + sd_nvic_critical_region_exit(is_nested_critical_region); } void common_hal_mcu_on_next_reset(mcu_runmode_t runmode) { diff --git a/ports/nrf/supervisor/port.c b/ports/nrf/supervisor/port.c index aee2d63e1d..e681e6825f 100644 --- a/ports/nrf/supervisor/port.c +++ b/ports/nrf/supervisor/port.c @@ -313,12 +313,21 @@ void port_sleep_until_interrupt(void) { // instruction will returned as long as an interrupt is // available, even though the actual handler won't fire until // we re-enable interrupts. - common_hal_mcu_disable_interrupts(); + // + // We do not use common_hal_mcu_disable_interrupts here because + // we truly require that interrupts be disabled, while + // common_hal_mcu_disable_interrupts actually just masks the + // interrupts that are not required to allow the softdevice to + // function (whether or not SD is enabled) + int nested = __get_PRIMASK(); + __disable_irq(); if (!tud_task_event_ready()) { __DSB(); __WFI(); } - common_hal_mcu_enable_interrupts(); + if (!nested) { + __enable_irq(); + } } } From 1474fccd2fb1cf6ccd14979a3b5405d2a355054b Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Mon, 11 May 2020 08:37:20 -0500 Subject: [PATCH 03/14] supervisor: Add a linked list of background callbacks In time, we should transition interrupt driven background tasks out of the overall run_background_tasks into distinct background callbacks, so that the number of checks that occur with each tick is reduced. --- main.c | 3 + supervisor/background_callback.h | 82 +++++++++++++++++++ supervisor/shared/background_callback.c | 104 ++++++++++++++++++++++++ supervisor/shared/tick.c | 6 +- supervisor/supervisor.mk | 1 + 5 files changed, 192 insertions(+), 4 deletions(-) create mode 100644 supervisor/background_callback.h create mode 100644 supervisor/shared/background_callback.c diff --git a/main.c b/main.c index c3787122d3..201022f1af 100755 --- a/main.c +++ b/main.c @@ -45,6 +45,7 @@ #include "background.h" #include "mpconfigboard.h" +#include "supervisor/background_callback.h" #include "supervisor/cpu.h" #include "supervisor/memory.h" #include "supervisor/port.h" @@ -161,6 +162,8 @@ void stop_mp(void) { MP_STATE_VM(vfs_cur) = vfs; #endif + background_callback_reset(); + gc_deinit(); } diff --git a/supervisor/background_callback.h b/supervisor/background_callback.h new file mode 100644 index 0000000000..82025f6b7a --- /dev/null +++ b/supervisor/background_callback.h @@ -0,0 +1,82 @@ +/* + * This file is part of the MicroPython project, http://micropython.org/ + * + * The MIT License (MIT) + * + * Copyright (c) 2020 Jeff Epler for Adafruit Industries + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#ifndef CIRCUITPY_INCLUDED_SUPERVISOR_BACKGROUND_CALLBACK_H +#define CIRCUITPY_INCLUDED_SUPERVISOR_BACKGROUND_CALLBACK_H + +/** Background callbacks are a linked list of tasks to call in the background. + * + * Include a member of type `background_callback_t` inside an object + * which needs to queue up background work, and zero-initialize it. + * + * To schedule the work, use background_callback_add, with fun as the + * function to call and data pointing to the object itself. + * + * Next time run_background_tasks_if_tick is called, the callback will + * be run and removed from the linked list. + * + * Queueing a task that is already queued does nothing. Unconditionally + * re-queueing it from its own background task will cause it to run during the + * very next background-tasks invocation, leading to a CircuitPython freeze, so + * don't do that. + * + * background_callback_add can be called from interrupt context. + */ +typedef void (*background_callback_fun)(void *data); +typedef struct background_callback { + background_callback_fun fun; + void *data; + struct background_callback *next; + struct background_callback *prev; +} background_callback_t; + +/* Add a background callback for which 'fun' and 'data' were previously set */ +void background_callback_add_core(background_callback_t *cb); + +/* Add a background callback to the given function with the given data. When + * the callback involves an object on the GC heap, the 'data' must be a pointer + * to that object itself, not an internal pointer. Otherwise, it can be the + * case that no other references to the object itself survive, and the object + * becomes garbage collected while an outstanding background callback still + * exists. + */ +void background_callback_add(background_callback_t *cb, background_callback_fun fun, void *data); + +/* Run all background callbacks. Normally, this is done by the supervisor + * whenever the list is non-empty */ +void background_callback_run_all(void); + +/* During soft reset, remove all pending callbacks and clear the critical section flag */ +void background_callback_reset(void); + +/* Sometimes background callbacks must be blocked. Use these functions to + * bracket the section of code where this is the case. These calls nest, and + * begins must be balanced with ends. + */ +void background_callback_begin_critical_section(void); +void background_callback_end_critical_section(void); + +#endif diff --git a/supervisor/shared/background_callback.c b/supervisor/shared/background_callback.c new file mode 100644 index 0000000000..99607b7a85 --- /dev/null +++ b/supervisor/shared/background_callback.c @@ -0,0 +1,104 @@ +/* + * This file is part of the MicroPython project, http://micropython.org/ + * + * The MIT License (MIT) + * + * Copyright (c) 2020 Jeff Epler for Adafruit Industries + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#include "py/mpconfig.h" +#include "supervisor/background_callback.h" +#include "supervisor/shared/tick.h" +#include "shared-bindings/microcontroller/__init__.h" + +STATIC volatile background_callback_t *callback_head, *callback_tail; + +#define CALLBACK_CRITICAL_BEGIN (common_hal_mcu_disable_interrupts()) +#define CALLBACK_CRITICAL_END (common_hal_mcu_enable_interrupts()) + +void background_callback_add_core(background_callback_t *cb) { + CALLBACK_CRITICAL_BEGIN; + if (cb->prev || callback_head == cb) { + CALLBACK_CRITICAL_END; + return; + } + cb->next = 0; + cb->prev = (background_callback_t*)callback_tail; + if (callback_tail) { + callback_tail->next = cb; + cb->prev = (background_callback_t*)callback_tail; + } + if (!callback_head) { + callback_head = cb; + } + callback_tail = cb; + CALLBACK_CRITICAL_END; +} + +void background_callback_add(background_callback_t *cb, background_callback_fun fun, void *data) { + cb->fun = fun; + cb->data = data; + background_callback_add_core(cb); +} + +static bool in_background_callback; +void background_callback_run_all() { + CALLBACK_CRITICAL_BEGIN; + if(in_background_callback) { + CALLBACK_CRITICAL_END; + return; + } + in_background_callback = true; + background_callback_t *cb = (background_callback_t*)callback_head; + callback_head = NULL; + callback_tail = NULL; + while (cb) { + background_callback_t *next = cb->next; + cb->next = cb->prev = NULL; + background_callback_fun fun = cb->fun; + void *data = cb->data; + CALLBACK_CRITICAL_END; + // Leave the critical section in order to run the callback function + if (fun) { + fun(data); + } + CALLBACK_CRITICAL_BEGIN; + cb = next; + } + in_background_callback = false; + CALLBACK_CRITICAL_END; +} + +void background_callback_begin_critical_section() { + CALLBACK_CRITICAL_BEGIN; +} + +void background_callback_end_critical_section() { + CALLBACK_CRITICAL_END; +} + +void background_callback_reset() { + CALLBACK_CRITICAL_BEGIN; + callback_head = NULL; + callback_tail = NULL; + in_background_callback = false; + CALLBACK_CRITICAL_END; +} diff --git a/supervisor/shared/tick.c b/supervisor/shared/tick.c index 01a554e156..7a2f313eb8 100644 --- a/supervisor/shared/tick.c +++ b/supervisor/shared/tick.c @@ -29,6 +29,7 @@ #include "py/mpstate.h" #include "supervisor/linker.h" #include "supervisor/filesystem.h" +#include "supervisor/background_callback.h" #include "supervisor/port.h" #include "supervisor/shared/autoreload.h" @@ -86,10 +87,7 @@ uint32_t supervisor_ticks_ms32() { extern void run_background_tasks(void); void PLACE_IN_ITCM(supervisor_run_background_tasks_if_tick)() { - // TODO: Add a global that can be set by anyone to indicate we should run background tasks. That - // way we can short circuit the background tasks early. We used to do it based on time but it - // breaks cases where we wake up for a short period and then sleep. If we skipped the last - // background task or more before sleeping we may end up starving a task like USB. + background_callback_run_all(); run_background_tasks(); } diff --git a/supervisor/supervisor.mk b/supervisor/supervisor.mk index 21803ae0a3..1885366865 100644 --- a/supervisor/supervisor.mk +++ b/supervisor/supervisor.mk @@ -2,6 +2,7 @@ SRC_SUPERVISOR = \ main.c \ supervisor/port.c \ supervisor/shared/autoreload.c \ + supervisor/shared/background_callback.c \ supervisor/shared/board.c \ supervisor/shared/filesystem.c \ supervisor/shared/flash.c \ From 8c4a9f644407ee6fccf3720fd3b727085a2ff109 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Thu, 9 Jul 2020 09:47:10 -0500 Subject: [PATCH 04/14] supervisor: tick: only run background tasks once per tick --- supervisor/shared/tick.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/supervisor/shared/tick.c b/supervisor/shared/tick.c index 7a2f313eb8..46172d0f30 100644 --- a/supervisor/shared/tick.c +++ b/supervisor/shared/tick.c @@ -52,6 +52,14 @@ static volatile uint64_t PLACE_IN_DTCM_BSS(background_ticks); #define WATCHDOG_EXCEPTION_CHECK() 0 #endif +static background_callback_t callback; + +extern void run_background_tasks(void); + +void background_task_tick(void *unused) { + run_background_tasks(); +} + void supervisor_tick(void) { #if CIRCUITPY_FILESYSTEM_FLUSH_INTERVAL_MS > 0 filesystem_tick(); @@ -69,6 +77,7 @@ void supervisor_tick(void) { #endif } #endif + background_callback_add(&callback, background_task_tick, NULL); } uint64_t supervisor_ticks_ms64() { @@ -84,11 +93,9 @@ uint32_t supervisor_ticks_ms32() { return supervisor_ticks_ms64(); } -extern void run_background_tasks(void); void PLACE_IN_ITCM(supervisor_run_background_tasks_if_tick)() { background_callback_run_all(); - run_background_tasks(); } void mp_hal_delay_ms(mp_uint_t delay) { From 742aa740f651c72a5991250d9ac898492e99389d Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Tue, 7 Jul 2020 10:11:29 -0500 Subject: [PATCH 05/14] samd: audio: Move to background callback Testing performed: Played half of the Bartlebeats album :) :) --- ports/atmel-samd/audio_dma.c | 54 +++++++++++++++++++++++------------ ports/atmel-samd/audio_dma.h | 2 ++ ports/atmel-samd/background.c | 3 -- 3 files changed, 37 insertions(+), 22 deletions(-) diff --git a/ports/atmel-samd/audio_dma.c b/ports/atmel-samd/audio_dma.c index 93cd96b985..c6c636160d 100644 --- a/ports/atmel-samd/audio_dma.c +++ b/ports/atmel-samd/audio_dma.c @@ -31,7 +31,7 @@ #include "shared-bindings/audiocore/RawSample.h" #include "shared-bindings/audiocore/WaveFile.h" -#include "supervisor/shared/tick.h" +#include "supervisor/background_callback.h" #include "py/mpstate.h" #include "py/runtime.h" @@ -61,7 +61,6 @@ void audio_dma_free_channel(uint8_t channel) { assert(audio_dma_allocated[channel]); audio_dma_disable_channel(channel); audio_dma_allocated[channel] = false; - supervisor_disable_tick(); } void audio_dma_disable_channel(uint8_t channel) { @@ -73,7 +72,6 @@ void audio_dma_disable_channel(uint8_t channel) { void audio_dma_enable_channel(uint8_t channel) { if (channel >= AUDIO_DMA_CHANNEL_COUNT) return; - supervisor_enable_tick(); dma_enable_channel(channel); } @@ -259,6 +257,15 @@ audio_dma_result audio_dma_setup_playback(audio_dma_t* dma, dma->beat_size *= 2; } +#ifdef SAM_D5X_E5X + int irq = dma->event_channel < 4 ? EVSYS_0_IRQn + dma->event_channel : EVSYS_4_IRQn; +#else + int irq = EVSYS_IRQn; +#endif + + NVIC_DisableIRQ(irq); + NVIC_ClearPendingIRQ(irq); + DmacDescriptor* first_descriptor = dma_descriptor(dma_channel); setup_audio_descriptor(first_descriptor, dma->beat_size, output_spacing, output_register_address); if (single_buffer) { @@ -281,6 +288,8 @@ audio_dma_result audio_dma_setup_playback(audio_dma_t* dma, dma_configure(dma_channel, dma_trigger_source, true); audio_dma_enable_channel(dma_channel); + NVIC_EnableIRQ(irq); + return AUDIO_DMA_OK; } @@ -321,9 +330,6 @@ void audio_dma_reset(void) { for (uint8_t i = 0; i < AUDIO_DMA_CHANNEL_COUNT; i++) { audio_dma_state[i] = NULL; audio_dma_pending[i] = false; - if (audio_dma_allocated[i]) { - supervisor_disable_tick(); - } audio_dma_allocated[i] = false; audio_dma_disable_channel(i); dma_descriptor(i)->BTCTRL.bit.VALID = false; @@ -343,29 +349,39 @@ bool audio_dma_get_playing(audio_dma_t* dma) { return (status & DMAC_CHINTFLAG_TERR) == 0; } -// WARN(tannewt): DO NOT print from here. Printing calls background tasks such as this and causes a -// stack overflow. +// WARN(tannewt): DO NOT print from here, or anything it calls. Printing calls +// background tasks such as this and causes a stack overflow. +STATIC void dma_callback_fun(void *arg) { + audio_dma_t* dma = arg; + if (dma == NULL) { + return; + } -void audio_dma_background(void) { + audio_dma_load_next_block(dma); +} + +void evsyshandler_common(void) { for (uint8_t i = 0; i < AUDIO_DMA_CHANNEL_COUNT; i++) { - if (audio_dma_pending[i]) { - continue; - } audio_dma_t* dma = audio_dma_state[i]; if (dma == NULL) { continue; } - bool block_done = event_interrupt_active(dma->event_channel); if (!block_done) { continue; } - - // audio_dma_load_next_block() can call Python code, which can call audio_dma_background() - // recursively at the next background processing time. So disallow recursive calls to here. - audio_dma_pending[i] = true; - audio_dma_load_next_block(dma); - audio_dma_pending[i] = false; + background_callback_add(&dma->callback, dma_callback_fun, (void*)dma); } } + +#ifdef SAM_D5X_E5X +void EVSYS_0_Handler(void) { evsyshandler_common(); } +void EVSYS_1_Handler(void) { evsyshandler_common(); } +void EVSYS_2_Handler(void) { evsyshandler_common(); } +void EVSYS_3_Handler(void) { evsyshandler_common(); } +void EVSYS_4_Handler(void) { evsyshandler_common(); } +#else +void EVSYS_Handler(void) { evsyshandler_common(); } +#endif + #endif diff --git a/ports/atmel-samd/audio_dma.h b/ports/atmel-samd/audio_dma.h index 1ebec6f7e9..4fffd06b8f 100644 --- a/ports/atmel-samd/audio_dma.h +++ b/ports/atmel-samd/audio_dma.h @@ -31,6 +31,7 @@ #include "py/obj.h" #include "shared-module/audiocore/RawSample.h" #include "shared-module/audiocore/WaveFile.h" +#include "supervisor/background_callback.h" typedef struct { mp_obj_t sample; @@ -49,6 +50,7 @@ typedef struct { uint8_t* second_buffer; bool first_descriptor_free; DmacDescriptor* second_descriptor; + background_callback_t callback; } audio_dma_t; typedef enum { diff --git a/ports/atmel-samd/background.c b/ports/atmel-samd/background.c index 767c7f3b6b..b903456ee8 100644 --- a/ports/atmel-samd/background.c +++ b/ports/atmel-samd/background.c @@ -77,9 +77,6 @@ void run_background_tasks(void) { assert_heap_ok(); running_background_tasks = true; - #if CIRCUITPY_AUDIOIO || CIRCUITPY_AUDIOBUSIO - audio_dma_background(); - #endif #if CIRCUITPY_DISPLAYIO displayio_background(); #endif From bdab6c12d4332523fdbd8967c1233b3708f4883e Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Mon, 11 May 2020 08:51:41 -0500 Subject: [PATCH 06/14] MP3Decoder: take advantage of background callback Before this, the mp3 file would be read into the in-memory buffer only when new samples were actually needed. This meant that the time to read mp3 content always counted against the ~22ms audio buffer length. Now, when there's at least 1 full disk block of free space in the input buffer, we can request that the buffer be filled _after_ returning from audiomp3_mp3file_get_buffer and actually filling the DMA pointers. In this way, the time taken for reading MP3 data from flash/SD is less likely to cause an underrun of audio DMA. The existing calls to fill the inbuf remain, but in most cases during streaming these become no-ops because the buffer will be over half full. --- shared-module/audiomp3/MP3Decoder.c | 54 +++++++++++++++++++++++------ shared-module/audiomp3/MP3Decoder.h | 2 ++ 2 files changed, 45 insertions(+), 11 deletions(-) diff --git a/shared-module/audiomp3/MP3Decoder.c b/shared-module/audiomp3/MP3Decoder.c index 30357c6161..5f3f42a51b 100644 --- a/shared-module/audiomp3/MP3Decoder.c +++ b/shared-module/audiomp3/MP3Decoder.c @@ -36,11 +36,12 @@ #include "shared-module/audiomp3/MP3Decoder.h" #include "supervisor/shared/translate.h" +#include "supervisor/background_callback.h" #include "lib/mp3/src/mp3common.h" #define MAX_BUFFER_LEN (MAX_NSAMP * MAX_NGRAN * MAX_NCHAN * sizeof(int16_t)) -/** Fill the input buffer if it is less than half full. +/** Fill the input buffer unconditionally. * * Returns true if the input buffer contains any useful data, * false otherwise. (The input buffer will be padded to the end with @@ -50,10 +51,7 @@ * * Sets self->eof if any read of the file returns 0 bytes */ -STATIC bool mp3file_update_inbuf(audiomp3_mp3file_obj_t* self) { - // If buffer is over half full, do nothing - if (self->inbuf_offset < self->inbuf_length/2) return true; - +STATIC bool mp3file_update_inbuf_always(audiomp3_mp3file_obj_t* self) { // If we didn't previously reach the end of file, we can try reading now if (!self->eof) { @@ -87,6 +85,26 @@ STATIC bool mp3file_update_inbuf(audiomp3_mp3file_obj_t* self) { return self->inbuf_offset < self->inbuf_length; } +/** Update the inbuf from a background callback. + * + * This variant is introduced so that at the site of the + * add_background_callback_core call, the prototype matches. + */ +STATIC void mp3file_update_inbuf_cb(void* self) { + mp3file_update_inbuf_always(self); +} + +/** Fill the input buffer if it is less than half full. + * + * Returns the same as mp3file_update_inbuf_always. + */ +STATIC bool mp3file_update_inbuf_half(audiomp3_mp3file_obj_t* self) { + // If buffer is over half full, do nothing + if (self->inbuf_offset < self->inbuf_length/2) return true; + + return mp3file_update_inbuf_always(self); +} + #define READ_PTR(self) (self->inbuf + self->inbuf_offset) #define BYTES_LEFT(self) (self->inbuf_length - self->inbuf_offset) #define CONSUME(self, n) (self->inbuf_offset += n) @@ -94,7 +112,7 @@ STATIC bool mp3file_update_inbuf(audiomp3_mp3file_obj_t* self) { // http://id3.org/d3v2.3.0 // http://id3.org/id3v2.3.0 STATIC void mp3file_skip_id3v2(audiomp3_mp3file_obj_t* self) { - mp3file_update_inbuf(self); + mp3file_update_inbuf_half(self); if (BYTES_LEFT(self) < 10) { return; } @@ -129,11 +147,11 @@ STATIC void mp3file_skip_id3v2(audiomp3_mp3file_obj_t* self) { */ STATIC bool mp3file_find_sync_word(audiomp3_mp3file_obj_t* self) { do { - mp3file_update_inbuf(self); + mp3file_update_inbuf_half(self); int offset = MP3FindSyncWord(READ_PTR(self), BYTES_LEFT(self)); if (offset >= 0) { CONSUME(self, offset); - mp3file_update_inbuf(self); + mp3file_update_inbuf_half(self); return true; } CONSUME(self, MAX(0, BYTES_LEFT(self) - 16)); @@ -209,12 +227,14 @@ void common_hal_audiomp3_mp3file_construct(audiomp3_mp3file_obj_t* self, } void common_hal_audiomp3_mp3file_set_file(audiomp3_mp3file_obj_t* self, pyb_file_obj_t* file) { + background_callback_begin_critical_section(); + self->file = file; f_lseek(&self->file->fp, 0); self->inbuf_offset = self->inbuf_length; self->eof = 0; self->other_channel = -1; - mp3file_update_inbuf(self); + mp3file_update_inbuf_half(self); mp3file_find_sync_word(self); // It **SHOULD** not be necessary to do this; the buffer should be filled // with fresh content before it is returned by get_buffer(). The fact that @@ -224,7 +244,9 @@ void common_hal_audiomp3_mp3file_set_file(audiomp3_mp3file_obj_t* self, pyb_file memset(self->buffers[0], 0, MAX_BUFFER_LEN); memset(self->buffers[1], 0, MAX_BUFFER_LEN); MP3FrameInfo fi; - if(!mp3file_get_next_frame_info(self, &fi)) { + bool result = mp3file_get_next_frame_info(self, &fi); + background_callback_end_critical_section(); + if (!result) { mp_raise_msg(&mp_type_RuntimeError, translate("Failed to parse MP3 file")); } @@ -277,13 +299,15 @@ void audiomp3_mp3file_reset_buffer(audiomp3_mp3file_obj_t* self, } // We don't reset the buffer index in case we're looping and we have an odd number of buffer // loads + background_callback_begin_critical_section(); f_lseek(&self->file->fp, 0); self->inbuf_offset = self->inbuf_length; self->eof = 0; self->other_channel = -1; - mp3file_update_inbuf(self); + mp3file_update_inbuf_half(self); mp3file_skip_id3v2(self); mp3file_find_sync_word(self); + background_callback_end_critical_section(); } audioio_get_buffer_result_t audiomp3_mp3file_get_buffer(audiomp3_mp3file_obj_t* self, @@ -321,6 +345,14 @@ audioio_get_buffer_result_t audiomp3_mp3file_get_buffer(audiomp3_mp3file_obj_t* uint8_t *inbuf = READ_PTR(self); int err = MP3Decode(self->decoder, &inbuf, &bytes_left, buffer, 0); CONSUME(self, BYTES_LEFT(self) - bytes_left); + + if (self->inbuf_offset >= 512) { + background_callback_add( + &self->inbuf_fill_cb, + mp3file_update_inbuf_cb, + self); + } + if (err) { return GET_BUFFER_DONE; } diff --git a/shared-module/audiomp3/MP3Decoder.h b/shared-module/audiomp3/MP3Decoder.h index 9ee1d0949b..f91f102a27 100644 --- a/shared-module/audiomp3/MP3Decoder.h +++ b/shared-module/audiomp3/MP3Decoder.h @@ -28,6 +28,7 @@ #ifndef MICROPY_INCLUDED_SHARED_MODULE_AUDIOIO_MP3FILE_H #define MICROPY_INCLUDED_SHARED_MODULE_AUDIOIO_MP3FILE_H +#include "supervisor/background_callback.h" #include "extmod/vfs_fat.h" #include "py/obj.h" @@ -36,6 +37,7 @@ typedef struct { mp_obj_base_t base; struct _MP3DecInfo *decoder; + background_callback_t inbuf_fill_cb; uint8_t* inbuf; uint32_t inbuf_length; uint32_t inbuf_offset; From af520729fe9d941c293942730f520b50ec1daf52 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Wed, 8 Jul 2020 10:32:06 -0500 Subject: [PATCH 07/14] displayio, framebufferio: Enable supervisor tick when a display is auto-refresh This is a step towards restoring the efficiency of the background tasks --- shared-module/displayio/Display.c | 12 ++++++++++-- shared-module/displayio/EPaperDisplay.c | 4 ++++ shared-module/framebufferio/FramebufferDisplay.c | 12 ++++++++++-- 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/shared-module/displayio/Display.c b/shared-module/displayio/Display.c index a3d877f1a9..46bb6fdb57 100644 --- a/shared-module/displayio/Display.c +++ b/shared-module/displayio/Display.c @@ -137,7 +137,7 @@ void common_hal_displayio_display_construct(displayio_display_obj_t* self, // Set the group after initialization otherwise we may send pixels while we delay in // initialization. common_hal_displayio_display_show(self, &circuitpython_splash); - self->auto_refresh = auto_refresh; + common_hal_displayio_display_set_auto_refresh(self, auto_refresh); } bool common_hal_displayio_display_show(displayio_display_obj_t* self, displayio_group_t* root_group) { @@ -383,6 +383,13 @@ bool common_hal_displayio_display_get_auto_refresh(displayio_display_obj_t* self void common_hal_displayio_display_set_auto_refresh(displayio_display_obj_t* self, bool auto_refresh) { self->first_manual_refresh = !auto_refresh; + if (auto_refresh != self->auto_refresh) { + if (auto_refresh) { + supervisor_enable_tick(); + } else { + supervisor_disable_tick(); + } + } self->auto_refresh = auto_refresh; } @@ -409,6 +416,7 @@ void displayio_display_background(displayio_display_obj_t* self) { } void release_display(displayio_display_obj_t* self) { + common_hal_displayio_display_set_auto_refresh(self, false); release_display_core(&self->core); #if (CIRCUITPY_PULSEIO) if (self->backlight_pwm.base.type == &pulseio_pwmout_type) { @@ -423,7 +431,7 @@ void release_display(displayio_display_obj_t* self) { } void reset_display(displayio_display_obj_t* self) { - self->auto_refresh = true; + common_hal_displayio_display_set_auto_refresh(self, true); self->auto_brightness = true; common_hal_displayio_display_show(self, NULL); } diff --git a/shared-module/displayio/EPaperDisplay.c b/shared-module/displayio/EPaperDisplay.c index 6a55687b9c..6d9e915b44 100644 --- a/shared-module/displayio/EPaperDisplay.c +++ b/shared-module/displayio/EPaperDisplay.c @@ -187,6 +187,7 @@ void displayio_epaperdisplay_finish_refresh(displayio_epaperdisplay_obj_t* self) displayio_display_core_begin_transaction(&self->core); self->core.send(self->core.bus, DISPLAY_COMMAND, self->chip_select, &self->refresh_display_command, 1); displayio_display_core_end_transaction(&self->core); + supervisor_enable_tick(); self->refreshing = true; displayio_display_core_finish_refresh(&self->core); @@ -301,6 +302,7 @@ bool common_hal_displayio_epaperdisplay_refresh(displayio_epaperdisplay_obj_t* s if (self->refreshing && self->busy.base.type == &digitalio_digitalinout_type) { if (common_hal_digitalio_digitalinout_get_value(&self->busy) != self->busy_state) { + supervisor_disable_tick(); self->refreshing = false; // Run stop sequence but don't wait for busy because busy is set when sleeping. send_command_sequence(self, false, self->stop_sequence, self->stop_sequence_len); @@ -342,6 +344,7 @@ void displayio_epaperdisplay_background(displayio_epaperdisplay_obj_t* self) { refresh_done = supervisor_ticks_ms64() - self->core.last_refresh > self->refresh_time; } if (refresh_done) { + supervisor_disable_tick(); self->refreshing = false; // Run stop sequence but don't wait for busy because busy is set when sleeping. send_command_sequence(self, false, self->stop_sequence, self->stop_sequence_len); @@ -352,6 +355,7 @@ void displayio_epaperdisplay_background(displayio_epaperdisplay_obj_t* self) { void release_epaperdisplay(displayio_epaperdisplay_obj_t* self) { if (self->refreshing) { wait_for_busy(self); + supervisor_disable_tick(); self->refreshing = false; // Run stop sequence but don't wait for busy because busy is set when sleeping. send_command_sequence(self, false, self->stop_sequence, self->stop_sequence_len); diff --git a/shared-module/framebufferio/FramebufferDisplay.c b/shared-module/framebufferio/FramebufferDisplay.c index f296da4095..7d09e0baeb 100644 --- a/shared-module/framebufferio/FramebufferDisplay.c +++ b/shared-module/framebufferio/FramebufferDisplay.c @@ -79,7 +79,7 @@ void common_hal_framebufferio_framebufferdisplay_construct(framebufferio_framebu // Set the group after initialization otherwise we may send pixels while we delay in // initialization. common_hal_framebufferio_framebufferdisplay_show(self, &circuitpython_splash); - self->auto_refresh = auto_refresh; + common_hal_framebufferio_framebufferdisplay_set_auto_refresh(self, auto_refresh); } bool common_hal_framebufferio_framebufferdisplay_show(framebufferio_framebufferdisplay_obj_t* self, displayio_group_t* root_group) { @@ -280,6 +280,13 @@ bool common_hal_framebufferio_framebufferdisplay_get_auto_refresh(framebufferio_ void common_hal_framebufferio_framebufferdisplay_set_auto_refresh(framebufferio_framebufferdisplay_obj_t* self, bool auto_refresh) { self->first_manual_refresh = !auto_refresh; + if (auto_refresh != self->auto_refresh) { + if (auto_refresh) { + supervisor_enable_tick(); + } else { + supervisor_disable_tick(); + } + } self->auto_refresh = auto_refresh; } @@ -297,12 +304,13 @@ void framebufferio_framebufferdisplay_background(framebufferio_framebufferdispla } void release_framebufferdisplay(framebufferio_framebufferdisplay_obj_t* self) { + common_hal_framebufferio_framebufferdisplay_set_auto_refresh(self, false); release_display_core(&self->core); self->framebuffer_protocol->deinit(self->framebuffer); } void reset_framebufferdisplay(framebufferio_framebufferdisplay_obj_t* self) { - self->auto_refresh = true; + common_hal_framebufferio_framebufferdisplay_set_auto_refresh(self, true); common_hal_framebufferio_framebufferdisplay_show(self, NULL); } From 36b46465167f15eaf8535608b16859e6de10fac8 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Fri, 10 Jul 2020 14:42:21 -0500 Subject: [PATCH 08/14] background_callback: Avoid CALLBACK_CRITICAL_BEGIN with nothing to do CALLBACK_CRITICAL_BEGIN is heavyweight, but we can be confident we do not have work to do as long as callback_head is NULL. This gives back performance on nRF. --- supervisor/shared/background_callback.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/supervisor/shared/background_callback.c b/supervisor/shared/background_callback.c index 99607b7a85..e45c9b5c3d 100644 --- a/supervisor/shared/background_callback.c +++ b/supervisor/shared/background_callback.c @@ -61,8 +61,11 @@ void background_callback_add(background_callback_t *cb, background_callback_fun static bool in_background_callback; void background_callback_run_all() { + if (!callback_head) { + return; + } CALLBACK_CRITICAL_BEGIN; - if(in_background_callback) { + if (in_background_callback) { CALLBACK_CRITICAL_END; return; } From 6160d11c5a04f60e54ce977fd73fafa5ba858aa0 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Tue, 7 Jul 2020 11:14:43 -0500 Subject: [PATCH 09/14] supervisor: factor out, Handle USB via background callback --- ports/atmel-samd/background.c | 1 - ports/atmel-samd/supervisor/usb.c | 12 +++++++----- ports/litex/mphalport.c | 3 ++- ports/mimxrt10xx/supervisor/usb.c | 3 ++- ports/nrf/background.c | 1 - ports/nrf/supervisor/usb.c | 7 +++++-- ports/stm/supervisor/usb.c | 2 +- supervisor/shared/usb/usb.c | 11 +++++++++++ supervisor/usb.h | 3 +++ 9 files changed, 31 insertions(+), 12 deletions(-) diff --git a/ports/atmel-samd/background.c b/ports/atmel-samd/background.c index b903456ee8..5c499ab3a8 100644 --- a/ports/atmel-samd/background.c +++ b/ports/atmel-samd/background.c @@ -85,7 +85,6 @@ void run_background_tasks(void) { network_module_background(); #endif filesystem_background(); - usb_background(); running_background_tasks = false; assert_heap_ok(); diff --git a/ports/atmel-samd/supervisor/usb.c b/ports/atmel-samd/supervisor/usb.c index 650ed6a397..ea940f8988 100644 --- a/ports/atmel-samd/supervisor/usb.c +++ b/ports/atmel-samd/supervisor/usb.c @@ -29,6 +29,8 @@ #include "hpl/gclk/hpl_gclk_base.h" #include "hal_gpio.h" #include "lib/tinyusb/src/device/usbd.h" +#include "supervisor/background_callback.h" +#include "supervisor/usb.h" void init_usb_hardware(void) { #ifdef SAMD21 @@ -61,24 +63,24 @@ void init_usb_hardware(void) { #ifdef SAMD21 void USB_Handler(void) { - tud_int_handler(0); + usb_irq_handler(); } #endif #ifdef SAM_D5X_E5X void USB_0_Handler (void) { - tud_int_handler(0); + usb_irq_handler(); } void USB_1_Handler (void) { - tud_int_handler(0); + usb_irq_handler(); } void USB_2_Handler (void) { - tud_int_handler(0); + usb_irq_handler(); } void USB_3_Handler (void) { - tud_int_handler(0); + usb_irq_handler(); } #endif diff --git a/ports/litex/mphalport.c b/ports/litex/mphalport.c index 84a5467951..862f163939 100644 --- a/ports/litex/mphalport.c +++ b/ports/litex/mphalport.c @@ -31,6 +31,7 @@ #include "py/mphal.h" #include "py/mpstate.h" #include "py/gc.h" +#include "supervisor/usb.h" #include "csr.h" #include "generated/soc.h" @@ -49,7 +50,7 @@ void isr(void) { #ifdef CFG_TUSB_MCU if (irqs & (1 << USB_INTERRUPT)) - tud_int_handler(0); + usb_irq_handler(); #endif if (irqs & (1 << TIMER0_INTERRUPT)) SysTick_Handler(); diff --git a/ports/mimxrt10xx/supervisor/usb.c b/ports/mimxrt10xx/supervisor/usb.c index 1bc7ea9b56..af259405a3 100644 --- a/ports/mimxrt10xx/supervisor/usb.c +++ b/ports/mimxrt10xx/supervisor/usb.c @@ -27,6 +27,7 @@ #include "fsl_clock.h" #include "tusb.h" +#include "supervisor/usb.h" void init_usb_hardware(void) { CLOCK_EnableUsbhs0PhyPllClock(kCLOCK_Usbphy480M, 480000000U); @@ -56,5 +57,5 @@ void init_usb_hardware(void) { } void USB_OTG1_IRQHandler(void) { - tud_int_handler(0); + usb_irq_handler(); } diff --git a/ports/nrf/background.c b/ports/nrf/background.c index 966c56e0b7..bc7c0d7d32 100644 --- a/ports/nrf/background.c +++ b/ports/nrf/background.c @@ -59,7 +59,6 @@ void run_background_tasks(void) { } running_background_tasks = true; filesystem_background(); - usb_background(); #if CIRCUITPY_AUDIOPWMIO audiopwmout_background(); #endif diff --git a/ports/nrf/supervisor/usb.c b/ports/nrf/supervisor/usb.c index 3d2527faaa..771e86ce03 100644 --- a/ports/nrf/supervisor/usb.c +++ b/ports/nrf/supervisor/usb.c @@ -30,6 +30,7 @@ #include "lib/utils/interrupt_char.h" #include "lib/mp-readline/readline.h" #include "lib/tinyusb/src/device/usbd.h" +#include "supervisor/background_callback.h" #ifdef SOFTDEVICE_PRESENT #include "nrf_sdm.h" @@ -42,7 +43,9 @@ extern void tusb_hal_nrf_power_event(uint32_t event); void init_usb_hardware(void) { - // 2 is max priority (0, 1 are reserved for SD) + // 2 is max priority (0, 1, and 4 are reserved for SD) + // 5 is max priority that still allows calling SD functions such as + // sd_softdevice_is_enabled NVIC_SetPriority(USBD_IRQn, 2); // USB power may already be ready at this time -> no event generated @@ -89,5 +92,5 @@ void init_usb_hardware(void) { } void USBD_IRQHandler(void) { - tud_int_handler(0); + usb_irq_handler(); } diff --git a/ports/stm/supervisor/usb.c b/ports/stm/supervisor/usb.c index 3dd0acafd0..3d53fa3749 100644 --- a/ports/stm/supervisor/usb.c +++ b/ports/stm/supervisor/usb.c @@ -130,5 +130,5 @@ void init_usb_hardware(void) { } void OTG_FS_IRQHandler(void) { - tud_int_handler(0); + usb_irq_handler(); } diff --git a/supervisor/shared/usb/usb.c b/supervisor/shared/usb/usb.c index edf8101188..472be96d52 100644 --- a/supervisor/shared/usb/usb.c +++ b/supervisor/shared/usb/usb.c @@ -27,6 +27,7 @@ #include "py/objstr.h" #include "shared-bindings/microcontroller/Processor.h" #include "shared-module/usb_midi/__init__.h" +#include "supervisor/background_callback.h" #include "supervisor/port.h" #include "supervisor/usb.h" #include "lib/utils/interrupt_char.h" @@ -82,6 +83,16 @@ void usb_background(void) { } } +static background_callback_t callback; +static void usb_background_do(void* unused) { + usb_background(); +} + +void usb_irq_handler(void) { + tud_int_handler(0); + background_callback_add(&callback, usb_background_do, NULL); +} + //--------------------------------------------------------------------+ // tinyusb callbacks //--------------------------------------------------------------------+ diff --git a/supervisor/usb.h b/supervisor/usb.h index 29280c725b..1bed9bbb4b 100644 --- a/supervisor/usb.h +++ b/supervisor/usb.h @@ -33,6 +33,9 @@ // alive and responsive. void usb_background(void); +// Ports must call this from their particular USB IRQ handler +void usb_irq_handler(void); + // Only inits the USB peripheral clocks and pins. The peripheral will be initialized by // TinyUSB. void init_usb_hardware(void); From 1df48176ce2e4111c4629269cb86d2a6e365a3d2 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Mon, 13 Jul 2020 10:33:58 -0500 Subject: [PATCH 10/14] supervisor: factor supervisor_background_tasks from sundry ports --- main.c | 2 - ports/atmel-samd/background.c | 48 ++--------------- ports/atmel-samd/background.h | 5 -- .../common-hal/frequencyio/FrequencyIn.c | 3 +- ports/atmel-samd/common-hal/pulseio/PulseIn.c | 3 +- ports/cxd56/background.c | 24 ++------- ports/cxd56/background.h | 3 -- ports/esp32s2/background.c | 25 ++------- ports/esp32s2/background.h | 3 -- ports/litex/background.c | 32 ++--------- ports/litex/background.h | 5 -- ports/mimxrt10xx/background.c | 53 ++----------------- ports/mimxrt10xx/background.h | 7 --- ports/mimxrt10xx/common-hal/pulseio/PulseIn.c | 2 +- ports/nrf/background.c | 27 ++-------- ports/nrf/background.h | 5 -- ports/stm/background.c | 28 ++-------- ports/stm/background.h | 5 -- supervisor/port.h | 8 +++ supervisor/shared/tick.c | 52 ++++++++++++++++-- supervisor/shared/tick.h | 16 +++--- 21 files changed, 95 insertions(+), 261 deletions(-) diff --git a/main.c b/main.c index 201022f1af..ce95de95f0 100755 --- a/main.c +++ b/main.c @@ -101,8 +101,6 @@ void start_mp(supervisor_allocation* heap) { reset_status_led(); autoreload_stop(); - background_tasks_reset(); - // Stack limit should be less than real stack size, so we have a chance // to recover from limit hit. (Limit is measured in bytes.) mp_stack_ctrl_init(); diff --git a/ports/atmel-samd/background.c b/ports/atmel-samd/background.c index 5c499ab3a8..62c233a3f8 100644 --- a/ports/atmel-samd/background.c +++ b/ports/atmel-samd/background.c @@ -39,59 +39,21 @@ #include "shared-module/displayio/__init__.h" #endif -volatile uint64_t last_finished_tick = 0; - -bool stack_ok_so_far = true; - -static bool running_background_tasks = false; - #ifdef MONITOR_BACKGROUND_TASKS // PB03 is physical pin "SCL" on the Metro M4 express // so you can't use this code AND an i2c peripheral // at the same time unless you change this -STATIC void start_background_task(void) { +void port_start_background_task(void) { REG_PORT_DIRSET1 = (1<<3); REG_PORT_OUTSET1 = (1<<3); } -STATIC void finish_background_task(void) { +void port_finish_background_task(void) { REG_PORT_OUTCLR1 = (1<<3); } #else -STATIC void start_background_task(void) {} -STATIC void finish_background_task(void) {} +void port_start_background_task(void) {} +void port_finish_background_task(void) {} #endif -void background_tasks_reset(void) { - running_background_tasks = false; -} - -void run_background_tasks(void) { - // Don't call ourselves recursively. - if (running_background_tasks) { - return; - } - - start_background_task(); - - assert_heap_ok(); - running_background_tasks = true; - - #if CIRCUITPY_DISPLAYIO - displayio_background(); - #endif - - #if CIRCUITPY_NETWORK - network_module_background(); - #endif - filesystem_background(); - running_background_tasks = false; - assert_heap_ok(); - - last_finished_tick = port_get_raw_ticks(NULL); - finish_background_task(); -} - -bool background_tasks_ok(void) { - return port_get_raw_ticks(NULL) - last_finished_tick < 1024; -} +void port_background_task(void) {} diff --git a/ports/atmel-samd/background.h b/ports/atmel-samd/background.h index d9866a6abc..2a89c3b1b8 100644 --- a/ports/atmel-samd/background.h +++ b/ports/atmel-samd/background.h @@ -29,9 +29,4 @@ #include -void background_tasks_reset(void); -void run_background_tasks(void); -void run_background_vm_tasks(void); -bool background_tasks_ok(void); - #endif // MICROPY_INCLUDED_ATMEL_SAMD_BACKGROUND_H diff --git a/ports/atmel-samd/common-hal/frequencyio/FrequencyIn.c b/ports/atmel-samd/common-hal/frequencyio/FrequencyIn.c index 1952df5637..02d0482dca 100644 --- a/ports/atmel-samd/common-hal/frequencyio/FrequencyIn.c +++ b/ports/atmel-samd/common-hal/frequencyio/FrequencyIn.c @@ -46,6 +46,7 @@ #include "hpl_gclk_config.h" #include "shared-bindings/time/__init__.h" +#include "supervisor/shared/tick.h" #include "supervisor/shared/translate.h" #ifdef SAMD21 @@ -132,7 +133,7 @@ void frequencyin_interrupt_handler(uint8_t index) { } // Check if we've reached the upper limit of detection - if (!background_tasks_ok() || self->errored_too_fast) { + if (!supervisor_background_tasks_ok() || self->errored_too_fast) { self->errored_too_fast = true; frequencyin_emergency_cancel_capture(i); } diff --git a/ports/atmel-samd/common-hal/pulseio/PulseIn.c b/ports/atmel-samd/common-hal/pulseio/PulseIn.c index b825579dbe..ae58b089de 100644 --- a/ports/atmel-samd/common-hal/pulseio/PulseIn.c +++ b/ports/atmel-samd/common-hal/pulseio/PulseIn.c @@ -42,6 +42,7 @@ #include "samd/timers.h" #include "shared-bindings/microcontroller/__init__.h" #include "shared-bindings/pulseio/PulseIn.h" +#include "supervisor/shared/tick.h" #include "supervisor/shared/translate.h" // This timer is shared amongst all PulseIn objects as a higher resolution clock. @@ -87,7 +88,7 @@ void pulsein_interrupt_handler(uint8_t channel) { uint32_t current_count = tc->COUNT16.COUNT.reg; pulseio_pulsein_obj_t* self = get_eic_channel_data(channel); - if (!background_tasks_ok() || self->errored_too_fast) { + if (!supervisor_background_tasks_ok() || self->errored_too_fast) { self->errored_too_fast = true; common_hal_pulseio_pulsein_pause(self); return; diff --git a/ports/cxd56/background.c b/ports/cxd56/background.c index ade257dd24..6de6d7275b 100644 --- a/ports/cxd56/background.c +++ b/ports/cxd56/background.c @@ -30,24 +30,6 @@ #include "supervisor/filesystem.h" #include "supervisor/shared/stack.h" -static bool running_background_tasks = false; - -void background_tasks_reset(void) { - running_background_tasks = false; -} - -void run_background_tasks(void) { - // Don't call ourselves recursively. - if (running_background_tasks) { - return; - } - - assert_heap_ok(); - running_background_tasks = true; - - usb_background(); - filesystem_background(); - - running_background_tasks = false; - assert_heap_ok(); -} +void port_background_task(void) {} +void port_start_background_task(void) {} +void port_finish_background_task(void) {} diff --git a/ports/cxd56/background.h b/ports/cxd56/background.h index a38e3faed4..5f76e64429 100644 --- a/ports/cxd56/background.h +++ b/ports/cxd56/background.h @@ -27,7 +27,4 @@ #ifndef MICROPY_INCLUDED_CXD56_BACKGROUND_H #define MICROPY_INCLUDED_CXD56_BACKGROUND_H -void background_tasks_reset(void); -void run_background_tasks(void); - #endif // MICROPY_INCLUDED_CXD56_BACKGROUND_H diff --git a/ports/esp32s2/background.c b/ports/esp32s2/background.c index a90fa7d0aa..40ce9ecfdf 100644 --- a/ports/esp32s2/background.c +++ b/ports/esp32s2/background.c @@ -35,27 +35,12 @@ #include "shared-module/displayio/__init__.h" #endif -static bool running_background_tasks = false; - -void background_tasks_reset(void) { - running_background_tasks = false; -} - -void run_background_tasks(void) { - // Don't call ourselves recursively. - if (running_background_tasks) { - return; - } +void port_background_task(void) { // Zero delay in case FreeRTOS wants to switch to something else. vTaskDelay(0); - running_background_tasks = true; - filesystem_background(); - - #if CIRCUITPY_DISPLAYIO - displayio_background(); - #endif - running_background_tasks = false; - - assert_heap_ok(); } + +void port_start_background_task(void) {} + +void port_finish_background_task(void) {} diff --git a/ports/esp32s2/background.h b/ports/esp32s2/background.h index 0e1fb7a568..cb850d4e5a 100644 --- a/ports/esp32s2/background.h +++ b/ports/esp32s2/background.h @@ -29,7 +29,4 @@ #include -void background_tasks_reset(void); -void run_background_tasks(void); - #endif // MICROPY_INCLUDED_ESP32S2_BACKGROUND_H diff --git a/ports/litex/background.c b/ports/litex/background.c index 8c18970434..174d9588ac 100644 --- a/ports/litex/background.c +++ b/ports/litex/background.c @@ -29,32 +29,6 @@ #include "supervisor/usb.h" #include "supervisor/shared/stack.h" -#if CIRCUITPY_DISPLAYIO -#include "shared-module/displayio/__init__.h" -#endif - -static bool running_background_tasks = false; - -void background_tasks_reset(void) { - running_background_tasks = false; -} - -void run_background_tasks(void) { - // Don't call ourselves recursively. - if (running_background_tasks) { - return; - } - running_background_tasks = true; - filesystem_background(); - - #if USB_AVAILABLE - usb_background(); - #endif - - #if CIRCUITPY_DISPLAYIO - displayio_background(); - #endif - running_background_tasks = false; - - assert_heap_ok(); -} +void port_background_task(void) {} +void port_start_background_task(void) {} +void port_finish_background_task(void) {} diff --git a/ports/litex/background.h b/ports/litex/background.h index 09551c7fbb..c80fbbe5cb 100644 --- a/ports/litex/background.h +++ b/ports/litex/background.h @@ -27,9 +27,4 @@ #ifndef MICROPY_INCLUDED_LITEX_BACKGROUND_H #define MICROPY_INCLUDED_LITEX_BACKGROUND_H -#include - -void background_tasks_reset(void); -void run_background_tasks(void); - #endif // MICROPY_INCLUDED_LITEX_BACKGROUND_H diff --git a/ports/mimxrt10xx/background.c b/ports/mimxrt10xx/background.c index ff53ea44f4..a8a613d41a 100644 --- a/ports/mimxrt10xx/background.c +++ b/ports/mimxrt10xx/background.c @@ -24,58 +24,13 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN * THE SOFTWARE. */ -#include "background.h" -//#include "audio_dma.h" -#include "supervisor/filesystem.h" -#include "supervisor/shared/tick.h" -#include "supervisor/usb.h" - -#include "py/runtime.h" -#include "shared-module/network/__init__.h" -#include "supervisor/linker.h" -#include "supervisor/shared/stack.h" - -#ifdef CIRCUITPY_DISPLAYIO -#include "shared-module/displayio/__init__.h" -#endif - -volatile uint64_t last_finished_tick = 0; - -bool stack_ok_so_far = true; - -static bool running_background_tasks = false; - -void background_tasks_reset(void) { - running_background_tasks = false; -} - -void PLACE_IN_ITCM(run_background_tasks)(void) { - // Don't call ourselves recursively. - if (running_background_tasks) { - return; - } - assert_heap_ok(); - running_background_tasks = true; +#include "supervisor/port.h" +void port_background_task(void) { #if CIRCUITPY_AUDIOIO || CIRCUITPY_AUDIOBUSIO audio_dma_background(); #endif - #if CIRCUITPY_DISPLAYIO - displayio_background(); - #endif - - #if CIRCUITPY_NETWORK - network_module_background(); - #endif - filesystem_background(); - usb_background(); - running_background_tasks = false; - assert_heap_ok(); - - last_finished_tick = supervisor_ticks_ms64(); -} - -bool background_tasks_ok(void) { - return supervisor_ticks_ms64() - last_finished_tick < 1000; } +void port_start_background_task(void) {} +void port_finish_background_task(void) {} diff --git a/ports/mimxrt10xx/background.h b/ports/mimxrt10xx/background.h index 52789d0389..a3fe102acc 100644 --- a/ports/mimxrt10xx/background.h +++ b/ports/mimxrt10xx/background.h @@ -28,11 +28,4 @@ #ifndef MICROPY_INCLUDED_MIMXRT10XX_BACKGROUND_H #define MICROPY_INCLUDED_MIMXRT10XX_BACKGROUND_H -#include - -void background_tasks_reset(void); -void run_background_tasks(void); -void run_background_vm_tasks(void); -bool background_tasks_ok(void); - #endif // MICROPY_INCLUDED_MIMXRT10XX_BACKGROUND_H diff --git a/ports/mimxrt10xx/common-hal/pulseio/PulseIn.c b/ports/mimxrt10xx/common-hal/pulseio/PulseIn.c index d8bf2017ea..ec02908612 100644 --- a/ports/mimxrt10xx/common-hal/pulseio/PulseIn.c +++ b/ports/mimxrt10xx/common-hal/pulseio/PulseIn.c @@ -64,7 +64,7 @@ // // last ms. // current_us = 1000 - current_us; // pulseio_pulsein_obj_t* self = get_eic_channel_data(channel); -// if (!background_tasks_ok() || self->errored_too_fast) { +// if (!supervisor_background_tasks_ok() || self->errored_too_fast) { // self->errored_too_fast = true; // common_hal_pulseio_pulsein_pause(self); // return; diff --git a/ports/nrf/background.c b/ports/nrf/background.c index bc7c0d7d32..10543ddb21 100644 --- a/ports/nrf/background.c +++ b/ports/nrf/background.c @@ -46,35 +46,14 @@ #include "common-hal/_bleio/bonding.h" #endif -static bool running_background_tasks = false; +void port_start_background_task(void) {} +void port_finish_background_task(void) {} -void background_tasks_reset(void) { - running_background_tasks = false; -} - -void run_background_tasks(void) { - // Don't call ourselves recursively. - if (running_background_tasks) { - return; - } - running_background_tasks = true; - filesystem_background(); +void port_background_task(void) { #if CIRCUITPY_AUDIOPWMIO audiopwmout_background(); #endif #if CIRCUITPY_AUDIOBUSIO i2s_background(); #endif - -#if CIRCUITPY_BLEIO - supervisor_bluetooth_background(); - bonding_background(); -#endif - - #if CIRCUITPY_DISPLAYIO - displayio_background(); - #endif - running_background_tasks = false; - - assert_heap_ok(); } diff --git a/ports/nrf/background.h b/ports/nrf/background.h index d53681c0fd..64a768cf9b 100644 --- a/ports/nrf/background.h +++ b/ports/nrf/background.h @@ -27,9 +27,4 @@ #ifndef MICROPY_INCLUDED_NRF_BACKGROUND_H #define MICROPY_INCLUDED_NRF_BACKGROUND_H -#include - -void background_tasks_reset(void); -void run_background_tasks(void); - #endif // MICROPY_INCLUDED_NRF_BACKGROUND_H diff --git a/ports/stm/background.c b/ports/stm/background.c index 8c18970434..d83a0ccec7 100644 --- a/ports/stm/background.c +++ b/ports/stm/background.c @@ -33,28 +33,6 @@ #include "shared-module/displayio/__init__.h" #endif -static bool running_background_tasks = false; - -void background_tasks_reset(void) { - running_background_tasks = false; -} - -void run_background_tasks(void) { - // Don't call ourselves recursively. - if (running_background_tasks) { - return; - } - running_background_tasks = true; - filesystem_background(); - - #if USB_AVAILABLE - usb_background(); - #endif - - #if CIRCUITPY_DISPLAYIO - displayio_background(); - #endif - running_background_tasks = false; - - assert_heap_ok(); -} +void port_background_task(void) {} +void port_start_background_task(void) {} +void port_finish_background_task(void) {} diff --git a/ports/stm/background.h b/ports/stm/background.h index 6225429f89..e57aa40dd7 100644 --- a/ports/stm/background.h +++ b/ports/stm/background.h @@ -27,9 +27,4 @@ #ifndef MICROPY_INCLUDED_STM32_BACKGROUND_H #define MICROPY_INCLUDED_STM32_BACKGROUND_H -#include - -void background_tasks_reset(void); -void run_background_tasks(void); - #endif // MICROPY_INCLUDED_STM32_BACKGROUND_H diff --git a/supervisor/port.h b/supervisor/port.h index 8a12d34c8a..ad5b3cf32a 100644 --- a/supervisor/port.h +++ b/supervisor/port.h @@ -91,4 +91,12 @@ void port_interrupt_after_ticks(uint32_t ticks); // Sleep the CPU until an interrupt is received. void port_sleep_until_interrupt(void); +// Execute port specific actions during background tasks. +void port_background_task(void); + +// Take port specific actions at the beginning and end of background tasks. +// This is used e.g., to set a monitoring pin for debug purposes. "Actual +// work" should be done in port_background_task() instead. +void port_start_background_task(void); +void port_finish_background_task(void); #endif // MICROPY_INCLUDED_SUPERVISOR_PORT_H diff --git a/supervisor/shared/tick.c b/supervisor/shared/tick.c index 46172d0f30..bc270030f3 100644 --- a/supervisor/shared/tick.c +++ b/supervisor/shared/tick.c @@ -32,8 +32,16 @@ #include "supervisor/background_callback.h" #include "supervisor/port.h" #include "supervisor/shared/autoreload.h" +#include "supervisor/shared/stack.h" -static volatile uint64_t PLACE_IN_DTCM_BSS(background_ticks); +#if CIRCUITPY_BLEIO +#include "supervisor/shared/bluetooth.h" +#include "common-hal/_bleio/bonding.h" +#endif + +#if CIRCUITPY_DISPLAYIO +#include "shared-module/displayio/__init__.h" +#endif #if CIRCUITPY_GAMEPAD #include "shared-module/gamepad/__init__.h" @@ -43,6 +51,10 @@ static volatile uint64_t PLACE_IN_DTCM_BSS(background_ticks); #include "shared-module/gamepadshift/__init__.h" #endif +#if CIRCUITPY_NETWORK +#include "shared-module/network/__init__.h" +#endif + #include "shared-bindings/microcontroller/__init__.h" #if CIRCUITPY_WATCHDOG @@ -52,12 +64,42 @@ static volatile uint64_t PLACE_IN_DTCM_BSS(background_ticks); #define WATCHDOG_EXCEPTION_CHECK() 0 #endif +static volatile uint64_t PLACE_IN_DTCM_BSS(background_ticks); + static background_callback_t callback; -extern void run_background_tasks(void); +volatile uint64_t last_finished_tick = 0; -void background_task_tick(void *unused) { - run_background_tasks(); +void supervisor_background_tasks(void *unused) { + port_start_background_task(); + + assert_heap_ok(); + + #if CIRCUITPY_DISPLAYIO + displayio_background(); + #endif + + #if CIRCUITPY_NETWORK + network_module_background(); + #endif + filesystem_background(); + + #if CIRCUITPY_BLEIO + supervisor_bluetooth_background(); + bonding_background(); + #endif + + port_background_task(); + + assert_heap_ok(); + + last_finished_tick = port_get_raw_ticks(NULL); + + port_finish_background_task(); +} + +bool supervisor_background_tasks_ok(void) { + return port_get_raw_ticks(NULL) - last_finished_tick < 1024; } void supervisor_tick(void) { @@ -77,7 +119,7 @@ void supervisor_tick(void) { #endif } #endif - background_callback_add(&callback, background_task_tick, NULL); + background_callback_add(&callback, supervisor_background_tasks, NULL); } uint64_t supervisor_ticks_ms64() { diff --git a/supervisor/shared/tick.h b/supervisor/shared/tick.h index e7e8080581..3a01bd6222 100644 --- a/supervisor/shared/tick.h +++ b/supervisor/shared/tick.h @@ -28,6 +28,7 @@ #define __INCLUDED_SUPERVISOR_TICK_H #include +#include /** @brief To be called once every ms * @@ -36,13 +37,6 @@ * interrupt context. */ extern void supervisor_tick(void); -/** @brief Cause background tasks to be called soon - * - * Normally, background tasks are only run once per tick. For other cases where - * an event noticed from an interrupt context needs to be completed by a background - * task activity, the interrupt can call supervisor_fake_tick. - */ -extern void supervisor_fake_tick(void); /** @brief Get the lower 32 bits of the time in milliseconds * * This can be more efficient than supervisor_ticks_ms64, for sites where a wraparound @@ -67,4 +61,12 @@ extern void supervisor_run_background_if_tick(void); extern void supervisor_enable_tick(void); extern void supervisor_disable_tick(void); +/** + * @brief Return true if tick-based background tasks ran within the last 1s + * + * Note that when ticks are not enabled, this function can return false; this is + * intended. + */ +extern bool supervisor_background_tasks_ok(void); + #endif From 81105cb9ef68c4d88eab206f111af448440c8f30 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Tue, 14 Jul 2020 10:08:07 -0500 Subject: [PATCH 11/14] supervisor: usb: note that it's unusual to need to call usb_background --- supervisor/usb.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/supervisor/usb.h b/supervisor/usb.h index 1bed9bbb4b..2a447c3686 100644 --- a/supervisor/usb.h +++ b/supervisor/usb.h @@ -29,8 +29,10 @@ #include -// Ports must call this as frequently as they can in order to keep the USB connection -// alive and responsive. +// Ports must call this as frequently as they can in order to keep the USB +// connection alive and responsive. Normally this is called from background +// tasks after the USB IRQ handler is executed, but in specific circumstances +// it may be necessary to call it directly. void usb_background(void); // Ports must call this from their particular USB IRQ handler From a18a39210919a886d4efb4c52b85a309b3403abf Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Fri, 17 Jul 2020 08:36:26 -0500 Subject: [PATCH 12/14] background_callback: Add gc collect callback A background callback must never outlive its related object. By collecting the head of the linked list of background tasks, this will not happen. One hypothetical case where this could happen is if an MP3Decoder is deleted while its callback to fill its buffer is scheduled. --- main.c | 2 ++ supervisor/background_callback.h | 5 +++++ supervisor/shared/background_callback.c | 6 ++++++ 3 files changed, 13 insertions(+) diff --git a/main.c b/main.c index ce95de95f0..f928d0d62f 100755 --- a/main.c +++ b/main.c @@ -493,6 +493,8 @@ void gc_collect(void) { // have lost their references in the VM even though they are mounted. gc_collect_root((void**)&MP_STATE_VM(vfs_mount_table), sizeof(mp_vfs_mount_t) / sizeof(mp_uint_t)); + background_callback_gc_collect(); + #if CIRCUITPY_DISPLAYIO displayio_gc_collect(); #endif diff --git a/supervisor/background_callback.h b/supervisor/background_callback.h index 82025f6b7a..535dd656be 100644 --- a/supervisor/background_callback.h +++ b/supervisor/background_callback.h @@ -79,4 +79,9 @@ void background_callback_reset(void); void background_callback_begin_critical_section(void); void background_callback_end_critical_section(void); +/* + * Background callbacks may stop objects from being collected + */ +void background_callback_gc_collect(void); + #endif diff --git a/supervisor/shared/background_callback.c b/supervisor/shared/background_callback.c index e45c9b5c3d..1be3cae2ba 100644 --- a/supervisor/shared/background_callback.c +++ b/supervisor/shared/background_callback.c @@ -24,6 +24,7 @@ * THE SOFTWARE. */ +#include "py/gc.h" #include "py/mpconfig.h" #include "supervisor/background_callback.h" #include "supervisor/shared/tick.h" @@ -105,3 +106,8 @@ void background_callback_reset() { in_background_callback = false; CALLBACK_CRITICAL_END; } + +void background_callback_gc_collect(void) { + background_callback_t *cb = (background_callback_t*)callback_head; + gc_collect_ptr(cb); +} From 6912d31560a617a134fcadbfdc1800a259f5e465 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Fri, 17 Jul 2020 14:32:05 -0500 Subject: [PATCH 13/14] uchip: reclaim some flash space --- ports/atmel-samd/boards/uchip/mpconfigboard.mk | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ports/atmel-samd/boards/uchip/mpconfigboard.mk b/ports/atmel-samd/boards/uchip/mpconfigboard.mk index 90b5600dcb..196068a1e0 100644 --- a/ports/atmel-samd/boards/uchip/mpconfigboard.mk +++ b/ports/atmel-samd/boards/uchip/mpconfigboard.mk @@ -9,3 +9,10 @@ CHIP_FAMILY = samd21 INTERNAL_FLASH_FILESYSTEM = 1 LONGINT_IMPL = NONE CIRCUITPY_FULL_BUILD = 0 + +# Tweak inlining depending on language. +ifeq ($(TRANSLATION), zh_Latn_pinyin) +CFLAGS_INLINE_LIMIT = 45 +else +CFLAGS_INLINE_LIMIT = 70 +endif From 98eef79faaa36adbdd0b074c712aad5df7ef37a0 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Fri, 17 Jul 2020 14:55:46 -0500 Subject: [PATCH 14/14] background_callback_gc_collect: We must traverse the whole list --- supervisor/shared/background_callback.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/supervisor/shared/background_callback.c b/supervisor/shared/background_callback.c index 1be3cae2ba..d10579c4f9 100644 --- a/supervisor/shared/background_callback.c +++ b/supervisor/shared/background_callback.c @@ -108,6 +108,23 @@ void background_callback_reset() { } void background_callback_gc_collect(void) { + // We don't enter the callback critical section here. We rely on + // gc_collect_ptr _NOT_ entering background callbacks, so it is not + // possible for the list to be cleared. + // + // However, it is possible for the list to be extended. We make the + // minor assumption that no newly added callback is for a + // collectable object. That is, we only plug the hole where an + // object becomes collectable AFTER it is added but before the + // callback is run, not the hole where an object was ALREADY + // collectable but adds a background task for itself. + // + // It's necessary to traverse the whole list here, as the callbacks + // themselves can be in non-gc memory, and some of the cb->data + // objects themselves might be in non-gc memory. background_callback_t *cb = (background_callback_t*)callback_head; - gc_collect_ptr(cb); + while(cb) { + gc_collect_ptr(cb->data); + cb = cb->next; + } }