Refactor hue to split bridge support from light platform #10691

andreacampi · 2017-11-20T00:19:51Z

Description:

This is in preparation to supporting sensors.

Related issue (if applicable): fixes #10609

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

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

hue:
  bridges:
    - host: 192.168.0.40
    - host: 192.168.0.80
    - filename: /tmp/phue.conf

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.

homeassistant · 2017-11-20T00:19:53Z

Hi @andreacampi,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

houndci-bot · 2017-11-20T00:19:57Z

homeassistant/components/hue.py

+        self.config_request_id = configurator.request_config(
+            "Philips Hue",
+            lambda data: self.setup(),
+            description=("Press the button on the bridge to register Philips Hue "


line too long (82 > 79 characters)

houndci-bot · 2017-11-20T00:19:57Z

homeassistant/components/hue.py

+        # We got an error if this method is called while we are configuring
+        if self.config_request_id:
+            configurator.notify_errors(
+                self.config_request_id, "Failed to register, please try again.")


line too long (80 > 79 characters)

houndci-bot · 2017-11-20T00:19:57Z

homeassistant/components/hue.py

+
+        try:
+            api = self.bridge.get_api()
+        except phue.PhueRequestTimeout:


undefined name 'phue'

houndci-bot · 2017-11-20T00:19:57Z

homeassistant/components/hue.py

+            os.path.join(os.path.dirname(__file__), 'services.yaml'))
+        self.hass.services.register(DOMAIN, SERVICE_HUE_SCENE, hue_activate_scene,
+                               descriptions.get(SERVICE_HUE_SCENE),
+                               schema=SCENE_SCHEMA)


continuation line under-indented for visual indent

houndci-bot · 2017-11-20T00:19:57Z

homeassistant/components/hue.py

+        descriptions = load_yaml_config_file(
+            os.path.join(os.path.dirname(__file__), 'services.yaml'))
+        self.hass.services.register(DOMAIN, SERVICE_HUE_SCENE, hue_activate_scene,
+                               descriptions.get(SERVICE_HUE_SCENE),


continuation line under-indented for visual indent

houndci-bot · 2017-11-20T00:19:58Z

homeassistant/components/hue.py

+
+GROUP_NAME_ALL_HUE_LIGHTS = "All Hue Lights"
+
+def _find_host_from_config(hass, filename=PHUE_CONFIG_FILE):


expected 2 blank lines, found 1

houndci-bot · 2017-11-20T00:19:58Z

tests/components/test_hue.py

+        self.assertEqual(1, len(self.hass.states.all()))
+        self.assertEqual('configure', self.hass.states.all()[0].state)
+        self.assertEqual('Failed to register, please try again.',
+            self.hass.states.all()[0].attributes.get(configurator.ATTR_ERRORS))


continuation line under-indented for visual indent

houndci-bot · 2017-11-20T00:19:58Z

tests/components/test_hue.py

+
+        # We should see a total of two identical calls
+        args = call('localhost',
+            config_file_path=get_test_config_dir(hue.PHUE_CONFIG_FILE))


continuation line under-indented for visual indent

houndci-bot · 2017-11-20T00:19:58Z

tests/components/test_hue.py

+
+        # We should see a total of two identical calls
+        args = call('localhost',
+            config_file_path=get_test_config_dir(hue.PHUE_CONFIG_FILE))


continuation line under-indented for visual indent

houndci-bot · 2017-11-20T00:19:59Z

tests/components/test_hue.py

+
+        # We should see a total of two identical calls
+        args = call('localhost',
+            config_file_path=get_test_config_dir(hue.PHUE_CONFIG_FILE))


continuation line under-indented for visual indent

andreacampi · 2017-11-20T00:23:10Z

There are a few things I still have to clean up, but I wanted to get the ball rolling and get early feedback.

To do:

support multiple bridges;
support the older config schema and provide upgrade instructions;
add back support for discovery;
more unit tests, including:
- the bridge service (hue_activate_scene)
- lights

Re config, this changes from:

light:
  - platform: hue
    host: DEVICE_IP_ADDRESS

to:

hue:
  - host: DEVICE_IP_ADDRESS

I find this makes more sense when the component can support more than just lights, yes?

andreacampi · 2017-11-20T00:24:17Z

cc @robmarkcole

homeassistant · 2017-11-20T00:34:27Z

Hi @andreacampi,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

houndci-bot · 2017-11-20T00:34:37Z

tests/components/test_hue.py

+
+                    mock_bridge.assert_called_once_with(
+                        'localhost',
+                        config_file_path=get_test_config_dir(hue.PHUE_CONFIG_FILE))


line too long (83 > 79 characters)

houndci-bot · 2017-11-20T00:34:37Z

tests/components/test_hue.py

+
+        with assert_setup_component(1):
+            with patch('homeassistant.components.hue._find_host_from_config',
+                    return_value='localhost') as mock_config:


continuation line under-indented for visual indent
local variable 'mock_config' is assigned to but never used

houndci-bot · 2017-11-20T00:34:37Z

tests/components/test_hue.py

+            with patch('homeassistant.helpers.discovery.load_platform') \
+                    as mock_load:
+                self.assertTrue(setup_component(
+                    self.hass, hue.DOMAIN, {hue.DOMAIN: {CONF_HOST: 'localhost'}}))


line too long (83 > 79 characters)

houndci-bot · 2017-11-20T00:34:37Z

tests/components/test_hue.py

+
+from homeassistant.components import configurator, hue
+# import homeassistant.components.configurator as configurator
+from homeassistant.const import CONF_HOST, CONF_PLATFORM


'homeassistant.const.CONF_PLATFORM' imported but unused

houndci-bot · 2017-11-20T00:34:37Z

tests/components/test_hue.py

+import logging
+import unittest
+from unittest.mock import call, MagicMock, patch
+import os


'os' imported but unused

houndci-bot · 2017-11-20T00:34:37Z

tests/components/test_hue.py

+
+import logging
+import unittest
+from unittest.mock import call, MagicMock, patch


'unittest.mock.MagicMock' imported but unused

homeassistant · 2017-11-20T00:37:38Z

Hi @andreacampi,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

balloob · 2017-11-20T01:37:15Z

It seems like you have some commits in this branch that are not yours, please base your work off a fresh copy of the dev branch.

balloob · 2017-11-20T01:38:12Z

I haven't looked at the code at all, but yes, migrating hue from just a platform to be a component for auth and a platform for lights = 👍

robmarkcole · 2017-11-20T19:35:43Z

Im not sure I follow this refactor, appears to be a lot of light specific code in components/hue.py which is the hub/auth..?

andreacampi · 2017-11-20T21:54:14Z

@robmarkcole yeah that's fair, not sure why I decided to go that way. I'll clean that up and/or explain.

andreacampi · 2017-11-20T21:54:44Z

I'll rebase and check the failed tests; just not today as I've had a long day on ~4 hours of sleep.

houndci-bot · 2017-11-23T00:30:05Z

tests/components/light/test_hue.py

+                self.mock_bridge, mock.ANY, self.mock_bridge_type,
+                self.mock_bridge.allow_unreachable,
+                self.mock_bridge.allow_in_emulated_hue,
+                True),


continuation line under-indented for visual indent

houndci-bot · 2017-11-23T00:30:05Z

tests/components/light/test_hue.py

+            call(0, {'name': 'All Hue Lights', 'state': {'any_on': True}},
+                self.mock_bridge, mock.ANY, self.mock_bridge_type,
+                self.mock_bridge.allow_unreachable,
+                self.mock_bridge.allow_in_emulated_hue,


continuation line under-indented for visual indent

houndci-bot · 2017-11-23T00:30:05Z

tests/components/light/test_hue.py

+                self.mock_bridge.allow_in_emulated_hue, True),
+            call(0, {'name': 'All Hue Lights', 'state': {'any_on': True}},
+                self.mock_bridge, mock.ANY, self.mock_bridge_type,
+                self.mock_bridge.allow_unreachable,


continuation line under-indented for visual indent

houndci-bot · 2017-11-23T00:30:05Z

tests/components/light/test_hue.py

+                self.mock_bridge_type, self.mock_bridge.allow_unreachable,
+                self.mock_bridge.allow_in_emulated_hue, True),
+            call(0, {'name': 'All Hue Lights', 'state': {'any_on': True}},
+                self.mock_bridge, mock.ANY, self.mock_bridge_type,


continuation line under-indented for visual indent

houndci-bot · 2017-11-23T00:30:05Z

tests/components/light/test_hue.py

+        mock_hue_light.assert_has_calls([
+            call(2, {'state': 'off'}, self.mock_bridge, mock.ANY,
+                self.mock_bridge_type, self.mock_bridge.allow_unreachable,
+                self.mock_bridge.allow_in_emulated_hue, True),


continuation line under-indented for visual indent

houndci-bot · 2017-11-23T00:30:06Z

tests/components/light/test_hue.py

+                self.mock_bridge.allow_in_emulated_hue, True),
+            call(2, {'state': 'off'}, self.mock_bridge, mock.ANY,
+                self.mock_bridge_type, self.mock_bridge.allow_unreachable,
+                self.mock_bridge.allow_in_emulated_hue, True),


continuation line under-indented for visual indent

houndci-bot · 2017-11-23T00:30:06Z

tests/components/light/test_hue.py

+                self.mock_bridge_type, self.mock_bridge.allow_unreachable,
+                self.mock_bridge.allow_in_emulated_hue, True),
+            call(2, {'state': 'off'}, self.mock_bridge, mock.ANY,
+                self.mock_bridge_type, self.mock_bridge.allow_unreachable,


continuation line under-indented for visual indent

houndci-bot · 2017-11-23T00:30:06Z

tests/components/light/test_hue.py

+        mock_hue_light.assert_has_calls([
+            call(1, {'state': 'on'}, self.mock_bridge, mock.ANY,
+                self.mock_bridge_type, self.mock_bridge.allow_unreachable,
+                self.mock_bridge.allow_in_emulated_hue, True),


continuation line under-indented for visual indent

houndci-bot · 2017-11-23T00:30:06Z

tests/components/light/test_hue.py

+        self.assertEquals(len(ret), 3)
+        mock_hue_light.assert_has_calls([
+            call(1, {'state': 'on'}, self.mock_bridge, mock.ANY,
+                self.mock_bridge_type, self.mock_bridge.allow_unreachable,


continuation line under-indented for visual indent

houndci-bot · 2017-11-23T00:30:06Z

tests/components/light/test_hue.py

+        self.mock_api.get.return_value = {1: {'state': 'on'}, 2: {'state': 'off'}}
+
+        ret = hue_light.process_groups(self.hass,
+            self.mock_api, self.mock_bridge, self.mock_bridge_type)


continuation line under-indented for visual indent

Not only this looks nicer, but it also nicely solves additional bridges being added after initial setup (e.g. pairing a second bridge should work now, I believe it required a restart before).

…t phue in tests, yay!

balloob · 2017-12-10T18:15:11Z

🎉 🐬 Awesome, great job !

…ent-being-nonexistent * origin/dev: Volvo on call: Optional use of Scandinavian miles. Also add average fuel consumption property (home-assistant#11051) add custom bypass status to total connect (home-assistant#11042) Refactor hue to split bridge support from light platform (home-assistant#10691) Add GPS coords to meraki (home-assistant#10998) Add a caldav calendar component (home-assistant#10842) Added support for cover in tellstick (home-assistant#10858)

…into dev * 'dev' of https://github.com/home-assistant/home-assistant: Volvo on call: Optional use of Scandinavian miles. Also add average fuel consumption property (home-assistant#11051) add custom bypass status to total connect (home-assistant#11042) Refactor hue to split bridge support from light platform (home-assistant#10691) Add GPS coords to meraki (home-assistant#10998) Add a caldav calendar component (home-assistant#10842) Added support for cover in tellstick (home-assistant#10858)

mezz64 · 2017-12-12T18:26:16Z

These changes seem to be allowing HASS to discover it's own emulated hue bridge as a bridge device and throws an error because it can't find the config file.

m2k2 · 2017-12-13T17:33:28Z

I'm also having some problems since this commit after re-working my configuration. Once the hubs are added with emulated hue disabled, I see the error below for each light. Should I open up a new issue?

Update for light.garage__right fails
Traceback (most recent call last):
  File "/srv/homeassistant/homeassistant_venv/lib/python3.5/site-packages/homeassistant/helpers/entity.py", line 199, in async_update_ha_state
    yield from self.async_device_update()
  File "/srv/homeassistant/homeassistant_venv/lib/python3.5/site-packages/homeassistant/helpers/entity.py", line 306, in async_device_update
    yield from self.hass.async_add_job(self.update)
  File "/usr/lib/python3.5/asyncio/futures.py", line 380, in __iter__
    yield self  # This tells Task to wait for completion.
  File "/usr/lib/python3.5/asyncio/tasks.py", line 304, in _wakeup
    future.result()
  File "/usr/lib/python3.5/asyncio/futures.py", line 293, in result
    raise self._exception
  File "/usr/lib/python3.5/concurrent/futures/thread.py", line 55, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/srv/homeassistant/homeassistant_venv/lib/python3.5/site-packages/homeassistant/components/light/hue.py", line 385, in update
    self.update_lights(no_throttle=True)
  File "/srv/homeassistant/homeassistant_venv/lib/python3.5/site-packages/homeassistant/components/light/hue.py", line 149, in <lambda>
    lambda **kw: update_lights(hass, bridge, add_devices, **kw))
  File "/srv/homeassistant/homeassistant_venv/lib/python3.5/site-packages/homeassistant/util/__init__.py", line 306, in wrapper
    result = method(*args, **kwargs)
  File "/srv/homeassistant/homeassistant_venv/lib/python3.5/site-packages/homeassistant/util/__init__.py", line 306, in wrapper
    result = method(*args, **kwargs)
TypeError: update_lights() got an unexpected keyword argument 'no_throttle'

andreacampi · 2017-12-13T20:58:23Z

Both of these are pretty bad :( I'll try to report and open tasks as needed, but @mezz64 @m2k2 if you want to do that'd be helpful.

@m2k2 unless I broke something last second while iterating on unit tests, I don't know that I have enough to reproduce it. I'll ping you if I have trouble.

andreacampi · 2017-12-13T21:01:45Z

@balloob thoughts on me removing the @Throttle annotation? that would remove the issue @m2k2 is seeing.

andreacampi · 2017-12-13T22:20:13Z

See #11126

andreacampi · 2017-12-13T22:49:35Z

Another pull request: #11128

* Update documentation for Philips Hue changes. See home-assistant/core#10691 for more info. * Update hue.markdown

…ent. See: home-assistant/core#10691

brent20 · 2017-12-22T20:05:24Z

How would one set the scan_interval now on the new hue component? Before I was able to set it per the docs here: https://home-assistant.io/docs/configuration/platform_options/

Leaving this in place with the new Hue component seemed to break the configuration. Once I know the new way to set a scan_interval, I'll go ahead and submit a PR to update the docs.

Thanks!

MartinHjelmare · 2017-12-22T21:21:20Z

Please open an issue if you suspect a bug. If you need help please use our help channels:
https://home-assistant.io/help/#communication-channels

Merged PRs should not be used for support or bug reports. Thanks!

mcoms · 2017-12-28T10:42:34Z

This work appears to change the filename of the default phue.conf to phue-<bridge serial number>.conf for discovered bridges. After upgrading, this causes HomeAssistant to boot with:

2017-12-28 10:15:26 INFO (MainThread) [homeassistant.setup] Setting up hue
2017-12-28 10:15:26 INFO (MainThread) [homeassistant.setup] Setup of domain hue took 0.0 seconds.
2017-12-28 10:15:26 INFO (MainThread) [homeassistant.core] Bus:Handling <Event component_loaded[L]: component=hue>
2017-12-28 10:15:26 INFO (MainThread) [homeassistant.core] Bus:Handling <Event platform_discovered[L]: service=philips_hue, discovered=host=192.168.1.150, port=80, ssdp_description=http://192.168.1.150:80/description.xml, name=Fhue (192.168.1.150), model_name=Philips hue bridge 2015, model_number=BSB002, serial=00178848ac26>
2017-12-28 10:15:26 INFO (SyncWorker_6) [phue] Attempting to connect to the bridge...
2017-12-28 10:15:26 INFO (SyncWorker_6) [phue] Error opening config file, will attempt bridge registration
2017-12-28 10:15:26 WARNING (SyncWorker_6) [homeassistant.components.hue] Connected to Hue at 192.168.1.150 but not registered.
2017-12-28 10:15:27 INFO (SyncWorker_6) [homeassistant.loader] Loaded configurator from homeassistant.components.configurator
2017-12-28 10:15:27 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=configurator, service=configure>
2017-12-28 10:15:27 INFO (MainThread) [homeassistant.core] Bus:Handling <Event state_changed[L]: entity_id=configurator.philips_hue, old_state=None, new_state=<state configurator.philips_hue=configure; configure_id=139857224447368-1, fields=[], friendly_name=Philips Hue, entity_picture=/static/images/logo_philips_hue.png, description=
Press the button on the bridge to register Philips Hue with Home Assistant.

![Location of button on bridge](/static/images/config_philips_hue.jpg)
, submit_caption=I have pressed the button @ 2017-12-28T10:15:27.009680+00:00>>

And to fail to connect to the bridge.

My hue bridge is in the roof and pressing the button was quite a pain.

It would be handy to either, upgrade (i.e. rename) the existing phue.conf file automatically, or, to have a log message showing the filename the component was trying to read, so I could manually rename it.

May also be useful to update the docs to give the filename of the hue token file, since I looked there first of all to see if there'd been a change.

MartinHjelmare · 2017-12-28T11:52:36Z

Please open an issue in the appropriate reoo if you suspect a bug or there's a problem with documentation. If you need help please use our help channels:
https://home-assistant.io/help/#communication-channels

Merged PRs should not be used for support or bug reports. You can link to the PR that you want to tag from the issue. Thanks!

homeassistant added integration: hue merging-to-master This PR is merging into the master branch and should probably change the branch to `dev`. new-platform platform: light.hue labels Nov 20, 2017

homeassistant added the cla-needed label Nov 20, 2017

houndci-bot reviewed Nov 20, 2017

View reviewed changes

homeassistant added the cla-needed label Nov 20, 2017

houndci-bot reviewed Nov 20, 2017

View reviewed changes

homeassistant added cla-needed cla-signed and removed cla-needed labels Nov 20, 2017

balloob changed the base branch from master to dev November 20, 2017 01:36

balloob removed the merging-to-master This PR is merging into the master branch and should probably change the branch to `dev`. label Nov 21, 2017

andreacampi force-pushed the pr10609 branch from c328dc3 to af7ea69 Compare November 21, 2017 21:44

homeassistant added the cla-signed label Nov 21, 2017

andreacampi force-pushed the pr10609 branch from af7ea69 to d77d48f Compare November 21, 2017 21:57

homeassistant added the cla-signed label Nov 21, 2017

andreacampi force-pushed the pr10609 branch from d77d48f to 6b65e9a Compare November 23, 2017 00:29

homeassistant added the cla-signed label Nov 23, 2017

houndci-bot reviewed Nov 23, 2017

View reviewed changes

andreacampi added 5 commits December 9, 2017 20:34

Call load_platform from inside HueBridge.setup passing the bridge id.

a7f1781

Not only this looks nicer, but it also nicely solves additional bridges being added after initial setup (e.g. pairing a second bridge should work now, I believe it required a restart before).

Add a unit test for hue_activate_scene

622fb32

Address feedback from code review.

c49e0d7

After feedback from @andrey-git I was able to find a way to not impor…

4e3dbff

…t phue in tests, yay!

Inject a mock phue in a couple of places

d596762

andreacampi force-pushed the pr10609 branch from 8d90f04 to d596762 Compare December 9, 2017 20:34

homeassistant added the cla-signed label Dec 9, 2017

balloob approved these changes Dec 10, 2017

View reviewed changes

balloob added the breaking-change label Dec 10, 2017

balloob merged commit 8197488 into home-assistant:dev Dec 10, 2017

andreacampi deleted the pr10609 branch December 13, 2017 20:58

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

Update documentation for Philips Hue changes. (#4099)

2338f88

* Update documentation for Philips Hue changes. See home-assistant/core#10691 for more info. * Update hue.markdown

fabaff mentioned this pull request Dec 16, 2017

0.60 #11153

Merged

ntalekt added a commit to ntalekt/homeassistant that referenced this pull request Dec 18, 2017

hue refactor home-assistant/core#10691

0af8d5a

pschmitt added a commit to pschmitt/hass-config that referenced this pull request Dec 20, 2017

Update zhue.sh in accordance to latest changes to the hass hue compon…

bc19f48

…ent. See: home-assistant/core#10691

home-assistant locked and limited conversation to collaborators Dec 28, 2017

ghost removed the platform: light.hue label Mar 21, 2019


		GROUP_NAME_ALL_HUE_LIGHTS = "All Hue Lights"

		def _find_host_from_config(hass, filename=PHUE_CONFIG_FILE):

Refactor hue to split bridge support from light platform #10691

Refactor hue to split bridge support from light platform #10691

Conversation

andreacampi commented Nov 20, 2017 • edited Loading

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

homeassistant commented Nov 20, 2017

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

andreacampi commented Nov 20, 2017 • edited Loading

andreacampi commented Nov 20, 2017

homeassistant commented Nov 20, 2017

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

homeassistant commented Nov 20, 2017

balloob commented Nov 20, 2017 • edited Loading

balloob commented Nov 20, 2017

robmarkcole commented Nov 20, 2017

andreacampi commented Nov 20, 2017

andreacampi commented Nov 20, 2017

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 Dec 10, 2017

mezz64 commented Dec 12, 2017

m2k2 commented Dec 13, 2017

andreacampi commented Dec 13, 2017

andreacampi commented Dec 13, 2017

andreacampi commented Dec 13, 2017

andreacampi commented Dec 13, 2017

brent20 commented Dec 22, 2017 • edited Loading

MartinHjelmare commented Dec 22, 2017

mcoms commented Dec 28, 2017 • edited Loading

MartinHjelmare commented Dec 28, 2017

andreacampi commented Nov 20, 2017 •

edited

Loading

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

andreacampi commented Nov 20, 2017 •

edited

Loading

balloob commented Nov 20, 2017 •

edited

Loading

brent20 commented Dec 22, 2017 •

edited

Loading

mcoms commented Dec 28, 2017 •

edited

Loading