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 sensor support to opentherm_gw #17314

Merged
merged 11 commits into from Oct 19, 2018

Conversation

Projects
None yet
5 participants
@mvn23
Contributor

mvn23 commented Oct 10, 2018

Description:

Add the option to expose internal data as sensors.

Related issue (if applicable): ref. #16670

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

Example entry for configuration.yaml (if applicable):

Not all supported variables are shown, a full list of supported variables is available in the docs PR.

opentherm_gw:
  device: /dev/ttyUSB0
  monitored_variables:
  - otgw_about
  - otgw_build
  - control_setpoint
  - room_setpoint
  - room_temp
  - ch_water_temp

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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:

  • N/A

mvn23 added some commits Oct 6, 2018

@wafflebot wafflebot bot added the in progress label Oct 10, 2018

@mvn23 mvn23 changed the title from Opentherm gw to Add sensor support to opentherm_gw Oct 10, 2018

@mvn23 mvn23 referenced this pull request Oct 10, 2018

Merged

Update opentherm_gw docs for sensor support #6661

2 of 2 tasks complete

@mvn23 mvn23 changed the title from Add sensor support to opentherm_gw to [WIP] Add sensor support to opentherm_gw Oct 10, 2018

@mvn23

This comment has been minimized.

Contributor

mvn23 commented Oct 10, 2018

Messed up here, meant to push something else...

@mvn23 mvn23 changed the title from [WIP] Add sensor support to opentherm_gw to Add sensor support to opentherm_gw Oct 10, 2018

@awarecan awarecan added cla-recheck and removed cla-signed labels Oct 10, 2018

mvn23 added some commits Oct 12, 2018

Update dependency to v0.2b1
Old version had incorrect variable names for some of the sensors
@mvn23

This comment has been minimized.

Contributor

mvn23 commented Oct 18, 2018

@MartinHjelmare you've reviewed my previous opentherm_gw PRs as well. Could you take a look at this when you have time? I know it's hacktoberfest so you're probably quite busy as it is, I just don't want this PR to get snowed under ;)

else:
_LOGGER.error("Monitored variable not supported: %s", var)
if sensors:
hass.async_create_task(

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 18, 2018

Member

We can just await the coroutine here.

This comment has been minimized.

@mvn23

mvn23 Oct 18, 2018

Contributor

This was changed in 03be3ee, but I am considering to change it back, either now or in the next PR. The rationale behind it is that I want to add the binary sensors from here as well and I figured it would be more efficient to schedule both tasks at once rather than awaiting them.

@@ -38,10 +40,11 @@
DOMAIN: vol.Schema({
vol.Required(CONF_DEVICE): cv.string,
vol.Optional(CONF_CLIMATE, default={}): CLIMATE_SCHEMA,
vol.Optional(CONF_MONITORED_VARIABLES, default=[]): cv.ensure_list,

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 18, 2018

Member

This should be a list of strings. Please validate this.

Show resolved Hide resolved homeassistant/components/sensor/opentherm_gw.py
class OpenThermSensor(Entity):
"""Representation of an OpenTherm Gateway sensor."""

def __init__(self, hass, var, device_class, unit, friendly_name):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 18, 2018

Member

Don't pass in hass. It will be set on the entity when it has been added to home assistant.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 18, 2018

Member

Pass in the entity_id instead.

@mvn23

This comment has been minimized.

Contributor

mvn23 commented Oct 18, 2018

Thanks for the quick review @MartinHjelmare, all findings have been addressed.

@MartinHjelmare

Nice!

@MartinHjelmare

MartinHjelmare requested changes Oct 18, 2018 edited

Sorry, I missed coverage. Please update .coveragerc and exclude all opentherm_gw platforms from coverage calculation.

@mvn23

This comment has been minimized.

Contributor

mvn23 commented Oct 18, 2018

Ah, indeed. I thought I changed it to wildcard notation with the rewrite to a component. I will update it tonight.

@MartinHjelmare MartinHjelmare merged commit 7baffed into home-assistant:dev Oct 19, 2018

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 increased (+0.1%) to 93.597%
Details

@wafflebot wafflebot bot removed the in progress label Oct 19, 2018

@mvn23 mvn23 deleted the mvn23:opentherm_gw branch Oct 19, 2018

@balloob balloob referenced this pull request Oct 26, 2018

Merged

0.81 #17809

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