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

Add SmartThings component and switch platform #20148

Merged
merged 13 commits into from Jan 31, 2019

Conversation

@andrewsayre
Copy link
Contributor

andrewsayre commented Jan 16, 2019

Description:

Adds new component SmartThings with a switch platform. This is a cloud-push integration with the new SmartThings cloud API. Highlights:

  • Configured exclusively through config entries (and supports unloading)
  • Dependencies (pysmartthings and pysmartapp) are fully async
  • Supports any number of SmartThings accounts/locations controlled from any number of HASS instances
  • Requires no broker, bridge, or other dependencies
  • Entities automatically add, update, or remove when changed in SmartThings and HASS is reloaded

Integrating into the SmartThings ecosystem is fairly complex, thus the size of this PR. I have slimmed it down as much has possible to still be functional and there's over 99% test coverage. Additional component enhancements and platforms will follow-on.

Need help with translations.

Looking forward to your feedback!

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

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@balloob
Copy link
Member

balloob left a comment

This looks like it's off to a great start. The biggest change is that we should use a webhook to receive the data.

Show resolved Hide resolved homeassistant/components/smartthings/.translations/en.json Outdated
Show resolved Hide resolved homeassistant/components/smartthings/smartapp.py Outdated
Show resolved Hide resolved homeassistant/components/switch/smartthings.py Outdated
Show resolved Hide resolved homeassistant/components/switch/smartthings.py Outdated
@andrewsayre

This comment has been minimized.

Copy link
Contributor Author

andrewsayre commented Jan 18, 2019

@balloob I think this is ready when you get a chance to look, unless we're waiting for others to review. Thanks!

@andrewsayre andrewsayre force-pushed the andrewsayre:dev branch 2 times, most recently from e1020a8 to c3926dc Jan 20, 2019

@andrewsayre andrewsayre referenced this pull request Jan 21, 2019

Merged

Minor webhook and http component improvements #20295

4 of 4 tasks complete

@andrewsayre andrewsayre force-pushed the andrewsayre:dev branch from c3926dc to b34547a Jan 21, 2019

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Jan 25, 2019

General question: is it possible in the future to add encryption so that the communication between app and integration can be encrypted?

@andrewsayre

This comment has been minimized.

Copy link
Contributor Author

andrewsayre commented Jan 25, 2019

General question: is it possible in the future to add encryption so that the communication between app and integration can be encrypted?

SmartThings will only callback to a HTTPS end-point and the payload includes a digitally signed signature in the header which is used to validate the authenticity of the message. So it's encryption during transport, singing at the message level. AFAIK there's no message-level encryption support at this time.

Show resolved Hide resolved tests/components/smartthings/test_init.py Outdated
Show resolved Hide resolved tests/components/smartthings/test_switch.py Outdated
Show resolved Hide resolved tests/components/smartthings/test_switch.py Outdated

@MartinHjelmare MartinHjelmare changed the title Added SmartThings component and switch platform Add SmartThings component and switch platform Jan 26, 2019

@andrewsayre

This comment has been minimized.

Copy link
Contributor Author

andrewsayre commented Jan 26, 2019

I created the following conceptual model to depict how the SmartThings ecosystem is represented in HASS:

hass-st-conceptual

Note: SmartThings Apps can have the same webhook_url which allows us to only need one webhook_id for the entire component for simplicity.

@andrewsayre

This comment has been minimized.

Copy link
Contributor Author

andrewsayre commented Jan 31, 2019

@balloob @MartinHjelmare @rohankapoorcom All comments have been addressed... What's next to move this forward? Ping me on Discord if you have concerns/questions. I can also share a test SmartThings account you can use to try out the integration.

@balloob
Copy link
Member

balloob left a comment

🎉

@balloob balloob merged commit 69ec798 into home-assistant:dev Jan 31, 2019

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 First build on dev at 93.043%
Details

@wafflebot wafflebot bot removed the in progress label Jan 31, 2019

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Jan 31, 2019

This looks great!

@ryanwinter

This comment has been minimized.

Copy link

ryanwinter commented Jan 31, 2019

Awesome, can't wait to see additional components added.

@balloob balloob referenced this pull request Feb 6, 2019

Merged

0.87.0 #20794

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