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 IntesisHome controllable air conditioners #8316

Closed
wants to merge 21 commits into from
Closed

Conversation

jnimmo
Copy link
Contributor

@jnimmo jnimmo commented Jul 3, 2017

Description:

Added support for IntesisHome controlled air conditioners.
Work in progress, first time submitting a component so please let me know if anything needs changing.
This platform has been in use by a number of testers on https://community.home-assistant.io/t/add-support-for-intesishome-wifi-ac-control/5172 - initially was added as an IntesisHome platform and climate component however combined it into the one climate component.

Pull request in home-assistant.github.io with documentation: home-assistant/home-assistant.io#2925

Example entry for configuration.yaml (if applicable):

climate:
  - platform: intesishome
    username: 'user'
    password: 'pass'

Checklist:

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

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

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

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@homeassistant
Copy link
Contributor

Hi @jnimmo,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@property
def is_away_mode_on(self):
"""Return if away mode is on."""
return None

Choose a reason for hiding this comment

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

no newline at end of file

def update_callback(self):
"""Called when data is received by pyIntesishome"""
_LOGGER.debug("IntesisHome sent a status update.")
self.hass.async_add_job(self.update_ha_state,True)

Choose a reason for hiding this comment

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

missing whitespace after ','

intesishome.poll_status(shouldCallback)


@property

Choose a reason for hiding this comment

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

too many blank lines (2)

self._swing = STATE_UNKNOWN


@Throttle(MIN_TIME_BETWEEN_UPDATES)

Choose a reason for hiding this comment

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

too many blank lines (2)

self._current_operation = STATE_UNKNOWN

# Target temperature
if self._current_operation in [STATE_OFF,STATE_FAN]:

Choose a reason for hiding this comment

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

missing whitespace after ','

self._fan_speed = STATE_UNKNOWN
self._current_operation = STATE_UNKNOWN

self._operation_list = [STATE_AUTO, STATE_COOL, STATE_HEAT, STATE_DRY, STATE_FAN, STATE_OFF]

Choose a reason for hiding this comment

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

line too long (100 > 79 characters)

class IntesisAC(ClimateDevice):
def __init__(self, deviceid, device):
"""Initialize the thermostat"""
_LOGGER.debug('Added climate device with state: %s',repr(device))

Choose a reason for hiding this comment

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

missing whitespace after ','

for deviceid, device in intesishome.get_devices().items()])
return True

class IntesisAC(ClimateDevice):

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

add_devices([IntesisAC(deviceid, device)
for deviceid, device in intesishome.get_devices().items()])
return True

Choose a reason for hiding this comment

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

blank line contains whitespace

intesishome.connect()

if intesishome.error_message:
persistent_notification.create(hass, intesishome.error_message, "IntesisHome Error", 'intesishome')

Choose a reason for hiding this comment

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

line too long (107 > 79 characters)

@homeassistant
Copy link
Contributor

Hi @jnimmo,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

self._operation_list = [STATE_AUTO, STATE_COOL, STATE_HEAT, STATE_DRY,
STATE_FAN, STATE_OFF]
self._fan_list = [STATE_AUTO, STATE_QUIET, STATE_LOW, STATE_MEDIUM,
STATE_HIGH]

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

self._current_operation = STATE_UNKNOWN

self._operation_list = [STATE_AUTO, STATE_COOL, STATE_HEAT, STATE_DRY,
STATE_FAN, STATE_OFF]

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent


if intesishome.error_message:
persistent_notification.create(hass, intesishome.error_message,
"IntesisHome Error", 'intesishome')

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

_pass = config.get(CONF_PASSWORD)

if intesishome is None:
intesishome = IntesisHome(_user,_pass, hass.loop)

Choose a reason for hiding this comment

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

missing whitespace after ','


_user = config.get(CONF_USERNAME)
_pass = config.get(CONF_PASSWORD)

Choose a reason for hiding this comment

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

blank line contains whitespace

})

# Return cached results if last scan time was less than this value.
# If a persistent connection is established for the controller, changes to values are in realtime.

Choose a reason for hiding this comment

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

line too long (98 > 79 characters)

from homeassistant.components.climate import ( ClimateDevice,
PLATFORM_SCHEMA, ATTR_TARGET_TEMP_HIGH, ATTR_TARGET_TEMP_LOW,
ATTR_TEMPERATURE, ATTR_OPERATION_MODE)
from homeassistant.const import (TEMP_CELSIUS, CONF_SCAN_INTERVAL, STATE_UNKNOWN)

Choose a reason for hiding this comment

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

'homeassistant.const.CONF_SCAN_INTERVAL' imported but unused
line too long (81 > 79 characters)

from homeassistant.components import persistent_notification
from homeassistant.components.climate import ( ClimateDevice,
PLATFORM_SCHEMA, ATTR_TARGET_TEMP_HIGH, ATTR_TARGET_TEMP_LOW,
ATTR_TEMPERATURE, ATTR_OPERATION_MODE)

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

from homeassistant.util import Throttle
from homeassistant.components import persistent_notification
from homeassistant.components.climate import ( ClimateDevice,
PLATFORM_SCHEMA, ATTR_TARGET_TEMP_HIGH, ATTR_TARGET_TEMP_LOW,

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

from homeassistant.const import (CONF_PASSWORD, CONF_USERNAME, CONF_STRUCTURE)
from homeassistant.util import Throttle
from homeassistant.components import persistent_notification
from homeassistant.components.climate import ( ClimateDevice,

Choose a reason for hiding this comment

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

'homeassistant.components.climate.ATTR_TARGET_TEMP_HIGH' imported but unused
'homeassistant.components.climate.ATTR_TARGET_TEMP_LOW' imported but unused
whitespace after '('

})

# Return cached results if last scan time was less than this value.
# If a persistent connection is established for the controller,

Choose a reason for hiding this comment

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

trailing whitespace

def is_away_mode_on(self):
"""Return if away mode is on."""
return None

Choose a reason for hiding this comment

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

blank line at end of file


class IntesisAC(ClimateDevice):
"""Representation of an IntesisHome thermostat."""

Choose a reason for hiding this comment

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

blank line contains whitespace

@jnimmo
Copy link
Contributor Author

jnimmo commented Jul 4, 2017

I'm not sure how to resolve the following issues from Travis - is someone able to kindly advise?
The intesishome variable is designed to be a singleton i.e. one interface back to the IntesisHome API, and then the IntesisAC objects maintain their state from that one intesishome object.

C: 55, 0: Invalid constant name "intesishome" (invalid-name)
C: 61, 4: Invalid constant name "intesishome" (invalid-name)
R:251, 4: Method could be a function (no-self-use)
W: 48, 4: Unused ensure_future imported from asyncio (unused-import)

@Landrash
Copy link
Contributor

Landrash commented Jul 4, 2017

Suggest you update the description file to include your .io PR.
home-assistant/home-assistant.io#2925

def setup_platform(hass, config, add_devices, discovery_info=None):
"""Set up the IntesisHome interface."""
from pyintesishome import IntesisHome
global INTESISHOME
Copy link
Member

Choose a reason for hiding this comment

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

Please use hass.data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review Fabian, I've now switched the platform over to using hass.data

.coveragerc Outdated
@@ -228,6 +228,7 @@ omit =
homeassistant/components/climate/proliphix.py
homeassistant/components/climate/radiotherm.py
homeassistant/components/climate/sensibo.py
homeassistant/components/climate/intesishome.py
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the list alphabetically ordered.

Replaced with hass.data['climate_intesishome']
Migrated to using hass.data[‘climate_intesishome’]
self._intesishome.set_mode_dry(self._deviceid)

if self._target_temp:
self._intesishome.set_temperature(self._deviceid, self._target_temp)

Choose a reason for hiding this comment

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

line too long (84 > 79 characters)

self._intesishome.set_mode_dry(self._deviceid)

if self._target_temp:
self._intesishome.set_temperature(self._deviceid,

Choose a reason for hiding this comment

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

trailing whitespace

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Use the common climate states already defined in the climate component for representing the operation_mode state in home assistant. Implement conversion logic to go between home assistant states and device specific states.


import logging
from datetime import timedelta
import voluptuous as vol
Copy link
Member

Choose a reason for hiding this comment

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

Add a blank line between standard library, third party and homeassistant imports.
https://www.python.org/dev/peps/pep-0008/#imports

from datetime import timedelta

import voluptuous as vol

from datetime import timedelta
import voluptuous as vol
import homeassistant.helpers.config_validation as cv
from homeassistant.const import (CONF_PASSWORD, CONF_USERNAME, CONF_STRUCTURE)
Copy link
Member

Choose a reason for hiding this comment

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

Sort the import lines alphabetically within each bigger group, standard, 3rd party and homeassistant, and also sort each imported object alphabetially on the same line.

_LOGGER = logging.getLogger(__name__)

DEFAULT_NAME = 'IntesisHome'
REQUIREMENTS = ['pyintesishome==0.4']
Copy link
Member

Choose a reason for hiding this comment

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

STATE_LOW = 'Low'
STATE_MEDIUM = 'Medium'
STATE_HIGH = 'High'
STATE_OFF = 'Off'
Copy link
Member

Choose a reason for hiding this comment

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

Home assistant should use its own state names for the climate platforms in the GUI and automation API and convert to device specific states before interacting with a climate device.

Implement two conversion dicts that converts the device specific states to home assistant states and vice versa. Rename the constants you've defined here so that it's clear that they are device specific, and then import the home assistant states from climate base component and const.py.

# Python 3.4.3 and ealier has this as async
# pylint: disable=unused-import
from asyncio import async
ensure_future = async
Copy link
Member

Choose a reason for hiding this comment

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

ensure_future is not used.

fan_speed = self._intesishome.get_fan_speed(self._deviceid)
if fan_speed:
# Capitalize fan speed from pyintesishome
self._fan_speed = fan_speed[:1].upper() + fan_speed[1:]
Copy link
Member

Choose a reason for hiding this comment

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

return self._target_temp

@property
def target_temperature_low(self):
Copy link
Member

Choose a reason for hiding this comment

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

Remove if not overwritten.

return None

@property
def target_temperature_high(self):
Copy link
Member

Choose a reason for hiding this comment

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

Remove if not overwritten.

return None

@property
def is_away_mode_on(self):
Copy link
Member

Choose a reason for hiding this comment

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

Remove if not overwritten.

if 42 in device.get('widgets'):
self._has_swing_control = True

self._intesishome.add_update_callback(self.update_callback)
Copy link
Member

Choose a reason for hiding this comment

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

Move this to a async_added_to_hass method.

@andrey-git
Copy link
Contributor

@jnimmo any updates on this PR?

def update_callback(self):
"""Called when data is received by pyIntesishome."""
_LOGGER.debug("IntesisHome sent a status update.")
self.hass.async_add_job(self.update_ha_state, True)

Choose a reason for hiding this comment

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

Hi there,

I have a lot of deprecation warning assuming caused by this line.

WARNING (Thread-5) [homeassistant.helpers.entity] 'update_ha_state' is deprecated. Use 'schedule_update_ha_state' instead.

@pandaedward
Copy link

Waiting on this PR as well.

@jnimmo Just put in a comment in intesishome.py line 277, lots of deprecation warning

@pvizeli
Copy link
Member

pvizeli commented Aug 25, 2017

Can be reopen if all comments are addressed

@pvizeli pvizeli closed this Aug 25, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Dec 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants