search

ph1p / ikea-led-obegraensad

ESP32/Arduino hack for the ikea OBEGRÄNSAD led wall lamp
MIT License
596 stars 81 forks source link

Possible brightness bug #52

Open schw4rzlicht opened 10 months ago

schw4rzlicht commented 10 months ago

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.

https://github.com/ph1p/ikea-led-obegraensad/blob/5ad09fa4c06fedfa00d01e3898202d4ef1ea0244/src/screen.cpp#L96-L110

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 10 months ago

Interesting, I believe a similar issue might be present in here https://github.com/ph1p/ikea-led-obegraensad/blob/5ad09fa4c06fedfa00d01e3898202d4ef1ea0244/src/screen.cpp#L19

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 commented 10 months ago

Can this be closed with #64 being merged?

schw4rzlicht commented 10 months ago

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 #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.