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

Update pixel_order default description #168

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mattcocca
Copy link

This PR just changes the documentation to describe how the "default" value of pixel_order changes based on the value of bpp.

Explanation

I was recently going through the Circuit Playground Express Neopixel example with someone and was trying to explain the use of default arguments. Unfortunately the way the Neopixel class is implemented is a bit confusing for this topic.

The docs currently explain that the default value for "pixel_order" is "GRBW" (even though the "class" line shows that the actual default value is None). I was attempting to show that initialization is functionally the same whether you provide the "Default" value or not. However if you provide "GRBW" as the "Default" for a board with bpp set to 3, the code does not function properly anymore. This is because the "true" default value is None as specified in the "class" line, and logic in the library is being used to set the "default" value based on the value of bpp.

Changing the docs is the least intrusive way to make the behavior clearer.

Possible alternative changes

  1. I would be happy to submit a separate PR removing the bpp argument entirely, because it is effectively superseded by the pixel_order argument (Ideally you shouldn't be able to specify a 4 byte pixel order, but only 3 bytes per pixel and vice-versa), but I understand the desire to keep the code backwards compatible (as discussed in #1) and it's been 6 years since then, so backwards compatibility is only a stronger argument now).

  2. Alternatively the code could be changed to enforce that the byte lengths match. Example: if bpp=3 and pixel_order="GRBW" were provided, the init would use "GRB" as the pixel order. Then the default value for pixel_order could be changed to truly be "RGBW". This would also break backwards compatibility, but only in cases where users are specifying mismatching bpp and pixel_order values. But due to the current default value for bpp there are likely many cases where people are specifying pixel_order and not providing a value for bpp, relying on pixel_order to completely override the bpp value (as a result technically specifying mismatching bytes). I think it would be relatively unintrusive to make this change compared to removing bpp but it's probably about just as confusing as the way it is now.

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

Successfully merging this pull request may close these issues.

1 participant