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

Add support for ZhongHong HVAC Controllers #14552

Merged
merged 24 commits into from
Jun 14, 2018
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .coveragerc
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,7 @@ omit =
homeassistant/components/climate/sensibo.py
homeassistant/components/climate/touchline.py
homeassistant/components/climate/venstar.py
homeassistant/components/climate/zhong_hong.py
homeassistant/components/cover/garadget.py
homeassistant/components/cover/gogogate2.py
homeassistant/components/cover/homematic.py
Expand Down
227 changes: 227 additions & 0 deletions homeassistant/components/climate/zhong_hong.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,227 @@
"""
Support for ZhongHong HVAC Controller.

For more details about this platform, please refer to the documentation at
https://home-assistant.io/components/climate.zhong_hong/
"""
import logging

import voluptuous as vol

from homeassistant.components.climate import (
ATTR_OPERATION_MODE, PLATFORM_SCHEMA, STATE_COOL, STATE_DRY,
STATE_FAN_ONLY, STATE_HEAT, SUPPORT_FAN_MODE, SUPPORT_ON_OFF,
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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'homeassistant.exceptions.PlatformNotReady' imported but unused

import homeassistant.helpers.config_validation as cv
from homeassistant.helpers.dispatcher import (async_dispatcher_connect,
async_dispatcher_send)

_LOGGER = logging.getLogger(__name__)

CONF_GATEWAY_ADDRRESS = 'gateway_address'

REQUIREMENTS = ['zhong_hong_hvac==1.0.9']
SIGNAL_DEVICE_ADDED = 'zhong_hong_device_added'
SIGNAL_ZHONG_HONG_HUB_START = 'zhong_hong_hub_start'

PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Required(CONF_HOST):
cv.string,
vol.Optional(CONF_PORT, default=9999):
vol.Coerce(int),
vol.Optional(CONF_GATEWAY_ADDRRESS, default=1):
vol.Coerce(int),
})


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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I still use async_add_job here to avoid the problem?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

]

except Exception as exc:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_LOGGER.error("ZhongHong controller is not ready", exc_info=exc)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

raise PlatformNotReady

_LOGGER.debug("We got %s zhong_hong climate devices", len(devices))

hub_is_initialized = False

async def startup():
"""Start hub socket after all climate entity is setted up."""
nonlocal hub_is_initialized
if not all([device.is_initialized for device in devices]):
return

if hub_is_initialized:
return

_LOGGER.debug("zhong_hong hub start listen event")
await hass.async_add_job(hub.start_listen)
await hass.async_add_job(hub.query_all_status)
hub_is_initialized = True

async_dispatcher_connect(hass, SIGNAL_DEVICE_ADDED, startup)

# add devices after SIGNAL_DEVICE_SETTED_UP event is listend
add_devices(devices)

def stop_listen(event):
"""Stop ZhongHongHub socket."""
hub.stop_listen()

hass.bus.listen_once(EVENT_HOMEASSISTANT_STOP, stop_listen)


class ZhongHongClimate(ClimateDevice):
"""Representation of a ZhongHong controller support HVAC."""

def __init__(self, hub, addr_out, addr_in):
"""Set up the ZhongHong climate devices."""
from zhong_hong_hvac.hvac import HVAC
self._device = HVAC(hub, addr_out, addr_in)
self._hub = hub
self._current_operation = None
self._current_temperature = None
self._target_temperature = None
self._current_fan_mode = None
self._is_on = None
self.is_initialized = False

async def async_added_to_hass(self):
"""Register callbacks."""
self._device.register_update_callback(self._after_update)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be moved to the entity coroutine method async_added_to_hass. Otherwise there might be an error if the callback is called before the entity has been added to home assistant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Finally found the root cause of starting problem

self.is_initialized = True
async_dispatcher_send(self.hass, SIGNAL_DEVICE_ADDED)

def update(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is never called?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@MartinHjelmare MartinHjelmare Jun 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""Update device status from hub."""
self._device.update()

def _after_update(self, climate):
"""Callback to update state."""
_LOGGER.debug("async update ha state")
if self._device.current_operation:
self._current_operation = self._device.current_operation.lower()
if self._device.current_temperature:
self._current_temperature = self._device.current_temperature
if self._device.current_fan_mode:
self._current_fan_mode = self._device.current_fan_mode
if self._device.target_temperature:
self._target_temperature = self._device.target_temperature
self._is_on = self._device.is_on
self.schedule_update_ha_state()

@property
def should_poll(self):
"""Return the polling state."""
return False

@property
def name(self):
"""Return the name of the thermostat, if any."""
return self.unique_id

@property
def unique_id(self):
"""Return the unique ID of the HVAC."""
return "zhong_hong_hvac_{}_{}".format(self._device.addr_out,
self._device.addr_in)

@property
def supported_features(self):
"""Return the list of supported features."""
return (SUPPORT_TARGET_TEMPERATURE | SUPPORT_FAN_MODE
| SUPPORT_OPERATION_MODE | SUPPORT_ON_OFF)

@property
def temperature_unit(self):
"""Return the unit of measurement used by the platform."""
return TEMP_CELSIUS

@property
def current_operation(self):
"""Return current operation ie. heat, cool, idle."""
return self._current_operation

@property
def operation_list(self):
"""Return the list of available operation modes."""
return [STATE_COOL, STATE_HEAT, STATE_DRY, STATE_FAN_ONLY]

@property
def current_temperature(self):
"""Return the current temperature."""
return self._current_temperature

@property
def target_temperature(self):
"""Return the temperature we try to reach."""
return self._target_temperature

@property
def target_temperature_step(self):
"""Return the supported step of target temperature."""
return 1

@property
def is_on(self):
"""Return true if on."""
return self._device.is_on

@property
def current_fan_mode(self):
"""Return the fan setting."""
return self._current_fan_mode

@property
def fan_list(self):
"""Return the list of available fan modes."""
return self._device.fan_list

@property
def min_temp(self):
"""Return the minimum temperature."""
return self._device.min_temp

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

def turn_on(self):
"""Turn on ac."""
return self._device.turn_on()

def turn_off(self):
"""Turn off ac."""
return self._device.turn_off()

def set_temperature(self, **kwargs):
"""Set new target temperature."""
temperature = kwargs.get(ATTR_TEMPERATURE)
if temperature is not None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this cannot happen

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just make the code looks like the same. Is that really matter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The set temperature service schema allows two ways of providing temperature, either with one temperature or with high and low temperatures. So the case on this line here can happen and is a good check.

self._device.set_temperature(temperature)

operation_mode = kwargs.get(ATTR_OPERATION_MODE)
if operation_mode is not None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mode is not part of setting a temperature?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self._device.set_operation_mode(operation_mode)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@crhan crhan Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


def set_operation_mode(self, operation_mode):
"""Set new target operation mode."""
self._device.set_operation_mode(operation_mode.upper())

def set_fan_mode(self, fan_mode):
"""Set new target fan mode."""
self._device.set_fan_mode(fan_mode)
3 changes: 3 additions & 0 deletions requirements_all.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1396,6 +1396,9 @@ zengge==0.2
# homeassistant.components.zeroconf
zeroconf==0.20.0

# homeassistant.components.climate.zhong_hong
zhong_hong_hvac==1.0.9

# homeassistant.components.media_player.ziggo_mediabox_xl
ziggo-mediabox-xl==1.0.0

Expand Down