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

Merge insteon_plm and insteon_local to insteon component #16102

Merged
merged 51 commits into from Aug 22, 2018

Conversation

Projects
None yet
5 participants
@teharris1
Contributor

teharris1 commented Aug 21, 2018

Description:

The insteon_plm and insteon_local components both provide Insteon services for the two different interfaces (PLM and Hub respectively). This PR merges the two so that only one component will serve both Insteon PLM and Insteon Hub users. The summary of changes is:

  • Rename insteon_plm component to insteon component (including all platforms)
  • Add Hub connectivity to insteon component (includes an upgrade to the underlying module insteonplm)
  • Add log messages for insteon_plm and insteon_local users to change to the new insteon component
  • Remove insteon_local from all platforms
  • Remove insteon_hub from all platforms since this old component was deactivated in release 0.32

Related issue (if applicable): fixes #13178

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

Example entry for configuration.yaml (if applicable):

# PLM configuration variables
insteon:
  port: SERIAL_PORT

or

# Hub configuration variables
insteon:
  host: HOST
  ip_port: IP_PORT
  username: USERNAME
  password: PASSWORD

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated 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:

  • Tests have been added to verify that the new code works.

teharris1 added some commits May 19, 2018

@balloob

This comment has been minimized.

Show comment
Hide comment
@balloob

balloob Aug 21, 2018

Member

This looks great! Up to you if you want to keep with the warning or want to make it a persistent notification. We probably also want a comment that we should remove the old platforms in a couple of releases.

Member

balloob commented Aug 21, 2018

This looks great! Up to you if you want to keep with the warning or want to make it a persistent notification. We probably also want a comment that we should remove the old platforms in a couple of releases.

@teharris1

This comment has been minimized.

Show comment
Hide comment
@teharris1

teharris1 Aug 21, 2018

Contributor

I added the note to remove in release 0.90. I figured this should be plenty of time for people who don't upgrade regularly. I think you are on a 2 week release cycle so this would be 13 releases or 26 weeks.

Contributor

teharris1 commented Aug 21, 2018

I added the note to remove in release 0.90. I figured this should be plenty of time for people who don't upgrade regularly. I think you are on a 2 week release cycle so this would be 13 releases or 26 weeks.

@balloob balloob merged commit a31501d into home-assistant:dev Aug 22, 2018

5 checks passed

Hound No violations found. Woof!
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 increased (+0.007%) to 93.803%
Details

@wafflebot wafflebot bot removed the in progress label Aug 22, 2018

@balloob

This comment has been minimized.

Show comment
Hide comment
@balloob

balloob Aug 22, 2018

Member

Perfect, awesome work.

Member

balloob commented Aug 22, 2018

Perfect, awesome work.

@Madelinot

This comment has been minimized.

Show comment
Hide comment
@Madelinot

Madelinot Aug 26, 2018

I have a question. I have BOTH a PLM and a Hub connected in my setup and they work just fine. Like this:

insteon_plm:
  port: /dev/ttyUSB0

insteon_local:
  host: 192.168.x.x
  username: !secret insteon_username
  password: !secret insteon_password
  timeout: 60
  port: xxxxx

Will I have to choose which one I want to use or can I configure both at the same time using the new configuration?

Madelinot commented Aug 26, 2018

I have a question. I have BOTH a PLM and a Hub connected in my setup and they work just fine. Like this:

insteon_plm:
  port: /dev/ttyUSB0

insteon_local:
  host: 192.168.x.x
  username: !secret insteon_username
  password: !secret insteon_password
  timeout: 60
  port: xxxxx

Will I have to choose which one I want to use or can I configure both at the same time using the new configuration?

@teharris1

This comment has been minimized.

Show comment
Hide comment
@teharris1

teharris1 Aug 26, 2018

Contributor

Not a use case I had ever considered so, yes. Just out of curiosity, why do you do this?

Contributor

teharris1 commented Aug 26, 2018

Not a use case I had ever considered so, yes. Just out of curiosity, why do you do this?

@Madelinot

This comment has been minimized.

Show comment
Hide comment
@Madelinot

Madelinot Aug 26, 2018

Well, the main reason is that HA did not support leak sensors connected to the Hub. Maybe that's changed now.

The way I have it set up now is that all my light switches at connected to the Hub and the leak sensors are connected to the PLM. They both work fine.

Madelinot commented Aug 26, 2018

Well, the main reason is that HA did not support leak sensors connected to the Hub. Maybe that's changed now.

The way I have it set up now is that all my light switches at connected to the Hub and the leak sensors are connected to the PLM. They both work fine.

@teharris1

This comment has been minimized.

Show comment
Hide comment
@teharris1

teharris1 Aug 27, 2018

Contributor

Yes, all of the devices work with the Hub in the new design. But we should not comment in the PR. We can move the disucssion here: https://community.home-assistant.io/t/merging-insteon-plm-and-insteonlocal/60323/9

Contributor

teharris1 commented Aug 27, 2018

Yes, all of the devices work with the Hub in the new design. But we should not comment in the PR. We can move the disucssion here: https://community.home-assistant.io/t/merging-insteon-plm-and-insteonlocal/60323/9

@teharris1 teharris1 deleted the teharris1:hub branch Aug 27, 2018

@balloob balloob referenced this pull request Aug 29, 2018

Merged

0.77 #16256

cyberjacob pushed a commit to cyberjacob/home-assistant that referenced this pull request Sep 4, 2018

Merge insteon_plm and insteon_local to insteon component (#16102)
* Implement X10

* Add X10 after add_device_callback

* Ref device by id not hex and add x10OnOffSwitch name

* X10 services and add sensor device

* Correctly reference X10_HOUSECODE_SCHEMA

* Log adding of X10 devices

* Add X10 All Units Off, All Lights On and All Lights Off devices

* Correct ref to X10 states vs devices

* Add X10 All Units Off, All Lights On and All Lights Off devices

* Correct X10 config

* Debug x10 device additions

* Config x10 from bool to housecode char

* Pass PLM to X10 device create

* Remove PLM to call to add_x10_device

* Unconfuse x10 config and method names

* Correct spelling of x10_all_lights_off_housecode

* Bump insteonplm to 0.10.0 to support X10

* Add host to config options

* Add username and password to config for hub connectivity

* Add username and password to config for hub

* Convert port to int if host is defined

* Add KeypadLinc

* Update config schema to require either port or host

* Solidify Hub and PLM configuration to ensure proper settings

* Update hub schema

* Bump insteonplm version

* Fix pylint and flake issues

* Bump insteonplm to 0.12.1

* Merge insteon_plm and insteon_local to insteon

* Rename insteon_plm to insteon

* Bump insteonplm to 0.12.2

* Flake8 cleanup

* Update .coveragerc for insteon_plm, insteon_local and insteon changes

* Add persistent notification

* Fix reference to insteon_plm

* Fix indentation

* Shorten message and fix grammer

* Add comment to remove in release 0.90

* Hound fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment