Add ability for boards to provide a custom pixel order in neopixelWrite() #10128
Conversation
|
|
👋 Hello sblantipodi, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
|
@sblantipodi - Have you tested that? |
|
@SuGlider sure... Lolin ESP32-C3 Mini for example uses RGB color order, Programmers should know what kind of color order to use for the board of their interest and call that function accordingly. For example: To light up the LED red on Lolin S3 I need to call To light up the LED red on Lolin C3 I need to call but this is not really correct because C3 is using RGB color order and S3 is using GRB color order. Having a double conversion inside the neopixelWrite function is somewhat misleading and not really needed in my opinion. Please take into consideration that color orders may varies a lot, from my experience there are a lot of leds using those color order:
even though RGB and GRB are the most used ones. |
|
@sblantipodi - I see. Thanks for the explanation. |
|
@sblantipodi - The way the PR is set, it doesn't work for the ESP DevKits that I have here. The order used by their LEDs ir GRB. |
@SuGlider yes, that will be the best way to do it IMHO. give me some minutes :) |
|
please leave arguments order to be red green blue as this is the logical order of colors used everywhere |
|
I added a I tested it on S3 from Lolin, Adafruit, UnexpectedMaker ESP DevKits will continue to work as before with no breaking changes, |
sblantipodi
changed the title
Test Results 56 files - 83 56 suites - 83 5m 10s ⏱️ - 1h 37m 50s Results for commit ec3d597. ± Comparison against base commit def319a. This pull request removes 9 tests.♻️ This comment has been updated with latest results. |
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Aren't there multiple independent implementations of neopixelwrite? If you're proposing a new API, it should be available in all of them. Isn't neopixelwrite really meant to control the on-board status indicator and not arbitrary strips you might connect? IMO, you're way better off just calling the constructor with the order you need and not trying to create a new API here. Otherwise, you're on the road to turn this into AdaFruitNeoPixel, FastLED, Makuna NeoPixel, or idf's led_strip . In fact, shouldn't this code really just call the latter? It's a bit ironic that this very week, we've been working this same issue in two of those projects, such as It's a messy topic. It's quite reasonable to want to use this library's neopixelWrite() to indicate, say, WiFi connection status for the onboard pixel while |
|
There is point here: it shall be as simple as possible and address the onboard LED only. In order to simplify it, a suggestion would be to define the RGB order in the The API would continue to be just Example: #define RGB_BUILTIN_ORDER LED_COLOR_ORDER_RGB
typedef enum {
LED_COLOR_ORDER_RGB,
LED_COLOR_ORDER_BGR,
LED_COLOR_ORDER_BRG,
LED_COLOR_ORDER_RBG,
LED_COLOR_ORDER_GBR,
LED_COLOR_ORDER_GRB
} rgb_led_color_order_t;
rgb_led_color_order_t rgb_led_color_order = LED_COLOR_ORDER_RGB;
#ifdef RGB_BUILTIN_ORDER
rgb_led_color_order = RGB_BUILTIN_ORDER;
#endif
// + the switch() and so on as proposed in the PR ....For instance, the Lolin ESP32-S3 Mini that uses GRB color order, would declare in its own #define RGB_BUILTIN_ORDER LED_COLOR_ORDER_GRB Would it be better @robertlipe, @sblantipodi and @me-no-dev? |
|
I agree with the goal of simplicity here. But first, a diversion into yak-shaving, so we can get all our parlance consistent. World Semi brand 2811's (the chips often on strips, commonly in 12 and 24v applications so you can attach multiple bulbs per pixel) often have bulbs in crazy orders despite the pins having labels like "green". The WS2812 is a package that integrates (the equivalent of) that 2811 chip into a package with three LEDs of different colors. Those LED appear in the packet order as GRB in the data packet. From this, we should derive that a WS2811 (which allows crazy color orders and voltages > 5 because the LEDs are separate from the TTL) should be UNcommon on a dev board and a real WS2812 (which allows only GRB) would be far more likely to appear on a dev board. Based on this, I'm going to ask and confirm if everyone is really really sure that they A) have a dev board (where cost and size rule) that is either taking the cost and size hit of a WS2811 and wiring them in a crazy order, B) have some kind of mutant device that isn't really a World Semi part (WS2812's are always GRB), or C) are actually talking external strips. There might have been a short time some 15 years ago (?) where WS2811 + 3 5V LEDs on a dev board made sense to save two GPIOs (At least to someone that never learned about Charlieplexing...) , but I just can't imagine we have a lot of A in the world of dev boards. There might be some kind of clone chips that do their own thing in color order, but that seems an awfully brave move to break compatiblity in such an important way. (Yes, I DO know what companies will do to save a sixteenth of a cent somewhere :-) ) SuGlider and I are agreeing this file/function isn't about C) at all. The number of places we can introduce inconsistencies here is large and if we start trying to fix software to reflect that one vendor is using some kind of hardware that isn't really a WS2812 at all, we open the door to battling later. The number of times I've had to dig into this to find the effects layer is coded for one order but the API is coded for a second, the driver layer is coded for a third, and the the strips are soldered in a wacky fourth order but it doesn't all QUITE cancel out (sometimes it does!) is ridiculous. The schematic for the non-compliant board you cite says they're using World Semi 2812B's in a 35x35mm package. There is no way to miswire that part to give different color orders. On to SuGlider's proposal: if you're not worried about binary compat or supporting multiple pilot lights or whatever, just "fix" it at compile time when you unpack the values into the the teensy little buffer that you're handing off to the RMT here. No runtime switch per pixel needed. But first, let's figure out what problem is REALLY trying to be solved here. TL;DR: I'm highly skeptical there is an RGB WS2812b. Stick your scope on the data pin and if they're real WS2812b's or even WS2812c, they're GRB at the packet level. P.S. May name may not be well known here, but I help heavily with a couple of Blinky LED projects. |
Not to over-correct my superiour (logical "elder" if not necessarily physically so :-) ) here in our first meeting, but the default is, and should remain, G R B. The incoming arguments in $a1, $a2, $a3 (sweeten calling convention to taste) are copied into a three byte buffer here: int color[] = {green_val, red_val, blue_val}; The bytes in color are then unpacked into the bits of the WS281x protocol and shoved into RMT format before being handed off to the RMT's DMAc to be shoved off the edge of the chip. So: The function argument order (which was changed above and then, thankfully, reverted) remain R G B , but the orders on the wire (the RMT output and the GPIO output and the LED's data input) are G R B. We all call it "RGB" because that's what it's been called everywhere forever, but the bits on the wire for these specific blinkies are ordered G R B. As a mnemonic, I remember it like the icky little potato worms or the Linux bootloader. The fact that I associate those two items is telling... And all of this is why this function should also not care about cold white or warm white, either. |
@robertlipe I double checked some of my ESPs and the Lolin ESP32-S3 is using RGB and not GRB, same thing for the TinyS3 from Unexpected Maker. As per WS2812B in general my firmware has lots of users using WS2812B RGB strips leave alone the more popular WLED, this just to tell you that the chip used on the LED does not have a specific color order. |
|
I just checked the schematic
https://www.wemos.cc/en/latest/_static/files/sch_s3_v1.0.0.pdf on the cited
board. It claims to use WS2812B which, per the manufacturer sheet I
referenced before, is GRB.
WLED, like
https://github.com/sblantipodi/glow_worm_luciferin/blob/master/platformio.ini[1]
it seems, uses Makuna Neopixel, which I also referenced before. It allows
arbitrary mappings because it touches strips and not just chips. As such,
they're not particularly partisan on default, but most of them defalt to
GRB, e.g.
https://github.com/Makuna/NeoPixelBus/blob/bf72b6ecc93c53f359ed28aea65776a753c376c8/examples/bitmaps/NeoPixelDibTest/NeoPixelDibTest.ino#L11
WLED's own default to GRB, but also allow infinite customizations since you
might have multiple strips of different types.
https://kno.wled.ge/features/settings/#led-outputs
I think we've agreed this API isn't for strips. Let's focus on the chip on
your board. Are you saying the WS2812b that is claimed to be on two
different boards from the same maker acts differently when it sees the same
waveform on the DIN pin? Or does one of these boards really not have a
World Semi 2812B in that position and instead features some other kind of
chip there? I'll accept answer (C) that world semi makes two different
types of WS2812Bs that are incompatibile, but I'll need to see a data
sheet, please, to recommend a way forward.
It may sound like I'm splitting hairs (that's a big part of what
engineering is about) but I'm trying to understand why two boards that
claim to be using the same chip are acting differently. That's not expected.
[1] But I don't see a call to arduino-esp32's neopixelwrite() in that code
anywhere
<https://github.com/search?q=repo%3Asblantipodi%2Fglow_worm_luciferin%20neopixelwrite&type=code>
anyway.
…On Fri, Aug 9, 2024 at 9:26 AM Davide Perini ***@***.***> wrote:
TL;DR: I'm highly skeptical there is an RGB WS2812b. Stick your scope on
the data pin and if they're real WS2812b's or even WS2812c
<https://cdn.sparkfun.com/assets/e/1/0/f/b/WS2812C-2020_V1.2_EN_19112716191654.pdf>,
they're GRB at the packet level.
@robertlipe <https://github.com/robertlipe> I double checked some of my
ESPs and the Lolin ESP32-S3 is using RGB and not GRB.
As per WS2812B in general my firmware
<https://github.com/sblantipodi/glow_worm_luciferin> has lots of users
using WS2812B RGB strips leave alone the more popular WLED
<https://kno.wled.ge/>
—
Reply to this email directly, view it on GitHub
<#10128 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCSD34WQUPEREFU3KN3S3LZQTGRHAVCNFSM6AAAAABMEFCKMCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZYGA3TGNZZHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@sblantipodi @robertlipe - Let's be practical. @sblantipodi says that he has a couple boards that have an RGB LED that works with a different color order. This can be tested using ESP32 Arduino Considering that it happens, some users may have some trouble with the onboard RGB LED. For this PR, I still suggest the approach described in #10128 (comment) |
|
Please consider solutions that don't insert RGB_BUILTIN_ORDER, rgb_led_color_order_t, and and rgb_led_color_order into the user's namespace. Those symbols are likely to collide for someone. You can do with this without introducing any extra CPU overhead (not an issue for an on-board status light; big issue for 10,000 pixels) that populates the struct in #10128 (comment) in the intended error. But, yes, if someone out there is mislabeling WS2812B's, then let's find a way to special case it in those board header, provide a sensible default, and as little runtime special casing (I'm leaning toward "zero") as possible. |
Something like this is not the most elegant way, but it for sure efficient.
// neopixelWrite() RMT will use G, R, B bits order
#define RGB_BUILTIN_COLOR_ORDER_STRUCT {green_val, red_val, blue_val}
#ifdef RGB_BUILTIN_COLOR_ORDER_STRUCT
int color[] = RGB_BUILTIN_COLOR_ORDER_STRUCT;
#else
// WS2812B color bit order is G, R, B
int color[] = {green_val, red_val, blue_val};
#endifFor instance, the Lolin ESP32-S3 Mini that uses GRB color order, would declare in its own // neopixelWrite() RMT will use R, G, B bits order
#define RGB_BUILTIN_COLOR_ORDER_STRUCT {red_val, green_val, blue_val}@robertlipe, @sblantipodi @me-no-dev |
I like this approach, I will not have access to my PC for some days, if no one will do it before me, I'll commit this idea as soon as I return home. |
I confirm my Lolin S3 Mini has different color order than my Lolin C3 mini. Don't remember who is RGB and who is GRB currently but using the same code, on one board the led is red and on another board is green. |
|
@sblantipodi - I'll modify it as requested by @me-no-dev, mixing some ideas we have discussed before. PR main goals:
|
|
@sblantipodi - please test using your LolinS3 Mini board ... CI won't test it at all. |
|
given that the |
|
@SuGlider it works as expected |
Made GRB default + switch/case exceptions.
If RGB_BUILTIN_LED_COLOR_ORDER is not defined, the type rgb_led_color_order_t won't be declared.
|
@sblantipodi @robertlipe @SuGlider PTAL at the latest changes |
| void rgbLedWrite(uint8_t pin, uint8_t red_val, uint8_t green_val, uint8_t blue_val); | ||
|
|
||
| // Backward compatibility | ||
| #define neopixelWrite(p, r, g, b) rgbLedWrite(p, r, g, b) |
There was a problem hiding this comment.
this is something that we will deal with separately for other reasons.
The hint for the builtin neopixleWrite() function has the wrong information.
It shows
void neopixelWrite(uint8_t pin, uint8_t red_val, uint8_t green_val, uint8_t blue_val)but then the color order is swapped to GRB.
Supposed that we want to use RGB color order as default one, function must match this order.
The BlinkRGB.ino example also is incorrect in the color order with the current implementation.