Proposal for Niko Home Control II #23105

filipvh · 2019-04-14T19:50:49Z

Breaking Change:

Nothing should break, but it's my first time writing something in python, so a good review would be gold. I am aware that not yet everything is in order, and i'll try to do what needs to be done.
Getting this all to work has been very tiresome as I've never written Python before.

Description:

Basic integration of Niko Home Control II (lights/switches)

Example entry for `configuration.yaml` (if applicable):

nhc2:
  host: '192.168.0.2'
  port: 8883
  username: 'abcdefgh-ijkl-mnop-qrst-uvwxyz012345'
  password: !secret nhc2_password

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:

Documentation added/updated in home-assistant.io
Not yet new to this, will have to see how.

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

The manifest file has all fields filled out correctly (example).
New dependencies have been added to requirements in the manifest (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.

homeassistant · 2019-04-14T19:50:50Z

Hi @filipvh,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

awarecan · 2019-04-14T20:33:57Z

Thank you for your contribution. After a quick look, there are serval action items

Please do not add language json file other then en. Add your string resources to string.json then execute script/translate_develop.py. Other translation should be done in lokalise.co back end translation project. See https://developers.home-assistant.io/docs/en/internationalization_backend_localization.html
Please trim this PR to minimum viable product, i.e., limit to only one platform, you can choose the most simple one. That will help your pull request get quick review feedback.

awarecan · 2019-04-14T22:37:02Z

What different this and the exists niko_home_control?

filipvh · 2019-04-15T06:22:27Z

Hi @awarecan thanks for your comments/questions

What different this and the exists niko_home_control?

This is v2, that one is v1. While they have some compatibility when it comes to hardware, on the software/network side, they are different. So is the communication.
(see Can I upgrade an existing Niko Home Control installation?)

Please trim this PR to minimum viable product, i.e., limit to only one platform, you can choose the most simple one. That will help your pull request get quick review feedback.

As it stands now, light and switch are pretty much identical. If this doesn't matter, I'll remove one...

Please do not add language json file other then en. Add your string resources to string.json then execute script/translate_develop.py ...

I'll try to look at that asap.

balloob · 2019-04-15T17:57:42Z

We should not add a new integration for a new version. Please merge this with the other integration . Once you establish a connection you can see which API is in use and pick the appropriate lib and files to use

filipvh · 2019-04-15T18:20:05Z

We should not add a new integration for a new version. Please merge this with the other integration . Once you establish a connection you can see which API is in use and pick the appropriate lib and files to use

@balloob
The connection itself is also a different protocol. The Home Control II runs it own MQTT Broker, while the previous version has nothing of the sort.
The discovery is different, the protocol is different, the APIs are different. It's like angularjs and angular, but the name, they are nothing the same.

I understand that this would be preferred, but I don't know enough about HA how and if we can/want to do this. The config would probably be different, as now it's basic, but the steps that would be added to a more complete integration, would require different flows for different systems.

RvanBaarle · 2019-04-16T06:20:56Z

homeassistant/components/nhc2/light.py

+async def async_setup_entry(hass, config_entry, async_add_entities):
+    """Load NHC2 lights based on a config entry."""
+    hass.data.setdefault(KEY_ENTITY, {})[config_entry.entry_id] = []
+    gateway: CoCo = hass.data[KEY_GATEWAY][config_entry.entry_id]


I don't think this is valid syntax. If it is however, what is it supposed to do?

This is Python 3.6 typing syntax, we support a minimum of python 3.5.3

Will look at those this weekend, thx!

RvanBaarle · 2019-04-16T06:21:37Z

homeassistant/components/nhc2/switch.py

+async def async_setup_entry(hass, config_entry, async_add_entities):
+    """Load NHC2 switches based on a config entry."""
+    hass.data.setdefault(KEY_ENTITY, {})[config_entry.entry_id] = []
+    gateway: CoCo = hass.data[KEY_GATEWAY][config_entry.entry_id]


Same thing here.

RvanBaarle · 2019-04-16T06:23:08Z

homeassistant/components/upnp/.translations/en.json

@@ -1,28 +1,13 @@
 {
    "config": {
        "abort": {
-            "already_configured": "UPnP/IGD is already configured",


Why are parts of this file, and some others, changed? It seems unrelated to your other changes.

MartinHjelmare · 2019-07-06T10:33:02Z

This PR has gone stale. I'll close it now. Please open a new PR when ready to finish.

Proposal for Niko Home Control II

0a11e18

filipvh requested a review from a team as a code owner April 14, 2019 19:50

homeassistant added cla-needed core integration: nhc2 new-integration new-platform labels Apr 14, 2019

ghost added the in progress label Apr 14, 2019

homeassistant added cla-signed and removed cla-needed labels Apr 14, 2019

Ue nhc2-coco v 0.0.2

0de6485

Fix some formatting issues

2d8c9e1

Fixed some formatting issues and removed i18n

e8a51f9

Fixed some formatting issues and added missing comments

aad6ef0

RvanBaarle suggested changes Apr 16, 2019

View reviewed changes

MartinHjelmare closed this Jul 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal for Niko Home Control II #23105

Proposal for Niko Home Control II #23105

filipvh commented Apr 14, 2019 •

edited

Loading

homeassistant commented Apr 14, 2019

awarecan commented Apr 14, 2019

awarecan commented Apr 14, 2019

filipvh commented Apr 15, 2019

balloob commented Apr 15, 2019

filipvh commented Apr 15, 2019 •

edited

Loading

RvanBaarle Apr 16, 2019

balloob Apr 16, 2019

filipvh Apr 17, 2019 •

edited

Loading

RvanBaarle Apr 16, 2019

RvanBaarle Apr 16, 2019

MartinHjelmare commented Jul 6, 2019

Proposal for Niko Home Control II #23105

Proposal for Niko Home Control II #23105

Conversation

filipvh commented Apr 14, 2019 • edited Loading

Breaking Change:

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

homeassistant commented Apr 14, 2019

awarecan commented Apr 14, 2019

awarecan commented Apr 14, 2019

filipvh commented Apr 15, 2019

balloob commented Apr 15, 2019

filipvh commented Apr 15, 2019 • edited Loading

RvanBaarle Apr 16, 2019

Choose a reason for hiding this comment

balloob Apr 16, 2019

Choose a reason for hiding this comment

filipvh Apr 17, 2019 • edited Loading

Choose a reason for hiding this comment

RvanBaarle Apr 16, 2019

Choose a reason for hiding this comment

RvanBaarle Apr 16, 2019

Choose a reason for hiding this comment

MartinHjelmare commented Jul 6, 2019

filipvh commented Apr 14, 2019 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

filipvh commented Apr 15, 2019 •

edited

Loading

filipvh Apr 17, 2019 •

edited

Loading