RGBW NeoPixel setter: W only works with 4-element tuples, is ignored in packed integers

[PaintYourDragon]

CircuitPython version

Adafruit CircuitPython 7.0.0 but probably affects all versions.

Code/REPL

import board
import neopixel

pixels = neopixel.NeoPixel(board.D5, 8, brightness=1.0, auto_write=False, pixel_order=neopixel.GRBW)

pixels[0] = 0x40FF0000       # Should be white+red
pixels[1] = 0x4000FF00       # Should be white+green
pixels[2] = 0x400000FF       # Should be white+blue
pixels[3] = 0x40000000       # Should be white
pixels[4] = (255, 0, 0, 64) # Is white + red
pixels[5] = (0, 255, 0, 64) # Is green + red
pixels[6] = (0, 0, 255, 64) # Is blue + red
pixels[7] = (0, 0, 0, 64)   # Is white
pixels.show()

Behavior

No error, but incorrect colors from integer values; white is not set (tuples are correct).

What’s more, and particularly strange, is when attempting the white+blue integer setting above, if white is anything other than 0 or 255, the LED is lit green rather than blue (it might be a different primary if a strip uses different pixel_order, but this is what I’m seeing here with a particular RGBW strip), which kind of suggests some bits aren’t being properly masked or something.

Description

Please see this issue:
adafruit/Adafruit_CircuitPython_FancyLED#25

Additional information

No response

[PaintYourDragon]

More on the white+blue peculiarity:

pixels[2] = 0x400000FF # Should be white+blue
print(pixels[2])

One would expect:
(0, 0, 255, 64)
(or at least (0, 0, 255, 0) if you just want to say that RGBW ints aren’t allowed)
But instead yields:
(0, 1, 0, 0)

[dunkmann00]

Neopixels don't support setting the white pixel explicitly when setting with a number. You can set it implicitly if you set each rgb value equal to each other (i.e. 0xffffff), as then the library will turn off the color pixels and set the white pixel to that value. So:

>>> pixels[0] = 0xffffff
>>> pixels[0]
(0, 0, 0, 255)

If you'd like to explicitly set the white pixel, you need to set it as a tuple.

So most of the behavior you are describing makes sense. However, what's happening with the white+blue combo seems like a bug. The output should be (0, 0, 255, 0).

[tannewt]

The internal pixelbuf does the numeric conversion here: https://github.com/adafruit/circuitpython/blob/main/shared-module/adafruit_pixelbuf/PixelBuf.c#L155-L198

The Python version is here: https://github.com/adafruit/Adafruit_CircuitPython_Pixelbuf/blob/main/adafruit_pixelbuf.py#L218-L265

[dunkmann00]

@tannewt I checked the pixelbuf code, the problem only happens when using the CP version, the python version spits out an error.

Since it is only when using a number and not a tuple, the only line I can think that would be the culprit is:

circuitpython/shared-module/adafruit_pixelbuf/PixelBuf.c

Line 166 in 0292622

mp_int_t value = mp_obj_is_int(color) ? mp_obj_get_int_truncated(color) : mp_obj_get_float(color);

I could be wrong about that line being the problem, but either way I'd be happy to try and debug it later. Is there anything about that line that jumps out to you? I know there are different functions you can use to get an int, does mp_obj_get_int_truncated make sense here?

[tannewt]

Ah, right. This could have a problem on boards without long int support. Otherwise we won't be able to have all 32 bits set.

I think the functionality may need to change a little. If only <=24 bits are used then do the current behavior of using white when all channels equal. If >24 bits are used then assume the highest byte is white.

[PaintYourDragon]

Is this a good place to object to the behavior of setting the white channel when R==G==B?

• There are at least three different color temperatures of RGBW NeoPixels, and in none of them will R+G+B white accurately correspond to W white in either tint or brightness.
• Altering the numbers in results in the getter returning a value different than was passed to the setter, and this is certain to result in misunderstandings. While not exactly the same thing, the way the Arduino library handles brightness (doing the math when setting the pixel, not when issuing bits, because AVRs are slow) results in a similar set-and-get-don’t-match, and (even though documented) it’s been a source of confusion for users since day one, so much so that I discourage folks from even using setBrightness() and instead do their own math.

Really really very strongly feel that W in RGBW should be maintained as its own distinct thing, and up to user code how that distinct thing is handled.
Are boards without long int support a thing?

[tannewt]

Is this a good place to object to the behavior of setting the white channel when R==G==B?

This is a good place to discuss it.

• There are at least three different color temperatures of RGBW NeoPixels, and in none of them will R+G+B white accurately correspond to W white in either tint or brightness.

Ya, I get that. This was done as a "just works" option.

• Altering the numbers in results in the getter returning a value different than was passed to the setter, and this is certain to result in misunderstandings. While not exactly the same thing, the way the Arduino library handles brightness (doing the math when setting the pixel, not when issuing bits, because AVRs are slow) results in a similar set-and-get-don’t-match, and (even though documented) it’s been a source of confusion for users since day one, so much so that I discourage folks from even using setBrightness() and instead do their own math.

I'm not exactly sure what you are arguing for here. I think it's important to note that this behavior is not new for this library. I believe I added this W behavior in the first versions of the NeoPixel library four years ago or so. I don't think we have any super confusing aspects of the current implementation.

Really really very strongly feel that W in RGBW should be maintained as its own distinct thing, and up to user code how that distinct thing is handled.

W is available as it's own distinct thing with the tuple form for setting. I suppose you are arguing that it should be for the getter as well.

Are boards without long int support a thing?

Yes definitely. Most are SAMD21s without external flash. You can search for LONGINT_IMPL = NONE to see what boards don't set it.

Even with longints, a full 32 bit int will cause an allocation and be closer to what using a tuple would be. Maybe the goal here then should be to make the getter return tuple instead of int.

[PaintYourDragon]

Good points. Maybe user’s issue is best fixed over in FancyLED by providing some alternative to pack() that instead provides an integer 3-tuple. In the meantime, I’ll suggest they just keep doing what they’re doing. I mean it’s not wrong, just peculiar, and the first time I’ve seen it come up.