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 Hydrawise component #14055

Merged
merged 11 commits into from May 26, 2018

Conversation

@ptcryan
Contributor

ptcryan commented Apr 23, 2018

Description:

The hydrawise component allows you to integrate your Hunter Hydrawise Wi-Fi irrigation controller system in Home Assistant.

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml (if applicable):

# Example configuration.yaml entry
hydrawise:
  access_token: YOUR_API_KEY

switch:
  - platform: hydrawise
    watering_minutes: 5

sensor:
  - platform: hydrawise

binary_sensor:
  - platform: hydrawise

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 dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.
@homeassistant

This comment has been minimized.

homeassistant commented Apr 23, 2018

Hi @ptcryan,

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!

@fabaff

A couple of quick comments to get started. Also, please address ptcryan/hydrawiser#1

Support for Hydrawise cloud.
For more details about this platform, please refer to the documentation at
https://home-assistant.io/components/hydrawise

This comment has been minimized.

@fabaff

fabaff May 9, 2018

Member

/ missing at he end of the link.

This comment has been minimized.

@ptcryan

ptcryan May 18, 2018

Contributor

Fixed.

'next_cycle': '',
'status': '',
'watering_time': 'min',
}

This comment has been minimized.

@fabaff

fabaff May 9, 2018

Member

I would suggest to merge those dicts to something like

'auto_watering': ['Automatic Watering', 'mdi:autorenew', '']
....

Easier to maintain in the future.

This comment has been minimized.

@ptcryan

ptcryan May 18, 2018

Contributor

Done.

_LOGGER.debug("Updating Hydrawise binary sensor: %s", self._name)
mydata = self.hass.data['hydrawise'].data
if self._sensor_type == 'status':
self._state = mydata.status == 'All good!'

This comment has been minimized.

@fabaff

fabaff May 9, 2018

Member

Should be True or False, it's a binary sensor.

This comment has been minimized.

@ptcryan

ptcryan May 18, 2018

Contributor

The expression always evaluates to True or False. However, I changed to an if/else to make it clearer.

This comment has been minimized.

@ptcryan

ptcryan May 18, 2018

Contributor

Changed back to self._state = mydata.status == 'All good!' otherwise fails lint.

This comment has been minimized.

@ptcryan

ptcryan May 23, 2018

Contributor

Is there any feedback on this? The expression always resolves to True or False.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 23, 2018

Member

That is fine. Don't change it.

'mdi:cloud-outline' if self.is_on else 'mdi:cloud-off-outline')
elif self._sensor_type == 'rain_sensor':
return 'mdi:water' if self.is_on else 'mdi:water-off'
return ICON_MAP.get(self._sensor_type)

This comment has been minimized.

@fabaff

fabaff May 9, 2018

Member

Replace def icon(self): with def device_class(self): and use one from the defined DEVICE_CLASSES.

This comment has been minimized.

@ptcryan

ptcryan May 18, 2018

Contributor

Done.

sensors.append(HydrawiseBinarySensor(zone_data, sensor_type))
add_devices(sensors, True)
return True

This comment has been minimized.

@fabaff

fabaff May 9, 2018

Member

Not needed.

This comment has been minimized.

@ptcryan

ptcryan May 18, 2018

Contributor

Done.

class HydrawiseSwitch(HydrawiseEntity, SwitchDevice):
"""A switch implementation for hydrawise device."""

This comment has been minimized.

@fabaff

fabaff May 9, 2018

Member

See above.

This comment has been minimized.

@ptcryan

ptcryan May 18, 2018

Contributor

Fixed.

_LOGGER = logging.getLogger(__name__)
PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Optional(CONF_MONITORED_CONDITIONS, default=list(SWITCHES)):

This comment has been minimized.

@fabaff

fabaff May 9, 2018

Member

SWITCHES is defined as list.

This comment has been minimized.

@ptcryan

ptcryan May 18, 2018

Contributor

Fixed.

return {
ATTR_ATTRIBUTION: CONF_ATTRIBUTION,
'identifier': self.data.get('relay'),
'last contact:': temp

This comment has been minimized.

@fabaff

fabaff May 9, 2018

Member

See above.

This comment has been minimized.

@ptcryan

ptcryan May 18, 2018

Contributor

Same as above.

SWITCHES = ['auto_watering', 'manual_watering']
SCAN_INTERVAL = timedelta(seconds=20)

This comment has been minimized.

@fabaff

fabaff May 9, 2018

Member

Is there a benefit to go with 20 s instead of the default of 30 s?

This comment has been minimized.

@ptcryan

ptcryan May 18, 2018

Contributor

No benefit. I changed back to 30.

_LOGGER = logging.getLogger(__name__)
PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Optional(CONF_MONITORED_CONDITIONS, default=list(BINARY_SENSORS)):

This comment has been minimized.

@fabaff

fabaff May 9, 2018

Member

BINARY_SENSORS is a list: BINARY_SENSORS = ['is_watering', 'status', 'rain_sensor']

This comment has been minimized.

@ptcryan

ptcryan May 18, 2018

Contributor

Fixed.

@ptcryan

This comment has been minimized.

Contributor

ptcryan commented May 9, 2018

Thanks! I'll get started on these.

@ptcryan ptcryan changed the title from Added the Hydrawise component. to WIP Added the Hydrawise component. May 18, 2018

@ptcryan ptcryan changed the title from WIP Added the Hydrawise component. to Added the Hydrawise component. May 18, 2018

@property
def device_class(self):
"""Return the device class of the sensor type."""
return DEVICE_MAP.get(self._sensor_type)[

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 18, 2018

Member

If the sensor type is missing from the device map, this line will error. Don't use dict.get if you're sure that the key exists in the dict. If you're not sure about that, you need to handle the case of a possible missing key.

This comment has been minimized.

@ptcryan

ptcryan May 19, 2018

Contributor

OK. Removed the .get

self._sensor_type = sensor_type
self._name = "{0} {1}".format(
self.data.get('name'),
DEVICE_MAP.get(self._sensor_type)[

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 18, 2018

Member

Same as above.

This comment has been minimized.

@ptcryan

ptcryan May 19, 2018

Contributor

Got it.

async_dispatcher_connect(
self.hass, SIGNAL_UPDATE_HYDRAWISE, self._update_callback)
def _update_callback(self):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 18, 2018

Member

Decorate this with @callback imported from core.py.

This comment has been minimized.

@ptcryan

ptcryan May 19, 2018

Contributor

Done.

def _update_callback(self):
"""Call update method."""
self.schedule_update_ha_state(True)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 18, 2018

Member

Change this to async_schedule_update_ha_state.

This comment has been minimized.

@ptcryan

ptcryan May 19, 2018

Contributor

Got it.

@property
def unit_of_measurement(self):
"""Return the units of measurement."""
return DEVICE_MAP.get(self._sensor_type)[

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 18, 2018

Member

See similar comments above.

This comment has been minimized.

@ptcryan

ptcryan May 19, 2018

Contributor

Got this one too.

_LOGGER.debug("Updating Hydrawise switch: %s", self._name)
if self._sensor_type == 'manual_watering':
if mydata.running is None or not mydata.running:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 18, 2018

Member

The first check isnt needed when the second check is there.

This comment has been minimized.

@ptcryan

ptcryan May 19, 2018

Contributor

Fixed.

elif self._sensor_type == 'auto_watering':
self.hass.data['hydrawise'].data.suspend_zone(
0, (self.data.get('relay')-1))
self._state = True

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 18, 2018

Member

State will be changed by the update method which will be called directly after the call to this method in the service call. So you can remove this line.

This comment has been minimized.

@ptcryan

ptcryan May 19, 2018

Contributor

Done.

elif self._sensor_type == 'auto_watering':
self.hass.data['hydrawise'].data.suspend_zone(
365, (self.data.get('relay')-1))
self._state = False

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 18, 2018

Member

Same as above.

This comment has been minimized.

@ptcryan

ptcryan May 19, 2018

Contributor

And done.

return {
ATTR_ATTRIBUTION: CONF_ATTRIBUTION,
"identifier": self.data.get('relay'),
"last contact": temp

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 18, 2018

Member

Use snakecase lowercase strings for attributes.

This comment has been minimized.

@ptcryan

ptcryan May 19, 2018

Contributor

Fixed.

@property
def icon(self):
"""Return the icon to use in the frontend, if any."""
return DEVICE_MAP.get(self._sensor_type)[

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 18, 2018

Member

See comments about device map access.

This comment has been minimized.

@ptcryan

ptcryan May 19, 2018

Contributor

Got this one too.

@ptcryan

This comment has been minimized.

Contributor

ptcryan commented May 18, 2018

Ready for (re)review!

@ptcryan ptcryan changed the title from Added the Hydrawise component. to WIP Added the Hydrawise component. May 18, 2018

@ptcryan ptcryan changed the title from WIP Added the Hydrawise component. to Added the Hydrawise component. May 19, 2018

@ptcryan

This comment has been minimized.

Contributor

ptcryan commented May 19, 2018

Thanks @fabaff and @MartinHjelmare for your time and support. You guys are awesome!

Hopefully this is good to go now.

DATA_HYDRAWISE, DEFAULT_WATERING_TIME, HydrawiseEntity, SWITCHES,
DEVICE_MAP, DEVICE_MAP_INDEX)
from homeassistant.components.switch import PLATFORM_SCHEMA, SwitchDevice
from homeassistant.const import (

This comment has been minimized.

@houndci-bot

houndci-bot May 23, 2018

'homeassistant.const.ATTR_ATTRIBUTION' imported but unused

ptcryan added some commits May 24, 2018

@home-assistant home-assistant deleted a comment from houndci-bot May 24, 2018

if sensor['name'] == 'Rain':
self._state = sensor['active'] == 1
elif self._sensor_type == 'is_watering':
if mydata.running is None or not mydata.running:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 24, 2018

Member

The first check isn't needed when the second check is there.

This comment has been minimized.

@ptcryan

ptcryan May 25, 2018

Contributor

Thanks. I should have caught these from the earlier review.

def setup(hass, config):
"""Set up the Hunter Hydrawise component."""
conf = config[DOMAIN]
access_token = conf.get(CONF_ACCESS_TOKEN)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 24, 2018

Member

Don't use dict.get(key) for required config items, use dict[key].

This comment has been minimized.

@ptcryan

ptcryan May 25, 2018

Contributor

Fixed.

return {
ATTR_ATTRIBUTION: CONF_ATTRIBUTION,
'identifier': self.data.get('relay'),
'last_contact': temp

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 24, 2018

Member

Isn't this time the same as last_updated state attribute? If so, please remove this attribute here.

This comment has been minimized.

@ptcryan

ptcryan May 25, 2018

Contributor

last_contact is the time the Hydrawise cloud service last connected to the irrigation controller. That's not always the same as last_updated.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 25, 2018

Member

So a hydrawise state can change without contact to the cloud? What about the other way around, has a state always changed after the cloud contacted the controller?

This comment has been minimized.

@ptcryan

ptcryan May 25, 2018

Contributor

I see your point. I removed last_contact.

mydata = self.hass.data['hydrawise'].data
_LOGGER.debug("Updating Hydrawise sensor: %s", self._name)
if self._sensor_type == 'watering_time':
if mydata.running is None or not mydata.running:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 24, 2018

Member

See above.

This comment has been minimized.

@ptcryan

ptcryan May 25, 2018

Contributor

Yup.

if relay.get('nicetime') == 'Not scheduled':
self._state = 'not_scheduled'
else:
self._state = relay.get('nicetime').split(',')[0] + \

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 24, 2018

Member

What time is this and how often does it change?

This comment has been minimized.

@ptcryan

ptcryan May 25, 2018

Contributor

This time (nicetime) indicates the next time the irrigation zone will run. If the user configures the Hydrawise cloud service to determine the next time the zone will run then nicetime is set by the cloud. Weather and soil conditions will dynamically change when the value of nicetime.

If the user has HA turn on irrigation zones then nicetime will reflect when the zone will run under manual control.

def turn_on(self, **kwargs):
"""Turn the device on."""
if self._sensor_type == 'manual_watering':
self.hass.data['hydrawise'].data.run_zone(

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 24, 2018

Member

Use DATA_HYDRAWISE as key.

This comment has been minimized.

@ptcryan

ptcryan May 25, 2018

Contributor

Replaced all occurrences.

"""Turn the device on."""
if self._sensor_type == 'manual_watering':
self.hass.data['hydrawise'].data.run_zone(
self._default_watering_timer, (self.data.get('relay')-1))

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 24, 2018

Member

The subtraction will error if the key relay isn't found in the data dict. Don't use dict.get if you know that the key is in the dict.

This comment has been minimized.

@ptcryan

ptcryan May 25, 2018

Contributor

Fixed this in all occurrences in all files.

@MartinHjelmare MartinHjelmare changed the title from Added the Hydrawise component. to Add Hydrawise component May 25, 2018

@MartinHjelmare

Great!

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented May 25, 2018

Not sure why hound failed. I'll hold a bit on the merge to see what's up.

@ptcryan

This comment has been minimized.

Contributor

ptcryan commented May 26, 2018

Is there something I need to do? I thought Hound passed when I pushed that last commit. I'm not sure what to do next?

@ptcryan ptcryan closed this May 26, 2018

@ptcryan ptcryan reopened this May 26, 2018

@MartinHjelmare MartinHjelmare merged commit dfd7ef1 into home-assistant:dev May 26, 2018

4 of 5 checks passed

Hound We've encountered an error while reviewing your code.
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.1%) to 93.993%
Details

iMicknl added a commit to iMicknl/home-assistant that referenced this pull request May 31, 2018

Add Hydrawise component (home-assistant#14055)
* Added the Hydrawise component.

* Fixed lint errors.

* Multiple changes due to review comments addressed.

* Simplified boolean test. Passes pylint.

* Need hydrawiser package version 0.1.1.

* Added a docstring to the device_class method.

* Addressed all review comments from MartinHjelmare.

* Changed keys to single quote. Removed unnecessary duplicate method.

* Removed unused imports.

* Changed state to lowercase snakecase.

* Changes & fixes from review comments.

@balloob balloob referenced this pull request Jun 8, 2018

Merged

0.71.0 #14876

cyberjacob pushed a commit to cyberjacob/home-assistant that referenced this pull request Sep 4, 2018

Add Hydrawise component (home-assistant#14055)
* Added the Hydrawise component.

* Fixed lint errors.

* Multiple changes due to review comments addressed.

* Simplified boolean test. Passes pylint.

* Need hydrawiser package version 0.1.1.

* Added a docstring to the device_class method.

* Addressed all review comments from MartinHjelmare.

* Changed keys to single quote. Removed unnecessary duplicate method.

* Removed unused imports.

* Changed state to lowercase snakecase.

* Changes & fixes from review comments.

@home-assistant home-assistant locked and limited conversation to collaborators Sep 5, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.