Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Sep 28, 2017

Conversation

Projects
None yet
5 participants
@ivivanov-bg
Copy link
Contributor

commented Sep 26, 2017

No description provided.

@ivivanov-bg ivivanov-bg force-pushed the ivivanov-bg:tradfri_binding_updates branch 2 times, most recently from 64d9739 to 2ffb936 Sep 26, 2017

@sjka

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2017

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

@ivivanov-bg

This comment has been minimized.

Copy link
Contributor Author

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

@ivivanov-bg ivivanov-bg force-pushed the ivivanov-bg:tradfri_binding_updates branch from 2ffb936 to e54d9f1 Sep 26, 2017

@sjka

This comment has been minimized.

Copy link
Contributor

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.

@sjka

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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 ?

@sjka

This comment has been minimized.

Copy link
Contributor

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.

@ivivanov-bg ivivanov-bg force-pushed the ivivanov-bg:tradfri_binding_updates branch 2 times, most recently from aad3dce to 486ef9f Sep 26, 2017

@hreichert

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2017

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

@ivivanov-bg ivivanov-bg force-pushed the ivivanov-bg:tradfri_binding_updates branch 2 times, most recently from be1d9ab to 49c9850 Sep 27, 2017

@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

@htreu
Copy link
Contributor

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

This comment has been minimized.

Copy link
@htreu

htreu Sep 27, 2017

Contributor

The value is 2 but the comment says 254.

This comment has been minimized.

Copy link
@htreu

htreu Sep 27, 2017

Contributor

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"));

This comment has been minimized.

Copy link
@htreu

htreu Sep 27, 2017

Contributor

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) {

This comment has been minimized.

Copy link
@htreu

htreu Sep 27, 2017

Contributor

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?

This comment has been minimized.

Copy link
@ivivanov-bg

ivivanov-bg Sep 27, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@htreu

htreu Sep 27, 2017

Contributor

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.

This comment has been minimized.

Copy link
@ivivanov-bg

ivivanov-bg Sep 27, 2017

Author Contributor

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

May be @sjka or @kaikreuzer can give more info

This comment has been minimized.

Copy link
@hreichert

hreichert Sep 27, 2017

Contributor

Yes, the range 0 to 254 is defined for Zigbee ZLL brightness and the IKEA gateway adopts this.

@ivivanov-bg ivivanov-bg force-pushed the ivivanov-bg:tradfri_binding_updates branch from 49c9850 to 8a17112 Sep 27, 2017

Improve handling of brightness value for Color Lights
Signed-off-by: Ivaylo Ivanov <ivivanov.bg@gmail.com>

@ivivanov-bg ivivanov-bg force-pushed the ivivanov-bg:tradfri_binding_updates branch from 8a17112 to 5edefd3 Sep 27, 2017

@htreu

htreu approved these changes Sep 27, 2017

@hreichert
Copy link
Contributor

left a comment

LGTM

@kaikreuzer kaikreuzer merged commit 21f9b2b into eclipse:master Sep 28, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details

@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 join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.