Added osramlightify groups. by deisi · Pull Request #7376 · home-assistant/core

deisi · 2017-04-30T08:31:08Z

Allows you to make use of the build in osram lightify groups. Group states get
handeled similar as in the case of phillips hue. A lightify group shows up as
light in the homeassistant webinterface. If one light of the
group is on, the complete group is considered to be on.

To use this feature, first define some groups within your lighrify bridge, then
add allow_lightify_groups=true to you osramlightify config.

Example entry for `configuration.yaml` (if applicable):

- platform: osramlightify
  host: IP-ADDRES
  allow_lightify_groups: true

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. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New 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:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
Tests have been added to verify that the new code works.

Allows you to make use of the build in osram lightify groups. Group states get handeled similar as in the case of phillips hue. A lightify group shows up as light in the homeassistant webinterface. If one light of the group is on, the complete group is considered to be on. To use this feature, first define some groups within your lighrify bridge, then set add `allow_lightify_groups=true` to you osramlightify config. It might look like: ````yaml - platform: osramlightify host: IP-ADDRES allow_lightify_groups: true ```

mention-bot · 2017-04-30T08:31:10Z

@deisi, thanks for your PR! By analyzing the history of the files in this pull request, we identified @olimpiurob, @tfriedel and @tchellomello to be potential reviewers.

balloob · 2017-04-30T18:07:31Z

 MIN_TIME_BETWEEN_SCANS = timedelta(seconds=10)
 MIN_TIME_BETWEEN_FORCED_SCANS = timedelta(milliseconds=100)
+CONF_ALLOW_LIGHTIFY_GROUPS = "allow_lightify_groups"
+DEFAULT_ALLOW_LIGHTIFY_GROUPS = False


Let's default it to True

balloob · 2017-04-30T18:09:19Z

+            bridge.update_all_light_status()
+            bridge.update_group_list()
+        except socket.error:
+            errno, errstr = sys.exc_info()[:2]


Wait, what are you doing here? You can just catch the actual error instead of hacking around it using exception frames:

except socket.error as err: if isinstance(err, socket.timeout): _LOGGER.error(…)

However, you are now swallowing all the not timeout related errors which means that something might go wrong but no one will know.

In python 3 socket.error is an alias of OSError.

Instead of checking the error type you can have multiple except statements. First one that catches TimeoutError, then a general one for OSError.

balloob · 2017-04-30T18:13:20Z

-
+        self._luminary.set_onoff(0)
+        self._state = False
        self.schedule_update_ha_state()


Can you remove this line? it's not needed since should_poll is defaulting to True and so this is automatically called.

You mean the self._state = False ?
Well yes I can remove it, but I saw, that without it, HA gets a lot more unresponsive, because otherwise it the GUI jumps back to the on state and then after a couple of seconds it jumps back to off. If that is okay, I'm willing to remove it, I just felt it works better with.

It's not something that we should fix in a platform but instead fix for everyone at once. We already wait 2 seconds for a status update: https://github.com/home-assistant/home-assistant-polymer/blob/master/src/components/entity/ha-entity-toggle.html#L135-L136

Please remove this line.

balloob · 2017-04-30T18:16:14Z


-class OsramLightifyLight(Light):
-    """Representation of an Osram Lightify Light."""
+class Luminary():


Why not have this class already inherit from Light ?

balloob · 2017-04-30T18:19:11Z

+
+        The group is on, if any of the lights in on.
+        """
+        states = []


This is inefficient as you now first have to gather all results before checking if any are True.

lights = self._bridge.lights() return any(lights[light_id].on() for light_id in self._light_ids)

balloob · 2017-04-30T18:20:09Z

+        super().update()
+        self._light_ids = self._luminary.lights()
+        self._light = self._bridge.lights()[self._light_ids[0]]
+        self._brightness = int(self._light.lum() * 2.25)


Brightness goes to 255.

balloob · 2017-04-30T18:22:48Z

+        """Update group status."""
+        super().update()
+        self._light_ids = self._luminary.lights()
+        self._light = self._bridge.lights()[self._light_ids[0]]


Why are you storing the light as an instance variable?

looking at it you are right I don't need to. I will remove it.

balloob · 2017-04-30T18:23:53Z

+    def update(self):
+        """Update group status."""
+        super().update()
+        self._light_ids = self._luminary.lights()


What is self._luminiary and is it only used to fetch the lights?

I think that things are getting very complicated because of the inheritance.

I think that things are getting very complicated because of the inheritance.

I first stared out without, but my code became 90% redundant and so I splittend i t up. The only difference between the two is the update and get_state method.

What is self._luminiary and is it only used to fetch the lights?

Well its the main interface to the lightify API. Essentially all communication with the lights goes through the self._luminary obj. It is called like this, because it is the name of the class within the lightify API itself. I use it, because it is the common ground between lights and groups of lights just like throughout this module as well.

balloob · 2017-04-30T18:36:45Z

Can you also remove all the debug statements in properties ? It is slowing down Hass a lot and it's unnecessary.

houndci-bot · 2017-05-03T05:20:33Z

 For more details about this platform, please refer to the documentation at
 https://home-assistant.io/components/light.osramlightify/
 """
+import sys


'sys' imported but unused

balloob · 2017-05-03T16:10:31Z

-def setup_bridge(bridge, add_devices_callback):
-    """Set up the Lightify bridge."""
+def setup_bridge(bridge, add_devices_callback, add_groups):
+    """Setup the Lightify bridge."""


Please revert this change.

If I remove it, the DEFAULT_ALLOW_LIGHTIFY_GROUPS = True config becomes useless.

balloob · 2017-05-03T16:13:51Z

This is ok to merge when the comment change has been reversed and linting has been fixed 🎉

deisi · 2017-05-04T04:38:08Z

I tried to rebase it, but I coudnt rebase anything before the merge. Shoud I make a new PR or is it okay the way it is?

MartinHjelmare · 2017-05-04T06:02:17Z

You should revert the change to the doc string as @balloob asked, before we can merge. See comment above. Just make a new commit where you change it back to like it was before.

chimpy · 2017-05-05T12:17:20Z

Awesome, this was a Feature Request I really wanted! Thank you! <3

deisi · 2017-05-05T12:21:14Z

:) I wanted it as well, and so I made it. chimpy <notifications@github.com> writes:

…

Awesome, this was a Feature Request I really wanted! Thank you! <3

duffrecords · 2017-05-12T00:35:07Z

This change breaks my Home Assistant setup. Each time I start HA the value of self._light_ids[0] seems to be incorrect but the error does not always happen with the same light. Maybe there's a discrepancy between the bridge and the states in HA. I've rolled back to 0.43.2 for now and the problem went away.

2017-05-10 13:31:46 ERROR (MainThread) [homeassistant.components.light] Error while setting up platform osramlightify
Traceback (most recent call last):
  File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/helpers/entity_component.py", line 155, in _async_setup_platform
    entity_platform.schedule_add_entities, discovery_info
  File "/opt/rh/rh-python35/root/usr/lib64/python3.5/asyncio/futures.py", line 358, in __iter__
    yield self  # This tells Task to wait for completion.
  File "/opt/rh/rh-python35/root/usr/lib64/python3.5/asyncio/tasks.py", line 290, in _wakeup
    future.result()
  File "/opt/rh/rh-python35/root/usr/lib64/python3.5/asyncio/futures.py", line 274, in result
    raise self._exception
  File "/opt/rh/rh-python35/root/usr/lib64/python3.5/concurrent/futures/thread.py", line 55, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/components/light/osramlightify.py", line 60, in setup_platform
    setup_bridge(bridge, add_devices, add_groups)
  File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/components/light/osramlightify.py", line 106, in setup_bridge
    update_lights()
  File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/util/__init__.py", line 303, in wrapper
    result = method(*args, **kwargs)
  File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/util/__init__.py", line 303, in wrapper
    result = method(*args, **kwargs)
  File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/components/light/osramlightify.py", line 97, in update_lights
    update_lights)
  File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/components/light/osramlightify.py", line 268, in __init__
    super().__init__(group, update_lights)
  File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/components/light/osramlightify.py", line 121, in __init__
    self.update()
  File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/components/light/osramlightify.py", line 282, in update
    light = self._bridge.lights()[self._light_ids[0]]
KeyError: 9518399593890067646

deisi · 2017-05-12T08:41:02Z

Hey Ian Would you be willing to help me fix it? I cant reproduce it, so I need somebody to test a fix and probably give me some debug output. Cheers. Ian Young <notifications@github.com> writes:

…

This change breaks my Home Assistant setup. Each time I start HA the value of self._light_ids[0] seems to be incorrect but the error does not always happen with the same light. Maybe there's a discrepancy between the bridge and the states in HA. I've rolled back to 0.43.2 for now and the problem went away. `2017-05-10 13:31:46 ERROR (MainThread) [homeassistant.components.light] Error while setting up platform osramlightify Traceback (most recent call last): File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/helpers/entity_component.py", line 155, in _async_setup_platform entity_platform.schedule_add_entities, discovery_info File "/opt/rh/rh-python35/root/usr/lib64/python3.5/asyncio/futures.py", line 358, in __iter__ yield self # This tells Task to wait for completion. File "/opt/rh/rh-python35/root/usr/lib64/python3.5/asyncio/tasks.py", line 290, in _wakeup future.result() File "/opt/rh/rh-python35/root/usr/lib64/python3.5/asyncio/futures.py", line 274, in result raise self._exception File "/opt/rh/rh-python35/root/usr/lib64/python3.5/concurrent/futures/thread.py", line 55, in run result = self.fn(*self.args, **self.kwargs) File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/components/light/osramlightify.py", line 60, in setup_platform setup_bridge(bridge, add_devices, add_groups) File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/components/light/osramlightify.py", line 106, in setup_bridge update_lights() File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/util/__init__.py", line 303, in wrapper result = method(*args, **kwargs) File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/util/__init__.py", line 303, in wrapper result = method(*args, **kwargs) File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/components/light/osramlightify.py", line 97, in update_lights update_lights) File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/components/light/osramlightify.py", line 268, in __init__ super().__init__(group, update_lights) File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/components/light/osramlightify.py", line 121, in __init__ self.update() File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/components/light/osramlightify.py", line 282, in update light = self._bridge.lights()[self._light_ids[0]] KeyError: 9518399593890067646`

duffrecords · 2017-05-12T19:40:53Z

I won't be available until Wednesday but I can help you after then.

balloob · 2017-05-13T03:09:32Z

Please open an issue and discuss a potential fix there.

homeassistant added cla-signed platform: light.osramlightify labels Apr 30, 2017

balloob requested changes Apr 30, 2017

View reviewed changes

Fixed Pylint errors.

52720b7

homeassistant added the cla-signed label Apr 30, 2017

balloobbot added platform requirement labels Apr 30, 2017

dale3h changed the title ~~Added osramlighrify groups.~~ Added osramlightify groups. May 2, 2017

Included requests.

8236b3e

homeassistant added the cla-signed label May 3, 2017

houndci-bot reviewed May 3, 2017

View reviewed changes

deisi and others added 2 commits May 3, 2017 07:33

Included more requests.

4dea47f

Merge branch 'dev' into lightify_groups

354a4f5

balloob reviewed May 3, 2017

View reviewed changes

Fixed setup bridge and removed _light attribute.

dc08bdc

deisi force-pushed the lightify_groups branch from 0ae18ce to dc08bdc Compare May 3, 2017 20:58

homeassistant added the cla-signed label May 3, 2017

Update osramlightify.py

f65f7ec

homeassistant added the cla-signed label May 4, 2017

balloob approved these changes May 4, 2017

View reviewed changes

balloob merged commit e3bb45c into home-assistant:dev May 4, 2017

balloob mentioned this pull request May 5, 2017

0.44 #7444

Merged

home-assistant locked and limited conversation to collaborators May 13, 2017

deisi deleted the lightify_groups branch May 22, 2017 18:40

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

Uh oh!

Conversation

deisi commented Apr 30, 2017

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

mention-bot commented Apr 30, 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!

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!

Choose a reason for hiding this comment

Uh oh!

balloob commented Apr 30, 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!

balloob commented May 3, 2017

Uh oh!

deisi commented May 4, 2017

Uh oh!

MartinHjelmare commented May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chimpy commented May 5, 2017

Uh oh!

deisi commented May 5, 2017 via email

Uh oh!

duffrecords commented May 12, 2017 • edited by balloob Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deisi commented May 12, 2017 via email

Uh oh!

duffrecords commented May 12, 2017

Uh oh!

balloob commented May 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Example entry for `configuration.yaml` (if applicable):

MartinHjelmare commented May 4, 2017 •

edited

Loading

duffrecords commented May 12, 2017 •

edited by balloob

Loading