Change Osram to use Github lightify dep #4256

tfriedel · 2016-11-06T21:26:56Z

Description:

Related issue (if applicable): fixes #3089

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Example entry for configuration.yaml (if applicable):

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.

…tify/commits/master/osramlightify.py ) from Oct 2, 2016 and changed the REQUIRMENTS line to use the fixed lightify component with thread safety fixes

mention-bot · 2016-11-06T21:26:57Z

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

balloobbot · 2016-11-06T21:27:30Z

homeassistant/components/light/osramlightify.py

@@ -19,7 +19,7 @@
    SUPPORT_COLOR_TEMP, SUPPORT_RGB_COLOR, SUPPORT_TRANSITION, PLATFORM_SCHEMA)
 import homeassistant.helpers.config_validation as cv

-REQUIREMENTS = ['lightify==1.0.3']
+REQUIREMENTS = ['https://github.com/tfriedel/python-lightify/archive/master.zip#lightify==0.0.4']


farcy v1.1

80: E501 line too long (97 > 79 characters)

balloobbot · 2016-11-06T21:27:30Z

homeassistant/components/light/osramlightify.py


        if ATTR_TRANSITION in kwargs:
-            fade = kwargs[ATTR_TRANSITION] * 10
+            transition = kwargs[ATTR_TRANSITION] * 10
+            _LOGGER.debug("turn_on requested transition time for light: %s is: %s ",


farcy v1.1

80: E501 line too long (84 > 79 characters)

balloobbot · 2016-11-06T21:27:31Z

homeassistant/components/light/osramlightify.py

        else:
-            fade = 0
+            transition = 0
+            _LOGGER.debug("turn_on requested transition time for light: %s is: %s ",


farcy v1.1

80: E501 line too long (84 > 79 characters)

balloobbot · 2016-11-06T21:27:31Z

homeassistant/components/light/osramlightify.py

-
-        if ATTR_BRIGHTNESS in kwargs:
-            brightness = int(kwargs[ATTR_BRIGHTNESS] / 2.55)
+            _LOGGER.debug("turn_on requested ATTR_RGB_COLOR for light: %s is: %s %s %s ",


farcy v1.1

80: E501 line too long (89 > 79 characters)

balloobbot · 2016-11-06T21:27:31Z

homeassistant/components/light/osramlightify.py

-                                fade)
-
-        self._light.set_luminance(brightness, fade)
+            kelvin = int(((TEMP_MAX - TEMP_MIN) * (color_t - TEMP_MIN_HASS) / (TEMP_MAX_HASS - TEMP_MIN_HASS)) + TEMP_MIN)


farcy v1.1

80: E501 line too long (122 > 79 characters)

balloobbot · 2016-11-06T21:27:31Z

homeassistant/components/light/osramlightify.py

-
-        self._light.set_luminance(brightness, fade)
+            kelvin = int(((TEMP_MAX - TEMP_MIN) * (color_t - TEMP_MIN_HASS) / (TEMP_MAX_HASS - TEMP_MIN_HASS)) + TEMP_MIN)
+            _LOGGER.debug("turn_on requested set_temperature for light: %s: %s ",


farcy v1.1

80: E501 line too long (81 > 79 characters)

balloobbot · 2016-11-06T21:27:32Z

homeassistant/components/light/osramlightify.py

+            self._brightness = kwargs[ATTR_BRIGHTNESS]
+            _LOGGER.debug("turn_on requested brightness for light: %s is: %s ",
+                          self._light.name, self._brightness)
+            self._brightness = self._light.set_luminance(int(self._brightness / 2.55), transition)


farcy v1.1

80: E501 line too long (98 > 79 characters)

balloobbot · 2016-11-06T21:27:32Z

homeassistant/components/light/osramlightify.py

+                                    random.randrange(0, 255),
+                                    random.randrange(0, 255),
+                                    transition)
+                _LOGGER.debug("turn_on requested random effect for light: %s with transition %s ",


farcy v1.1

80: E501 line too long (98 > 79 characters)

balloobbot · 2016-11-06T21:27:32Z

homeassistant/components/light/osramlightify.py

        if ATTR_TRANSITION in kwargs:
-            fade = kwargs[ATTR_TRANSITION] * 10
+            transition = kwargs[ATTR_TRANSITION] * 10
+            _LOGGER.debug("turn_off requested transition time for light: %s is: %s ",


farcy v1.1

80: E501 line too long (85 > 79 characters)

balloobbot · 2016-11-06T21:27:32Z

homeassistant/components/light/osramlightify.py

-            fade = 0
-        self._light.set_luminance(0, fade)
+            transition = 0
+            _LOGGER.debug("turn_off requested transition time for light: %s is: %s ",


farcy v1.1

80: E501 line too long (85 > 79 characters)

balloobbot · 2016-11-07T00:05:10Z

homeassistant/components/light/osramlightify.py


        if ATTR_COLOR_TEMP in kwargs:
            color_t = kwargs[ATTR_COLOR_TEMP]
            kelvin = int(((TEMP_MAX - TEMP_MIN) * (color_t - TEMP_MIN_HASS) /
-                          (TEMP_MAX_HASS - TEMP_MIN_HASS)) + TEMP_MIN)
-            self._light.set_temperature(kelvin, fade)
+                (TEMP_MAX_HASS - TEMP_MIN_HASS)) + TEMP_MIN)


farcy v1.1

17: E128 continuation line under-indented for visual indent

balloob · 2016-11-07T07:43:19Z

What is the benefit from switching to your lib instead of a lib published on PyPi?

tfriedel · 2016-11-07T08:06:48Z

See this thread
https://community.home-assistant.io/t/written-some-osram-lightify-support/43/49
Basically the guy who published the lib for pypi is not developing the lib
and he did not react to my pull request a month ago. My lib includes fixes
to make it thread safe and more robust. It was unusable for me an other
users before.

Paulus Schoutsen notifications@github.com schrieb am Mo., 7. Nov. 2016,
08:43:

What is the benefit from switching to your lib instead of a lib published
on PyPi?

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#4256 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABu6xvnOsdA5qWeNz1jzauje_XpdXYyQks5q7taggaJpZM4Kqr3L
.

tchellomello · 2016-11-07T23:12:47Z

I have been using this version for some time already and it is very stable and working fine. Thanks for your PR @tfriedel

balloob · 2016-11-11T04:31:30Z

homeassistant/components/light/osramlightify.py

@@ -19,7 +19,8 @@
    SUPPORT_COLOR_TEMP, SUPPORT_RGB_COLOR, SUPPORT_TRANSITION, PLATFORM_SCHEMA)
 import homeassistant.helpers.config_validation as cv

-REQUIREMENTS = ['lightify==1.0.3']
+REQUIREMENTS = ['https://github.com/tfriedel/python-lightify/archive/'
+                'master.zip#lightify==0.0.4']


Please don't point it at the master branch but at the zip of a specific commit.

You also need to specify the exact version number as is contained in the zip, so I think 1.0.4 ?

balloob · 2016-11-11T04:32:20Z

homeassistant/components/light/osramlightify.py

@@ -101,6 +104,9 @@ def name(self):
    @property
    def rgb_color(self):
        """Last RGB color value set."""
+        _LOGGER.debug("rgb_color light state for light: %s is: %s %s %s ",
+                      self._light.name(), self._light.red(),
+                      self._light.green(), self._light.blue())
        return self._light.rgb()


We should not make any requests to lights in properties. Instead, all access has to be moved to the update method and data should be stored in instance variables.

…pdate in update method

balloob · 2016-11-19T06:18:08Z

homeassistant/components/light/osramlightify.py


    @property
    def is_on(self):
        """Update Status to True if device is on."""
-        self.update_lights()
+        self.update()


Home Assistant will take care of calling update. You cannot do any I/O inside properties.

balloob · 2016-11-19T06:18:34Z

requirements_all.txt

@@ -223,6 +223,9 @@ https://github.com/robbiet480/pygtfs/archive/00546724e4bbcb3053110d844ca44e22462
 # homeassistant.components.scene.hunterdouglas_powerview
 https://github.com/sander76/powerviewApi/archive/246e782d60d5c0addcc98d7899a0186f9d5640b0.zip#powerviewApi==0.3.15

+# homeassistant.components.light.osramlightify
+https://github.com/tfriedel/python-lightify/archive/master.zip#lightify==0.0.4


This is not in sync anymore with the platform requirement.

balloob · 2016-11-23T06:10:42Z

Looks good! 🐬

used MindrustUK's version ( https://github.com/MindrustUK/python-ligh…

dd92610

…tify/commits/master/osramlightify.py ) from Oct 2, 2016 and changed the REQUIRMENTS line to use the fixed lightify component with thread safety fixes

balloobbot reviewed Nov 6, 2016

View reviewed changes

reformatted long lines

eb11eef

tfriedel closed this Nov 6, 2016

updated osramlightify requirements in requirements_all.txt

2649922

tfriedel reopened this Nov 6, 2016

ran script gen_requirements_all.py

35ca74a

balloobbot reviewed Nov 7, 2016

View reviewed changes

thomas.friedel added 2 commits November 7, 2016 01:14

rerun requirements gen script on linux

6c80b8f

fixed some inspection warnings

1e25daf

robbiet480 changed the title ~~used MindrustUK's version ( https://github.com/MindrustUK/python-ligh…~~ Change Osram to use Github lightify dep Nov 7, 2016

balloob reviewed Nov 11, 2016

View reviewed changes

thomas.friedel added 2 commits November 12, 2016 15:35

zip file points to a specific commit

56df3ed

no requests to lights in properties, instead instance variables are u…

e13fb05

…pdate in update method

balloob reviewed Nov 19, 2016

View reviewed changes

regenerated requirements_all.txt

73be46a

thomas.friedel added 2 commits November 19, 2016 17:32

removed call to update from is_on() property

bab8d5b

Merge remote-tracking branch 'origin/dev' into dev

ea886b0

balloob merged commit 0c47434 into home-assistant:dev Nov 23, 2016

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Osram to use Github lightify dep #4256

Change Osram to use Github lightify dep #4256

tfriedel commented Nov 6, 2016 •

edited

mention-bot commented Nov 6, 2016

balloobbot Nov 6, 2016

balloobbot Nov 6, 2016

balloobbot Nov 6, 2016

balloobbot Nov 6, 2016

balloobbot Nov 6, 2016

balloobbot Nov 6, 2016

balloobbot Nov 6, 2016

balloobbot Nov 6, 2016

balloobbot Nov 6, 2016

balloobbot Nov 6, 2016

balloobbot Nov 7, 2016

balloob commented Nov 7, 2016

tfriedel commented Nov 7, 2016

tchellomello commented Nov 7, 2016

balloob Nov 11, 2016

balloob Nov 11, 2016

balloob Nov 19, 2016

balloob Nov 19, 2016

balloob commented Nov 23, 2016

Change Osram to use Github lightify dep #4256

Change Osram to use Github lightify dep #4256

Conversation

tfriedel commented Nov 6, 2016 • edited

mention-bot commented Nov 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob commented Nov 7, 2016

tfriedel commented Nov 7, 2016

tchellomello commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob commented Nov 23, 2016

tfriedel commented Nov 6, 2016 •

edited