Upgrade insteonplm to 0.8.2 (required refactoring) #12534
Conversation
| return self._address | ||
| def __init__(self, device, state_key): | ||
| """Initialize the INSTEON PLM binary sensor.""" | ||
| InsteonPLMEntity.__init__(self, device, state_key) |
There was a problem hiding this comment.
super().__init__(device, state_key)|
|
||
| async_add_devices(device_list) | ||
| _LOGGER.debug('Adding device %s entity %s to Binary Sensor platform.', |
There was a problem hiding this comment.
Don't end the log message with a period.
| device = plm.devices[address] | ||
| state_key = discovery_info['state_key'] | ||
|
|
||
| _LOGGER.debug('Adding device %s entity %s to Fan platform.', |
There was a problem hiding this comment.
Remove period at the end.
| """Turn on the entity.""" | ||
| if speed is None: | ||
| speed = SPEED_MEDIUM | ||
| self.async_set_speed(speed) |
There was a problem hiding this comment.
This needs to be addressed.
| @asyncio.coroutine | ||
| def async_turn_off(self, **kwargs) -> None: | ||
| """Turn off the entity.""" | ||
| self.async_set_speed(SPEED_OFF) |
There was a problem hiding this comment.
yield from self.async_set_speed(...)
There was a problem hiding this comment.
Somewhat related question. async_setup_platform passes in an add device handler (in this code it is called async_add_devices. Should that be yield from?
There was a problem hiding this comment.
No, async_add_devices is not a coroutine.
| device = plm.devices[address] | ||
| state_key = discovery_info['state_key'] | ||
|
|
||
| _LOGGER.debug('Adding device %s entity %s to Sensor platform.', |
There was a problem hiding this comment.
I applied this to all the platforms :)
|
|
||
| class InsteonPLMSwitchDevice(SwitchDevice): | ||
| """A Class for an Insteon device.""" | ||
| _LOGGER.debug('Adding device %s entity %s to Switch platform.', |
| plm.protocol.devices.add_override( | ||
| device['address'], 'capabilities', [device['platform']]) | ||
| address = device_override.get('address') | ||
| if address is not None: |
There was a problem hiding this comment.
After adding the override config validation schema, this check can be removed.
| address = device_override.get('address') | ||
| if address is not None: | ||
| if device_override.get('cat', False): | ||
| plm.devices.add_override(address, |
There was a problem hiding this comment.
Maybe do this with a for loop, iterating over the four optional keys of the override. If there is a value for the key in the override, add the override.
| device_override['subcat']) | ||
| if device_override.get('firmware', False): | ||
| plm.devices.add_override(address, | ||
| 'product_key', |
There was a problem hiding this comment.
Is it correct to use 'product_key' here? That doesn't match the 'firmware' key.
There was a problem hiding this comment.
It is. They are used interchangeably in the underlying module unfortunately. It is one of the things that has to be changed there.
| CONF_PRODUCT_KEY = 'product_key' | ||
| CONF_PLATFORM = "platform" | ||
|
|
||
| CONF_DEVICE_OVERRIDE_SCHEMA = vol.All( |
| CONF_SUBCAT = 'subcat' | ||
| CONF_FIRMWARE = 'firmware' | ||
| CONF_PRODUCT_KEY = 'product_key' | ||
| CONF_PLATFORM = "platform" |
There was a problem hiding this comment.
This can be imported from const.py.
| device['address'], 'capabilities', [device['platform']]) | ||
| address = device_override.get('address') | ||
| for prop in device_override: | ||
| if prop in [CONF_ADDRESS, CONF_CAT, CONF_SUBCAT]: |
There was a problem hiding this comment.
Can you override the address too? This wasn't in the previous version, right?
|
@MartinHjelmare Thank you very much for all your help. Do I assume I need to squash commits again but what is the deal with the Travis CI build? The build fails in 3.6 |
|
Yeah, I think you need to squash again to let the CLA bot finish. The failed test is probably a flaky test. Don't worry about it. Hopefully it'll pass on the next build. |
|
@MartinHjelmare Sqash done so CLA working now. 3.6 is passing now. CI Test failing now in 3.5 test set 3 |
|
@MartinHjelmare Looks like all tests have passed! THANK YOU!. I really appreciate all the effort you put in to this. This is a much better code base to work from going forward because of your efforts. It is people like you that will make this HA platform successful. Thanks again. |
|
Appreciate the feedback, thanks! Great work on the PR! 🎉 |
Description:
Breaking change If you have created platform overrides in your configuration.yaml file to change a your INSTEON device to map to a different Home Assistant platform, that mapping will no longer be in effect. Please see the new device override capabilities in the insteon_plm documentation.
Significant upgrade to allow more complex devices to be created and managed. Significant improvement to message handling, especially the fixing of a race issue that causes the system to hang
Wide support for switch and dimmer devices (cat 0x01 and cat 0x02)
Additional complex devices added:
Related issue (if applicable):
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4695
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.If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass