Add support for Insteon FanLinc fan by jawilson · Pull Request #6959 · home-assistant/core

jawilson · 2017-04-06T15:13:38Z

Description:

Insteon FanLinc fan support was added in version 0.41 of the insteonlocal depency. This adds support for that in HomeAssistant. I have been using this for some time and it works great. I also bumped the insteonlocal dependency since there's a new one out.

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

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

insteon_local:
  host: YOUR HUB IP
  username: YOUR HUB USERNAME
  password: YOUR HUB PASSWORD
  timeout: 10
  port: 25105

fan:
  - platform: insteon_local

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.

mention-bot · 2017-04-06T15:13:47Z

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

houndci-bot · 2017-04-06T15:13:49Z

+
+    def turn_on(self: ToggleEntity, speed: str=None, **kwargs) -> None:
+        """Turn device on."""
+        if speed == None:


comparison to None should be 'if cond is None:'

jawilson · 2017-04-06T15:42:17Z

Looks like one completely un-related flaky test failed.

balloob

Sorry for taking so long to review, looks like all our reviewers are pretty busy.

balloob · 2017-04-19T06:15:06Z

+        setup_fan(device_id, data.get('name'), insteonhub, hass,
+                  add_devices_callback)
+
+    _CONFIGURING[device_id] = configurator.request_config(


This is a very weird use case of the configurator. We shouldn't have people configure it like this, they can much better rename the devices via customize later.

Let's remove this functionality.

Fine with me. Just as a heads up, this was directly copied from Insteon Local Light (this entire implementation is basically just a copy replacing light with fan).

Yeah we should remove it there too, for a future PR. Let's just make sure we don't introduce new instances for now.

balloob

This is ok to merge once the configurator stuff has been removed.

houndci-bot · 2017-05-05T01:38:32Z

+    conf_fans = config_from_file(hass.config.path(INSTEON_LOCAL_FANS_CONF))
+    for device_id in conf_fans:
+        add_devices_callback([InsteonLocalFanDevice(insteonhub.fan(device_id),
+                                                    name)])


undefined name 'name'

houndci-bot · 2017-05-05T01:38:32Z

+
+    conf_fans = config_from_file(hass.config.path(INSTEON_LOCAL_FANS_CONF))
+    for device_id in conf_fans:
+        add_devices_callback([InsteonLocalFanDevice(insteonhub.fan(device_id),


undefined name 'add_devices_callback'

houndci-bot · 2017-05-05T01:38:32Z

+    ATTR_SPEED, SPEED_OFF, SPEED_LOW, SPEED_MEDIUM, SPEED_HIGH,
+    SUPPORT_SET_SPEED, FanEntity)
+from homeassistant.helpers.entity import ToggleEntity
+from homeassistant.loader import get_component


'homeassistant.loader.get_component' imported but unused

robbiet480 · 2017-05-05T01:40:52Z

@jawilson I removed the configurator stuff, can you please test this and let us know if it still works and is good to merge?

jawilson · 2017-05-11T20:18:34Z

@robbiet480 your changes completely remove all discovery of linked devices on the hub, insteonhub.get_linked() was completely removed.

@balloob the reason for the very weird usage of the configurator is because there is no way to infer device names from insteonlocal. So without the user informing HASS of each device's name, you'll end up with very vague device object_ids. I very much prefer having fan.office_fan and fan.living_room instead of fan.fanlinc_1 and fan.fanlinc_2. Sure I can change the display name, but writing configs will get extremely confusing when you have a handful of lights/fans without any meaningful names. Unless you have any other thoughts, I suggest we revert @robbiet480's changes and use the configurator how it's being using in lights.insteon_local.

jawilson · 2017-05-11T20:21:38Z

Alternatively, we could add an Insteon Local panel that allows you to set the object_ids of insteon_local devices. But I haven't even looked at front-end development yet (nor do I really want to).

balloob · 2017-05-13T03:54:53Z

It should not be a concern of a component to address naming of object ids. There was an issue (can't find it right now) that was addressing object id renaming in a global fashion.

balloob · 2017-06-03T04:42:07Z

I'm down with reverting it for now since rest of insteon local needs fixing too. Since it's your branch, can you revert the commit and rebase on latest dev to resolve merge conflicts ?

This reverts commit 04d1f7f. This reverts commit 7b8278d.

Conflicts: homeassistant/components/insteon_local.py requirements_all.txt

jawilson · 2017-06-14T15:22:06Z

@balloob reverted and updated

* Add support for Insteon FanLinc fan * Upgrade insteonlocal dependency to 0.49 * Lint/flake fixes * Remove configurator * Make Hound fixes * Revert "Make Hound fixes" and "Remove configurator" This reverts commit 04d1f7f. This reverts commit 7b8278d.

jawilson added 2 commits April 6, 2017 10:47

Add support for Insteon FanLinc fan

590b3d0

Upgrade insteonlocal dependency to 0.49

8c0abfd

homeassistant added integration: insteon_local new-platform platform: fan.insteon_local labels Apr 6, 2017

houndci-bot reviewed Apr 6, 2017

View reviewed changes

jawilson mentioned this pull request Apr 6, 2017

Insteon local fan home-assistant/home-assistant.io#2390

Merged

Lint/flake fixes

b917d37

homeassistant added the cla-signed label Apr 6, 2017

balloobbot added component platform requirement labels Apr 6, 2017

Merge remote-tracking branch 'upstream/dev' into insteon-fanlinc

6f6312d

homeassistant added the cla-signed label Apr 11, 2017

balloob requested changes Apr 19, 2017

View reviewed changes

balloob requested changes Apr 30, 2017

View reviewed changes

Remove configurator

7b8278d

houndci-bot reviewed May 5, 2017

View reviewed changes

Make Hound fixes

04d1f7f

jawilson added 2 commits June 14, 2017 11:19

Revert "Make Hound fixes" and "Remove configurator"

dce769f

This reverts commit 04d1f7f. This reverts commit 7b8278d.

Merge remote-tracking branch 'upstream/dev' into insteon-fanlinc

15adaef

Conflicts: homeassistant/components/insteon_local.py requirements_all.txt

homeassistant added the cla-signed label Jun 14, 2017

balloob added the cla-recheck label Jun 19, 2017

homeassistant added cla-signed and removed cla-recheck labels Jun 19, 2017

balloob approved these changes Jun 19, 2017

View reviewed changes

balloob merged commit 756768e into home-assistant:dev Jun 19, 2017

balloob mentioned this pull request Jul 1, 2017

0.48 #8270

Merged

jawilson deleted the insteon-fanlinc branch July 6, 2017 03:58

home-assistant locked and limited conversation to collaborators Oct 20, 2017

ghost removed the platform: fan.insteon_local label Mar 21, 2019

Uh oh!

Conversation

jawilson commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

mention-bot commented Apr 6, 2017

Uh oh!

houndci-bot Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

jawilson commented Apr 6, 2017

Uh oh!

balloob left a comment

Choose a reason for hiding this comment

Uh oh!

balloob Apr 19, 2017

Choose a reason for hiding this comment

Uh oh!

jawilson Apr 19, 2017

Choose a reason for hiding this comment

Uh oh!

balloob Apr 20, 2017

Choose a reason for hiding this comment

Uh oh!

balloob left a comment

Choose a reason for hiding this comment

Uh oh!

houndci-bot May 5, 2017

Choose a reason for hiding this comment

Uh oh!

houndci-bot May 5, 2017

Choose a reason for hiding this comment

Uh oh!

houndci-bot May 5, 2017

Choose a reason for hiding this comment

Uh oh!

robbiet480 commented May 5, 2017

Uh oh!

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

Uh oh!

Uh oh!

jawilson commented May 11, 2017

Uh oh!

balloob commented May 13, 2017

Uh oh!

balloob commented Jun 3, 2017

Uh oh!

jawilson commented Jun 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jawilson commented Apr 6, 2017 •

edited

Loading

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

jawilson commented May 11, 2017 •

edited

Loading