Questions re comments in Kaleidoscope-LEDEffect-Rainbow.{h,cpp}

Programming Plugins
1

My questions are re these lines in Kaleidoscope-LEDEffect-Rainbow.h:

uint16_t rainbow_hue = 0;   //  stores 0 to 614
uint8_t rainbow_steps = 1;  //  number of hues we skip in a 360 range per update

However comparing to the actual usage in Kaleidoscope-LEDEffect-Rainbow.cpp:

rainbow_hue += rainbow_steps;
if (rainbow_hue >= 255) {
  rainbow_hue -= 255;
}

It seems that the comments are somewhat misleading because the hue value can never exceed 255. Perhaps the comments are a leftover from another context?

2

In fact it would seem that by simply changing the type of hue to 8-bit rather than 16-bit, one could simply remove the if block, because addition automatically wraps around…

3

You can probably do just that. We also would like to use default values in the constructor so that we can get rid of the extra constructors.

4

What extra constructors? I only see one!

5

Ah, my mistake, I believe at one point there were more. Sorry for the confusion!

6

Another question. This line:

  uint8_t rainbow_steps = 1;  //  number of hues we skip in a 360 range per update

and this one:

  uint8_t rainbow_wave_steps = 1;  //  number of hues we skip in a 360 range per update

why are these lines even present? They are only used thus:

  rainbow_hue += rainbow_steps;

and thus:

  rainbow_hue += rainbow_wave_steps;

Neither are they modified anywhere in the program nor are they exposed to the user to be configurable. I’ve submitted a PR with some cleanups and can add a commit either exposing this to the user or conversely just integrating the constant into the code.

7

Looks like you’re talking about this…

8

Code clarity, mostly. rainbow_hue += rainbow_steps is easier to understand than rainbow_hue += 1 - no magic constants. Mind you, marking this const or something may help, but I’d keep it for the name, and let the compiler do the inlining.

1 Like
9

Aw come on! 1 is a “magic constant”? hue += 1 clearly means “go to next hue”!

10

Only until we don’t ever want to change it, to speed things up, for example. Then hue++ would be fine. If we want to keep changing it an option, hue += step feels - to me - nicer. Under the hood, they can likely compile to the same thing, but the += step variant is a tiny bit easier to change later on, or to make public, if so desired.

On the other hand, this is not a very strong opinion, so if @jesse is fine with just inlining, that works for me as well.

11

It’s my preference to not throw away the steps variables. We’ve had different default values at different times, there’s no run time overhead and I prefer the explicitness.

1 Like
12

Ohkay, so then we’ll expose it as a signed integer so people can have reverse rainbows too!

1 Like