Skip to content
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

Multiple devices support for opentherm_gw #22932

Merged
merged 9 commits into from Jun 21, 2019

Conversation

@mvn23
Copy link
Contributor

commented Apr 9, 2019

Breaking Change:

  • Breaks configuration structure: we need to specify and identify more than one gateway.
  • Breaks configuration structure: monitored_variables option has been removed, all sensors are added by default.
  • Breaks services: add a required parameter to all services to identify the gateway upon which the service will be called.
  • Breaks entity_ids: new format to keep entity_ids unique for each configured gateway.

Description:

Add support for more than one OpenTherm Gateway device.

Related issue (if applicable): N/A

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

Example entry for configuration.yaml (if applicable):

opentherm_gw:
  living_room:
    device: /dev/ttyUSB0

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:

  • 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:
N/A

@ghost ghost added the in progress label Apr 9, 2019

@mvn23 mvn23 changed the title Multiple device support for opentherm_gw Multiple devices support for opentherm_gw Apr 9, 2019

@mvn23 mvn23 force-pushed the mvn23:otgw_multi_support branch from fc59a1a to 93d65ca May 11, 2019

@codecov

This comment was marked as outdated.

Copy link

commented May 11, 2019

Codecov Report

Merging #22932 into dev will decrease coverage by 0.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #22932      +/-   ##
==========================================
- Coverage   93.95%   93.85%   -0.11%     
==========================================
  Files         468      449      -19     
  Lines       37971    36683    -1288     
==========================================
- Hits        35677    34429    -1248     
+ Misses       2294     2254      -40
Impacted Files Coverage Δ
homeassistant/components/mqtt/fan.py 74.33% <0%> (-23.64%) ⬇️
homeassistant/components/mqtt/light/schema_json.py 73.06% <0%> (-20.65%) ⬇️
homeassistant/bootstrap.py 58.08% <0%> (-17.21%) ⬇️
homeassistant/components/mqtt/lock.py 91.83% <0%> (-8.17%) ⬇️
homeassistant/components/axis/device.py 93.02% <0%> (-6.98%) ⬇️
homeassistant/components/hassio/ingress.py 71.3% <0%> (-6.75%) ⬇️
homeassistant/helpers/template.py 95.54% <0%> (-4.46%) ⬇️
homeassistant/components/html5/notify.py 85.6% <0%> (-2.43%) ⬇️
homeassistant/components/deconz/config_flow.py 97.77% <0%> (-2.23%) ⬇️
homeassistant/components/zha/core/registries.py 85% <0%> (-1.96%) ⬇️
... and 217 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f80389...93d65ca. Read the comment docs.

1 similar comment
@codecov

This comment was marked as outdated.

Copy link

commented May 11, 2019

Codecov Report

Merging #22932 into dev will decrease coverage by 0.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #22932      +/-   ##
==========================================
- Coverage   93.95%   93.85%   -0.11%     
==========================================
  Files         468      449      -19     
  Lines       37971    36683    -1288     
==========================================
- Hits        35677    34429    -1248     
+ Misses       2294     2254      -40
Impacted Files Coverage Δ
homeassistant/components/mqtt/fan.py 74.33% <0%> (-23.64%) ⬇️
homeassistant/components/mqtt/light/schema_json.py 73.06% <0%> (-20.65%) ⬇️
homeassistant/bootstrap.py 58.08% <0%> (-17.21%) ⬇️
homeassistant/components/mqtt/lock.py 91.83% <0%> (-8.17%) ⬇️
homeassistant/components/axis/device.py 93.02% <0%> (-6.98%) ⬇️
homeassistant/components/hassio/ingress.py 71.3% <0%> (-6.75%) ⬇️
homeassistant/helpers/template.py 95.54% <0%> (-4.46%) ⬇️
homeassistant/components/html5/notify.py 85.6% <0%> (-2.43%) ⬇️
homeassistant/components/deconz/config_flow.py 97.77% <0%> (-2.23%) ⬇️
homeassistant/components/zha/core/registries.py 85% <0%> (-1.96%) ⬇️
... and 217 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f80389...93d65ca. Read the comment docs.

@mvn23

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

Updated to be compatible with #23810



async def register_services(hass, gateway):
async def register_services(hass):

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 19, 2019

Member

This doesn't seem to need to be a coroutine. There's no await inside. Just make it a regular function.

for gw_id, cfg in conf.items():
gateway = OpenThermGatewayDevice(hass, gw_id, cfg, config)
hass.data[DATA_OPENTHERM_GW][DATA_GATEWAYS][gw_id] = gateway
hass.async_create_task(register_services(hass))

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 19, 2019

Member

We don't need to schedule a task, it seems.

"""Register services for the component."""
gw_vars = hass.data[DATA_OPENTHERM_GW][DATA_GW_VARS]
import pyotgw.vars as gw_vars

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 19, 2019

Member

This can be moved to the top of the module.

This comment has been minimized.

Copy link
@mvn23

mvn23 Jun 20, 2019

Author Contributor

I thought dependency imports were only allowed inside functions that use them? Or does that rule expand to modules as well?

edit: applies to #22932 (comment) as well

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 20, 2019

Member

This changed after we introduced manifest.json. We can now import 3rd party modules and packages that are integration requirements at the top of the module. This can make some more complicated structures, that were required before, obsolete.


def __init__(self, hass, gw_id, config, hass_config):
"""Initialize the OpenTherm Gateway."""
import pyotgw

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 19, 2019

Member

This can be moved to the top of the module.

self.status = {}
self.update_signal = '{}_{}_update'.format(DATA_OPENTHERM_GW, gw_id)
self.gateway = pyotgw.pyotgw()
self.vars = pyotgw.vars

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 19, 2019

Member

This doesn't need to be an instance attribute.

async def setup_monitored_vars(self, monitored_vars, hass_config):
"""Set up requested sensors."""
sensor_type_map = {
COMP_BINARY_SENSOR: [

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 19, 2019

Member

These constants can be moved to the module level. Preferably break them out into a const.py module under this package.

elif var in sensor_type_map[COMP_BINARY_SENSOR]:
self.binary_sensors.append(var)
else:
_LOGGER.error("Monitored variable not supported: %s", var)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 19, 2019

Member

This should be validated by the config schema.

self.gw_id, hass_config))
monitored_vars = config[CONF_MONITORED_VARIABLES]
if monitored_vars:
hass.async_create_task(

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 19, 2019

Member

There should be no side effects in init method. Please have a dedicated setup method for these types of actions.

@@ -20,96 +20,102 @@
"""Set up the OpenTherm Gateway binary sensors."""
if discovery_info is None:
return
gw_vars = hass.data[DATA_OPENTHERM_GW][DATA_GW_VARS]
gw_dev = hass.data[DATA_OPENTHERM_GW][DATA_GATEWAYS][discovery_info]
sensor_info = {

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 19, 2019

Member

This can move to const.py under this package.

@mvn23

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

Thanks for the review, I will address the issues over the coming days.

mvn23 added 8 commits Apr 3, 2019
Breaking change: Rewrite opentherm_gw to add support for more than on…
…e OpenTherm Gateway.

Breaks config layout and child entity ids and adds a required parameter to all service calls (gateway_id).
Add optional name attribute in config to be used for friendly names.
Fix bugs in binary_sensor and climate platforms.
Address issues that were brought up (requested changes):
- Move imports to module level
- Change certain functions from async to sync
- Move constants to const.py (new file)
- Call gateway setup from outside of __init__()
- Move validation of monitored_variables to config schema

@mvn23 mvn23 force-pushed the mvn23:otgw_multi_support branch from 93d65ca to aae5ef5 Jun 20, 2019

@mvn23

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Updated and rebased, all mentioned issues should be addressed.

import voluptuous as vol

from homeassistant.components.binary_sensor import DOMAIN as COMP_BINARY_SENSOR
from homeassistant.components.climate import DOMAIN as COMP_CLIMATE
from homeassistant.components.opentherm_gw.const import (

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 20, 2019

Member

Please make all intra package imports relative. This will allow custom integrations to override builtin integrations.

from .const import (...)

This comment has been minimized.

Copy link
@mvn23

mvn23 Jun 20, 2019

Author Contributor

I used relative imports while it was a WIP, decided to change to the absolute form to make the imports more uniform. I will change it back with the next commit.

vol.Required(CONF_DEVICE): cv.string,
vol.Optional(CONF_CLIMATE, default={}): CLIMATE_SCHEMA,
vol.Optional(CONF_MONITORED_VARIABLES, default=[]): vol.All(
cv.ensure_list, [cv.string]),
vol.Optional(CONF_MONITORED_VARIABLES, default=[]): [vol.Any(

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 20, 2019

Member

There was a recent architecture decision that we should avoid user selectable monitored variables: https://github.com/home-assistant/architecture/blob/master/adr/0003-monitor-condition-and-data-selectors.md.

Can we motivate keeping this option here after reading the decision or should we remove the option?

This comment has been minimized.

Copy link
@mvn23

mvn23 Jun 20, 2019

Author Contributor

For the last few weeks/months, I haven't really kept up with HA development so I was not aware of this. All data is obtained from the device regardless of the configured monitored_variables, so the option is just there to reduce the amount of entities the integration exposes. As lovelace is now the preferred way to 'filter' this information, I will remove the option to select which data is exposed and create all entities by default.
home-assistant/home-assistant.io#9176 will be updated accordingly as well.

Address requested changes:
- Make module imports relative
- Move more functions from async to sync, decorate with @callback where necessary
- Remove monitored_variables option, add all sensors by default
@MartinHjelmare
Copy link
Member

left a comment

Great!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Please update the breaking change paragraph in the PR description, regarding removing monitored variables.

Ping me when that is done and build passes and I can merge.

@mvn23

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

This should be good to go. Thanks for the help @MartinHjelmare

@MartinHjelmare MartinHjelmare merged commit 43a6be6 into home-assistant:dev Jun 21, 2019

13 checks passed

build Workflow: build
Details
ci/circleci: pre-install-all-requirements Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.7 Your tests passed on CircleCI!
Details
ci/circleci: pylint Your tests passed on CircleCI!
Details
ci/circleci: static-check Your tests passed on CircleCI!
Details
ci/circleci: test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: test 3.7 Your tests passed on CircleCI!
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 86e5053...83ea38d
Details
codecov/project 94.12% (target 90%)
Details

@mvn23 mvn23 deleted the mvn23:otgw_multi_support branch Jun 21, 2019

mxworm added a commit to mxworm/home-assistant that referenced this pull request Jun 21, 2019
Merge branch 'dev' into current
* dev: (69 commits)
  Improve autodiscovered yeelights model detection (home-assistant#24671)
  Update azure-pipelines-release.yml for Azure Pipelines
  Add device class support for Ambient PWS sensors (home-assistant#24677)
  Updated frontend to 20190620.0
  Prefere binary with wheels (home-assistant#24669)
  Clean up Google Config (home-assistant#24663)
  Vlc telnet (home-assistant#24290)
  Update azure-pipelines-wheels.yml for Azure Pipelines
  Multiple devices support for opentherm_gw (home-assistant#22932)
  Update azure-pipelines-wheels.yml for Azure Pipelines
  Fix downloader_download_failed event not firing for HTTP response errors (home-assistant#24640)
  braviatv, nmap_tracker: use getmac for getting MAC addresses (home-assistant#24628)
  Fix AttributeError: 'NoneType' object has no attribute 'group' with sytadin component (home-assistant#24652)
  Bump pysmartthings (home-assistant#24659)
  Update LIFX brightness during long transitions (home-assistant#24653)
  Upgrade blinkpy==0.14.1 for startup bugfix (home-assistant#24656)
  Warn when user tries run custom config flow (home-assistant#24657)
  Bump ZHA dependencies. (home-assistant#24637)
  Fix device tracker see for entity registry entities (home-assistant#24633)
  Bumped version to 0.94.4
  ...
@balloob balloob referenced this pull request Jul 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.