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

Water heater support #17058

Merged
merged 21 commits into from Oct 8, 2018

Conversation

@w1ll1am23
Collaborator

w1ll1am23 commented Oct 1, 2018

Description:

Support for water heater demo platform and tests.

This was requested by @balloob in #13539 awhile back, but I was not able to test as I don't have a water heater compatible with either platform. I finally found a user willing to help out and got around to moving it over.

This requires home-assistant/home-assistant-polymer#1661 in order to work correctly in the frontend.

Related issue (if applicable):
This lays the foundation for resolving #13539.

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

Example entry for configuration.yaml (if applicable):

water_heater:
  - platform: demo

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:

  • Tests have been added to verify that the new code works.
state = self.hass.states.get(ENTITY_WATER_HEATER)
self.assertEqual("eco", state.attributes.get('operation_mode'))
self.assertEqual("eco", state.state)
water_heater.set_operation_mode(self.hass, "electric", ENTITY_WATER_HEATER)

This comment has been minimized.

@houndci-bot

houndci-bot Oct 1, 2018

line too long (83 > 79 characters)

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

This comment has been minimized.

@houndci-bot

houndci-bot Oct 1, 2018

'asyncio' imported but unused

vacation.delete()

This comment has been minimized.

@houndci-bot

houndci-bot Oct 1, 2018

blank line at end of file

def turn_away_mode_on(self):
"""Set a vacation for 1 year."""
self.add_vacation(None, (datetime.datetime.now() + datetime.timedelta(days=365)).timestamp())

This comment has been minimized.

@houndci-bot

houndci-bot Oct 1, 2018

line too long (101 > 79 characters)

self._unit_of_measurement = unit_of_measurement
self._away = away
self._current_operation = current_operation
self._operation_list = ['eco', 'electric', 'performance', 'high_demand', 'heat_pump', 'gas']

This comment has been minimized.

@houndci-bot

houndci-bot Oct 1, 2018

line too long (100 > 79 characters)

def setup_platform(hass, config, add_entities, discovery_info=None):
"""Set up the Demo water_heater devices."""
add_entities([
DemoWaterHeater('Demo Water Heater', 119, TEMP_FAHRENHEIT, False, 'eco')

This comment has been minimized.

@houndci-bot

houndci-bot Oct 1, 2018

line too long (80 > 79 characters)

SUPPORT_TARGET_TEMPERATURE,
SUPPORT_AWAY_MODE,
SUPPORT_OPERATION_MODE)
from homeassistant.const import TEMP_CELSIUS, TEMP_FAHRENHEIT, ATTR_TEMPERATURE

This comment has been minimized.

@houndci-bot

houndci-bot Oct 1, 2018

'homeassistant.const.TEMP_CELSIUS' imported but unused

_LOGGER.debug("set_temperature start data=%s", kwargs)
hass.services.call(DOMAIN, SERVICE_SET_TEMPERATURE, kwargs)
@bind_hass

This comment has been minimized.

@houndci-bot

houndci-bot Oct 1, 2018

expected 2 blank lines, found 1

hass.services.call(DOMAIN, SERVICE_SET_AWAY_MODE, data)
@bind_hass

This comment has been minimized.

@houndci-bot

houndci-bot Oct 1, 2018

expected 2 blank lines, found 1

from homeassistant.const import (
ATTR_ENTITY_ID, ATTR_TEMPERATURE, SERVICE_TURN_ON, SERVICE_TURN_OFF,
STATE_ON, STATE_OFF, STATE_UNKNOWN, TEMP_CELSIUS, PRECISION_WHOLE,
PRECISION_TENTHS, TEMP_FAHRENHEIT )

This comment has been minimized.

@houndci-bot

houndci-bot Oct 1, 2018

whitespace before ')'

@w1ll1am23

This comment has been minimized.

Collaborator

w1ll1am23 commented Oct 2, 2018

When I run ./script/gen_requirments_all.py i get the following error.

******* ERROR
Errors while importing:  homeassistant.components.google_assistant.auth
Make sure you import 3rd party libraries inside methods

I manually made the update to the file but tarvis doesn't seem to like that.

@awarecan

This comment has been minimized.

Contributor

awarecan commented Oct 2, 2018

FYI, there is a thread discuss about water_heater: home-assistant/architecture#59

homeassistant.components.google_assistant.auth this file already been deleted from dev branch, you still have a local copy?

@bind_hass
def set_away_mode(hass, away_mode, entity_id=None):

This comment has been minimized.

@balloob

balloob Oct 2, 2018

Member

Drop these methods, we have just deleted them from the rest of the code.

This comment has been minimized.

@w1ll1am23

w1ll1am23 Oct 2, 2018

Collaborator

So do I need to create a common.py in the test like climate is doing or just remove these can call the services directly?

# Describes the format for available climate services
set_aux_heat:
description: Turn auxiliary heater on/off for climate device.

This comment has been minimized.

@balloob

balloob Oct 2, 2018

Member

climate?

description: Name(s) of entities to change.
example: 'climate.water_heater'
sensibo_assume_state:

This comment has been minimized.

@balloob
@@ -176,8 +176,7 @@
})
WINK_COMPONENTS = [
'binary_sensor', 'sensor', 'light', 'switch', 'lock', 'cover', 'climate',
'fan', 'alarm_control_panel', 'scene'
'water_heater'

This comment has been minimized.

@balloob

balloob Oct 2, 2018

Member

This seems like a mistake.

This comment has been minimized.

@w1ll1am23

w1ll1am23 Oct 2, 2018

Collaborator

Opps yeah I was using that while testing so I didn't get all of my Wink devices pulled in. Good catch.

@@ -526,6 +525,7 @@ def siren_service_handle(service):
discovery.load_platform(hass, wink_component, DOMAIN, {}, config)
component = EntityComponent(_LOGGER, DOMAIN, hass)
return True

This comment has been minimized.

@balloob
@balloob

This comment has been minimized.

Member

balloob commented Oct 2, 2018

Overall this looks good.

Could you move wink and econet platforms to each their own PR, to be submitted after the main component + tests + demo platform has been merged.

Will also need a PR for the developer website to document the new entity type in the architecture section.

@w1ll1am23

This comment has been minimized.

Collaborator

w1ll1am23 commented Oct 2, 2018

@balloob Sounds good I'll move the econet and wink stuff out of this and address the other comments.

@w1ll1am23

This comment has been minimized.

Collaborator

w1ll1am23 commented Oct 2, 2018

@awarecan yeah I think I have had that happen before. If I run python3 setup.py install it doesn't delete old stuff in the build folder. I guess I need to wipe it out and try again.

@MartinHjelmare

I didn't look at tests.

cv.has_at_least_one_key(
ATTR_TEMPERATURE),
{
vol.Exclusive(ATTR_TEMPERATURE, 'temperature'): vol.Coerce(float),

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 3, 2018

Member

This seems not updated. This key should be required.

vol.Required(ATTR_AWAY_MODE): cv.boolean,
})
SET_TEMPERATURE_SCHEMA = vol.Schema(vol.All(
cv.has_at_least_one_key(

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 3, 2018

Member

Remove this.

return self.current_operation
if self.is_on:
return STATE_ON
return STATE_UNKNOWN

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 3, 2018

Member

Return None.

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

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 3, 2018

Member

Stale docstring.

"""Set new target temperature."""
raise NotImplementedError()
def async_set_temperature(self, **kwargs):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 3, 2018

Member

Make this a coroutine.

This comment has been minimized.

@w1ll1am23

w1ll1am23 Oct 3, 2018

Collaborator

Like this?

This method must be run in the event loop and returns a coroutine.
"""
return self.hass.async_add_job(self.turn_away_mode_off)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 3, 2018

Member

See above.

def async_set_temperature(self, **kwargs):
"""Set new target temperature.
This method must be run in the event loop and returns a coroutine.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 3, 2018

Member

Update docstring. This shouldn't return a coroutine.

def set_temperature(self, **kwargs):
"""Set new target temperatures."""
if kwargs.get(ATTR_TEMPERATURE) is not None:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 3, 2018

Member

This will be a required argument, so we can remove this check.

description: Name(s) of entities to change.
example: 'water_heater.water_heater'
temperature:
description: New target temperature for HVAC.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 3, 2018

Member

Stale description.

example: 'water_heater.water_heater'
operation_mode:
description: New value of operation mode.
example: Eco

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 3, 2018

Member

Don't capitalize eco.

async def async_set_operation_mode(self, operation_mode):
"""Set new target operation mode."""
await self.hass.async_add_executor_job(self.set_operation_mode, operation_mode)

This comment has been minimized.

@houndci-bot

houndci-bot Oct 3, 2018

line too long (87 > 79 characters)

@w1ll1am23

This comment has been minimized.

Collaborator

w1ll1am23 commented Oct 3, 2018

@MartinHjelmare I think I have addressed all of your requests. Let me know if its okay. Thanks for reviewing!

@MartinHjelmare

Looks good! I think we should add some tests to make sure we cover all the possible states of the state property. Some of the cases are not covered yet. See the coveralls results.

@property
def state(self):
"""Return the current state."""
if self.current_operation:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

Looks like we don't need this check anymore.

Show resolved Hide resolved homeassistant/components/water_heater/demo.py

w1ll1am23 added some commits Sep 12, 2018

@home-assistant home-assistant deleted a comment from houndci-bot Oct 8, 2018

@MartinHjelmare

Looks good!

@MartinHjelmare MartinHjelmare merged commit f2d8f3b into home-assistant:dev Oct 8, 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.005%) to 93.509%
Details

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

@w1ll1am23 w1ll1am23 deleted the w1ll1am23:water_heater_support branch Oct 8, 2018

@w1ll1am23 w1ll1am23 referenced this pull request Oct 11, 2018

Merged

Moved econet from climate to water heater #17322

7 of 7 tasks complete

@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