Fixed overflow issue with getPixelColor

[dmadison]

This is an update to fix the issue described here.

Describe the scope of your change:
Added (uint16_t) data type conversion to the brightness correction of the getPixelColor function so that it works properly and avoids overflow.

When the product of a channel's color value and the current brightness level is greater than 2¹⁵ (i.e. the upper bound of a signed 16-bit type) the output of the function is erratic. When the product drops below 2¹⁵, either due to a lower input value or a lower brightness level, the function's output behaves normally. The solution is to force an unsigned conversion.

I'm using the Arduino IDE (1.6.9) and I've verified the problem with several compilers. This change appears to fix it entirely. I've tested both the RGB and RGBW functions and haven't found any issues.

Describe any known limitations with your change:
None.

Please run any tests or examples that can exercise your modified code.
I put together a test sketch to demonstrate the problem. You can find the sketch and the output (pre and post fix) here. The test checks both RGB and RGBW strips with three separate input levels and data arrangement.

For the test the sketch was compiled to an Arduino Uno using the 1.6.9 IDE and the AVRISPmkII programmer.

[houston47d]

I think what is happening in the original code is that the << operator is causing the uint8_t p to be promoted to an int (signed 16-bit) to match its right argument. The << 8 puts the msb of the original number in the sign bit, so if the original number is > 127 then the / brightness will preserve the sign bit (since it is a signed number). Since the numbers are then shifted and combined, it results in those replicated sign bits messing up the higher order bytes.
Your code looks fine, though I would be inclined to put the (uint16_t) cast on p[] rather than on the (p[] << 8). But either way works because it is unsigned at the time of the divide. So
return (((uint32_t)((uint16_t)p[rOffset] << 8) / brightness) << 16) |
(((uint32_t)((uint16_t)p[gOffset] << 8) / brightness) << 8) |
( (uint32_t)((uint16_t)p[bOffset] << 8) / brightness );
It is good to keep the shift and divide on 16-bit values as that is considerably cheaper than on a 32-bit value.

[dmadison]

Thanks for the feedback. You're right that logically I probably should've put the type conversion on the bit-shifted value rather than the expression, although it shouldn't matter. Out of curiosity I tested the execution speed with both methods and saw no difference (on my setup).