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 Danfoss Air HRV support #20138

Merged
merged 7 commits into from Jan 23, 2019

Conversation

@JonasPed
Copy link
Contributor

JonasPed commented Jan 15, 2019

Description:

Add initial support for Danfoss Air HRV systems. Further functionality will be added when initial PR have been merged. Currently it reads out various sensor and binary sensor values.

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml (if applicable):

danfoss_air:
    host: IP_OF_CCM_MODULE

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.
  • 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.
@rohankapoorcom
Copy link
Member

rohankapoorcom left a comment

Please move the platforms to be under homeassistant/components/danfoss_air/binary_sensor.py and homeassistant/components/danfoss_air/sensor.py and then update .coveragerc.

Show resolved Hide resolved homeassistant/components/binary_sensor/danfoss_air.py Outdated
Show resolved Hide resolved homeassistant/components/danfoss_air.py Outdated
Show resolved Hide resolved homeassistant/components/danfoss_air.py Outdated
@JonasPed

This comment has been minimized.

Copy link
Contributor Author

JonasPed commented Jan 19, 2019

Have updated PR with your comments. Thanks for the initial review.

Show resolved Hide resolved .coveragerc Outdated
"""
Support for Danfoss Air HRV.
Configuration:

This comment has been minimized.

@rohankapoorcom

rohankapoorcom Jan 21, 2019

Member

I believe per the style guidelines, we no longer put configuration in the component either.

https://developers.home-assistant.io/docs/en/development_guidelines.html

This comment has been minimized.

@JonasPed

JonasPed Jan 21, 2019

Author Contributor

I am pretty sure I actually read somewhere it was OK to add documentation example in the header. I am however not able to find it now. Anyway I have removed the example.

For more details about this component, please refer to the documentation at
https://home-assistant.io/components/sensor.danfoss_air/
"""
from homeassistant.components.danfoss_air import DOMAIN

This comment has been minimized.

@rohankapoorcom

rohankapoorcom Jan 21, 2019

Member

Because this platform is part of the sensor domain - this should be imported as DANFOSS_AIR_DOMAIN instead.

This comment has been minimized.

@JonasPed

JonasPed Jan 21, 2019

Author Contributor

Fixed

https://home-assistant.io/components/binary_sensor.danfoss_air/
"""
from homeassistant.components.binary_sensor import BinarySensorDevice
from homeassistant.components.danfoss_air import DOMAIN

This comment has been minimized.

@rohankapoorcom

rohankapoorcom Jan 21, 2019

Member

Because this platform is part of the binary_sensor domain - this should be imported as DANFOSS_AIR_DOMAIN instead.

This comment has been minimized.

@JonasPed

JonasPed Jan 21, 2019

Author Contributor

Fixed

@rohankapoorcom
Copy link
Member

rohankapoorcom left a comment

Couple of smaller things left to go, otherwise this looks good.

Remove config example from header documentation.
Correct import name for platforms.
@rohankapoorcom
Copy link
Member

rohankapoorcom left a comment

Can be merged once build succeeds.

@cgarwood cgarwood merged commit c6cee1c into home-assistant:dev Jan 23, 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 Coverage decreased (-0.05%) to 92.927%
Details

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

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Just some small comments. 👍

def get_value(self, item):
"""Get value for sensor."""
if item in self._data:
return self._data[item]

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jan 23, 2019

Member
def get_value(self, item):
    return self._data.get(item)

This comment has been minimized.

@JonasPed

JonasPed Jan 23, 2019

Author Contributor

I was not aware of the difference between [] and .get(). Python is not one of my primary programming languages :-) It will however make the code a bit more simple. I will correct this in new PR. Thanks for the hint.

as DANFOSS_AIR_DOMAIN


def setup_platform(hass, config, add_devices, discovery_info=None):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jan 23, 2019

Member

Rename add_devices to add_entities.

This comment has been minimized.

@JonasPed

JonasPed Jan 23, 2019

Author Contributor

Agree that add_entities is a more logical name. Will fix in a couple of days.

Show resolved Hide resolved homeassistant/components/danfoss_air/binary_sensor.py
from homeassistant.helpers.entity import Entity


def setup_platform(hass, config, add_devices, discovery_info=None):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jan 23, 2019

Member

See above.

class DanfossAir(Entity):
"""Representation of a Sensor."""

def __init__(self, data, name, sensorUnit, sensorType):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jan 23, 2019

Member

Use lowercase snakecase variable names.

This comment has been minimized.

@JonasPed

JonasPed Jan 23, 2019

Author Contributor

Will fix in a couple of days.

def update(self):
"""Update the new state of the sensor.
This is done through the DanfossAir object tthat does the actually

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jan 23, 2019

Member

that does the actual communication

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

Add Danfoss Air HRV support (home-assistant#20138)
* Add support for Danfoss Air HRV systems.

* Correct lint errors after initial commit of Danfoss Air HRV support.

* A couple of lint fixes for danfoss_air.

* Refactor to comply with HA standards.

* Style fix.

* Use wildcard for danfoss_air in .coveragerc.

* Remove config example from header documentation.
Correct import name for platforms.

@balloob balloob referenced this pull request Feb 6, 2019

Merged

0.87.0 #20794

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

Add Danfoss Air HRV support (home-assistant#20138)
* Add support for Danfoss Air HRV systems.

* Correct lint errors after initial commit of Danfoss Air HRV support.

* A couple of lint fixes for danfoss_air.

* Refactor to comply with HA standards.

* Style fix.

* Use wildcard for danfoss_air in .coveragerc.

* Remove config example from header documentation.
Correct import name for platforms.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment