Fix flux_led only-white controllers (and remove explicit declaration as RGBW in automatic add) by autinerd · Pull Request #22210 · home-assistant/core

autinerd · 2019-03-20T09:37:05Z

Description:

When using only-white Magic Home controllers, the update routine didn't work properly. Now only-white controllers are treated as before my update for RGBW controllers.

Old description:

When flux_led devices are found with automatic discovery, they are automatically seen as RGBW devices, even when they are actually RGB devices, This pull request fixes this issue.
Issue #22161 will be completely fixed in the next pull request, because it needs an update to the flux_led module as well

Related issue (if applicable): fixes #22405

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.

If the code communicates with devices, web services, or third-party tools:

New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

Tests have been added to verify that the new code works.

amelchio

This is tricky, we need to find someone that can test it.

amelchio · 2019-03-25T22:05:55Z

        if ipaddr in light_ips:
            continue
        device['name'] = '{} {}'.format(device['id'], ipaddr)
-        device[ATTR_MODE] = MODE_RGBW


The device[ATTR_MODE] value is always read in __init__() so this will make it always raise an exception. I think it could work if you set it to None instead.

Just a note for others, the mode is set automatically if not explicitly defined: https://github.com/home-assistant/home-assistant/blob/08e93224e0689c9a20a2b6b43057f60348bb7c14/homeassistant/components/flux_led/light.py#L185-L191

(whether that works for all devices, I don't know)

autinerd · 2019-04-05T08:43:18Z

@amelchio this pull request shall remove the bug from #22405 and my previous pull request

amelchio · 2019-04-05T09:23:24Z

Great. I can review after the weekend. Meanwhile, is this tested with affected devices?

autinerd · 2019-04-05T19:31:16Z

The problem is that I don't have a only-white controller, so I can't test it. The behaviour for other types has not changed.

amelchio · 2019-04-09T17:51:28Z

+        if not self.is_on:
+            self._bulb.turnOn()
+            if kwargs.get(ATTR_BRIGHTNESS) is None:
+                return


This is wrong. I think you are addressing a UI issue but we can turn on with any parameters using light.turn_on.

Also, preferably turn on unconditionally because our local state could be stale.

I am not quite sure what you are trying to do so I cannot advise on a better way :)

One (in my opinion) not so optimal thing is, that turn_on is for turning stuff on and changing the state of an entity. What I want to do is that when the lamp is off and the user want to turn it on, it just turns on and nothing else (except when brightness is set, because it's possible to turn the light on with the brightness slider as well). When the lamp is already on, then the state should be changed.

Yes, light.turn_on is indeed overloaded but each light platform should not work around that. It is quite possible to turn on to an RGB color, for example with an automation.

Instead, only call self._bulb.setRgb() and friends if relevant kwargs are actually passed.

Oh yeah, I forgot the automation, thank you. I will correct this.

…o flux_led_fix

autinerd · 2019-04-10T06:51:40Z

I will test the stuff in the evening at home.

autinerd · 2019-04-10T19:42:26Z

So, I have now tested the changes, they are fully working for RGBW devices (now waiting time is reduced to 1 second as well, this is enough)

amelchio

This is getting really complex, so many cases to handle :-(. I have some proposals that I think will simplify the code a bit.

amelchio · 2019-04-10T20:38:17Z

    def _turn_on(self, **kwargs):
        """Turn the specified or all lights on."""
-        self._bulb.turnOn()
+        if not self.is_on:


Is there a reason for this condition? As I said, the state can be stale so it is better to not trust it.

amelchio · 2019-04-10T20:42:46Z

-                    self._custom_effect[CONF_COLORS],
-                    self._custom_effect[CONF_SPEED_PCT],
-                    self._custom_effect[CONF_TRANSITION])
+        if all(None for item in [hs_color, brightness, effect, white]):


That looks weird, it will never be true? You probably meant it like this:

if all(item is None for item in [hs_color, brightness, effect, white]):

amelchio · 2019-04-10T20:44:18Z

-        # handle RGB mode
+            if brightness is not None:
+                self._bulb.setWarmWhite255(brightness)
+                return


Unindent this return and remove the else: below

amelchio · 2019-04-10T20:49:59Z

+                    self._bulb.setPresetPattern(EFFECT_MAP[effect], 50)
+                return
+            # Preserve current brightness on color/white level change
+            if brightness is None:


Move this block into the if hs_color is not None: below.

amelchio · 2019-04-10T20:51:24Z

+
+            if hs_color is not None:
+                color = (hs_color[0], hs_color[1], brightness / 255 * 100)
+            elif brightness is not None and hs_color is None:


Remove and hs_color is None, that's the "else" part of the "elif".

…o flux_led_fix

amelchio

Thanks, I think that improved the readability. One issue left though.

amelchio · 2019-04-11T12:57:37Z

+            return
+        # Preserve current brightness on color/white level change
+        if hs_color is not None:
+            if brightness is not None:


Should be if brightness is None.

Thank you very much for your patience 😅

amelchio

Looks good now, I just moved some code to fix a too long line.

It would still be good to get someone to test the fixes with the light types that you do not have around.

amelchio · 2019-04-12T19:51:14Z

Actually, can we just remove the bit that I moved since the same code seems to be present a bit further down in update()?

balloob · 2019-04-15T20:21:39Z

@autinerd I think that when @amelchio said "we", he meant that it's a change you should make to get this PR approved.

autinerd · 2019-04-16T06:49:39Z

Oh yeah, thank you, I was just a bit confused. Thank you very much and hopefully there are no more problems with the logic of this component!

kineticscreen · 2019-04-30T06:16:57Z

So since a few flux updates ago, my RGBW Led Strip has started 'toggling' between RGB and W. This problem was introduced several months ago but fixed. I was hoping that it would get fixed with this PR but sadly not.

Essentially it can't light all 4 channels at once - turning on the W turns off the RGB and vice versa.

amelchio · 2019-04-30T07:01:50Z

I think it makes sense to turn off RGB when setting W and vice-versa. However, if setting both, we should allow that. I think there is an inconclusive architecture issue about this behavior :-/

Anyway, please file a new GitHub issue about this since we should not be discussing in closed tickets.

This reverts commit 4813818.

* Revert Black * Revert "Introduce support for color temperature (#25503)" This reverts commit e1d884a. * Revert "Fix flux_led only-white controllers (#22210)" This reverts commit 4813818. * Revert "Fix MagicHome LEDs with flux_led component (#20733)" This reverts commit 1444a68. * Re-Black * Use mode detection for scanned bulbs

Remove explicit declaration of automatic found devices as RGBW

08e9322

homeassistant added cla-signed small-pr PRs with less than 30 lines. integration: flux_led labels Mar 20, 2019

ghost added the in progress label Mar 20, 2019

amelchio requested changes Mar 25, 2019

View reviewed changes

fixes for Magic Home only-white controllers

5f9ccd2

autinerd changed the title ~~Remove explicit declaration of automatic found devices as RGBW in flux_led~~ Fix flux_led only-white controllers (and remove explicit declaration as RGBW in automatic add) Apr 4, 2019

autinerd added 2 commits April 4, 2019 08:33

mode is now set to None instead of removed entirely

e23c366

flux_led now changes no values when turned on from off state.

52cb222

amelchio mentioned this pull request Apr 5, 2019

Flux_Led Not working with single colour Magic Home led controller since 0.90 #22405

Closed

Merge 'upstream/dev'

b66a628

ghost assigned amelchio Apr 9, 2019

amelchio requested changes Apr 9, 2019

View reviewed changes

autinerd added 2 commits April 10, 2019 08:48

better checking for changed values in turn_on

05d8200

Merge branch 'flux_led_fix' of github.com:autinerd/home-assistant int…

0254abb

…o flux_led_fix

Reduce waiting time to 1 second

f568097

amelchio requested changes Apr 10, 2019

View reviewed changes

autinerd added 2 commits April 11, 2019 13:10

Correction of turn on logic

b46cbbe

Merge branch 'flux_led_fix' of github.com:autinerd/home-assistant int…

afd673d

…o flux_led_fix

amelchio requested changes Apr 11, 2019

View reviewed changes

Sidney and others added 2 commits April 11, 2019 17:42

Remove accidentally inserted 'not'

f780d73

Remove lint

f104fc0

amelchio approved these changes Apr 11, 2019

View reviewed changes

Remove redundant code

b476390

balloob merged commit 4813818 into home-assistant:dev Apr 16, 2019

ghost removed the in progress label Apr 16, 2019

amelchio added a commit to amelchio/home-assistant that referenced this pull request Aug 2, 2019

Revert "Fix flux_led only-white controllers (home-assistant#22210)"

830400c

This reverts commit 4813818.

amelchio mentioned this pull request Aug 2, 2019

Revert flux_led to 0.89 #25653

Merged

4 tasks

Uh oh!

Conversation

autinerd commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Old description:

Checklist:

Uh oh!

amelchio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

autinerd commented Apr 5, 2019

Uh oh!

amelchio commented Apr 5, 2019

Uh oh!

autinerd commented Apr 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

autinerd commented Apr 10, 2019

Uh oh!

autinerd commented Apr 10, 2019

Uh oh!

amelchio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amelchio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amelchio left a comment

Choose a reason for hiding this comment

Uh oh!

amelchio commented Apr 12, 2019

Uh oh!

balloob commented Apr 15, 2019

Uh oh!

autinerd commented Apr 16, 2019

Uh oh!

kineticscreen commented Apr 30, 2019

Uh oh!

amelchio commented Apr 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

autinerd commented Mar 20, 2019 •

edited

Loading