search

adafruit / Adafruit_CircuitPython_seesaw

seesaw helper IC driver for circuitPython
MIT License
64 stars 36 forks source link

Change NeoPixel pixel order default behavior? #103

Closed caternuson closed 2 years ago

caternuson commented 2 years ago

Not sure why GRBW was chosen as default: https://github.com/adafruit/Adafruit_CircuitPython_seesaw/blob/1230da9e11381767c7298d41c029ba63672ee1a6/adafruit_seesaw/neopixel.py#L73

This PR: https://github.com/adafruit/Adafruit_CircuitPython_seesaw/pull/102 changed the behavior in a way that seems to be breaking things elsewhere. https://github.com/adafruit/Adafruit_CircuitPython_NeoTrellis/issues/18 https://github.com/adafruit/Adafruit_CircuitPython_NeoKey/issues/7

There's a simple fix for those - just explicitly specify pixel order, ex: https://github.com/adafruit/Adafruit_CircuitPython_NeoTrellis/pull/19

But maybe RGB or GRB is a better default?

Also, could potentially change to using strings instead of tuples to match the main NeoPixel library behavior. https://github.com/adafruit/Adafruit_CircuitPython_NeoPixel/blob/839b55ce88eaf04a60b3d147b13dbaa7d7404c14/neopixel.py#L115-L119

dastels commented 2 years ago

I don't think there is a good default. Either it's part of a set product in which case the api module (e.g. NeoKey1x4) can specify the order, or the choice of NeoPixel product is the user's choice in which case they can/should provide the order to match. In the seesaw.neopixel.NeoPixel constructor, the order should IMO be a required parameter.

caternuson commented 2 years ago

@dastels Agree. But I think providing a default is important for the sake of simplicity. So things "just work" without one needing to understand pixel order, etc. Of course the question then is what is the best default behavior? I guess that's been answered by #104.

Also agree it's a good idea for the set products like NeoKey to be explicit. Can work up some simple PRs to update those as we go - those PR's will be in the product specific libraries.