ph1p/ikea-led-obegraensad

Possible brightness bug

schw4rzlicht opened this issue · 3 comments

Hey there,

I just noticed that when calculating brightness when using Screen.setPixelAtIndex() or Screen.setPixel(), uint8_t value is multiplied with uint8_t brightness and the result is stored to the renderBuffer which is an array of unit8_t.

void Screen_::setPixelAtIndex(uint8_t index, uint8_t value, uint8_t brightness)
{
if (index >= 0 && index < COLS * ROWS)
{
this->renderBuffer_[index] = value * brightness;
}
}
void Screen_::setPixel(uint8_t x, uint8_t y, uint8_t value, uint8_t brightness)
{
if (x >= 0 && y >= 0 && x < 16 && y < 16)
{
this->renderBuffer_[y * 16 + x] = value * brightness;
}
}

I would interpret value as well as brightness to be something between 0 and 255, but that would quickly lead to an integer overflow, e.g. value = 255, brightness = 255 → renderBuffer[i] = 1.

Is this really the desired behavior? If not, I'd volunteer to implement a fix, but I'd like to discuss the how first.

Thanks!

jekkos commented

Interesting, I believe a similar issue might be present in here

this->renderBuffer_[i] = renderBuffer[i] * 255;

Could it be that some data is written beyond the render buffer? My guess is that this might be the actual cause of #49 instead.

Can this be closed with #64 being merged?

I am not really sure as it's unclear to me how to interpret value and brightness. Imo there are two ways:

  1. value should be the brightness of the pixel which is then scaled based on brightnessresult = round( value * brightness / 255 )
  2. value doesn't matter and is more or less only an indicator for on / off (that's how it was implemented in #64) which makes it redundant and can therefore be removed (only rely on brightness from now on)

Of course the overflow issue is fixed now but I think the intended usage of setPixel / setPixelAtIndex became less intuitive.