fix(radio): RGB leds signal left floating between frames

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);

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);

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);