adafruit / Adafruit_CircuitPython_seesaw

seesaw helper IC driver for circuitPython
MIT License
60 stars 35 forks source link

RGBW NeoPixel support slightly broken #101

Closed caternuson closed 2 years ago

caternuson commented 2 years ago

The bpp parameter is being set separately from the pixel_order parameter with a default value of 3, and bpp has precedence. So the following example code results in unexpected behavior when using RGBW neopixels:

Adafruit CircuitPython 7.2.5 on 2022-04-06; Adafruit Feather M4 Express with samd51j19
>>> import board
>>> from adafruit_seesaw import seesaw, neopixel
>>> ss = seesaw.Seesaw(board.I2C())
>>> pixels = neopixel.NeoPixel(ss, 20, 12, pixel_order=neopixel.GRBW)
>>> pixels.fill(0xFF0000)
>>> 

image

By explicitly setting bpp=4 in addition to pixel_order, the behavior is correct.

Adafruit CircuitPython 7.2.5 on 2022-04-06; Adafruit Feather M4 Express with samd51j19
>>> import board
>>> from adafruit_seesaw import seesaw, neopixel
>>> ss = seesaw.Seesaw(board.I2C())
>>> pixels = neopixel.NeoPixel(ss, 20, 12, bpp=4, pixel_order=neopixel.GRBW)
>>> pixels.fill(0xFF0000)
>>> 

image

ahodsdon commented 2 years ago

It looks like there is some existing code that depends on the old (incorrect) behavior. For instance, the NeoKey1x4 driver has this:

https://github.com/adafruit/Adafruit_CircuitPython_NeoKey/blob/main/adafruit_neokey/neokey1x4.py: self.pixels = NeoPixel( self, _NEOKEY1X4_NEOPIX_PIN, _NEOKEY1X4_NUM_KEYS, brightness=0.2 )

The NeoKey1x4 only has an 24-bit Neopixel, though, and with #102 setting a color now overflows the pixel buffer by 1 byte.

vpicone commented 2 years ago

The neopixel trellis examples are broken, seems related to this. Reverting to 1.10.10 fixed the issue.

code.py output:
Traceback (most recent call last):
  File "code.py", line 44, in <module>
  File "adafruit_seesaw/neopixel.py", line 123, in __setitem__
ValueError: need more than 3 values to unpack
caternuson commented 2 years ago

@vpicone Thanks. This is sort of whack-a-mole. Fix one thing and it breaks another thing that was relying on the semi-broken behavior. This is essentially a new issue. Opened one here: https://github.com/adafruit/Adafruit_CircuitPython_NeoTrellis/issues/18

@ladyada Any idea why the GRBW default? instead of just RGB? https://github.com/adafruit/Adafruit_CircuitPython_seesaw/blob/1230da9e11381767c7298d41c029ba63672ee1a6/adafruit_seesaw/neopixel.py#L73

ladyada commented 2 years ago

@caternuson can this be closed now?

caternuson commented 2 years ago

@ladyada yep, i think so. i recreated original issue and verified fix. can reopen if needed if others still run into oddness.