Skip to content
This repository has been archived by the owner. It is now read-only.

[Tradfri] Fix color light bug when brightness is less than 2% #4344

Merged
merged 1 commit into from Sep 28, 2017
Merged

[Tradfri] Fix color light bug when brightness is less than 2% #4344

merged 1 commit into from Sep 28, 2017

Conversation

@ivivanov-bg
Copy link
Contributor

@ivivanov-bg ivivanov-bg commented Sep 26, 2017

No description provided.

@sjsf
Copy link
Contributor

@sjsf sjsf commented Sep 26, 2017

What exactly is "the bug"? And could you please also amend TradfriColorTest accordingly?

@ivivanov-bg
Copy link
Contributor Author

@ivivanov-bg ivivanov-bg commented Sep 26, 2017

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

@sjsf
Copy link
Contributor

@sjsf sjsf commented Sep 26, 2017

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.

@sjsf
Copy link
Contributor

@sjsf sjsf commented Sep 26, 2017

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?

@ivivanov-bg
Copy link
Contributor Author

@ivivanov-bg ivivanov-bg commented Sep 26, 2017

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 ColorLightHandler ?

@sjsf
Copy link
Contributor

@sjsf sjsf commented Sep 26, 2017

No, not really - just the huge overlap, I guess. The only places where this distinction is made are in the onUpdate() method, afaics. So I also don't see a need for adding a class hierarchy right now. The experience with others (e.g. hue, lifx,...) was the same.

@hreichert
Copy link
Contributor

@hreichert hreichert commented Sep 26, 2017

@ivivanov-bg Thanks for fixing, I did not thought of this when writing the calculation.

@ivivanov-bg ivivanov-bg changed the title Fix color light bug when brightness is less than 2% [Tradfri] Fix color light bug when brightness is less than 2% Sep 27, 2017
Copy link
Contributor

@htreu htreu left a comment

Looks good so far, just minor comments.

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
Copy link
Contributor

@htreu htreu Sep 27, 2017

The value is 2 but the comment says 254.

Copy link
Contributor

@htreu htreu Sep 27, 2017

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"));
Copy link
Contributor

@htreu htreu Sep 27, 2017

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) {
Copy link
Contributor

@htreu htreu Sep 27, 2017

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?

Copy link
Contributor Author

@ivivanov-bg ivivanov-bg Sep 27, 2017

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

Copy link
Contributor

@htreu htreu Sep 27, 2017

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.

Copy link
Contributor Author

@ivivanov-bg ivivanov-bg Sep 27, 2017

I guess this comes from the IKEA API (not not really sure)

May be @SJKA or @kaikreuzer can give more info

Copy link
Contributor

@hreichert hreichert Sep 27, 2017

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>
htreu
htreu approved these changes Sep 27, 2017
Copy link
Contributor

@hreichert hreichert left a comment

LGTM

@kaikreuzer kaikreuzer merged commit 21f9b2b into eclipse-archived:master Sep 28, 2017
2 checks passed
@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017
@kaikreuzer kaikreuzer added the bug label Dec 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants