Update pixel_order default description #168
Open
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR just changes the documentation to describe how the "default" value of
pixel_order
changes based on the value ofbpp
.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 isNone
). 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 withbpp
set to 3, the code does not function properly anymore. This is because the "true" default value isNone
as specified in the "class" line, and logic in the library is being used to set the "default" value based on the value ofbpp
.Changing the docs is the least intrusive way to make the behavior clearer.
Possible alternative changes
I would be happy to submit a separate PR removing the
bpp
argument entirely, because it is effectively superseded by thepixel_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).Alternatively the code could be changed to enforce that the byte lengths match. Example: if
bpp=3
andpixel_order="GRBW"
were provided, the init would use "GRB" as the pixel order. Then the default value forpixel_order
could be changed to truly be "RGBW". This would also break backwards compatibility, but only in cases where users are specifying mismatchingbpp
andpixel_order
values. But due to the current default value forbpp
there are likely many cases where people are specifyingpixel_order
and not providing a value forbpp
, relying onpixel_order
to completely override thebpp
value (as a result technically specifying mismatching bytes). I think it would be relatively unintrusive to make this change compared to removingbpp
but it's probably about just as confusing as the way it is now.