Add support for Zehnder Comfoconnect LAN C bridge. by michaelarnauts · Pull Request #7246 · home-assistant/core

michaelarnauts · 2017-04-23T12:46:44Z

Description:

This PR adds support for the Zehnder ComfoAir Q350/450/600 ventilation systems. You'll need a ComfoConnect LAN C bridge to interface with it.

The bridge can report sensor data, and you can adjust the ventilation mode (away, low, medium and high).

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

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

climate:
  name: ventilation
  platform: comfoconnect
  host: 192.168.1.213

Checklist:

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

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

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

mention-bot · 2017-04-23T12:46:46Z

@michaelarnauts, thanks for your PR! By analyzing the history of the files in this pull request, we identified @turbokongen, @balloob and @pvizeli to be potential reviewers.

houndci-bot · 2017-04-23T12:46:57Z

line too long (83 > 79 characters)

houndci-bot · 2017-04-23T12:46:57Z

line too long (83 > 79 characters)

michaelarnauts · 2017-04-23T12:50:55Z

Note I had to remove the ATTR_TEMPERATURE from climate's default state_attributes and add it only if it is present.

This caused the interface to always render the target_temperature picker, also when the platform didn't support setting it.

michaelarnauts · 2017-04-23T12:59:07Z

Also note that I had to write a python library for this (https://github.com/michaelarnauts/comfoconnect), and this is my first one. There are so many ways I could have done something wrong there, but it seems to work fine.

Is it safe for example to use threading.Thread in a library? I need it to keep on listening for callback messages.

pvizeli

Leave a comment

pvizeli · 2017-04-25T12:16:59Z

Make sure that the callback only start after EVENT_HOMEASSISTANT_START event is receive with a event listener (You can also Register the callback inside async_added_to_hass and hass.async_add_job. Make also sure that the callback thread is closing with a EVENT_HOMEASSISTANT_STOP event listener.

The callback thread of comfoconnect is only started when the update is executed (when self._comfoconnect.connect is executed). It is stopped in self._comfoconnect.disconnect in the finally clause. It only stays awake for maximum 5 seconds (during wait_for_sensor_values).

I initially used the EVENT_HOMEASSISTANT_[START|STOP] event, and kept the callback thread open, but the bridge only allows one open connection at the same time, so I decided to stay with polling and open the connection during update.

pvizeli · 2017-04-25T12:17:29Z

Not needed, remove that and use self.hass they will set on core

Actually, I don't need hass. I will remove it out of the __init__.

… to be important.

michaelarnauts · 2017-04-27T20:34:19Z

I've rebased with dev, and the travis errors are gone now.

michaelarnauts · 2017-04-29T10:47:16Z

@pvizeli and @turbokongen
is this okay?

balloob · 2017-04-30T03:41:19Z

        }
+
+        target_temperature = self.target_temperature
+        if target_temperature is not None:


I already mentioned this in the chat, this is not allowed. Please revert this change.

The idea of the climate component is for devices that can control the temperature. If your device can't control the temperature, it is a fan. And so please implement a platform for the fan component.

You can have an extra sensor platform to expose the current temperature.

Hmm, I didn't know the rule that only temperature controllable things could be climate platforms. This contradicts the climate documentation:

The climate component is built for the controlling and monitoring of HVAC (heating, ventilating, and air conditioning) and thermostat devices.

I will try to find some time to move over to the fan component, although it's unfortunate that that component doesn't support the temperature view.

If you implement the temperature as a separate sensor, you get graphs too.

Alright, I've setup my hass to use template sensors and fetch them from the attributes. Works fine!

michaelarnauts · 2017-04-30T08:25:33Z

I've moved the platform to the fan component. There still seems to be a frontend since it is also displaying a "Oscillate" toggle, what isn't returned in the supported_features call, I also don't set a direction in my attributes.

balloob · 2017-04-30T19:44:20Z

+        data = {}
+
+        for key, value in self._data.items():
+            if key == SENSOR_FAN_SPEED_MODE:


You won't have to add this to the state attributes, the fan already exposes this.

balloob · 2017-04-30T19:46:20Z

+                data[ATTR_OUTSIDE_HUMIDITY] = value
+
+            elif key == SENSOR_TEMPERATURE_EXTRACT:
+                data[ATTR_CURRENT_TEMPERATURE] = value / 10.0


Wouldn't it make more sense to have these be added as a sensor platform? That way we get graphs and easy to automate against it.

Move auth to components/comfoconnect.py and have it store the ComfoConnect object in hass.data

add sensor/comfoconnect.py to expose sensors

update the fan platform to use the ComfoConnect object in hass.data

balloob · 2017-04-30T19:46:45Z

+            _LOGGER.debug('Subscribed to sensors.')
+
+            # Wait maximum of 5 seconds for all the sensor values.
+            self.wait_for_sensor_values(5)


Is this doing a sleep?

balloob · 2017-04-30T19:51:09Z

+                _LOGGER.error('Timeout while waiting for sensor data.')
+                return
+
+            time.sleep(1)


Home Assistant only has 10 threads to do work for synchronous components/platforms. Blocking a thread with a sleep is not allowed. Instead, use track_point_in_time to schedule a function to be called again

michaelarnauts · 2017-05-03T15:48:33Z

Alright, I will close this PR for now, since I have to rework some stuff to handle the threads better.

homeassistant added cla-signed integration: climate new-platform platform: climate.comfoconnect labels Apr 23, 2017

balloobbot added platform and removed cla-signed integration: climate new-platform platform: climate.comfoconnect labels Apr 23, 2017

michaelarnauts mentioned this pull request Apr 23, 2017

ComfoConnect climate platform documentation. home-assistant/home-assistant.io#2483

Merged

houndci-bot reviewed Apr 23, 2017

View reviewed changes

homeassistant added the cla-signed label Apr 23, 2017

balloobbot added requirement and removed cla-signed labels Apr 23, 2017

homeassistant added the cla-signed label Apr 23, 2017

pvizeli requested a review from turbokongen April 25, 2017 12:05

pvizeli requested changes Apr 25, 2017

View reviewed changes

homeassistant added the cla-signed label Apr 25, 2017

Michaël Arnauts added 5 commits April 27, 2017 19:37

Adds support for Zehnder Comfoconnect LAN C bridge.

3f9f93d

Require tagged version 0.1 of pycomfoconnect.

7605131

Add current_humidity method.

408ffd3

Not sure why, but order of dependencies in requirements_all.txt seems…

ccdde61

… to be important.

Don't store the hass variable.

c57a02b

michaelarnauts force-pushed the comfoconnect branch from 2297048 to c57a02b Compare April 27, 2017 17:37

homeassistant added the cla-signed label Apr 27, 2017

Add debug logging.

e63643c

homeassistant added the cla-signed label Apr 29, 2017

balloob requested changes Apr 30, 2017

View reviewed changes

Move ComfoConnect platform to fan component.

3f869cf

requirements.txt and .coveragerc should also reflect the move to fan.

cda7406

balloob requested changes Apr 30, 2017

View reviewed changes

michaelarnauts closed this May 3, 2017

michaelarnauts mentioned this pull request Jun 17, 2017

Comfoconnect fan component #8073

Merged

6 tasks

home-assistant locked and limited conversation to collaborators Aug 12, 2017

Uh oh!

Conversation

michaelarnauts commented Apr 23, 2017 • 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!

mention-bot commented Apr 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelarnauts commented Apr 23, 2017

Uh oh!

michaelarnauts commented Apr 23, 2017

Uh oh!

pvizeli left a comment

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!

michaelarnauts commented Apr 27, 2017

Uh oh!

michaelarnauts commented Apr 29, 2017

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!

michaelarnauts commented Apr 30, 2017

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!

michaelarnauts commented May 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

michaelarnauts commented Apr 23, 2017 •

edited

Loading

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