From 596d469ecac8800db25b49cb1823a9f558fe6469 Mon Sep 17 00:00:00 2001 From: 3djc <3djc@gh.com> Date: Wed, 5 Aug 2026 14:42:05 +0200 Subject: [PATCH] fix: RGB leds signal left floating between frames --- .../src/targets/common/arm/stm32/stm32_dma.h | 26 ++++ .../common/arm/stm32/stm32_rgbleds.cpp | 122 ++++++++++-------- .../targets/common/arm/stm32/stm32_rgbleds.h | 5 - 3 files changed, 91 insertions(+), 62 deletions(-) diff --git a/radio/src/targets/common/arm/stm32/stm32_dma.h b/radio/src/targets/common/arm/stm32/stm32_dma.h index f12b4a849fc..6294ed1c39b 100644 --- a/radio/src/targets/common/arm/stm32/stm32_dma.h +++ b/radio/src/targets/common/arm/stm32/stm32_dma.h @@ -46,6 +46,18 @@ inline static bool stm32_dma_check_ht_flag(DMA_TypeDef* DMAx, uint32_t DMA_Strea return true; } +// A stream cannot be re-enabled while any of its event flags is still set +inline static void stm32_dma_clear_flags(DMA_TypeDef* DMAx, uint32_t DMA_Stream) +{ + LL_DMA_ClearFlag_TC(DMAx, DMA_Stream); + LL_DMA_ClearFlag_HT(DMAx, DMA_Stream); + LL_DMA_ClearFlag_DTE(DMAx, DMA_Stream); + LL_DMA_ClearFlag_ULE(DMAx, DMA_Stream); + LL_DMA_ClearFlag_USE(DMAx, DMA_Stream); + LL_DMA_ClearFlag_SUSP(DMAx, DMA_Stream); + LL_DMA_ClearFlag_TO(DMAx, DMA_Stream); +} + #else // STM32H7RS inline static bool stm32_dma_check_tc_flag(DMA_TypeDef* DMAx, uint32_t DMA_Stream) @@ -128,6 +140,20 @@ inline static bool stm32_dma_check_ht_flag(DMA_TypeDef* DMAx, uint32_t DMA_Strea return true; } +// FEIF | DMEIF | TEIF | HTIF | TCIF of one stream, in LIFCR (0-3) / HIFCR (4-7). +// A stream cannot be re-enabled while any of its event flags is still set. +inline static void stm32_dma_clear_flags(DMA_TypeDef* DMAx, uint32_t DMA_Stream) +{ + static const uint8_t _flag_pos[] = {0, 6, 16, 22}; + uint32_t mask = 0x3DUL << _flag_pos[DMA_Stream & 0x3]; + + if (DMA_Stream <= LL_DMA_STREAM_3) { + DMAx->LIFCR = mask; + } else { + DMAx->HIFCR = mask; + } +} + #endif // !STM32H7RS void stm32_dma_enable_clock(DMA_TypeDef* DMAx); diff --git a/radio/src/targets/common/arm/stm32/stm32_rgbleds.cpp b/radio/src/targets/common/arm/stm32/stm32_rgbleds.cpp index 739e5b5e49e..2cd193c7d5c 100644 --- a/radio/src/targets/common/arm/stm32/stm32_rgbleds.cpp +++ b/radio/src/targets/common/arm/stm32/stm32_rgbleds.cpp @@ -21,11 +21,10 @@ #include "stm32_rgbleds.h" #include "stm32_dma.h" +#include "stm32_gpio.h" -#if defined(DEBUG_RGBLEDS) - // LED_STRIP_DEBUG_GPIO && LED_STRIP_DEBUG_GPIO_PIN - #include "hal.h" -#endif +// LED_STRIP_LENGTH (and LED_STRIP_DEBUG_GPIO when DEBUG_RGBLEDS) +#include "hal.h" #include "definitions.h" @@ -41,12 +40,20 @@ static uint8_t _r_offset; static uint8_t _g_offset; static uint8_t _b_offset; -// DMA buffer contains data for 2 LEDs and is filled -// half by half on DMA HT and TC IRQs -#define RGBLEDS_DMA_BUFFER_HALF_LEN (RGBLEDS_BYTES_PER_LED * 8) -#define RGBLEDS_DMA_BUFFER_LEN (RGBLEDS_DMA_BUFFER_HALF_LEN * 2) +#define RGBLEDS_SLOTS_PER_LED (RGBLEDS_BYTES_PER_LED * 8) #define RGBLEDS_DMA_IRQ_PRIO 3 +// Zero slots appended to the frame: the line stays low while they are sent, +// so the end-of-frame IRQ may run late without emitting a spurious bit. +#define RGBLEDS_TRAIL_SLOTS 4 + +#if !defined(LED_STRIP_LENGTH) + #define LED_STRIP_LENGTH 1 +#endif + +#define RGBLEDS_FRAME_SLOTS \ + (LED_STRIP_LENGTH * RGBLEDS_SLOTS_PER_LED + RGBLEDS_TRAIL_SLOTS) + // Bit timing in ns (period / '1' HIGH / '0' HIGH), converted to timer ticks at // init from the actual timer clock so it holds on any clock domain. #if defined(RGB_LEDS_900NS) @@ -97,15 +104,17 @@ uint8_t pulse_inc = 1; static led_timer_value_t _led_one; static led_timer_value_t _led_zero; -// DMA buffer contains pulses for 2 LED at a time -// (allows for refill at HT and TC) +// The whole frame is emitted in a single DMA transfer: the buffer is filled +// before the transfer starts and no refill deadline exists, so an ISR delayed +// by a higher priority IRQ (SDMMC, USB) can no longer corrupt a frame. #if defined(STM32_SUPPORT_32BIT_TIMERS) -static led_timer_value_t _led_dma_buffer[RGBLEDS_DMA_BUFFER_LEN * 2] __DMA_NO_CACHE; +static led_timer_value_t _led_dma_buffer[RGBLEDS_FRAME_SLOTS * 2] __DMA_NO_CACHE; #else -static led_timer_value_t _led_dma_buffer[RGBLEDS_DMA_BUFFER_LEN] __DMA_NO_CACHE; +static led_timer_value_t _led_dma_buffer[RGBLEDS_FRAME_SLOTS] __DMA_NO_CACHE; #endif -static uint8_t _led_seq_cnt; +// Number of slots actually sent, from the strip length given at init +static uint32_t _frame_slots; static void _fill_byte(uint8_t c, led_timer_value_t* dma_buffer) { @@ -124,57 +133,34 @@ static void _fill_pulses(const uint8_t* colors, led_timer_value_t* dma_buffer, u } } -static inline uint32_t _calc_offset(uint8_t tc) -{ - return tc * RGBLEDS_DMA_BUFFER_HALF_LEN * pulse_inc; -} - -static void _update_dma_buffer(const stm32_pulse_timer_t* tim, uint8_t tc) +static void _end_of_frame(const stm32_pulse_timer_t* tim) { RGBLEDS_DBG_HIGH; - if (_led_seq_cnt < _led_strip_len) { - - auto idx = RGBLEDS_BYTES_PER_LED * _led_seq_cnt; - auto offset = _calc_offset(tc); - _fill_pulses(&_led_colors[idx], &_led_dma_buffer[offset], RGBLEDS_BYTES_PER_LED); - _led_seq_cnt++; - - } else if(_led_seq_cnt < _led_strip_len + RGBLEDS_TRAILING_RESET) { - - // no need to reset the buffer after 2 cycles - if (_led_seq_cnt < _led_strip_len + 2) { - auto offset = _calc_offset(tc); - auto size = RGBLEDS_DMA_BUFFER_HALF_LEN * sizeof(led_timer_value_t) * pulse_inc; - memset(&_led_dma_buffer[offset], 0, size); - } - _led_seq_cnt++; - } else { + LL_DMA_DisableIT_TC(tim->DMAx, tim->DMA_Stream); + LL_DMA_DisableStream(tim->DMAx, tim->DMA_Stream); - LL_DMA_DisableIT_TC(tim->DMAx, tim->DMA_Stream); - LL_DMA_DisableIT_HT(tim->DMAx, tim->DMA_Stream); - LL_DMA_DisableStream(tim->DMAx, tim->DMA_Stream); + uint32_t timeout = 1000; + while (LL_DMA_IsEnabledStream(tim->DMAx, tim->DMA_Stream) && timeout--) { + __NOP(); // Wait + } - uint32_t timeout = 1000; - while (LL_DMA_IsEnabledStream(tim->DMAx, tim->DMA_Stream) && timeout--) { - __NOP(); // Wait - } + // Stop the request source, but leave the channel and the counter running + // with a null compare value: the output is then actively held low between + // frames. Disabling the channel releases the pad instead (OSSR = 0), the + // line floats, and a WS2812 that misses its reset gap keeps counting bits + // across frames -- which shifts the whole strip by one LED. + LL_TIM_DisableDMAReq_UPDATE(tim->TIMx); + stm32_pulse_set_cmp_val(tim, 0); - LL_TIM_CC_DisableChannel(tim->TIMx, tim->TIM_Channel); - } RGBLEDS_DBG_LOW; } void rgbleds_dma_isr(const stm32_pulse_timer_t* tim) { - if (LL_DMA_IsEnabledIT_HT(tim->DMAx, tim->DMA_Stream) && - stm32_dma_check_ht_flag(tim->DMAx, tim->DMA_Stream)) { - _update_dma_buffer(tim, 0); - } - if (LL_DMA_IsEnabledIT_TC(tim->DMAx, tim->DMA_Stream) && stm32_dma_check_tc_flag(tim->DMAx, tim->DMA_Stream)) { - _update_dma_buffer(tim, 1); + _end_of_frame(tim); } } @@ -212,12 +198,17 @@ static void _init_timer(const stm32_pulse_timer_t* tim) _led_zero = RGBLEDS_NS_TO_TICKS(cnt_freq, RGBLEDS_T0H_NS); LL_TIM_SetAutoReload(tim->TIMx, period - 1); + // Insurance for the windows where the timer does not drive the pad (boot, + // de-init): a floating WS2812 input can miss the inter-frame reset. + LL_GPIO_SetPinPull(gpio_get_port(tim->GPIO), 1 << gpio_get_pin(tim->GPIO), + LL_GPIO_PULL_DOWN); + // pulse driver uses DMA to ARR, but we need CCRx _led_set_dma_periph_addr(tim); - LL_DMA_SetMode(tim->DMAx, tim->DMA_Stream, LL_DMA_MODE_CIRCULAR); - LL_DMA_SetDataLength(tim->DMAx, tim->DMA_Stream, RGBLEDS_DMA_BUFFER_LEN); - LL_DMA_SetMemoryAddress(tim->DMAx, tim->DMA_Stream, (uint32_t)_led_dma_buffer); + // One shot per frame: NDTR and the memory address are re-programmed by + // rgbleds_update() before every transfer. + LL_DMA_SetMode(tim->DMAx, tim->DMA_Stream, LL_DMA_MODE_NORMAL); // we need to use a higher prio to avoid having // issues with some other things used during boot @@ -230,8 +221,11 @@ void rgbleds_init(const stm32_pulse_timer_t* timer, uint8_t* strip_colors, RGBLEDS_DBG_INIT; pulse_inc = IS_TIM_32B_COUNTER_INSTANCE(timer->TIMx) ? 2 : 1; + if (strip_len > LED_STRIP_LENGTH) strip_len = LED_STRIP_LENGTH; + _led_colors = strip_colors; _led_strip_len = strip_len; + _frame_slots = strip_len * RGBLEDS_SLOTS_PER_LED + RGBLEDS_TRAIL_SLOTS; memset(_led_colors, 0, strip_len * RGBLEDS_BYTES_PER_LED); memset(_led_dma_buffer, 0, sizeof(_led_dma_buffer)); @@ -240,6 +234,12 @@ void rgbleds_init(const stm32_pulse_timer_t* timer, uint8_t* strip_colors, _b_offset = type & 0b11; _init_timer(timer); + + // Drive the data line low right away, and keep it driven until the first + // frame: an idle WS2812 input must never be left floating. + stm32_pulse_set_cmp_val(timer, 0); + LL_TIM_CC_EnableChannel(timer->TIMx, timer->TIM_Channel); + LL_TIM_EnableCounter(timer->TIMx); } void rgbleds_set_color_in_buf(uint8_t* buf, uint8_t led, @@ -279,13 +279,21 @@ void rgbleds_update(const stm32_pulse_timer_t* tim) RGBLEDS_DBG_HIGH; if (!stm32_pulse_if_not_running_disable(tim)) return; - _led_seq_cnt = 0; - memset(_led_dma_buffer, 0, sizeof(_led_dma_buffer)); + // Build the whole frame up front, trailing slots left at 0 + _fill_pulses(_led_colors, _led_dma_buffer, + _led_strip_len * RGBLEDS_BYTES_PER_LED); + memset(&_led_dma_buffer[_led_strip_len * RGBLEDS_SLOTS_PER_LED * pulse_inc], + 0, RGBLEDS_TRAIL_SLOTS * sizeof(led_timer_value_t) * pulse_inc); + + // NDTR and the address are not reloaded by enabling the stream again + stm32_dma_clear_flags(tim->DMAx, tim->DMA_Stream); + LL_DMA_SetMemoryAddress(tim->DMAx, tim->DMA_Stream, (uint32_t)_led_dma_buffer); + LL_DMA_SetDataLength(tim->DMAx, tim->DMA_Stream, _frame_slots); - LL_DMA_EnableIT_HT(tim->DMAx, tim->DMA_Stream); LL_DMA_EnableIT_TC(tim->DMAx, tim->DMA_Stream); LL_DMA_EnableStream(tim->DMAx, tim->DMA_Stream); + LL_TIM_SetCounter(tim->TIMx, 0); LL_TIM_EnableDMAReq_UPDATE(tim->TIMx); LL_TIM_CC_EnableChannel(tim->TIMx, tim->TIM_Channel); LL_TIM_EnableCounter(tim->TIMx); diff --git a/radio/src/targets/common/arm/stm32/stm32_rgbleds.h b/radio/src/targets/common/arm/stm32/stm32_rgbleds.h index 1f102481d45..907a18d1f07 100644 --- a/radio/src/targets/common/arm/stm32/stm32_rgbleds.h +++ b/radio/src/targets/common/arm/stm32/stm32_rgbleds.h @@ -30,11 +30,6 @@ // RGB #define RGBLEDS_BYTES_PER_LED 3 -// Number of LED periods used for trailing reset -#if !defined(RGBLEDS_TRAILING_RESET) -# define RGBLEDS_TRAILING_RESET 10 -#endif - void rgbleds_init(const stm32_pulse_timer_t* timer, uint8_t* strip_colors, uint8_t strip_len, uint8_t type); void rgbleds_update(const stm32_pulse_timer_t* timer);