Feat/support async api for led strip#681
Conversation
7220a92 to
4ea58b1
Compare
4ea58b1 to
d167f3c
Compare
d167f3c to
b172944
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds asynchronous refresh APIs for LED strip control, allowing the next frame to be computed while the current frame is being transmitted. It also adds support for custom LED timing configurations and runtime GPIO switching for the RMT backend.
Changes:
- Added async refresh APIs (
led_strip_refresh_asyncandled_strip_refresh_wait_async_done) to both RMT and SPI backends - Added
LED_MODEL_CUSTOMenum andled_strip_encoder_timings_tstruct for custom LED timing configuration in RMT backend - Added
led_strip_switch_gpioAPI for runtime GPIO switching (RMT backend only)
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| led_strip/idf_component.yml | Version bump from 3.0.3 to 3.0.4 |
| led_strip/CHANGELOG.md | Documents new async API, custom timing support, and GPIO switching feature |
| led_strip/include/led_strip_types.h | Adds LED_MODEL_CUSTOM enum and led_strip_encoder_timings_t struct; adds timings field to led_strip_config_t |
| led_strip/include/led_strip.h | Declares public async refresh and GPIO switching APIs |
| led_strip/include/led_strip_spi.h | Adds name to anonymous struct for better type identification |
| led_strip/interface/led_strip_interface.h | Adds function pointers for async refresh and GPIO switching to led_strip_t interface |
| led_strip/src/led_strip_api.c | Implements led_strip_switch_gpio wrapper function |
| led_strip/src/led_strip_rmt_encoder.h | Adds timings field to led_strip_encoder_config_t |
| led_strip/src/led_strip_rmt_encoder.c | Implements LED_MODEL_CUSTOM support with custom timings; converts if-else chain to switch statement; removes decimal points from WS2811 calculations |
| led_strip/src/led_strip_rmt_dev.c | Implements async refresh functions and GPIO switching for RMT backend |
| led_strip/src/led_strip_spi_dev.c | Implements async refresh functions for SPI backend; adds WS2816 clock resolution support; updates supported model warning |
| led_strip/examples/led_strip_rmt_ws2812/pytest_led_strip_rmt_ws2812.py | Adds pytest test case for RMT example |
| led_strip/examples/led_strip_spi_ws2812/pytest_led_strip_spi_ws2812.py | Adds pytest test case for SPI example |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,19 @@ | |||
| # SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD | |||
There was a problem hiding this comment.
The copyright year should be 2025 to match the other updated files in this PR (led_strip_spi_dev.c, led_strip_rmt_dev.c, led_strip_rmt_encoder.c all use 2022-2025). Since this is a new file being added in 2025, it should use "2025" instead of "2024".
| # SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD | |
| # SPDX-FileCopyrightText: 2025 Espressif Systems (Shanghai) CO LTD |
| led_model_t led_model; /*!< Specifies the LED strip model (e.g., WS2812, SK6812) */ | ||
| led_color_component_format_t color_component_format; /*!< Specifies the order of color components in each pixel. | ||
| Use helper macros like `LED_STRIP_COLOR_COMPONENT_FMT_GRB` to set the format */ | ||
| led_strip_encoder_timings_t timings; /*!< Encoder timings, only valid for RMT backend */ |
There was a problem hiding this comment.
The documentation for the timings field states "only valid for RMT backend", but it doesn't clarify what happens when LED_MODEL_CUSTOM is used. Consider adding a note that this field is required when using LED_MODEL_CUSTOM and must be initialized with appropriate values, otherwise it can be left uninitialized for other LED models.
| led_strip_encoder_timings_t timings; /*!< Encoder timings, only valid for RMT backend */ | |
| led_strip_encoder_timings_t timings; /*!< Encoder timings, only valid for RMT backend. | |
| When `led_model` is `LED_MODEL_CUSTOM`, this field is required and | |
| must be initialized with appropriate timing values. For other LED | |
| models, it may be left uninitialized, as model-specific defaults | |
| are used instead. */ |
| static esp_err_t led_strip_spi_refresh_async(led_strip_t *strip) | ||
| { | ||
| led_strip_spi_obj *spi_strip = __containerof(strip, led_strip_spi_obj, base); | ||
| spi_transaction_t tx_conf; | ||
| memset(&tx_conf, 0, sizeof(tx_conf)); | ||
| memset(&spi_strip->trans, 0, sizeof(spi_strip->trans)); | ||
| spi_strip->trans.length = spi_strip->strip_len * spi_strip->bytes_per_pixel * SPI_BITS_PER_COLOR_BYTE; | ||
| spi_strip->trans.tx_buffer = spi_strip->pixel_buf; | ||
| spi_strip->trans.rx_buffer = NULL; | ||
| ESP_RETURN_ON_ERROR(spi_device_queue_trans(spi_strip->spi_device, &spi_strip->trans, SPI_TRANS_MAX_DELAY), TAG, "transmit pixels by SPI failed"); |
There was a problem hiding this comment.
The async refresh API documentation states users should not modify LED colors until refresh_wait_async_done completes, but there's no enforcement mechanism. Since tx_buffer points directly to pixel_buf, modifying pixels via set_pixel or set_pixel_rgbw after calling refresh_async but before refresh_wait_async_done could corrupt the ongoing SPI transmission. Consider adding a flag to track whether an async operation is in progress and validate against concurrent modifications, or document this limitation more explicitly in the function documentation.
| #include "soc/spi_periph.h" | ||
| #include "led_strip.h" | ||
| #include "led_strip_interface.h" | ||
| #include "esp_heap_caps.h" |
There was a problem hiding this comment.
The include "esp_heap_caps.h" is duplicated. It appears on both line 12 and line 16. Please remove the duplicate include statement.
| #include "esp_heap_caps.h" |
|
|
||
| esp_err_t led_strip_switch_gpio(led_strip_handle_t strip, gpio_num_t new_gpio_num, bool invert_output) | ||
| { | ||
| ESP_RETURN_ON_FALSE(strip, ESP_ERR_INVALID_ARG, TAG, "invalid argument"); |
There was a problem hiding this comment.
The switch_gpio function pointer is called directly without checking if it's NULL. Since this function is only implemented for the RMT backend and not for the SPI backend, calling led_strip_switch_gpio on an SPI-backed LED strip will cause a NULL pointer dereference. Add a NULL check before calling the function pointer and return an appropriate error (e.g., ESP_ERR_NOT_SUPPORTED) if the backend doesn't support this operation.
| ESP_RETURN_ON_FALSE(strip, ESP_ERR_INVALID_ARG, TAG, "invalid argument"); | |
| ESP_RETURN_ON_FALSE(strip, ESP_ERR_INVALID_ARG, TAG, "invalid argument"); | |
| ESP_RETURN_ON_FALSE(strip->switch_gpio, ESP_ERR_NOT_SUPPORTED, TAG, "switch_gpio not supported by this backend"); |
| }, | ||
| .flags.msb_first = 1 | ||
| }; | ||
| reset_ticks = config->resolution / 1000000 * config->timings.reset / 2; // divide by 2... signal is sent twice |
There was a problem hiding this comment.
The reset_ticks calculation for LED_MODEL_CUSTOM may produce incorrect results due to integer division order. The expression "config->resolution / 1000000 * config->timings.reset / 2" should be reordered to "(config->resolution * config->timings.reset) / (1000000 * 2)" or use floating-point arithmetic to avoid precision loss. The timings.reset is in microseconds, so this needs to convert from microseconds to ticks correctly.
| @@ -29,6 +32,7 @@ typedef struct { | |||
| uint32_t strip_len; | |||
| uint8_t bytes_per_pixel; | |||
| led_color_component_format_t component_fmt; | |||
There was a problem hiding this comment.
The spi_transaction_t is stored in the led_strip_spi_obj structure and reused for each async refresh. This design limits the implementation to one in-flight transaction at a time. If a user calls refresh_async twice without calling refresh_wait_async_done in between, the second call will overwrite the transaction data and potentially cause incorrect behavior when spi_device_get_trans_result is called. Consider documenting this limitation or adding validation to prevent multiple concurrent async operations.
| led_color_component_format_t component_fmt; | |
| led_color_component_format_t component_fmt; | |
| // NOTE: This transaction object is reused for each async refresh. | |
| // This design only supports a single in-flight async SPI transaction | |
| // per led_strip_spi_obj. Users must not start a new async refresh | |
| // before the previous one has completed (e.g., before calling | |
| // refresh_wait_async_done for the prior operation). |
| esp_err_t led_strip_switch_gpio(led_strip_handle_t strip, gpio_num_t new_gpio_num, bool invert_output) | ||
| { | ||
| ESP_RETURN_ON_FALSE(strip, ESP_ERR_INVALID_ARG, TAG, "invalid argument"); | ||
| return strip->switch_gpio(strip, new_gpio_num, invert_output); | ||
| } |
There was a problem hiding this comment.
The new async refresh APIs (led_strip_refresh_async and led_strip_refresh_wait_async_done) are declared in the public header led_strip.h but are not implemented in led_strip_api.c. These wrapper functions should be added to maintain API consistency with other functions like led_strip_refresh, led_strip_clear, etc.
| dut.expect_exact('example: LED OFF!') | ||
| dut.expect_exact('example: LED ON!') | ||
|
|
||
|
|
There was a problem hiding this comment.
Trailing whitespace detected on line 19. Please remove the trailing whitespace to maintain code cleanliness.
| dut.expect_exact('example: LED OFF!') | ||
| dut.expect_exact('example: LED ON!') | ||
|
|
||
|
|
There was a problem hiding this comment.
Trailing whitespace detected on line 19. Please remove the trailing whitespace to maintain code cleanliness.
b172944 to
2757d85
Compare
Checklist
urlfield definedChange description
Please describe your change here