fix hue errors in plasma_stick_rainbows #883
Conversation
|
Correction - I forgot to account for 60 updates per second. The existing code should stop working after just 4 days and 10 hours, give or take. It takes 22918664 passes through the loop for the addition of 0.01f to stop working. |
|
Wouldn't it make more sense to just |
| if (offset > 1.0) { | ||
| offset -= 1.0; | ||
| } |
There was a problem hiding this comment.
| if (offset > 1.0) { | |
| offset -= 1.0; | |
| } | |
| offset %= 1 |
There was a problem hiding this comment.
Following up on https://github.com/LaaZa comment...
There was a problem hiding this comment.
this isn't legal code.
There was a problem hiding this comment.
You are totally right, the code proposal would then be only for the corresponding Python code, if you won't mind fixing the issue also over there :)
See e.g. bitcdr/plasma-leds#5 for a fix.
| @@ -34,10 +34,15 @@ int main() { | |||
| while(true) { | |||
|
|
|||
| offset += float(SPEED) / 2000.0f; | |||
| if (offset > 1.0) { | |||
| offset -= 1.0; | |||
| } | |||
|
|
|||
| for(auto i = 0u; i < NUM_LEDS; ++i) { | |||
There was a problem hiding this comment.
for i in range(NUM_LEDS):
hue = (offset + float(i) / NUM_LEDS) % 1
led_strip.set_hsv(i, hue, 1.0, 1.0)
There was a problem hiding this comment.
Following up on https://github.com/LaaZa comment and condensing the code a bit
No, you can't use modulo on floating point numbers in C / C++ |
Right... another reason for why we need to move on from C as a society. |
|
The proposed python fix was incomplete - it's necessary to constrain the range of the |
|
Thank you for the detailed explanation and fix, this looks reasonable to me! |
In this example the use of the monotonically increasing
offsetvariable in the hue calculations will cause the colours to stop rotating when the repeated addition of 20 / 2000 eventually becomes too small to make a difference to the accumulated value given the limited precision of afloat. That is, at some pointoffset += float(SPEED) / 2000.0fbecomes a no-op.Granted it will take a long time - 265 days with the current code - but in a modified version of this demo I've been able to trigger issues far more rapidly.This patch first makes sure that
offsetdoesn't increase without bound so that it stays within the numerical precision range of afloat, and secondly ensures that all hue values passed toRGBLED::set_hsv()are within the range [0..1)The second change begs the question of whether the various
FOO::set_hsv()methods in the library should perform this normalisation of thehparameter on entry to those methods. It would not only resolve some issues I've seen with negative hue values, but also avoid the need for thei % 6in theswitchsince withhconstrained to [0..1)iwill also be similarly constrained to [0..6).