Support for multiple Fibaro gateways by pbalogh77 · Pull Request #19705 · home-assistant/core

pbalogh77 · 2019-01-01T22:36:45Z

Description:

Fibaro component now supports multiple Fibaro HC gateways.

Added multiple gateway support.
Reworked parameter flow to platforms to enable multiple controllers.

Breaking change:
A list of gateways is expected instead of a single config.

Related issue (if applicable): fixes #

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

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

fibaro:
    gateways:
      - url: http://your.fibaro.url/api
        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
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 PR 8140

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

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.

Added multiple gateway support Reworked parameter flow to platforms to enable multiple controllers Breaking change to config, now a list of gateways is expected instead of a single config

Added new location of fibaro component

dgomes

Looks good!

balloob · 2019-01-05T16:42:00Z

ok to merge when comments addressed

Addressed issues raised by code review Added extended debug logging to get better reports from users if the device type mapping is not perfect

MartinHjelmare · 2019-01-08T05:41:22Z

-        hass.data[FIBARO_DEVICES] = controller.fibaro_devices
+        for controller in hass.data[FIBARO_CONTROLLERS].values():
+            controller.disable_state_handler()
+        hass.data[FIBARO_CONTROLLERS] = {}


If we don't need this key anymore we should pop it or delete it from the dict.

Wouldn't that be a less efficient algorithm, tho?
I know it's highly theoretical either way, since it's usually a single element list and it's probably equally readable either way, but this seems like a more appropriate solution on an embedded device. I could be wrong, I'm new here. Of course if you insist, I'll modify it.

I don't think we should worry about speed here, but clean up remaining references.

For now though we don't really need to clean up anything, since this is only done when home assistant shuts down. So we can remove this line completely. Clean up will be relevant if moving this component to use config entries.

Ok, I'll keep in mind.

Please remove this line.

Ok, I removed it. Just for my education, what was wrong with that line? I'm new to python but as far as I understand it did remove the reference to all items in the map and by that freeing up the memory. I understand your suggestion that you'd prefer enumerate + pop, but coming from decades of embedded c programming and knowing how enumerate and pop works, that's really a very wasteful way of solving the problem. Afaik, enumerate iterates through the list an extra time, creates a new index, and pop unlinks items one-by-one from the list. I could be totally wrong on my assumptions, but in general this construct feels like something to discourage on an embedded platform. I could be totally not pythonic in thinking this.
Anyhow, I'd love to learn. Thanks in advance.

Changes to how configuration is read and schemas Fix to device type mapping logic

oops

grr

MartinHjelmare · 2019-01-09T10:25:07Z

-        hass.data[FIBARO_DEVICES] = controller.fibaro_devices
+        for controller in hass.data[FIBARO_CONTROLLERS].values():
+            controller.disable_state_handler()
+        hass.data[FIBARO_CONTROLLERS] = {}


For now though we don't really need to clean up anything, since this is only done when home assistant shuts down. So we can remove this line completely. Clean up will be relevant if moving this component to use config entries.

changes based on code review

MartinHjelmare

Good!

MartinHjelmare · 2019-01-10T11:38:03Z

Can be merged when docs PR is linked in the PR description.

Documentation change based on home-assistant/core#19705

pbalogh77 · 2019-01-11T21:17:12Z

Updated documentation, added link to PR

* Updated based on changes Documentation change based on home-assistant/core#19705 * Update fibaro.markdown

pbalogh77 added 2 commits January 1, 2019 23:31

Preparing for transition to config flow

4868a77

Added multiple gateway support Reworked parameter flow to platforms to enable multiple controllers Breaking change to config, now a list of gateways is expected instead of a single config

Updated coveragerc

4dc95eb

Added new location of fibaro component

homeassistant added integration: fibaro platform: binary_sensor.fibaro cla-signed labels Jan 1, 2019

ghost added the in progress label Jan 1, 2019

pbalogh77 changed the title ~~WIP: Support for multiple Fibaro gateways~~ Support for multiple Fibaro gateways Jan 2, 2019

dgomes approved these changes Jan 5, 2019

View reviewed changes

dgomes added Ready for review and removed in progress labels Jan 5, 2019

balloob reviewed Jan 5, 2019

View reviewed changes

Comment thread homeassistant/components/fibaro/__init__.py Outdated

balloob reviewed Jan 5, 2019

View reviewed changes

Comment thread homeassistant/components/fibaro/__init__.py Outdated

balloob approved these changes Jan 5, 2019

View reviewed changes

Fixes based on code review and extended logging

ca565fc

Addressed issues raised by code review Added extended debug logging to get better reports from users if the device type mapping is not perfect

MartinHjelmare requested changes Jan 8, 2019

View reviewed changes

pbalogh77 added 3 commits January 8, 2019 15:55

Changhes based on code review

d07cff5

Changes to how configuration is read and schemas Fix to device type mapping logic

simplified reading config

8e4e157

oops

48cff00

oops

houndci-bot reviewed Jan 8, 2019

View reviewed changes

Comment thread homeassistant/components/fibaro/__init__.py Outdated

grr

43adef1

grr

MartinHjelmare reviewed Jan 9, 2019

View reviewed changes

pbalogh77 added 2 commits January 9, 2019 22:58

change based on code review

4172234

changes based on code review

d541d17

changes based on code review

MartinHjelmare approved these changes Jan 10, 2019

View reviewed changes

MartinHjelmare added breaking-change docs-missing and removed Ready for review labels Jan 10, 2019

pbalogh77 added a commit to pbalogh77/home-assistant.io that referenced this pull request Jan 11, 2019

Updated based on changes

5136274

Documentation change based on home-assistant/core#19705

pbalogh77 mentioned this pull request Jan 11, 2019

Updated based on changes home-assistant/home-assistant.io#8140

Merged

2 tasks

balloob merged commit 7dac7b9 into home-assistant:dev Jan 11, 2019

balloob pushed a commit to home-assistant/home-assistant.io that referenced this pull request Jan 11, 2019

Updated based on changes (#8140)

354cd73

* Updated based on changes Documentation change based on home-assistant/core#19705 * Update fibaro.markdown

pbalogh77 deleted the async_setup branch January 12, 2019 18:04

pbalogh77 mentioned this pull request Jan 21, 2019

Fibaro platform: climate device issue #19359

Closed

balloob mentioned this pull request Jan 23, 2019

0.86.0 #20354

Merged

ghost removed platform: binary_sensor.fibaro labels Mar 21, 2019

Uh oh!

Conversation

pbalogh77 commented Jan 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

dgomes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

balloob commented Jan 5, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare commented Jan 10, 2019

Uh oh!

pbalogh77 commented Jan 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pbalogh77 commented Jan 1, 2019 •

edited

Loading

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