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


(Shriramana Sharma) #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?


(Shriramana Sharma) #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…


(Ben Gemperline) #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.


(Shriramana Sharma) #4

What extra constructors? I only see one!


(Ben Gemperline) #5

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


(Shriramana Sharma) #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.


(Shriramana Sharma) #7

Looks like you’re talking about this


(Gergely Nagy) #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.


(Shriramana Sharma) #9

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


(Gergely Nagy) #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.


(Jesse) #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.


(Shriramana Sharma) #12

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