[Tradfri] Fix color light bug when brightness is less than 2% #4344
Conversation
|
What exactly is "the bug"? And could you please also amend |
|
The bug is that when the brightness of the color bulb is set to 1% - the xyBrightness / 2.54 returns 0 and the brightness value of the created HSB type is 0 and the brightness channel is updated with 0. So the bulb is shown as switched off. Update: what is the point of the tests if they are created to work with the already written code (even if it's buggy) and doesn't cover edge cases |
|
The point is to prevent regressions and to document how it is expected to behave. Otherwise the next one who cleans up some code or refactors it otherwise might run into the same issue without noticing it and then some users/customers will have to discover it. |
|
I just noticed that there already is a similar workaround for the brightness in the TradfriLightHandler. It is surprising to have the same workaround applied at different layers, for the brightness in the handler, for the color in TradfriColor. Do you see any chance to unify these? |
|
Ok - I will. One question: Any particular reason to have only one light handler and check if the light is a color one instead of inherit it to let's say |
|
No, not really - just the huge overlap, I guess. The only places where this distinction is made are in the |
|
@ivivanov-bg Thanks for fixing, I did not thought of this when writing the calculation. |
ivivanov-bg
changed the title
| assertNotNull(color); | ||
| assertEquals(0, (int) color.rgbR); | ||
| assertEquals(254, (int) color.rgbG); // 254 instead of 255 - only approximated calculation | ||
| assertEquals(2, (int) color.rgbG); // 254 instead of 255 - only approximated calculation |
There was a problem hiding this comment.
The value is 2 but the comment says 254.
There was a problem hiding this comment.
see my comment for line 107, I guess the test case is just fine as it is.
|
|
||
| @Test | ||
| public void testConversionReverse() { | ||
| // convert from HSBType | ||
| TradfriColor color = TradfriColor.fromHSBType(HSBType.GREEN); | ||
| TradfriColor color = TradfriColor.fromHSBType(new HSBType("120,100,1")); |
There was a problem hiding this comment.
the test is just fine w/o your changes. If the HSB(120,100,1) adds another corner case please add a separate test case.
| * @return {@link PercentType} with brightness level (0 = light is off, 1 = lowest, 100 = highest) | ||
| */ | ||
| public static PercentType xyBrightnessToPercentType(int xyBrightness) { | ||
| if (xyBrightness > 254) { |
There was a problem hiding this comment.
out of curiosity: why are values > 254 a problem? All we have to do is adopt the 2.54 in line 296 and we could also deal with 255 or greater. wdyt?
There was a problem hiding this comment.
Well - my understanding for a PercentType is to be between 0 and 100. So - if xyBrightness > 254 then xyBrightness / 2.54 will return value > 100 and it will throw IllegalArgumentException
There was a problem hiding this comment.
Yes I get that. All we have to adopt is the divisor (2.54) to a value that matches our upper bound (254). In case we move the upper bound to 255 the divisor should be adopted to 2.55 to match the PercentType. I just wanted to know where the 254 originates and why there are possible values grater then 254.
There was a problem hiding this comment.
I guess this comes from the IKEA API (not not really sure)
May be @SJKA or @kaikreuzer can give more info
There was a problem hiding this comment.
Yes, the range 0 to 254 is defined for Zigbee ZLL brightness and the IKEA gateway adopts this.
Signed-off-by: Ivaylo Ivanov <ivivanov.bg@gmail.com>
No description provided.