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

Explicitly cast float to mp_int_t #5746

Merged
merged 1 commit into from
Dec 18, 2021

Conversation

dunkmann00
Copy link

Not sure why this is necessary, but it prevents an off-by-one error in some (rare?) circumstances.

Fixes the bug found in #5725:

>>> import board, neopixel
>>> pixels = neopixel.NeoPixel(board.D5, 8, brightness=1.0, auto_write=False, pixel_order=neopixel.GRBW)
>>> pixels[2] = 0x400000FF
>>> pixels[2]
(0, 0, 255, 0)

This doesn't close #5725, since that it is also about whether or not the behavior of integer values with RGBW neopixels is appropriate.

Not sure why this is necessary, but it prevents an off-by-one error in
some (rare?) circumstances.
Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for finding this! It is "obvious in retrospect", for at least some definitions of obvious, but I had looked directly at this code just recently and still not seen it for myself.

I'm approving this PR, but with the caveat that we may wish to have the bugfix in the 7.1.x branch. @dhalbert please advise on that point.

Here's a rough expression based on my understanding of C: When using the conditional operator such as cond ? x1 : x2, the type of the whole expression depends on the types of x1 and x2. Here, x1 is some kind of integer and x2 is some kind of float. This means that the expression as a whole is of float type (due to the "usual arithmetic conversions", the same as for code like x1 + x2, making x1 be cast to a float and then the result of the whole conditional operation is cast back to an int. By adding the cast to the x2 expression, both x1 and x2 are integer types, so x1 doesn't undergo any undesirable implicit casting.

@dhalbert dhalbert merged commit 0ec839a into adafruit:main Dec 18, 2021
@dhalbert
Copy link
Collaborator

I will backport this to 7.1.x, no problem.

@dunkmann00
Copy link
Author

@jepler Interesting! It took me a while to figure out that the bug was simply coming from the inline conditional so I very much appreciate that explanation as to why. Thanks!

@dunkmann00 dunkmann00 deleted the pixelbuf-packed-int-error branch December 18, 2021 04:06
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.

RGBW NeoPixel setter: W only works with 4-element tuples, is ignored in packed integers
3 participants