Add support for ZhongHong HVAC Controllers #14552

crhan · 2018-05-20T05:02:21Z

Description:

This is a platform for using a HVAC Controller product from Zhong hong tech which sells in China. It can control a bunch of AC brand like Daikin, Hitachi, Meidi, Gree, Haier, Toshiba and Mitsubishi. You can find the product selling page on taobao.com.

Related issue (if applicable): fixes #

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

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

climate:
  - platform: zhong_hong
    host: 192.168.15.19

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:

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

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 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.

fabaff

Just some quick comments to get started.

fabaff · 2018-05-20T17:45:17Z

homeassistant/components/climate/zhong_hong.py

+
+_LOGGER = logging.getLogger(__name__)
+
+CONF_GW_ADDR = 'gw_addr'


Name it gateway_address. Will be easier to spot for humans.

fabaff · 2018-05-20T17:46:43Z

homeassistant/components/climate/zhong_hong.py

+    host = config.get(CONF_HOST)
+    port = config.get(CONF_PORT)
+    gw_addr = config.get(CONF_GW_ADDR)
+    hub = ZhongHongGateway(host, port, gw_addr)


I guess that there will be a traceback if the controller is not available. You need to decide what to do. Make the setup of the platform fails or raise PlatformNotReady.

fabaff · 2018-05-20T17:48:15Z

homeassistant/components/climate/zhong_hong.py

+        for (addr_out, addr_in) in hub.discovery_ac()
+    ]
+    add_devices(devices)
+    hub.start_listen()


Looks like that this should be attached to EVENT_HOMEASSISTANT_START

Why I have to put this after EVENT_HOMEASSISTANT_START? Just curious

I found some times this error will occur. What's wrong with it?

Exception in thread Thread-4: Traceback (most recent call last): File "/usr/local/lib/python3.6/threading.py", line 916, in _bootstrap_inner self.run() File "/usr/local/lib/python3.6/threading.py", line 864, in run self._target(*self._args, **self._kwargs) File "/config/deps/lib/python3.6/site-packages/zhong_hong_hvac/hub.py", line 131, in _listen_to_msg func(payload) File "/config/deps/lib/python3.6/site-packages/zhong_hong_hvac/hvac.py", line 46, in _status_update self._call_status_update() File "/config/deps/lib/python3.6/site-packages/zhong_hong_hvac/hvac.py", line 32, in _call_status_update func(self) File "/config/custom_components/climate/zhong_hong.py", line 74, in _after_update self.schedule_update_ha_state() File "/usr/src/app/homeassistant/helpers/entity.py", line 294, in schedule_update_ha_state self.hass.add_job(self.async_update_ha_state(force_refresh)) AttributeError: 'NoneType' object has no attribute 'add_job'

Why do we wait with adding the devices until home assistant start? @MartinHjelmare

Emmm, I don't know the reason. But I get the suggestion here.

fabaff · 2018-05-20T17:49:53Z

homeassistant/components/climate/zhong_hong.py

+    """Representation of a ZhongHong controller support HVAC."""
+
+    def __init__(self, hub, addr_out, addr_in):
+        """Setup platform."""


This should be in imperative mood. Perhaps something like: "Set up the ZhongHong climate devices."

fabaff · 2018-05-20T17:50:11Z

homeassistant/components/climate/zhong_hong.py

+    @property
+    def unique_id(self):
+        """Return the unique ID of the HVAC."""
+        return "zhong_hong_hvac_%s_%s" % (self._device.addr_out,


Please use string formatting.

Is there any further thought on using string format or % format?

crhan · 2018-05-21T16:30:34Z

BTW, what's the correct name for the mode of just blow the wind? Not making any cold or heat

andersonshatch · 2018-05-24T21:02:10Z

Fan only is probably what you're looking for, there's a constant defined here for it
https://github.com/home-assistant/home-assistant/blob/fa9b9105a813285dce9cdb12638e83188285b3bb/homeassistant/components/climate/__init__.py#L45

crhan · 2018-05-27T06:08:37Z

thanks, FAN_ONLY name done.

crhan

still cannot fix this problem:

  File "/usr/local/lib/python3.6/threading.py", line 916, in _bootstrap_inner
    self.run()
  File "/usr/local/lib/python3.6/threading.py", line 864, in run
    self._target(*self._args, **self._kwargs)
  File "/config/deps/lib/python3.6/site-packages/zhong_hong_hvac/hub.py", line 148, in _listen_to_msg
    func(payload)
  File "/config/deps/lib/python3.6/site-packages/zhong_hong_hvac/hvac.py", line 50, in _status_update
    self._call_status_update()
  File "/config/deps/lib/python3.6/site-packages/zhong_hong_hvac/hvac.py", line 32, in _call_status_update
    func(self)
  File "/config/custom_components/climate/zhong_hong.py", line 74, in _after_update
    self.schedule_update_ha_state()
  File "/usr/src/app/homeassistant/helpers/entity.py", line 294, in schedule_update_ha_state
    self.hass.add_job(self.async_update_ha_state(force_refresh))
AttributeError: 'NoneType' object has no attribute 'add_job'

crhan · 2018-05-21T15:39:41Z

homeassistant/components/climate/zhong_hong.py

+    @property
+    def unique_id(self):
+        """Return the unique ID of the HVAC."""
+        return "zhong_hong_hvac_%s_%s" % (self._device.addr_out,


Is there any further thought on using string format or % format?

crhan · 2018-05-21T16:23:47Z

homeassistant/components/climate/zhong_hong.py

+        for (addr_out, addr_in) in hub.discovery_ac()
+    ]
+    add_devices(devices)
+    hub.start_listen()


I found some times this error will occur. What's wrong with it?

Exception in thread Thread-4: Traceback (most recent call last): File "/usr/local/lib/python3.6/threading.py", line 916, in _bootstrap_inner self.run() File "/usr/local/lib/python3.6/threading.py", line 864, in run self._target(*self._args, **self._kwargs) File "/config/deps/lib/python3.6/site-packages/zhong_hong_hvac/hub.py", line 131, in _listen_to_msg func(payload) File "/config/deps/lib/python3.6/site-packages/zhong_hong_hvac/hvac.py", line 46, in _status_update self._call_status_update() File "/config/deps/lib/python3.6/site-packages/zhong_hong_hvac/hvac.py", line 32, in _call_status_update func(self) File "/config/custom_components/climate/zhong_hong.py", line 74, in _after_update self.schedule_update_ha_state() File "/usr/src/app/homeassistant/helpers/entity.py", line 294, in schedule_update_ha_state self.hass.add_job(self.async_update_ha_state(force_refresh)) AttributeError: 'NoneType' object has no attribute 'add_job'

balloob · 2018-05-29T13:09:55Z

homeassistant/components/climate/zhong_hong.py

+            ]
+        except Exception as exc:
+            _LOGGER.error("ZhongHong controller is not ready", exc_info=exc)
+            raise PlatformNotReady


This will not matter becaus you're no longer running this inside setup_platform. You need to test the credentials inside the setup_platform method.

Is this the right way to solve the problem?

def setup_platform(hass, config, add_devices, discovery_info=None): """Set up the ZhongHong HVAC platform.""" from zhong_hong_hvac.hub import ZhongHongGateway host = config.get(CONF_HOST) port = config.get(CONF_PORT) gw_addr = config.get(CONF_GATEWAY_ADDRRESS) hub = ZhongHongGateway(host, port, gw_addr) try: devices = [ ZhongHongClimate(hub, addr_out, addr_in) for (addr_out, addr_in) in hub.discovery_ac() ] except Exception as exc: _LOGGER.error("ZhongHong controller is not ready", exc_info=exc) raise PlatformNotReady def startup(event): """Add devices to HA and start hub socket.""" add_devices(devices) hub.start_listen() hub.query_all_status() hass.bus.listen_once(EVENT_HOMEASSISTANT_START, startup)

balloob · 2018-05-29T13:10:52Z

homeassistant/components/climate/zhong_hong.py

+    @property
+    def max_temp(self):
+        """Return the maximum temperature."""
+        return convert_temperature(self._device.max_temp, TEMP_CELSIUS,


You don't need to convert anything. Your device is already in CELSIUS and you are converting to CELSIUS. Just return self._device_max_temp

crhan · 2018-06-03T12:22:12Z

@MartinHjelmare Have a look~

MartinHjelmare

Looks good. A small comment on a name and a question.

MartinHjelmare · 2018-06-03T12:48:55Z

homeassistant/components/climate/zhong_hong.py

+CONF_GATEWAY_ADDRRESS = 'gateway_address'
+
+REQUIREMENTS = ['zhong_hong_hvac==1.0.9']
+SIGNAL_DEVICE_SETTED_UP = 'zhong_hong_device_setted_up'


SIGNAL_DEVICE_ADDED = 'zhong_hong_device_added'

MartinHjelmare · 2018-06-03T12:50:02Z

homeassistant/components/climate/zhong_hong.py

+            return
+
+        _LOGGER.debug("zhong_hong hub start listen event")
+        hub.start_listen()


Are these calls to the hub async safe? No I/O or blocking?

Yes, It just start a thread to process the tcp stream. https://github.com/crhan/ZhongHongHVAC/blob/master/zhong_hong_hvac/hub.py#L159-L174

BTW, how can I change the TCP socket process code to async friendly?

There's a potential socket interaction and sleep call here. Let's do this in a worker thread via use of async_add_job.

If you want to build an async library, you would use either of the two alternatives for socket communication in asyncio standard library, transports and protocols or streams.

https://docs.python.org/3/library/asyncio-protocol.html

https://docs.python.org/3/library/asyncio-stream.html

SIGNAL_DEVICE_SETTED_UP -> SIGNAL_DEVICE_ADDED

MartinHjelmare

I think you should change back to setup_platform and the sync api but keep startup a coroutine. There's less context switching needed that way since the library is not async.

MartinHjelmare · 2018-06-04T05:59:40Z

homeassistant/components/climate/zhong_hong.py

+            for (addr_out, addr_in) in hub.discovery_ac()
+        ]
+
+    except Exception as exc:


Reading the library, it looks like you already catch Exception in the library when dealing with the socket. When could an exception bubble up to here?

Maybe I will let the exception through out in the future. Must I remove the stupid try...except now?

You should never catch a bare exception. You should always only catch the exceptions that you expect.

So if you're using a Socket, see what the base class is and catch that.

This has to be updated to be the exception that is raised. You are not allowed to catch a bare exception

Sorry I missed this comment. It is really not necessary to catch this exception. Because I catch the exception in the device library.

MartinHjelmare · 2018-06-04T06:01:19Z

homeassistant/components/climate/zhong_hong.py

+            return
+
+        _LOGGER.debug("zhong_hong hub start listen event")
+        hub.start_listen()


There's a potential socket interaction and sleep call here. Let's do this in a worker thread via use of async_add_job.

MartinHjelmare · 2018-06-04T06:02:34Z

homeassistant/components/climate/zhong_hong.py

+
+        _LOGGER.debug("zhong_hong hub start listen event")
+        hub.start_listen()
+        hub.query_all_status()


This is sending messages to the device via socket and must be done in a worker thread (not inside event loop).

MartinHjelmare · 2018-06-04T06:07:25Z

homeassistant/components/climate/zhong_hong.py

+    try:
+        devices = [
+            ZhongHongClimate(hub, addr_out, addr_in)
+            for (addr_out, addr_in) in hub.discovery_ac()


This is doing I/O via socket and is not async safe. I would change back to the sync api and use setup_platform. You can still have startup be a coroutine.

Can I still use async_add_job here to avoid the problem?

Yes, but I would instead change to setup_platform as mentioned.

crhan · 2018-06-05T23:40:26Z

Is there any code I need to change? The async def async_setup_platform has changed to def setup_platform already.

MartinHjelmare · 2018-06-06T02:21:55Z

homeassistant/components/climate/zhong_hong.py

+
+        operation_mode = kwargs.get(ATTR_OPERATION_MODE)
+        if operation_mode is not None:
+            self._device.set_operation_mode(operation_mode)


Here you don't cast the operation mode to upper case like below. Is that not needed?

Nope, you are right. I think I should use self.set_operation_mode directly, but not use self._device.set_operation_mode.

balloob · 2018-06-08T02:56:17Z

homeassistant/components/climate/zhong_hong.py

+        ]
+
+    except Exception as exc:
+        _LOGGER.error("ZhongHong controller is not ready", exc_info=exc)


Don't print the full stack trace of the exception, that will confuse users. Stringify the exception instead

balloob · 2018-06-08T02:57:47Z

homeassistant/components/climate/zhong_hong.py

+        self.is_initialized = True
+        async_dispatcher_send(self.hass, SIGNAL_DEVICE_ADDED)
+
+    def update(self):


This is never called?

I suppose home assistant core logic will invoke this method in some situation?

Only if should_poll is true or if you pass True as second argument to add_devices or as first argument to schedule_update_ha_state.

MartinHjelmare · 2018-06-10T13:26:46Z

homeassistant/components/climate/zhong_hong.py

+        ]
+
+    except Exception as exc:
+        _LOGGER.error("ZhongHong controller is not ready: %s", str(exc))


Exceptions already have a magic string method. We don't need to copy to string.

balloob · 2018-06-10T15:48:29Z

homeassistant/components/climate/zhong_hong.py

+            for (addr_out, addr_in) in hub.discovery_ac()
+        ]
+
+    except Exception as exc:


This has to be updated to be the exception that is raised. You are not allowed to catch a bare exception

houndci-bot · 2018-06-11T00:37:57Z

homeassistant/components/climate/zhong_hong.py

+    SUPPORT_OPERATION_MODE, SUPPORT_TARGET_TEMPERATURE, ClimateDevice)
+from homeassistant.const import (ATTR_TEMPERATURE, CONF_HOST, CONF_PORT,
+                                 EVENT_HOMEASSISTANT_STOP, TEMP_CELSIUS)
+from homeassistant.exceptions import PlatformNotReady


'homeassistant.exceptions.PlatformNotReady' imported but unused

homeassistant added cla-signed new-platform platform: climate.zhong_hong labels May 20, 2018

crhan mentioned this pull request May 20, 2018

add zhong_hong components home-assistant/home-assistant.io#5402

Merged

2 tasks

crhan changed the title ~~[WIP]first blood for ZhongHong HVAC Controller~~ first blood for ZhongHong HVAC Controller May 20, 2018

homeassistant added the cla-signed label May 20, 2018

fabaff changed the title ~~first blood for ZhongHong HVAC Controller~~ Add support for ZhongHong HVAC Controllers May 20, 2018

fabaff reviewed May 20, 2018

View reviewed changes

homeassistant added the cla-signed label May 21, 2018

crhan added 9 commits May 27, 2018 13:34

first blood for ZhongHong HVAC Controller

75248c4

add requirements

9de5a40

requirements_all.txt updated

ac03c04

add zhong_hong.py to coveragerc

09ab2c5

add comments

94bf7cc

unique_id add platform name

af34d34

zhong_hong_hvac version bump to 1.0.1

55ac431

improve some coding style to match the project standard

1c739d8

zhong_hong_hvac version bump to 1.0.4

9b27983

crhan force-pushed the zhonghong_hvac branch from 95d1ca6 to 9b27983 Compare May 27, 2018 06:08

homeassistant added the cla-signed label May 27, 2018

zhong_hong_hvac version require 1.0.7

fcfbfa1

crhan commented May 27, 2018

View reviewed changes

crhan added 2 commits May 27, 2018 22:41

update requirements by script/gen_requirements_all.py

27cfc66

zhong_hong_hvac version bump to 1.0.8

4804146

balloob reviewed May 29, 2018

View reviewed changes

use dispatcher to setup hub

760ef67

MartinHjelmare reviewed Jun 3, 2018

View reviewed changes

var name change

09229fa

SIGNAL_DEVICE_SETTED_UP -> SIGNAL_DEVICE_ADDED

homeassistant added the cla-signed label Jun 4, 2018

MartinHjelmare reviewed Jun 4, 2018

View reviewed changes

async problem fix

fc8d8fa

MartinHjelmare reviewed Jun 6, 2018

View reviewed changes

bugfix: set_operation_mode forget to use upper case

41bcba4

ghost added the in progress label Jun 7, 2018

balloob reviewed Jun 8, 2018

View reviewed changes

balloob added the almost-done label Jun 8, 2018

stringify the exception instead of print full stack of traceback

183f503

homeassistant added the cla-signed label Jun 10, 2018

MartinHjelmare reviewed Jun 10, 2018

View reviewed changes

avoid to call str(exception) explicity

5cab3f6

balloob requested changes Jun 10, 2018

View reviewed changes

remove unnecessary try...except clause

c2ac55c

houndci-bot reviewed Jun 11, 2018

View reviewed changes

remove unused import

1fa2693

balloob approved these changes Jun 14, 2018

View reviewed changes

balloob merged commit c36c3f0 into home-assistant:dev Jun 14, 2018

ghost removed the in progress label Jun 14, 2018

balloob mentioned this pull request Jun 22, 2018

0.72 #15088

Merged

home-assistant locked and limited conversation to collaborators Dec 10, 2018

ghost added integration: zhong_hong and removed platform: climate.zhong_hong labels Mar 21, 2019


		_LOGGER = logging.getLogger(__name__)

		CONF_GW_ADDR = 'gw_addr'

Add support for ZhongHong HVAC Controllers #14552

Add support for ZhongHong HVAC Controllers #14552

Conversation

crhan commented May 20, 2018 • edited Loading

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

fabaff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Why do we wait with adding the devices until home assistant start? @MartinHjelmare

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crhan commented May 21, 2018 • edited Loading

andersonshatch commented May 24, 2018

crhan commented May 27, 2018

crhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crhan commented Jun 3, 2018

MartinHjelmare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinHjelmare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crhan commented Jun 5, 2018

Choose a reason for hiding this comment

crhan Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinHjelmare Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crhan commented May 20, 2018 •

edited

Loading

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

crhan commented May 21, 2018 •

edited

Loading

crhan Jun 7, 2018 •

edited

Loading

MartinHjelmare Jun 10, 2018 •

edited

Loading