Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change NeoPixel pixel order default behavior? #103

Closed
caternuson opened this issue Jun 16, 2022 · 2 comments
Closed

Change NeoPixel pixel order default behavior? #103

caternuson opened this issue Jun 16, 2022 · 2 comments

Comments

@caternuson
Copy link
Contributor

caternuson commented Jun 16, 2022

Not sure why GRBW was chosen as default:

self._pixel_order = GRBW if pixel_order is None else pixel_order

This PR:
#102
changed the behavior in a way that seems to be breaking things elsewhere.
adafruit/Adafruit_CircuitPython_NeoTrellis#18
adafruit/Adafruit_CircuitPython_NeoKey#7

There's a simple fix for those - just explicitly specify pixel order, ex:
adafruit/Adafruit_CircuitPython_NeoTrellis#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
Copy link
Collaborator

dastels commented Jun 16, 2022

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
Copy link
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants