Possible brightness bug

[schw4rzlicht]

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.

ikea-led-obegraensad/src/screen.cpp

Lines 96 to 110 in 5ad09fa

	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]

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

ikea-led-obegraensad/src/screen.cpp

Line 19 in 5ad09fa

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.

[BrookeDot]

Can this be closed with #64 being merged?

[schw4rzlicht]

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 brightness → result = 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 fix(brightness): add brightness boundaries #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.