GD32E230 feature: add WS2812 support #270
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds WS2812 LED strip support to the AM32 firmware for the GD32E230 microcontroller. The implementation allows for addressable RGB LED control using Timer and DMA peripherals, with configuration support for two different pin assignments (PA2 and PB4).
Key Changes:
- Added WS2812 LED driver implementation with DMA-based data transmission
- Integrated conditional compilation support based on
USE_LED_STRIPand pin assignment macros - Updated timer initialization to conditionally enable clocks based on LED pin configuration
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Mcu/e230/Src/WS2812.c | New driver implementation for WS2812 LED control via Timer PWM and DMA |
| Mcu/e230/Inc/WS2812.h | Header file defining WS2812 API and buffer size constant |
| Mcu/e230/Src/peripherals.c | Added conditional WS2812 initialization and timer clock configuration |
| Mcu/e230/Src/gd32e23x_it.c | Added DMA interrupt handler for WS2812 LED updates |
| Mcu/e230/Src/IO.c | Wrapped timer resets with conditional compilation for pin-specific timers |
| Inc/targets.h | Added LED timer/DMA channel definitions for three target configurations |
| Keil_Projects/Am32e230.uvprojx | Added WS2812.c to build configurations and updated compiler version |
| Keil_Projects/Am32e230.uvoptx | Updated project options and target settings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rcu_periph_clock_enable(RCU_TIMER2); | ||
| rcu_periph_clock_enable(RCU_GPIOB); | ||
| #endif | ||
| #ifdef LED_USES_PA2 | ||
| rcu_periph_clock_enable(RCU_TIMER14); |
There was a problem hiding this comment.
Timer/pin mapping is incorrect. According to comments on lines 95-96, PA2 maps to TIM14 and PB4 maps to TIM2. However, line 86 enables TIMER2 for LED_USES_PB4, and line 90 enables TIMER14 for LED_USES_PA2, which is backwards. The timer clock enable calls should be swapped to match the correct pin-to-timer mapping.
| rcu_periph_clock_enable(RCU_TIMER2); | |
| rcu_periph_clock_enable(RCU_GPIOB); | |
| #endif | |
| #ifdef LED_USES_PA2 | |
| rcu_periph_clock_enable(RCU_TIMER14); | |
| rcu_periph_clock_enable(RCU_TIMER14); | |
| rcu_periph_clock_enable(RCU_GPIOB); | |
| #endif | |
| #ifdef LED_USES_PA2 | |
| rcu_periph_clock_enable(RCU_TIMER2); |
| // Configure GPIO | ||
| //PA2 --> TIM14 | ||
| //PB4 --> TIM2 | ||
| #ifdef LED_USES_PB4// that means the esc is using PA2 as signal pin, so we configure PB4 as ws2812 data pin |
There was a problem hiding this comment.
Missing space after #ifdef LED_USES_PB4 before the comment delimiter. Should be #ifdef LED_USES_PB4 //.
| // Bit 0: 0*31+28 = 28 (~311ns HIGH, ~939ns LOW) - blog CODE_0 | ||
| // Bit 1: 1*31+28 = 59 (~656ns HIGH, ~594ns LOW) - blog CODE_1 |
There was a problem hiding this comment.
Corrected spelling of 'blog' to 'below' in timing comments.
| // Bit 0: 0*31+28 = 28 (~311ns HIGH, ~939ns LOW) - blog CODE_0 | |
| // Bit 1: 1*31+28 = 59 (~656ns HIGH, ~594ns LOW) - blog CODE_1 | |
| // Bit 0: 0*31+28 = 28 (~311ns HIGH, ~939ns LOW) - below CODE_0 | |
| // Bit 1: 1*31+28 = 59 (~656ns HIGH, ~594ns LOW) - below CODE_1 |
| #include "targets.h" | ||
| #include "WS2812.h" |
There was a problem hiding this comment.
The WS2812.h header is included unconditionally but is only used within the #ifdef USE_LED_STRIP block in this file. The include should be wrapped with the same conditional to avoid unnecessary header inclusion when LED strip support is not enabled.
| #include "targets.h" | |
| #include "WS2812.h" | |
| #include "targets.h" | |
| #ifdef USE_LED_STRIP | |
| #include "WS2812.h" | |
| #endif |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //PA2 --> TIM14 | ||
| //PB4 --> TIM2 |
There was a problem hiding this comment.
The comments incorrectly map pin-to-timer relationships. According to the code at lines 86 and 90, PA2 uses TIMER14 and PB4 uses TIMER2, but the comments state the opposite. The comments should read: 'PA2 --> TIMER14' and 'PB4 --> TIMER2'.
| //PA2 --> TIM14 | |
| //PB4 --> TIM2 | |
| //PA2 --> TIMER14 | |
| //PB4 --> TIMER2 |
| // Configure GPIO | ||
| //PA2 --> TIM14 | ||
| //PB4 --> TIM2 | ||
| #ifdef LED_USES_PB4// that means the esc is using PA2 as signal pin, so we configure PB4 as ws2812 data pin |
There was a problem hiding this comment.
Missing space after the conditional directive. Should be '#ifdef LED_USES_PB4 //' for better readability.
| #ifdef LED_USES_PB4// that means the esc is using PA2 as signal pin, so we configure PB4 as ws2812 data pin | |
| #ifdef LED_USES_PB4 // that means the esc is using PA2 as signal pin, so we configure PB4 as ws2812 data pin |
| #ifdef LED_USES_PA2 | ||
| rcu_periph_clock_enable(RCU_TIMER2); | ||
| rcu_periph_clock_enable(RCU_TIMER14); | ||
| TIMER_CAR(TIMER2) = 0xFFFF; | ||
| TIMER_PSC(TIMER2) = 10; | ||
| #endif | ||
| #ifdef LED_USES_PB4 | ||
| rcu_periph_clock_enable(RCU_TIMER14); | ||
| TIMER_CAR(TIMER14) = 0xFFFF; | ||
| TIMER_PSC(TIMER14) = 10; | ||
| #endif |
There was a problem hiding this comment.
Duplicate timer initialization logic in peripherals.c (lines 278-287) and WS2812.c (lines 85-92). The timer clock enable calls are redundant since WS2812_Init() already performs comprehensive timer initialization. Consider removing the timer clock enable from peripherals.c or add a comment explaining why both are needed.
1 participant
Tested Target: SIGSA KRIOS E230 - GD HW Group A