Skip to content

Commit

Permalink
feat(led_strip): Support RGB pixel order
Browse files Browse the repository at this point in the history
  • Loading branch information
@Kainarx
Kainarx committed Aug 6, 2024
1 parent d9b31d9 commit ee682b8
Show file tree
Hide file tree
Showing 8 changed files with 107 additions and 51 deletions.
2 changes: 1 addition & 1 deletion led_strip/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
include($ENV{IDF_PATH}/tools/cmake/version.cmake)

set(srcs "src/led_strip_api.c")
set(srcs "src/led_strip_api.c" "src/led_strip_common.c")

if("${IDF_VERSION_MAJOR}.${IDF_VERSION_MINOR}" VERSION_GREATER_EQUAL "5.0")
if(CONFIG_SOC_RMT_SUPPORTED)
Expand Down
2 changes: 1 addition & 1 deletion led_strip/idf_component.yml
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
version: "2.5.4"
version: "2.5.5"
description: Driver for Addressable LED Strip (WS2812, etc)
url: https://github.com/espressif/idf-extra-components/tree/master/led_strip
dependencies:
Expand Down
32 changes: 32 additions & 0 deletions led_strip/include/esp_private/led_strip_common.h
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD
*
* SPDX-License-Identifier: Apache-2.0
*/
#pragma once

#include <stdint.h>
#include "esp_err.h"
#include "led_strip_types.h"

#ifdef __cplusplus
extern "C" {
#endif

#define LED_PIXEL_FORMAT_3COLORS_MAX LED_PIXEL_FORMAT_RGB

/**
* @brief Config LED pixel order
*
* @param led_pixel_offset Each pixel's offset
* @param led_pixel_format Input LED strip pixel format
* @return
* - ESP_OK: Config LED pixel order successfully
* - ESP_ERR_INVALID_ARG: Config LED pixel order failed because of invalid argument
* - ESP_FAIL: Config LED pixel order failed because some other error
*/
esp_err_t led_strip_config_pixel_order(uint8_t *led_pixel_offset, led_pixel_format_t led_pixel_format);

This comment has been minimized.

Sign in to view

Copy link
@robertlipe

robertlipe Aug 6, 2024

Contributor

This symbol will leak into user namespace. Maybe that's just the norm with C applications and is accepted risk, but now my (totally not contrived :-) program that #includes your public header and has a global bool named "led_strip_config_pixel_order" (to show that I've placed an order for a config pixel, of course... I said I'm reaching) will fail to build.

If you have convention of calling this _espressif_private_led_strip_config_pixel_order() or something, that would be better.

However, I thnk this symbol is long enough that the real world likelihood of conflict is zero. It's absolutely a tiny point.

#ifdef __cplusplus
}
#endif
2 changes: 2 additions & 0 deletions led_strip/include/led_strip_types.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ extern "C" {
*/
typedef enum {
LED_PIXEL_FORMAT_GRB, /*!< Pixel format: GRB */
LED_PIXEL_FORMAT_RGB, /*!< Pixel format: RGB */
LED_PIXEL_FORMAT_GRBW, /*!< Pixel format: GRBW */
LED_PIXEL_FORMAT_RGBW, /*!< Pixel format: RGBW */
LED_PIXEL_FORMAT_INVALID /*!< Invalid pixel format */
} led_pixel_format_t;

Expand Down
37 changes: 37 additions & 0 deletions led_strip/src/led_strip_common.c
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
#include <stdbool.h>
#include "esp_log.h"
#include "esp_check.h"
#include "esp_private/led_strip_common.h"

static const char *TAG = "led_strip_common";
esp_err_t led_strip_config_pixel_order(uint8_t *led_pixel_offset, led_pixel_format_t led_pixel_format)
{
ESP_RETURN_ON_FALSE(led_pixel_offset, ESP_ERR_INVALID_ARG, TAG, "invalid argument");

This comment has been minimized.

Sign in to view

Copy link
@robertlipe

robertlipe Aug 6, 2024

Contributor

Perhaps this is a common Espressif convention, but I had to puzzle that here there was an implicit conversion to bool (which would probably be fussed about in C++).
If this is doing an "0 == led_pixel_offset" (which would also be difficult to accidentally do in C++) I'd suggest this is probably clearer as an explicit logical test instead of an implicit cast to int/bool and test for 0/false.

This comment has been minimized.

Sign in to view

Copy link
@Kainarx

Kainarx Aug 15, 2024

Author Contributor

Yes this is common in Espressif, to prevent the pointer is NULL.
switch (led_pixel_format) {
case LED_PIXEL_FORMAT_GRB:
led_pixel_offset[0] = 1;
led_pixel_offset[1] = 0;
led_pixel_offset[2] = 2;
break;
case LED_PIXEL_FORMAT_RGB:
led_pixel_offset[0] = 0;
led_pixel_offset[1] = 1;
led_pixel_offset[2] = 2;
break;
case LED_PIXEL_FORMAT_GRBW:
led_pixel_offset[0] = 0;
led_pixel_offset[1] = 2;
led_pixel_offset[2] = 1;
led_pixel_offset[3] = 3;
break;
case LED_PIXEL_FORMAT_RGBW:
led_pixel_offset[0] = 0;
led_pixel_offset[1] = 1;
led_pixel_offset[2] = 2;
led_pixel_offset[3] = 3;
break;
default:
ESP_RETURN_ON_FALSE(false, ESP_ERR_INVALID_ARG, TAG, "invalid pixel format");
}
return ESP_OK;
}
31 changes: 13 additions & 18 deletions led_strip/src/led_strip_rmt_dev.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "led_strip.h"
#include "led_strip_interface.h"
#include "led_strip_rmt_encoder.h"
#include "esp_private/led_strip_common.h"

#define LED_STRIP_RMT_DEFAULT_RESOLUTION 10000000 // 10MHz resolution
#define LED_STRIP_RMT_DEFAULT_TRANS_QUEUE_SIZE 4
Expand All @@ -30,6 +31,7 @@ typedef struct {
rmt_encoder_handle_t strip_encoder;
uint32_t strip_len;
uint8_t bytes_per_pixel;
uint8_t led_pixel_offset[4];
uint8_t pixel_buf[];
} led_strip_rmt_obj;

Expand All @@ -38,10 +40,10 @@ static esp_err_t led_strip_rmt_set_pixel(led_strip_t *strip, uint32_t index, uin
led_strip_rmt_obj *rmt_strip = __containerof(strip, led_strip_rmt_obj, base);
ESP_RETURN_ON_FALSE(index < rmt_strip->strip_len, ESP_ERR_INVALID_ARG, TAG, "index out of maximum number of LEDs");
uint32_t start = index * rmt_strip->bytes_per_pixel;
// In thr order of GRB, as LED strip like WS2812 sends out pixels in this order
rmt_strip->pixel_buf[start + 0] = green & 0xFF;
rmt_strip->pixel_buf[start + 1] = red & 0xFF;
rmt_strip->pixel_buf[start + 2] = blue & 0xFF;
// Support all kinds of pixel order
rmt_strip->pixel_buf[start + rmt_strip->led_pixel_offset[0]] = red & 0xFF;
rmt_strip->pixel_buf[start + rmt_strip->led_pixel_offset[1]] = green & 0xFF;
rmt_strip->pixel_buf[start + rmt_strip->led_pixel_offset[2]] = blue & 0xFF;
if (rmt_strip->bytes_per_pixel > 3) {
rmt_strip->pixel_buf[start + 3] = 0;
}
Expand All @@ -53,12 +55,12 @@ static esp_err_t led_strip_rmt_set_pixel_rgbw(led_strip_t *strip, uint32_t index
led_strip_rmt_obj *rmt_strip = __containerof(strip, led_strip_rmt_obj, base);
ESP_RETURN_ON_FALSE(index < rmt_strip->strip_len, ESP_ERR_INVALID_ARG, TAG, "index out of maximum number of LEDs");
ESP_RETURN_ON_FALSE(rmt_strip->bytes_per_pixel == 4, ESP_ERR_INVALID_ARG, TAG, "wrong LED pixel format, expected 4 bytes per pixel");
uint8_t *buf_start = rmt_strip->pixel_buf + index * 4;
uint32_t start = index * rmt_strip->bytes_per_pixel;
// SK6812 component order is GRBW
*buf_start = green & 0xFF;
*++buf_start = red & 0xFF;
*++buf_start = blue & 0xFF;
*++buf_start = white & 0xFF;
rmt_strip->pixel_buf[start + rmt_strip->led_pixel_offset[0]] = red & 0xFF;

This comment has been minimized.

Sign in to view

Copy link
@robertlipe

robertlipe Aug 6, 2024

Contributor

This is also WAY off in the weeds, and I'm not going to build up a Goldbolt to prove or disprove it, but just in case the optimizer can't prove that these stores don't overlap and modify rmt_strip or something horrible and has to do a reload of rmt_strip (look at the assembly) the last remaining juice to squeeze here might be:
uint8_t* led_pixel_offsets = &rmt_strip->led_pixel_offset;
auto pixbuff = &rmt_strip->pixel_buf;
pixbuf[offsets[0]] = red & 0xff;
pixbuf[offsets[1]] = green & 0xff;
pixbuf[offsets[2]] = blue & 0xff;
pixbuf[offsets[3]] = white & 0xff;
Even writing it, I don't think it's as clear as what you've written, but it MIGHT help pointer clobber analysis. I'm not in love with this idea and saving eight cached reads vs. clarity isn't necessarily a slam dunk.

What do you think? I won't hard sell this.
rmt_strip->pixel_buf[start + rmt_strip->led_pixel_offset[1]] = green & 0xFF;
rmt_strip->pixel_buf[start + rmt_strip->led_pixel_offset[2]] = blue & 0xFF;
rmt_strip->pixel_buf[start + rmt_strip->led_pixel_offset[3]] = white & 0xFF;
return ESP_OK;
}

Expand Down Expand Up @@ -100,14 +102,7 @@ esp_err_t led_strip_new_rmt_device(const led_strip_config_t *led_config, const l
esp_err_t ret = ESP_OK;
ESP_GOTO_ON_FALSE(led_config && rmt_config && ret_strip, ESP_ERR_INVALID_ARG, err, TAG, "invalid argument");
ESP_GOTO_ON_FALSE(led_config->led_pixel_format < LED_PIXEL_FORMAT_INVALID, ESP_ERR_INVALID_ARG, err, TAG, "invalid led_pixel_format");
uint8_t bytes_per_pixel = 3;
if (led_config->led_pixel_format == LED_PIXEL_FORMAT_GRBW) {
bytes_per_pixel = 4;
} else if (led_config->led_pixel_format == LED_PIXEL_FORMAT_GRB) {
bytes_per_pixel = 3;
} else {
assert(false);
}
uint8_t bytes_per_pixel = led_config->led_pixel_format <= LED_PIXEL_FORMAT_3COLORS_MAX ? 3 : 4;
rmt_strip = calloc(1, sizeof(led_strip_rmt_obj) + led_config->max_leds * bytes_per_pixel);
ESP_GOTO_ON_FALSE(rmt_strip, ESP_ERR_NO_MEM, err, TAG, "no mem for rmt strip");
uint32_t resolution = rmt_config->resolution_hz ? rmt_config->resolution_hz : LED_STRIP_RMT_DEFAULT_RESOLUTION;
Expand Down Expand Up @@ -138,7 +133,7 @@ esp_err_t led_strip_new_rmt_device(const led_strip_config_t *led_config, const l
.led_model = led_config->led_model
};
ESP_GOTO_ON_ERROR(rmt_new_led_strip_encoder(&strip_encoder_conf, &rmt_strip->strip_encoder), err, TAG, "create LED strip encoder failed");

ESP_GOTO_ON_ERROR(led_strip_config_pixel_order(rmt_strip->led_pixel_offset, led_config->led_pixel_format), err, TAG, "config pixel order failed");

rmt_strip->bytes_per_pixel = bytes_per_pixel;
rmt_strip->strip_len = led_config->max_leds;
Expand Down
22 changes: 8 additions & 14 deletions led_strip/src/led_strip_rmt_dev_idf4.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "driver/rmt.h"
#include "led_strip.h"
#include "led_strip_interface.h"
#include "esp_private/led_strip_common.h"

static const char *TAG = "led_strip_rmt";

Expand Down Expand Up @@ -43,6 +44,7 @@ typedef struct {
rmt_channel_t rmt_channel;
uint32_t strip_len;
uint8_t bytes_per_pixel;
uint8_t led_pixel_offset[4];

This comment has been minimized.

Sign in to view

Copy link
@robertlipe

robertlipe Aug 6, 2024

Contributor

Not to push our luck, but while we all have our heads in this code...

RGBWW strips exist. (I have some lovely ones that offer nice power savings when you can use one of the white or off-white bulbs instead of lighting up the three others.) There's usually a Someone, some day is going to ask for this code to support them.

On that day, that person, and it might be anyone reading - or writing - this :-), is going to have to track down all the occurrences of "4" in these files, make them fives and add some +4's around. We'll hope that we found the all.

Balancing the problem above of symbol namespace pollution and sprinkling around magic constants, what's the breaking point where we need
enum class led_color_offsets { lco_red, lco_green, lco_blue, lco_w0, lco_ww };

(These are terrible names; please use whatever convention is appropriate.)

then
uint8_t led_pixel_offset[sizeof(led_color_offsets)];
and below is
pixbuf[offsets[cpo_red]] = red & 0xff;
pixbuf[offsets[cpo_green]] = green & 0xff;
pixbuf[offsets[cpo_blue]] = blue & 0xff;
pixbuf[offsets[cpo_w0]] = white0 & 0xff;
pixbuf[offsets[cpo_w1]] = white1 & 0xff;

(In case you run with this idea and either add rgbww now or just lay down and use the enums, if you're going to ask if there's a convention for the ordering of tht two whites in such strips, I've never seen one. It's just outlaw country. )

I think if you support c < 11 (? My C standard knowledge is getting rusty) the enum may need an extra tag for cpo_last or something because you can't get the size of an enum list, so you may have to add a symbol for that. I don't fully understand the traditions and conventions of this code base.

RGBW tends to fit into code nicely. RGBWW sometimes gets ugly as it no longer fits in a 32-bit value.

This comment has been minimized.

Sign in to view

Copy link
@Kainarx

Kainarx Aug 15, 2024

Author Contributor

Sorry for the late reply. About more color order support. I intend to implement this later via callback functions, where users can use their own configuration functions (including pixel order and number). This would eliminate the need to define a bunch of clutter in macro definitions. At that point, I'll open a new api capable of configuring any number of pixels. what do you think?
Anyway, thanks for the suggestion!
uint8_t buffer[0];
} led_strip_rmt_obj;

Expand Down Expand Up @@ -83,10 +85,10 @@ static esp_err_t led_strip_rmt_set_pixel(led_strip_t *strip, uint32_t index, uin
led_strip_rmt_obj *rmt_strip = __containerof(strip, led_strip_rmt_obj, base);
ESP_RETURN_ON_FALSE(index < rmt_strip->strip_len, ESP_ERR_INVALID_ARG, TAG, "index out of the maximum number of leds");
uint32_t start = index * rmt_strip->bytes_per_pixel;
// In thr order of GRB
rmt_strip->buffer[start + 0] = green & 0xFF;
rmt_strip->buffer[start + 1] = red & 0xFF;
rmt_strip->buffer[start + 2] = blue & 0xFF;
// Support all kinds of pixel order
rmt_strip->buffer[start + rmt_strip->led_pixel_offset[0]] = red & 0xFF;
rmt_strip->buffer[start + rmt_strip->led_pixel_offset[1]] = green & 0xFF;
rmt_strip->buffer[start + rmt_strip->led_pixel_offset[2]] = blue & 0xFF;
if (rmt_strip->bytes_per_pixel > 3) {
rmt_strip->buffer[start + 3] = 0;
}
Expand Down Expand Up @@ -125,16 +127,7 @@ esp_err_t led_strip_new_rmt_device(const led_strip_config_t *led_config, const l
ESP_RETURN_ON_FALSE(led_config && dev_config && ret_strip, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
ESP_RETURN_ON_FALSE(led_config->led_pixel_format < LED_PIXEL_FORMAT_INVALID, ESP_ERR_INVALID_ARG, TAG, "invalid led_pixel_format");
ESP_RETURN_ON_FALSE(dev_config->flags.with_dma == 0, ESP_ERR_NOT_SUPPORTED, TAG, "DMA is not supported");

uint8_t bytes_per_pixel = 3;
if (led_config->led_pixel_format == LED_PIXEL_FORMAT_GRBW) {
bytes_per_pixel = 4;
} else if (led_config->led_pixel_format == LED_PIXEL_FORMAT_GRB) {
bytes_per_pixel = 3;
} else {
assert(false);
}

uint8_t bytes_per_pixel = led_config->led_pixel_format <= LED_PIXEL_FORMAT_3COLORS_MAX ? 3 : 4;
// allocate memory for led_strip object
rmt_strip = calloc(1, sizeof(led_strip_rmt_obj) + led_config->max_leds * bytes_per_pixel);
ESP_RETURN_ON_FALSE(rmt_strip, ESP_ERR_NO_MEM, TAG, "request memory for les_strip failed");
Expand Down Expand Up @@ -174,6 +167,7 @@ esp_err_t led_strip_new_rmt_device(const led_strip_config_t *led_config, const l

// adapter to translates the LES strip date frame into RMT symbols
rmt_translator_init((rmt_channel_t)dev_config->rmt_channel, ws2812_rmt_adapter);
ESP_GOTO_ON_ERROR(led_strip_config_pixel_order(rmt_strip->led_pixel_offset, led_config->led_pixel_format), err, TAG, "config pixel order failed");

rmt_strip->bytes_per_pixel = bytes_per_pixel;
rmt_strip->rmt_channel = (rmt_channel_t)dev_config->rmt_channel;
Expand Down
30 changes: 13 additions & 17 deletions led_strip/src/led_strip_spi_dev.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@
#include "esp_check.h"
#include "esp_rom_gpio.h"
#include "soc/spi_periph.h"
#include "hal/spi_hal.h"
#include "led_strip.h"
#include "led_strip_interface.h"
#include "hal/spi_hal.h"
#include "esp_private/led_strip_common.h"

#define LED_STRIP_SPI_DEFAULT_RESOLUTION (2.5 * 1000 * 1000) // 2.5MHz resolution
#define LED_STRIP_SPI_DEFAULT_TRANS_QUEUE_SIZE 4
Expand All @@ -28,6 +29,7 @@ typedef struct {
spi_device_handle_t spi_device;
uint32_t strip_len;
uint8_t bytes_per_pixel;
uint8_t led_pixel_offset[4];
uint8_t pixel_buf[];
} led_strip_spi_obj;

Expand All @@ -51,12 +53,12 @@ static esp_err_t led_strip_spi_set_pixel(led_strip_t *strip, uint32_t index, uin
{
led_strip_spi_obj *spi_strip = __containerof(strip, led_strip_spi_obj, base);
ESP_RETURN_ON_FALSE(index < spi_strip->strip_len, ESP_ERR_INVALID_ARG, TAG, "index out of maximum number of LEDs");
// LED_PIXEL_FORMAT_GRB takes 72bits(9bytes)
// 3 pixels take 72bits(9bytes)
uint32_t start = index * spi_strip->bytes_per_pixel * SPI_BYTES_PER_COLOR_BYTE;
memset(spi_strip->pixel_buf + start, 0, spi_strip->bytes_per_pixel * SPI_BYTES_PER_COLOR_BYTE);
__led_strip_spi_bit(green, &spi_strip->pixel_buf[start]);
__led_strip_spi_bit(red, &spi_strip->pixel_buf[start + SPI_BYTES_PER_COLOR_BYTE]);
__led_strip_spi_bit(blue, &spi_strip->pixel_buf[start + SPI_BYTES_PER_COLOR_BYTE * 2]);
__led_strip_spi_bit(red, &spi_strip->pixel_buf[start + SPI_BYTES_PER_COLOR_BYTE * spi_strip->led_pixel_offset[0]]);
__led_strip_spi_bit(green, &spi_strip->pixel_buf[start + SPI_BYTES_PER_COLOR_BYTE * spi_strip->led_pixel_offset[1]]);
__led_strip_spi_bit(blue, &spi_strip->pixel_buf[start + SPI_BYTES_PER_COLOR_BYTE * spi_strip->led_pixel_offset[2]]);
if (spi_strip->bytes_per_pixel > 3) {
__led_strip_spi_bit(0, &spi_strip->pixel_buf[start + SPI_BYTES_PER_COLOR_BYTE * 3]);
}
Expand All @@ -72,10 +74,10 @@ static esp_err_t led_strip_spi_set_pixel_rgbw(led_strip_t *strip, uint32_t index
uint32_t start = index * spi_strip->bytes_per_pixel * SPI_BYTES_PER_COLOR_BYTE;
// SK6812 component order is GRBW
memset(spi_strip->pixel_buf + start, 0, spi_strip->bytes_per_pixel * SPI_BYTES_PER_COLOR_BYTE);
__led_strip_spi_bit(green, &spi_strip->pixel_buf[start]);
__led_strip_spi_bit(red, &spi_strip->pixel_buf[start + SPI_BYTES_PER_COLOR_BYTE]);
__led_strip_spi_bit(blue, &spi_strip->pixel_buf[start + SPI_BYTES_PER_COLOR_BYTE * 2]);
__led_strip_spi_bit(white, &spi_strip->pixel_buf[start + SPI_BYTES_PER_COLOR_BYTE * 3]);
__led_strip_spi_bit(red, &spi_strip->pixel_buf[start + SPI_BYTES_PER_COLOR_BYTE * spi_strip->led_pixel_offset[0]]);
__led_strip_spi_bit(green, &spi_strip->pixel_buf[start + SPI_BYTES_PER_COLOR_BYTE * spi_strip->led_pixel_offset[1]]);
__led_strip_spi_bit(blue, &spi_strip->pixel_buf[start + SPI_BYTES_PER_COLOR_BYTE * spi_strip->led_pixel_offset[2]]);
__led_strip_spi_bit(white, &spi_strip->pixel_buf[start + SPI_BYTES_PER_COLOR_BYTE * spi_strip->led_pixel_offset[3]]);

return ESP_OK;
}
Expand Down Expand Up @@ -125,14 +127,7 @@ esp_err_t led_strip_new_spi_device(const led_strip_config_t *led_config, const l
esp_err_t ret = ESP_OK;
ESP_GOTO_ON_FALSE(led_config && spi_config && ret_strip, ESP_ERR_INVALID_ARG, err, TAG, "invalid argument");
ESP_GOTO_ON_FALSE(led_config->led_pixel_format < LED_PIXEL_FORMAT_INVALID, ESP_ERR_INVALID_ARG, err, TAG, "invalid led_pixel_format");
uint8_t bytes_per_pixel = 3;
if (led_config->led_pixel_format == LED_PIXEL_FORMAT_GRBW) {
bytes_per_pixel = 4;
} else if (led_config->led_pixel_format == LED_PIXEL_FORMAT_GRB) {
bytes_per_pixel = 3;
} else {
assert(false);
}
uint8_t bytes_per_pixel = led_config->led_pixel_format <= LED_PIXEL_FORMAT_3COLORS_MAX ? 3 : 4;
uint32_t mem_caps = MALLOC_CAP_DEFAULT;
if (spi_config->flags.with_dma) {
// DMA buffer must be placed in internal SRAM
Expand Down Expand Up @@ -186,6 +181,7 @@ esp_err_t led_strip_new_spi_device(const led_strip_config_t *led_config, const l
// clock_resolution between 2.2MHz to 2.8MHz is supported
ESP_GOTO_ON_FALSE((clock_resolution_khz < LED_STRIP_SPI_DEFAULT_RESOLUTION / 1000 + 300) && (clock_resolution_khz > LED_STRIP_SPI_DEFAULT_RESOLUTION / 1000 - 300), ESP_ERR_NOT_SUPPORTED, err,
TAG, "unsupported clock resolution:%dKHz", clock_resolution_khz);
ESP_GOTO_ON_ERROR(led_strip_config_pixel_order(spi_strip->led_pixel_offset, led_config->led_pixel_format), err, TAG, "config pixel order failed");

spi_strip->bytes_per_pixel = bytes_per_pixel;
spi_strip->strip_len = led_config->max_leds;
Expand Down

1 comment on commit ee682b8

@robertlipe
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firstly: thank you!
This perfectly captured my feedback. I'm very happy with the result and if I had an "approve" button instead of "comment" button, I'd press it. It's a clear step forward, clean, maintainable. I like it.

With my head around the whole PR, I did see one very small opportunity that may or may not be worth taking, but this should totally be able to saturate the busses so it's unlikely to matter.

Everything I mentioned above is so tiny that I'd respect you no less if you just committed this as-is. It's good. The additional stuff is to help my understanding or possibly make future changes easier. If you'd like me to do it instead (i.e., someone with approval power thinks it's worth doing, but you don't want to do it), I'll pick up any subset of this feedback and submit it in a PR myself. I don't intend to ask for infinite changes in a PR.

I'd be pleased with this incorporated with or without any of the feedback in this round.

Thank you!

Please sign in to comment.