From 34c9e00f08b77f5336db911e376f15512bf0790b Mon Sep 17 00:00:00 2001 From: Roy Hooper Date: Wed, 8 Jan 2020 15:14:53 -0500 Subject: [PATCH 1/6] try (re)using the buffer in neopixel_write --- .../nrf/common-hal/neopixel_write/__init__.c | 37 ++++++++++++------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/ports/nrf/common-hal/neopixel_write/__init__.c b/ports/nrf/common-hal/neopixel_write/__init__.c index 3052e908d..d097f96e3 100644 --- a/ports/nrf/common-hal/neopixel_write/__init__.c +++ b/ports/nrf/common-hal/neopixel_write/__init__.c @@ -116,6 +116,8 @@ void common_hal_neopixel_write (const digitalio_digitalinout_obj_t* digitalinout uint32_t pattern_size = PATTERN_SIZE(numBytes); uint16_t* pixels_pattern = NULL; + static uint16_t* pixels_pattern_heap = NULL; + static size_t pixels_pattern_heap_size = 0; bool pattern_on_heap = false; // Use the stack to store 1 pixels worth of PWM data for the status led. uint32_t to ensure alignment. @@ -132,16 +134,29 @@ void common_hal_neopixel_write (const digitalio_digitalinout_obj_t* digitalinout } else { uint8_t sd_en = 0; (void) sd_softdevice_is_enabled(&sd_en); - if (sd_en) { - // If the soft device is enabled then we must use PWM to - // transmit. This takes a bunch of memory to do so raise an - // exception if we can't. - pixels_pattern = (uint16_t *) m_malloc(pattern_size, false); - } else { - pixels_pattern = (uint16_t *) m_malloc_maybe(pattern_size, false); - } - pattern_on_heap = true; + if (!pattern_on_heap || pixels_pattern_heap_size < pattern_size) { + if (pattern_on_heap) { + m_free(pixels_pattern_heap); + pixels_pattern = NULL; + pixels_pattern_heap = NULL; + pixels_pattern_heap_size = 0; + } + + if (sd_en) { + // If the soft device is enabled then we must use PWM to + // transmit. This takes a bunch of memory to do so raise an + // exception if we can't. + pixels_pattern_heap = (uint16_t *) m_malloc(pattern_size, false); + } else { + pixels_pattern_heap = (uint16_t *) m_malloc_maybe(pattern_size, false); + } + if (pixels_pattern_heap) { + pattern_on_heap = true; + pixels_pattern_heap_size = pattern_size; + } + } + pixels_pattern = pixels_pattern_heap; } } @@ -223,10 +238,6 @@ void common_hal_neopixel_write (const digitalio_digitalinout_obj_t* digitalinout nrf_pwm_disable(pwm); nrf_pwm_pins_set(pwm, (uint32_t[]) {0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL} ); - if (pattern_on_heap) { - m_free(pixels_pattern); - } - } // End of DMA implementation // --------------------------------------------------------------------- else { From 4e040b01522ff955cc480489de74e784876680be Mon Sep 17 00:00:00 2001 From: Roy Hooper Date: Wed, 8 Jan 2020 15:15:27 -0500 Subject: [PATCH 2/6] add reset of heap to board reset for nrf port --- ports/nrf/common-hal/neopixel_write/__init__.c | 14 ++++++++++---- ports/nrf/supervisor/port.c | 2 ++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/ports/nrf/common-hal/neopixel_write/__init__.c b/ports/nrf/common-hal/neopixel_write/__init__.c index d097f96e3..cf1b36705 100644 --- a/ports/nrf/common-hal/neopixel_write/__init__.c +++ b/ports/nrf/common-hal/neopixel_write/__init__.c @@ -97,6 +97,16 @@ static NRF_PWM_Type* find_free_pwm (void) { return NULL; } +static uint16_t* pixels_pattern_heap = NULL; +static size_t pixels_pattern_heap_size = 0; +static bool pattern_on_heap = false; +// Called during reset_port() to free the pattern buffer +void neopixel_write_reset(void) { + pixels_pattern_heap = NULL; + pixels_pattern_heap_size = 0; + pattern_on_heap = false; +} + uint64_t next_start_tick_ms = 0; uint32_t next_start_tick_us = 1000; @@ -113,12 +123,8 @@ void common_hal_neopixel_write (const digitalio_digitalinout_obj_t* digitalinout // using DWT #define PATTERN_SIZE(numBytes) (numBytes * 8 * sizeof(uint16_t) + 2 * sizeof(uint16_t)) - uint32_t pattern_size = PATTERN_SIZE(numBytes); uint16_t* pixels_pattern = NULL; - static uint16_t* pixels_pattern_heap = NULL; - static size_t pixels_pattern_heap_size = 0; - bool pattern_on_heap = false; // Use the stack to store 1 pixels worth of PWM data for the status led. uint32_t to ensure alignment. // Make it at least as big as PATTERN_SIZE(3), for one pixel of RGB data. diff --git a/ports/nrf/supervisor/port.c b/ports/nrf/supervisor/port.c index af858aa4a..d240e3be0 100644 --- a/ports/nrf/supervisor/port.c +++ b/ports/nrf/supervisor/port.c @@ -47,6 +47,7 @@ #include "common-hal/pulseio/PulseOut.h" #include "common-hal/pulseio/PulseIn.h" #include "common-hal/rtc/RTC.h" +#include "common-hal/neopixel_write/__init__.h" #include "tick.h" #include "shared-bindings/rtc/__init__.h" @@ -106,6 +107,7 @@ void reset_port(void) { i2c_reset(); spi_reset(); uart_reset(); + neopixel_write_reset(); #if CIRCUITPY_AUDIOBUSIO i2s_reset(); From e1c1e32ceb002fb7b710344a9997f6e2a6f2398b Mon Sep 17 00:00:00 2001 From: Roy Hooper Date: Wed, 8 Jan 2020 15:17:54 -0500 Subject: [PATCH 3/6] address code review --- ports/nrf/common-hal/neopixel_write/__init__.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ports/nrf/common-hal/neopixel_write/__init__.c b/ports/nrf/common-hal/neopixel_write/__init__.c index cf1b36705..91c68556f 100644 --- a/ports/nrf/common-hal/neopixel_write/__init__.c +++ b/ports/nrf/common-hal/neopixel_write/__init__.c @@ -141,7 +141,7 @@ void common_hal_neopixel_write (const digitalio_digitalinout_obj_t* digitalinout uint8_t sd_en = 0; (void) sd_softdevice_is_enabled(&sd_en); - if (!pattern_on_heap || pixels_pattern_heap_size < pattern_size) { + if (pixels_pattern_heap_size < pattern_size) { if (pattern_on_heap) { m_free(pixels_pattern_heap); pixels_pattern = NULL; From 1caf6bd8d3924b2f7861cd8b0af0d16a608bb736 Mon Sep 17 00:00:00 2001 From: Roy Hooper Date: Wed, 8 Jan 2020 15:23:38 -0500 Subject: [PATCH 4/6] add missing .h file --- .../nrf/common-hal/neopixel_write/__init__.h | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 ports/nrf/common-hal/neopixel_write/__init__.h diff --git a/ports/nrf/common-hal/neopixel_write/__init__.h b/ports/nrf/common-hal/neopixel_write/__init__.h new file mode 100644 index 000000000..b8dce85ad --- /dev/null +++ b/ports/nrf/common-hal/neopixel_write/__init__.h @@ -0,0 +1,32 @@ +/* + * This file is part of the MicroPython project, http://micropython.org/ + * + * The MIT License (MIT) + * + * Copyright (c) 2019 Lucian Copeland 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 MICROPY_INCLUDED_STM32F4_COMMON_HAL_NEOPIXEL_WRITE_INIT_H +#define MICROPY_INCLUDED_STM32F4_COMMON_HAL_NEOPIXEL_WRITE_INIT_H + +void neopixel_write_reset(void); + +#endif // MICROPY_INCLUDED_STM32F4_COMMON_HAL_NEOPIXEL_WRITE_INIT_H \ No newline at end of file From 9cf46ec9470c479a342d02ac18d0162298657c75 Mon Sep 17 00:00:00 2001 From: Dan Halbert Date: Fri, 21 Feb 2020 08:44:25 -0500 Subject: [PATCH 5/6] put neopixel_write buffer in root pointers --- .../nrf/common-hal/neopixel_write/__init__.c | 26 +++++++++---------- ports/nrf/mpconfigport.h | 1 + 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/ports/nrf/common-hal/neopixel_write/__init__.c b/ports/nrf/common-hal/neopixel_write/__init__.c index cd1b4fe59..ec313d044 100644 --- a/ports/nrf/common-hal/neopixel_write/__init__.c +++ b/ports/nrf/common-hal/neopixel_write/__init__.c @@ -25,6 +25,7 @@ */ #include "py/mphal.h" +#include "py/mpstate.h" #include "shared-bindings/neopixel_write/__init__.h" #include "nrf_pwm.h" @@ -97,14 +98,11 @@ static NRF_PWM_Type* find_free_pwm (void) { return NULL; } -static uint16_t* pixels_pattern_heap = NULL; static size_t pixels_pattern_heap_size = 0; -static bool pattern_on_heap = false; // Called during reset_port() to free the pattern buffer void neopixel_write_reset(void) { - pixels_pattern_heap = NULL; + MP_STATE_VM(pixels_pattern_heap) = NULL; pixels_pattern_heap_size = 0; - pattern_on_heap = false; } uint64_t next_start_tick_ms = 0; @@ -148,10 +146,11 @@ void common_hal_neopixel_write (const digitalio_digitalinout_obj_t* digitalinout (void) sd_softdevice_is_enabled(&sd_en); if (pixels_pattern_heap_size < pattern_size) { - if (pattern_on_heap) { - m_free(pixels_pattern_heap); + // Current heap buffer is too small. + if (MP_STATE_VM(pixels_pattern_heap)) { + // Old pixels_pattern_heap will be gc'd; don't free it. pixels_pattern = NULL; - pixels_pattern_heap = NULL; + MP_STATE_VM(pixels_pattern_heap) = NULL; pixels_pattern_heap_size = 0; } @@ -159,16 +158,17 @@ void common_hal_neopixel_write (const digitalio_digitalinout_obj_t* digitalinout // If the soft device is enabled then we must use PWM to // transmit. This takes a bunch of memory to do so raise an // exception if we can't. - pixels_pattern_heap = (uint16_t *) m_malloc(pattern_size, false); + MP_STATE_VM(pixels_pattern_heap) = (uint16_t *) m_malloc(pattern_size, false); } else { - pixels_pattern_heap = (uint16_t *) m_malloc_maybe(pattern_size, false); + // Might return NULL. + MP_STATE_VM(pixels_pattern_heap) = (uint16_t *) m_malloc_maybe(pattern_size, false); } - if (pixels_pattern_heap) { - pattern_on_heap = true; + if (MP_STATE_VM(pixels_pattern_heap)) { pixels_pattern_heap_size = pattern_size; } } - pixels_pattern = pixels_pattern_heap; + // Might be NULL, which means we failed to allocate. + pixels_pattern = MP_STATE_VM(pixels_pattern_heap); } } @@ -176,7 +176,7 @@ void common_hal_neopixel_write (const digitalio_digitalinout_obj_t* digitalinout wait_until(next_start_tick_ms, next_start_tick_us); // Use the identified device to choose the implementation - // If a PWM device is available use DMA + // If a PWM device is available and we have a buffer, use DMA. if ( (pixels_pattern != NULL) && (pwm != NULL) ) { uint16_t pos = 0; // bit position diff --git a/ports/nrf/mpconfigport.h b/ports/nrf/mpconfigport.h index 473169e94..44e51fc5f 100644 --- a/ports/nrf/mpconfigport.h +++ b/ports/nrf/mpconfigport.h @@ -164,6 +164,7 @@ #define MICROPY_PORT_ROOT_POINTERS \ CIRCUITPY_COMMON_ROOT_POINTERS \ + uint16_t* pixels_pattern_heap; \ ble_drv_evt_handler_entry_t* ble_drv_evt_handler_entries; \ From f63b2c0d0c364788f7951859733e6a5ca6150e0d Mon Sep 17 00:00:00 2001 From: Dan Halbert Date: Fri, 21 Feb 2020 17:36:15 -0500 Subject: [PATCH 6/6] use realloc instead --- ports/nrf/common-hal/neopixel_write/__init__.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/ports/nrf/common-hal/neopixel_write/__init__.c b/ports/nrf/common-hal/neopixel_write/__init__.c index ec313d044..0a4036aae 100644 --- a/ports/nrf/common-hal/neopixel_write/__init__.c +++ b/ports/nrf/common-hal/neopixel_write/__init__.c @@ -150,18 +150,21 @@ void common_hal_neopixel_write (const digitalio_digitalinout_obj_t* digitalinout if (MP_STATE_VM(pixels_pattern_heap)) { // Old pixels_pattern_heap will be gc'd; don't free it. pixels_pattern = NULL; - MP_STATE_VM(pixels_pattern_heap) = NULL; pixels_pattern_heap_size = 0; } + // realloc routines fall back to a plain malloc if the incoming ptr is NULL. if (sd_en) { // If the soft device is enabled then we must use PWM to // transmit. This takes a bunch of memory to do so raise an // exception if we can't. - MP_STATE_VM(pixels_pattern_heap) = (uint16_t *) m_malloc(pattern_size, false); + MP_STATE_VM(pixels_pattern_heap) = + (uint16_t *) m_realloc(MP_STATE_VM(pixels_pattern_heap), pattern_size); } else { // Might return NULL. - MP_STATE_VM(pixels_pattern_heap) = (uint16_t *) m_malloc_maybe(pattern_size, false); + MP_STATE_VM(pixels_pattern_heap) = + // true means move if necessary. + (uint16_t *) m_realloc_maybe(MP_STATE_VM(pixels_pattern_heap), pattern_size, true); } if (MP_STATE_VM(pixels_pattern_heap)) { pixels_pattern_heap_size = pattern_size;