Fix insteon Hub v1 support#16472
Fix insteon Hub v1 support#16472MartinHjelmare merged 7 commits intohome-assistant:devfrom teharris1:HubV1
Conversation
homeassistant
added
integration: insteon
small-pr
ghost
added
the
| CONF_OVERRIDE = 'device_override' | ||
| CONF_PLM_HUB_MSG = ('Must configure either a PLM port or a Hub host, username ' | ||
| 'and password') | ||
| CONF_PLM_HUB_MSG = ('Must configure either a PLM port or a Hub host') |
There was a problem hiding this comment.
Parenthesis isn't needed.
|
|
||
| # Set the default value of the IP port based on Hub version if not | ||
| # explicitly set | ||
| if not ip_port: |
There was a problem hiding this comment.
Extract this into a config validator function, and use it in the config schema.
There was a problem hiding this comment.
OK, this was a good exercise. Figured it out but it was not obvious. Thanks for pushing me to do the right thing. You are really good at helping average developers like myself get to the right solution. I really appreciate it.
| elif not ip_port: | ||
| obj[CONF_IP_PORT] = 25105 | ||
| except KeyError: | ||
| raise vol.Invalid('Schema must contain {}.'.format( |
There was a problem hiding this comment.
This can't happen, since there's a default version.
| """Set the default port based on the Hub version.""" | ||
| def set_default(obj: Dict) -> Dict: | ||
| """Set ip_port default value based on hub_version.""" | ||
| if not isinstance(obj, dict): |
There was a problem hiding this comment.
This can't happen. The schema already validates the value to a dict.
| try: | ||
| # If the ip_port is found do nothing | ||
| # If it is not found (KeyError) the set the default | ||
| ip_port = obj[CONF_IP_PORT] |
There was a problem hiding this comment.
Use dict.get.
ip_port = obj.get(CONF_IP_PORT)
if ip_port is None:
hub_version = obj[CONF_HUB_VERSION]
if hub_version == 1:
obj[CONF_IP_PORT] = 9761
else:
obj[CONF_IP_PORT] = 25105
return obj|
|
||
| # Adapted from: | ||
| # https://github.com/alecthomas/voluptuous/issues/115#issuecomment-144464666 | ||
| def set_default_port() -> Callable: |
There was a problem hiding this comment.
There's no need to have this nesting, since we don't need to pass in any other arguments than the dict we want to validate.
There was a problem hiding this comment.
Not sure this is correct. The entire schema has to be passed to the method. I don't see how to pass this if it is not nested. I am following the same pattern as has_at_least_one_key. Why would this be different?
There was a problem hiding this comment.
Nevermind. I see what you mean now. Sorry.
| import asyncio | ||
| import collections | ||
| import logging | ||
| from typing import Callable, Dict |
There was a problem hiding this comment.
'typing.Callable' imported but unused
| # Found hub_version but not ip_port | ||
| if hub_version == 1: | ||
| schema[CONF_IP_PORT] = 9761 | ||
| elif not ip_port: |
There was a problem hiding this comment.
else: is enough here since we already checked that port is falsy above.
| vol.Exclusive(CONF_HOST, 'plm_or_hub', | ||
| msg=CONF_PLM_HUB_MSG): cv.string, | ||
| vol.Optional(CONF_IP_PORT, default=25105): int, | ||
| vol.Optional(CONF_IP_PORT): int, |
|
Can be merged when build passes. |
|
@MartinHjelmare I have a release management question. Since 0.78.0b0 was just released, What does that mean for this fix? Does that mean it can be part of 0.78.0 or will it mean it has to wait until 0.79 or can it be included in 0.77.4 if one is released? Keep in mind this functionality was working prior to 0.77 for |
|
We can tag it for 0.78. |
* Fix support for Hub version 1 (i.e. pre-2014 Hub model 2242) * Bump insteonplm to 0.14.1 * Code review changes * Clean up and better document set_default_port * Simplify set_default_port based on code review * Remove Callable type import * Simplify port setup
5 participants
Description:
When insteon_local and insteon_plm were merged, support for the Insteon Hub version 1 is broken. This fixes that support.
Related issue (if applicable): fixes #16342
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#6211
Example entry for
configuration.yaml(if applicable):Checklist:
tox. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
REQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices: