Move LIFX to aiolifx for driving the bulbs #6584

amelchio · 2017-03-13T18:03:21Z

Description:

This is more or less a rewrite of the LIFX component, migrating it to aiolifx. This makes it async, fixes all reported issues (and some unreported ones) and lays the ground for new features like support for the flash parameter.

The only tricky thing is that LIFX bulbs will report the old state for a short while after setting a new state. I therefore pause update requests to the bulb for 0.5s after setting a state.

The rework has been tested in #6435 but I am new to Python as well as Home Assistant so I expect some issues with implementation details.

Related issue (if applicable): fixes #3604, #4589, #5158, #6245, #6435

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2259

Checklist:

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

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

Local tests with tox run successfully.
New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
New dependencies are only imported inside functions that use them ([example][ex-import]).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.

mention-bot · 2017-03-13T18:03:26Z

@amelchio, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fabaff, @kitcorey and @BillyNate to be potential reviewers.

houndci-bot · 2017-03-13T18:03:33Z

homeassistant/components/light/lifx.py

+        if fade < BULB_LATENCY:
+            self.set_power(0)
+
+    def got_color(self,device,msg):


missing whitespace after ','

houndci-bot · 2017-03-13T18:03:33Z

homeassistant/components/light/lifx.py

        else:
            kelvin = self._kel

+        hsbk = [ hue, saturation, brightness, kelvin ]


whitespace after '['
whitespace before ']'

houndci-bot · 2017-03-13T18:03:34Z

homeassistant/components/light/lifx.py

-    def probe(self, address=None):
-        """Probe the light."""
-        self._liffylights.probe(address)
+    def unregister(self,device):


missing whitespace after ','

houndci-bot · 2017-03-13T18:03:34Z

homeassistant/components/light/lifx.py

+        self.hass = hass
+        self.async_add_devices = async_add_devices
+
+    def register(self,device):


missing whitespace after ','

arsaboo · 2017-03-13T18:52:19Z

homeassistant/components/light/lifx.py

+            self.set_power(0)
+
+    def got_color(self, device, msg):
+        """Callback that gets current power/color status"""


End with a period.

amelchio · 2017-03-13T19:32:28Z

Sorry for multiple fixups. I don't have a proper local environment yet.

I need help on the available setter method.

Remove the broadcast configuration variable.

keatontaylor · 2017-03-14T20:52:26Z

@amelchio, I've been looking over this code and testing it on my own and ran into a few problems.

It seems to work really well at detecting unavailable or offline bulbs, but in my testing when the bulbs come back online, home assistant takes a very long time to mark the bulbs as available again. (I tested by killing power to the bulb, waiting for it to be marked unavailable, then restoring power) How long should this take? What is the update interval.
Also, requesting an update on the bulb after a transition is complete is a good way to prevent the state in HA from becoming incorrect, but I would also like to see the ability to disable automatic polling all together as a feature and (polling on by default) and instead only using the force update code when the bulb state changes.
Make the polling interval configurable for those who wish to have more frequent views of the outside world, this would help with those who wish to control LIFX bulbs from external devices.

keatontaylor · 2017-03-14T20:58:02Z

Also, there is a potential problem I just realized. What happens when the bulb is unavailable and automations try and control the bulbs? Do the automations outright fail, or does the lifx component soft fail and allow the rest of the automation to continue.

keatontaylor · 2017-03-14T21:01:21Z

Also, we don't need to request an acknowledgement from the bulbs if we're not using it for anything. So on the aiolifx side we should be able to use fire and forget since the component is waiting a predefined interval, then checking the light state.

amelchio · 2017-03-14T21:33:29Z

@keatontaylor Thank you for looking at this!

My LIFX Color 1000 and LIFX White 800 consistently reappear as available in about 13 seconds. This should not be related to the update interval because the bulb broadcasts its state when booting up.
I don't like turning off polling completely, there are too many error cases. So please show it to be a problem first.
The polling is decided by the core scan_interval value so it can be configured for any interval you like. I believe the default is 15 seconds.
I actually don't know, the available property is also from core, I just set the value.
The ACK is used to retry packet sends. It also drives the unregister that sets lights unavailable.

keatontaylor · 2017-03-14T21:45:43Z

@amelchio I believe I discovered why it is taking so long for re-registering after bulbs are inactive. I am running the docker HA image and the component needed to be set to listen to incoming broadcasts packets from the right subnet, but this isn't easily configurable. So those of us using the docker image will need that added.

amelchio · 2017-03-14T23:02:04Z

@keatontaylor I admit ignorance on Docker so I must get it set up to test that. I assume that this is Docker on Linux? Also, how long is "so long"?

arsaboo · 2017-03-15T02:31:00Z

@keatontaylor I have been testing this PR for some time on RPi and it is significantly more responsive compared to the current Lifx component.

balloob

This already looks very good!

Just a few minor comments.

balloob · 2017-03-15T03:35:46Z

homeassistant/components/light/lifx.py


-    lifx_library.probe()
+    hass.loop.create_task(coro)


Please use hass.async_add_job (it does the same except that it allows us to only finish starting up when this task is done)

balloob · 2017-03-15T03:42:48Z

homeassistant/components/light/lifx.py

+        self.hass = hass
+        self.async_add_devices = async_add_devices
+
+    def register(self, device):


Please mark these classes with @callback since they are being called from inside the event loop. This will help readers of this code understand what is going on.

balloob · 2017-03-15T03:43:33Z

homeassistant/components/light/lifx.py

+            entity = self.entities[device.mac_addr]
+            _LOGGER.debug("%s register AGAIN", entity.ipaddr)
+            entity.available = True
+            entity.schedule_update_ha_state()


You are already in the event loop. Please use self.hass.async_add_job(entity.async_update_ha_state())

balloob · 2017-03-15T03:44:21Z

homeassistant/components/light/lifx.py

-    def poll(self, now):
-        """Polling for the light."""
-        self.probe()
+    def ready(self, device, msg):


Please add a @callback annotation.

balloob · 2017-03-15T03:44:47Z

homeassistant/components/light/lifx.py

-    def probe(self, address=None):
-        """Probe the light."""
-        self._liffylights.probe(address)
+    def unregister(self, device):


Please add a @callback annotation.

balloob · 2017-03-15T03:46:55Z

homeassistant/components/light/lifx.py

+        _LOGGER.debug("%s unregister", entity.ipaddr)
+        entity.available = False
+        entity.updated_event.set()
+        entity.schedule_update_ha_state()


You are already in the event loop. Please use self.hass.async_add_job(entity.async_update_ha_state())

balloob · 2017-03-15T03:47:44Z

homeassistant/components/light/lifx.py

-    def should_poll(self):
-        """No polling needed for LIFX light."""
-        return False
+    def available(self):


Instead of being all this fancy, you can also just have an available property on the entity (defined in __init__). No need abstracting it in a setter.

balloob · 2017-03-15T03:50:51Z

homeassistant/components/light/lifx.py

-    def turn_off(self, **kwargs):
+        self.update_later(0)
+        if fade < BULB_LATENCY:
+            self.set_power(1)


For other reviewers:

This is not doing I/O in the event loop. Checked it, aiolifx will schedule a task on the event loop when we call this. https://github.com/frawau/aiolifx/blob/master/aiolifx/aiolifx34.py#L143

amelchio · 2017-03-15T22:50:20Z

@balloob Thanks for the review and the intro text in particular, that made my day :-)

I now added the @callback and async_add_job that you requested, also in a few places where you didn't (hopefully that was correct).

But I cannot figure out how to get rid of the fancy @available.setter. The property getter is defined in class Entity and the property is set from (the class that I now renamed to) LIFXManager. Any simpler way that I tried ended up either not working, or producing lint.

balloob · 2017-03-16T05:26:24Z

Oh my bad about available. Sorry, let's keep it as is.

Remove the broadcast configuration variable.

kbickar · 2017-03-26T23:31:48Z

After updating to the version with this commit, my LiFX bulb no longer appears. It worked flawlessly in the previous version

amelchio · 2017-03-26T23:50:05Z

@kbickar I'm sorry to hear that but thanks for reporting it. Can you tell me a bit about your bulb and HA setup so we can locate the problem?

kbickar · 2017-03-27T03:22:35Z

@amelchio I have a LIFX Original - 02d279 v2.1 and the bulb is defined in my main configuration file as:
light 2:
platform: lifx
host: 192.168.1.39

amelchio · 2017-03-27T06:34:12Z

@kbickar Okay, because of the hostconfiguration I am guessing that you have multiple networks on the host. Is that right? As a test, are you able to open the file .homeassistant/deps/aiolifx/aiolifx.py and change the line

UDP_BROADCAST_IP = "255.255.255.255"

to

UDP_BROADCAST_IP = "192.168.1.255"

(or whatever your actual broadcast IP address is) and restart HA?

keatontaylor · 2017-03-27T06:50:36Z

Experiencing a similar issue, not all bulbs are showing up when HA is restarted. Changed the broadcast as you suggested without success.

amelchio · 2017-03-27T11:26:00Z

This change is a big rework, so some fallout was expected. I am, however, committed to fix these things up so the change ends up a net improvement.

It is probably best to file individual issues for the regressions that you experience so we can go through them one by one and see if they each have the same cause.

arsaboo · 2017-03-27T12:00:37Z

As a datapoint, it's working very well for me. Best to file individual bugs, so that these issues can be tracked.

kbickar · 2017-03-27T13:52:23Z

Setting the broadcast IP did not help. Additionally, I was watching in wireshark and I don't see any traffic going to either the IP of the bulb (the bulb occasionally broadcasts itself though), or to the UDP port 56700. Are there any changes that might be needed in the configuration file?

amelchio · 2017-03-27T15:44:12Z

@kbickar On UDP port 56700 you should at least see a broadcast every 180 seconds, probing for new bulbs. So if you do not see that, maybe the LIFX component is not even loaded. I am actually not sure how to verify that (I would just search the logfile for "lifx" and hope it has some relevant message).

For the configuration, host should be server (this is not new). But it only matters if you are actually running multiple IP networks?

Which OS are you on?

light 2:
  - platform: lifx
    server: 192.168.1.39

kbickar · 2017-03-27T18:20:00Z

I have multiple network interfaces for VMs running on Windows 10.
I switched the server variable, but that also doesn't work because it's then trying to bind to the remote IP. Setting server to 0.0.0.0 lets it continue but then it gets "Not implemented" error in _make_datagram_transport.

Does asyncio support windows?

amelchio · 2017-03-27T20:15:05Z

@kbickar oh my, it seems you are right. That will take a little while to sort out, so you may want to revert to the previous lifx.py for now.

keatontaylor · 2017-03-27T23:48:14Z

Oddly my issue was fixed not by changing the broadcast, but instead by power cycling the bulbs. Not sure why, but I've experienced these same issues before with the LIFX iOS app.

Move LIFX to aiolifx for driving the bulbs

2f1040e

homeassistant added platform: light.lifx cla-signed labels Mar 13, 2017

houndci-bot reviewed Mar 13, 2017

View reviewed changes

amelchio changed the title ~~[WIP] Move LIFX to aiolifx for driving the bulbs~~ Move LIFX to aiolifx for driving the bulbs Mar 13, 2017

Fix whitespace

7f83894

homeassistant added the cla-signed label Mar 13, 2017

balloobbot added platform requirement labels Mar 13, 2017

Fix more whitespace

ab0dc5b

homeassistant added the cla-signed label Mar 13, 2017

arsaboo reviewed Mar 13, 2017

View reviewed changes

Fix lint

7b537cf

Define _available in init

798ac7c

amelchio added a commit to amelchio/home-assistant.github.io that referenced this pull request Mar 14, 2017

Changes for home-assistant/core#6584

1cd91e4

Remove the broadcast configuration variable.

amelchio mentioned this pull request Mar 14, 2017

Changes for home-assistant/home-assistant#6584 home-assistant/home-assistant.io#2259

Merged

balloob requested changes Mar 15, 2017

View reviewed changes

amelchio added 3 commits March 15, 2017 22:57

Add @callback decorators

d23db1c

Use hass.async_add_job

db5afa5

Rename class

08350c8

homeassistant added the cla-signed label Mar 15, 2017

balloob approved these changes Mar 16, 2017

View reviewed changes

balloob merged commit 9ef084d into home-assistant:dev Mar 16, 2017

balloob pushed a commit to home-assistant/home-assistant.io that referenced this pull request Mar 16, 2017

Changes for home-assistant/core#6584 (#2259)

c093a92

Remove the broadcast configuration variable.

balloob mentioned this pull request Mar 24, 2017

0.41 #6756

Merged

amelchio mentioned this pull request Mar 27, 2017

LIFX broken on Windows in HA 0.41 #6820

Closed

home-assistant locked and limited conversation to collaborators Jul 17, 2017

ghost added integration: lifx and removed platform: light.lifx labels Mar 21, 2019

Uh oh!

Move LIFX to aiolifx for driving the bulbs #6584

Move LIFX to aiolifx for driving the bulbs #6584

Uh oh!

Conversation

amelchio commented Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Checklist:

Uh oh!

mention-bot commented Mar 13, 2017

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 commented Mar 13, 2017

Uh oh!

keatontaylor commented Mar 14, 2017

Uh oh!

keatontaylor commented Mar 14, 2017

Uh oh!

keatontaylor commented Mar 14, 2017

Uh oh!

amelchio commented Mar 14, 2017

Uh oh!

keatontaylor commented Mar 14, 2017

Uh oh!

amelchio commented Mar 14, 2017

Uh oh!

arsaboo commented Mar 15, 2017

Uh oh!

balloob 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!

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 commented Mar 15, 2017

Uh oh!

balloob commented Mar 16, 2017

Uh oh!

kbickar commented Mar 26, 2017

Uh oh!

amelchio commented Mar 26, 2017

Uh oh!

kbickar commented Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amelchio commented Mar 27, 2017

Uh oh!

keatontaylor commented Mar 27, 2017

Uh oh!

amelchio commented Mar 27, 2017

Uh oh!

arsaboo commented Mar 27, 2017

Uh oh!

kbickar commented Mar 27, 2017

Uh oh!

amelchio commented Mar 27, 2017

Uh oh!

amelchio commented Mar 13, 2017 •

edited

Loading

kbickar commented Mar 27, 2017 •

edited

Loading