Add support for Insteon FanLinc fan #6959
Conversation
|
@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. |
|
|
||
| def turn_on(self: ToggleEntity, speed: str=None, **kwargs) -> None: | ||
| """Turn device on.""" | ||
| if speed == None: |
There was a problem hiding this comment.
comparison to None should be 'if cond is None:'
|
Looks like one completely un-related flaky test failed. |
| setup_fan(device_id, data.get('name'), insteonhub, hass, | ||
| add_devices_callback) | ||
|
|
||
| _CONFIGURING[device_id] = configurator.request_config( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Yeah we should remove it there too, for a future PR. Let's just make sure we don't introduce new instances for now.
| 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)]) |
|
|
||
| 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), |
There was a problem hiding this comment.
undefined name 'add_devices_callback'
| 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 |
There was a problem hiding this comment.
'homeassistant.loader.get_component' imported but unused
|
@jawilson I removed the configurator stuff, can you please test this and let us know if it still works and is good to merge? |
|
@robbiet480 your changes completely remove all discovery of linked devices on the hub, @balloob the reason for the very weird usage of the configurator is because there is no way to infer device names from |
|
Alternatively, we could add an Insteon Local panel that allows you to set the |
|
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. |
|
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 ? |
Conflicts: homeassistant/components/insteon_local.py requirements_all.txt
|
@balloob reverted and updated |
Description:
Insteon FanLinc fan support was added in version 0.41 of the
insteonlocaldepency. This adds support for that in HomeAssistant. I have been using this for some time and it works great. I also bumped theinsteonlocaldependency 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):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
toxrun successfully. Your PR cannot be merged unless tests passREQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.