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

Support for multiple Fibaro gateways #19705

Merged
merged 9 commits into from Jan 11, 2019

Conversation

@pbalogh77
Copy link
Contributor

pbalogh77 commented Jan 1, 2019

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:

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.

pbalogh77 added some commits Jan 1, 2019

Preparing for transition to config flow
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
Added new location of fibaro component
@dgomes

dgomes approved these changes Jan 5, 2019

Copy link
Member

dgomes left a comment

Looks good!

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

@balloob

balloob approved these changes Jan 5, 2019

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Jan 5, 2019

ok to merge when comments addressed

Fixes based on code review and extended logging
Addressed issues raised by code review
Added extended debug logging to get better reports from users if the device type mapping is not perfect
Show resolved Hide resolved homeassistant/components/fibaro/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/fibaro/__init__.py Outdated
hass.data[FIBARO_DEVICES] = controller.fibaro_devices
for controller in hass.data[FIBARO_CONTROLLERS].values():
controller.disable_state_handler()
hass.data[FIBARO_CONTROLLERS] = {}

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jan 8, 2019

Member

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

This comment has been minimized.

@pbalogh77

pbalogh77 Jan 8, 2019

Author Contributor

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.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jan 8, 2019

Member

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

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jan 9, 2019

Member

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.

This comment has been minimized.

@pbalogh77

pbalogh77 Jan 9, 2019

Author Contributor

Ok, I'll keep in mind.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jan 10, 2019

Member

Please remove this line.

This comment has been minimized.

@pbalogh77

pbalogh77 Jan 10, 2019

Author Contributor

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.

Show resolved Hide resolved homeassistant/components/fibaro/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/fibaro/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/fibaro/__init__.py Outdated

pbalogh77 added some commits Jan 8, 2019

Changhes based on code review
Changes to how configuration is read and schemas
Fix to device type mapping logic
oops
oops
grr
grr
Show resolved Hide resolved homeassistant/components/fibaro/__init__.py Outdated
hass.data[FIBARO_DEVICES] = controller.fibaro_devices
for controller in hass.data[FIBARO_CONTROLLERS].values():
controller.disable_state_handler()
hass.data[FIBARO_CONTROLLERS] = {}

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jan 9, 2019

Member

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.

pbalogh77 added some commits Jan 9, 2019

changes based on code review
changes based on code review
@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Good!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 10, 2019

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

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

@pbalogh77 pbalogh77 referenced this pull request Jan 11, 2019

Merged

Updated based on changes #8140

2 of 2 tasks complete
@pbalogh77

This comment has been minimized.

Copy link
Contributor Author

pbalogh77 commented Jan 11, 2019

Updated documentation, added link to PR

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

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.07%) to 93.033%
Details

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

Updated based on changes (#8140)
* Updated based on changes

Documentation change based on home-assistant/home-assistant#19705

* Update fibaro.markdown

@pbalogh77 pbalogh77 deleted the pbalogh77:async_setup branch Jan 12, 2019

mxworm added a commit to mxworm/home-assistant that referenced this pull request Jan 12, 2019

Merge branch 'dev' into current
* dev: (150 commits)
  Update coveragerc file
  Upgrade greeneye_monitor to 1.0 (home-assistant#19631)
  Update doorbird events to include URLs on event_data (home-assistant#19262)
  Support for html5 notifications to suggest their names (home-assistant#19965)
  catch TypeError's in addition to ValueError's for unifi direct device tracker (home-assistant#19994)
  'latest_dir' referenced before assignment (home-assistant#19952)
  Repackage ZHA component (home-assistant#19989)
  Add support for HomeKit Controller Locks (home-assistant#19867)
  Don't set friendly_name in Zha entity. (home-assistant#19991)
  Lint
  Wink: Update pubnubsub-handler version to make it compatible with python 3.7 (home-assistant#19625)
  Upgrade pytest-cov to 2.6.1 (home-assistant#19988)
  Upgrade huawei-lte-api to 1.1.3 (home-assistant#19987)
  Support for multiple Fibaro gateways (home-assistant#19705)
  Split locative to a separate component (home-assistant#19964)
  Bumped version to 0.86.0.dev0
  Fix fail2ban tests
  Expose more information about shipments by PostNL (home-assistant#18334)
  Fix the anthemav component by removing a debugging line. (home-assistant#19979)
  Add support for 'via_hub' for device_info (home-assistant#19454)
  ...

# Conflicts:
#	homeassistant/components/homematicip_cloud/__init__.py
#	requirements_all.txt
#	requirements_test_all.txt

@balloob balloob referenced this pull request Jan 23, 2019

Merged

0.86.0 #20354

alandtse added a commit to alandtse/home-assistant that referenced this pull request Feb 12, 2019

Support for multiple Fibaro gateways (home-assistant#19705)
* Preparing for transition to config flow

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

Added new location of fibaro component

* Fixes based on code review and extended logging

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

* Changhes based on code review

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

* simplified reading config

* oops

oops

* grr

grr

* change based on code review

* changes based on code review

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