fix(led_strip): Update LedStrip to support DMA #583
Conversation
|
⚡ Static analysis result ⚡ 🔴 cppcheck found 11 issues! Click here to see details.espp/components/led_strip/include/led_strip.hpp Lines 151 to 156 in 14d4409 !Line: 151 - warning: inconclusive: Access of moved variable 'other'. [accessMoved]
!Line: 150 - note: Calling std::move(other)
!Line: 151 - note: Access of moved variable 'other'.espp/components/led_strip/include/led_strip.hpp Lines 152 to 157 in 14d4409 !Line: 152 - warning: inconclusive: Access of moved variable 'other'. [accessMoved]
!Line: 150 - note: Calling std::move(other)
!Line: 152 - note: Access of moved variable 'other'.espp/components/led_strip/include/led_strip.hpp Lines 153 to 158 in 14d4409 !Line: 153 - warning: inconclusive: Access of moved variable 'other'. [accessMoved]
!Line: 150 - note: Calling std::move(other)
!Line: 153 - note: Access of moved variable 'other'.espp/components/led_strip/include/led_strip.hpp Lines 154 to 159 in 14d4409 !Line: 154 - warning: inconclusive: Access of moved variable 'other'. [accessMoved]
!Line: 150 - note: Calling std::move(other)
!Line: 154 - note: Access of moved variable 'other'.espp/components/led_strip/include/led_strip.hpp Lines 155 to 160 in 14d4409 !Line: 155 - warning: inconclusive: Access of moved variable 'other'. [accessMoved]
!Line: 150 - note: Calling std::move(other)
!Line: 155 - note: Access of moved variable 'other'.espp/components/led_strip/include/led_strip.hpp Lines 156 to 161 in 14d4409 !Line: 156 - warning: inconclusive: Access of moved variable 'other'. [accessMoved]
!Line: 150 - note: Calling std::move(other)
!Line: 156 - note: Access of moved variable 'other'.espp/components/led_strip/include/led_strip.hpp Lines 157 to 162 in 14d4409 !Line: 157 - warning: inconclusive: Access of moved variable 'other'. [accessMoved]
!Line: 150 - note: Calling std::move(other)
!Line: 157 - note: Access of moved variable 'other'.espp/components/led_strip/include/led_strip.hpp Lines 158 to 163 in 14d4409 !Line: 158 - warning: inconclusive: Access of moved variable 'other'. [accessMoved]
!Line: 150 - note: Calling std::move(other)
!Line: 158 - note: Access of moved variable 'other'.espp/components/led_strip/include/led_strip.hpp Lines 159 to 164 in 14d4409 !Line: 159 - warning: inconclusive: Access of moved variable 'other'. [accessMoved]
!Line: 150 - note: Calling std::move(other)
!Line: 159 - note: Access of moved variable 'other'.espp/components/led_strip/include/led_strip.hpp Lines 160 to 165 in 14d4409 !Line: 160 - warning: inconclusive: Access of moved variable 'other'. [accessMoved]
!Line: 150 - note: Calling std::move(other)
!Line: 160 - note: Access of moved variable 'other'.espp/components/led_strip/include/led_strip.hpp Lines 161 to 166 in 14d4409 !Line: 161 - warning: inconclusive: Access of moved variable 'other'. [accessMoved]
!Line: 150 - note: Calling std::move(other)
!Line: 161 - note: Access of moved variable 'other'. |
There was a problem hiding this comment.
Pull request overview
This pull request updates the LedStrip component to support DMA-accelerated memory allocation for improved performance with LED strip control. The change replaces std::vector<uint8_t> with manual memory management using raw pointers, allowing the use of ESP-IDF's heap capabilities API to allocate DMA-capable memory when needed.
Changes:
- Replaced
std::vector<uint8_t>with raw pointer + size for LED data buffer management - Added configuration options
use_dmaanddma_allocation_flagsto control DMA memory allocation - Implemented proper move semantics (copy constructor/assignment deleted, move constructor/assignment added)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| components/led_strip/include/led_strip.hpp | Core implementation: replaced vector with manual memory management, added DMA support configuration, implemented move semantics, updated methods to work with raw pointers |
| components/led_strip/example/main/led_strip_example.cpp | Updated example to demonstrate DMA usage with RMT peripheral |
| components/led_strip/CMakeLists.txt | Added "heap" dependency for heap_caps_malloc API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!data_) { | ||
| logger_.error("Failed to allocate memory for LED strip data"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The constructor logs an error when memory allocation fails but continues to leave the object in an invalid state. Methods like set_pixel, shift_left, and shift_right will attempt to dereference the nullptr data_ without checking, leading to undefined behavior. Consider throwing an exception or marking the object as invalid in a way that all methods can check.
| LedStrip &operator=(LedStrip &&other) noexcept { | ||
| if (this != &other) { | ||
| if (data_) { | ||
| free(data_); | ||
| } | ||
|
|
||
| num_leds_ = other.num_leds_; | ||
| send_brightness_ = other.send_brightness_; | ||
| byte_order_ = other.byte_order_; | ||
| pixel_size_ = other.pixel_size_; | ||
| start_offset_ = other.start_offset_; | ||
| end_offset_ = other.end_offset_; | ||
| data_size_ = other.data_size_; | ||
| data_ = other.data_; | ||
| write_ = std::move(other.write_); | ||
| use_dma_ = other.use_dma_; | ||
|
|
||
| other.data_ = nullptr; | ||
| other.data_size_ = 0; | ||
| } | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
The move assignment operator does not move-assign the BaseComponent base class. This means the logger state from the base class is not transferred. Consider adding BaseComponent::operator=(std::move(other)) before copying member variables.
| // Allocate memory based on DMA preference | ||
| if (use_dma_) { | ||
| uint32_t dma_flags = config.dma_allocation_flags; | ||
| if (dma_flags == 0) { | ||
| dma_flags = MALLOC_CAP_DMA; | ||
| } | ||
| data_ = (uint8_t *)heap_caps_malloc(data_size_, dma_flags); | ||
| if (!data_) { | ||
| logger_.warn("Failed to allocate DMA memory, falling back to regular malloc"); | ||
| data_ = (uint8_t *)malloc(data_size_); | ||
| } | ||
| } else { | ||
| data_ = (uint8_t *)malloc(data_size_); | ||
| } |
There was a problem hiding this comment.
The allocated memory for data_ is not initialized to zero. Only the start_frame and end_frame regions are explicitly set via memcpy. The LED pixel data region (between start and end frames) contains uninitialized memory, which could lead to displaying garbage colors until set_pixel or set_all is called. Consider using calloc instead of malloc or explicitly zeroing the allocated memory.
| std::rotate(data_ + start_offset_, data_ + start_offset_ + pixel_size_ * shift_by, | ||
| data_ + data_size_ - end_offset_); | ||
| } |
There was a problem hiding this comment.
The shift_left method does not check if data_ is nullptr before dereferencing it. If the constructor fails to allocate memory, calling this method will result in undefined behavior.
| std::rotate(std::reverse_iterator<uint8_t *>(data_ + data_size_ - end_offset_), | ||
| std::reverse_iterator<uint8_t *>(data_ + data_size_ - end_offset_) + | ||
| pixel_size_ * shift_by, | ||
| std::reverse_iterator<uint8_t *>(data_ + start_offset_)); |
There was a problem hiding this comment.
The shift_right method does not check if data_ is nullptr before dereferencing it. If the constructor fails to allocate memory, calling this method will result in undefined behavior.
| , byte_order_(config.byte_order) | ||
| , write_(config.write) { | ||
| , write_(config.write) | ||
| , use_dma_(config.use_dma) { |
There was a problem hiding this comment.
There's no validation that num_leds is greater than 0. If num_leds is 0, data_size_ could be very small (just the size of start_frame and end_frame), and various operations like shift_left, shift_right, and set_pixel would behave unexpectedly. Consider adding validation in the constructor to ensure num_leds is at least 1.
| , use_dma_(config.use_dma) { | |
| , use_dma_(config.use_dma) { | |
| // validate the number of LEDs to ensure internal invariants | |
| if (num_leds_ == 0) { | |
| logger_.error("Config.num_leds must be at least 1; defaulting to 1"); | |
| num_leds_ = 1; | |
| } |
| if (!data_) { | ||
| logger_.warn("Failed to allocate DMA memory, falling back to regular malloc"); | ||
| data_ = (uint8_t *)malloc(data_size_); | ||
| } |
There was a problem hiding this comment.
When DMA allocation fails, the code silently falls back to regular malloc with only a warning log. This could lead to subtle runtime issues if the user expects DMA memory but gets regular memory instead (e.g., if their hardware requires DMA for the write operation). Consider making this behavior configurable or failing more explicitly when DMA is requested but unavailable.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…fix/582-led-strip-dma
Description
Update
LedStripto replace use ofstd::vectorwith a pointer + size approach for managing LED color data, and adds configuration options for whether to use DMA / allocation bits for the LED strip.Motivation and Context
Closes #582 - allowing the LedStrip to be DMA-accelerated.
How has this been tested?
Tested on actual hardware with both DMA and non-DMA configurations, ensuring correct LED color output in both cases.
Screenshots (if appropriate, e.g. schematic, board, console logs, lab pictures):
Types of changes
Checklist:
Software
.github/workflows/build.ymlfile to add my new test to the automated cloud build github action.