Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade insteonplm to 0.8.2 (required refactoring) #12534

Merged
merged 89 commits into from Feb 25, 2018

Conversation

Projects
None yet
5 participants
@teharris1
Copy link
Contributor

commented Feb 20, 2018

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:

  • FanLinc
  • I/O Linc (was there but now a bit cleaner)
  • On/Off outlet both top and bottom outlets controllable
  • Smokebridge (unconfirmed)

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):

# insteon_plm supported configuration variables
insteon_plm:
  port: SERIAL_PORT
  device_override:
     - address: INSTEON_ADDRESS
       cat: DEVICE_CATEGORY
       subcat: DEVICE_SUBCATEGORY
       firmware: DEVICE_FIRMWARE
       product_key: DEVICE_PRODUCT_KEY

Checklist:

  • The code change is tested and works locally.

If user exposed functionality or configuration variables are added/changed:

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.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

teharris1 added some commits Jan 21, 2018

@teharris1 teharris1 changed the title Upgrade insteonplm to 0.8.2 (required refactoring) WIP Upgrade insteonplm to 0.8.2 (required refactoring) Feb 23, 2018

@teharris1 teharris1 changed the title WIP Upgrade insteonplm to 0.8.2 (required refactoring) Upgrade insteonplm to 0.8.2 (required refactoring) Feb 24, 2018

@MartinHjelmare
Copy link
Member

left a comment

I had another look and noticed some things that should be changed. Sorry that I missed them before.

return self._address
def __init__(self, device, state_key):
"""Initialize the INSTEON PLM binary sensor."""
InsteonPLMEntity.__init__(self, device, state_key)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 24, 2018

Member
super().__init__(device, state_key)

async_add_devices(device_list)
_LOGGER.debug('Adding device %s entity %s to Binary Sensor platform.',

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 24, 2018

Member

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.',

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 24, 2018

Member

Remove period at the end.

homeassistant/components/fan/insteon_plm.py Outdated
"""Turn on the entity."""
if speed is None:
speed = SPEED_MEDIUM
self.async_set_speed(speed)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 24, 2018

Member

This needs to be addressed.

@asyncio.coroutine
def async_turn_off(self, **kwargs) -> None:
"""Turn off the entity."""
self.async_set_speed(SPEED_OFF)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 24, 2018

Member

yield from self.async_set_speed(...)

This comment has been minimized.

Copy link
@teharris1

teharris1 Feb 24, 2018

Author Contributor

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?

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 24, 2018

Member

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.',

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 24, 2018

Member

Same as above.

This comment has been minimized.

Copy link
@teharris1

teharris1 Feb 25, 2018

Author Contributor

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.',

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 24, 2018

Member

Same as above.

plm.protocol.devices.add_override(
device['address'], 'capabilities', [device['platform']])
address = device_override.get('address')
if address is not None:

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 24, 2018

Member

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,

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 24, 2018

Member

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',

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 24, 2018

Member

Is it correct to use 'product_key' here? That doesn't match the 'firmware' key.

This comment has been minimized.

Copy link
@teharris1

teharris1 Feb 24, 2018

Author Contributor

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(

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 25, 2018

multiple spaces after operator

CONF_SUBCAT = 'subcat'
CONF_FIRMWARE = 'firmware'
CONF_PRODUCT_KEY = 'product_key'
CONF_PLATFORM = "platform"

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 25, 2018

Member

This can be imported from const.py.

homeassistant/components/insteon_plm.py Outdated
device['address'], 'capabilities', [device['platform']])
address = device_override.get('address')
for prop in device_override:
if prop in [CONF_ADDRESS, CONF_CAT, CONF_SUBCAT]:

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 25, 2018

Member

Can you override the address too? This wasn't in the previous version, right?

@MartinHjelmare
Copy link
Member

left a comment

Looks good!

@teharris1

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2018

@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 TestHomeKit.test_homekit_pyhap_interaction. I cannot figure out why my build would fail on this test. Any ideas?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Feb 25, 2018

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.

@teharris1

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2018

@MartinHjelmare Sqash done so CLA working now. 3.6 is passing now. CI Test failing now in 3.5 test set 3 testpilight. I assume this has nothing to do with my code so hoping you can approve it to go forward.

@teharris1

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2018

@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.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Feb 25, 2018

Appreciate the feedback, thanks! Great work on the PR! 🎉

@MartinHjelmare MartinHjelmare merged commit 32166fd into home-assistant:dev Feb 25, 2018

5 checks passed

WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 94.072%
Details
hound No violations found. Woof!

hmn added a commit to hmn/home-assistant-dev that referenced this pull request Feb 27, 2018

Merge branch 'dev' into ps4
* dev: (148 commits)
  Fix a problem with calling `deconz.close` (home-assistant#12657)
  Frontend bump to 20180227.0
  Update core HSV color scaling to standard scales: (home-assistant#12649)
  Homekit schema gracefully fail with integer (home-assistant#12725)
  AsusWRT log exceptions (home-assistant#12668)
  Fix homekit: temperature calculation (home-assistant#12720)
  Unbreak tahoma (home-assistant#12719)
  Next generation of Xiaomi Aqara devices added (home-assistant#12659)
  Fix mysensor defaults (home-assistant#12687)
  Harmony: make activity optional (home-assistant#12679)
  Add history_graph component to demo (home-assistant#12681)
  Adds simulated sensor (home-assistant#12539)
  Added config validator for future group platforms (home-assistant#12592)
  KNX Component: Scene support and expose sensor values (home-assistant#11978)
  Roomba timeout (home-assistant#12645)
  Fix formatting of minutes for sleep start in the fitbit sensor (home-assistant#12664)
  Homekit Update, Support for TempSensor (°F) (home-assistant#12676)
  Remove braviatv_psk (home-assistant#12669)
  Upgrade insteonplm to 0.8.2 (required refactoring) (home-assistant#12534)
  Enable pytradfri during build, and include in Docker (home-assistant#12662)
  ...

@balloob balloob referenced this pull request Mar 9, 2018

Merged

0.65 #12995

@balloob balloob removed the new-platform label Mar 9, 2018

@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.