Rachio (Sprinklers) by Klikini · Pull Request #7600 · home-assistant/core

Klikini · 2017-05-15T00:05:52Z

Description:

~~Pull request in~~ Content for (I don't exactly know how it works) home-assistant.github.io with documentation: https://github.com/Klikini/home-assistant.github.io/issues/2

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

Example entry for `configuration.yaml`:

switch:
  - platform: rachio
    access_token: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
    manual_run_mins: 10

Checklist:

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:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable.
New dependencies are only imported inside functions that use them.
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.
Fix this bug for is_on

homeassistant · 2017-05-15T00:05:54Z

Hi @Klikini,

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!

houndci-bot · 2017-05-15T00:05:57Z

+        headers = self.r.device.stopWater(self._device.device_id)[0]
+        self.update(no_throttle=True)
+        return headers
+


blank line at end of file

Klikini · 2017-05-15T00:10:10Z

rachiopy uses the MIT license

balloob

You've checked the boxes in your PR that you added the file to .coveragerc and ran script/gen_requirements_all.py. Both those changes should be part of this PR too.

balloob · 2017-05-15T06:06:20Z

+# Set up the component
+# noinspection PyUnusedLocal
+def setup_platform(hass, config, add_devices, discovery_info=None):
+    global _IRO


Don't use global, use hass.data[DATA_RACHIO ] instead, which is a dict for this purpose.

balloob · 2017-05-15T06:06:46Z

+from homeassistant.components.switch import SwitchDevice, PLATFORM_SCHEMA
+from homeassistant.const import CONF_ACCESS_TOKEN
+
+REQUIREMENTS = ['https://github.com/Klikini/rachiopy'


Can you push your dependency to PyPi?

It's not mine, it's a fork of https://github.com/rfverbruggen/rachiopy/.

If this fork is intended to stay as a permanent code, could you push it to pypi?

balloob · 2017-05-15T06:08:32Z

+    _LOGGER.debug("Configuring Rachio API...")
+    from rachiopy import Rachio
+    r = Rachio(access_token)
+    person = _get_person(r)


Will you be able to verify that the given API token is valid?

balloob · 2017-05-15T06:10:09Z

+
+
+# Represents one Rachio Iro
+class RachioIro(object):


I would have expected this class to live in RachioPy

RachioPy is not my own creation, and in fact includes these classes, however, they only contain static methods and are otherwise useless. I'll add these methods to my fork of the library.

balloob · 2017-05-15T06:11:20Z

+            return False
+
+    # Pull updated zone info from the Rachio API
+    @util.Throttle(MIN_UPDATE_INTERVAL, MIN_FORCED_UPDATE_INTERVAL)


Why would you throttle this? The device property that fetches the data is already throttled.

Oh, okay. I didn't know how it works. This file contains every existing line of Python that I've written in my life.

👍

balloob · 2017-05-15T06:12:05Z

+
+        _LOGGER.info("Watering {} for {} sec".format(self.name, seconds))
+        headers = self.r.zone.start(self.zone_id, seconds)[0]
+        self.update(no_throttle=True)


Home Assistant will automatically call update on your entity after calling turn_on or turn_off

balloob · 2017-05-15T06:13:01Z

+    def is_on(self):
+        self._device.update(propagate=False)
+        schedule = self._device.current_schedule
+        if 'zoneId' in schedule:


get returns the value of a dictionary or None if not found. So you can replace this if…else with return self.zone_id == schedule.get('zoneId')

balloob · 2017-05-15T06:13:36Z

+    global manual_run_mins
+
+    # Get options
+    manual_run_mins = config.get(CONF_manual_run_mins) or manual_run_mins


This config parameter is not referenced in PLATFORM_SCHEMA

balloob · 2017-05-15T06:13:46Z

+CONF_manual_run_mins = 'manual_run_mins'
+manual_run_mins = 60
+
+STATUS_ONLINE = 'ONLINE'


This is not being used

balloob · 2017-05-15T06:13:56Z

+
+_LOGGER = logging.getLogger(__name__)
+
+DOMAIN = 'switch'


Not being used.

balloob · 2017-05-15T06:16:10Z

Also, your documentation PR should be a PR against our documentation repository at https://github.com/home-assistant/home-assistant.github.io

balloob

Alright, already looking a lot better! Found some more things that need fixing.

balloob · 2017-05-17T05:50:21Z

+# Set up the component
+# noinspection PyUnusedLocal
+def setup_platform(hass, config, add_devices, discovery_info=None):
+    global manual_run_mins


Please do not use global

balloob · 2017-05-17T05:52:18Z

+
+    # Get options
+    manual_run_mins = config.get(CONF_MANUAL_RUN_MINS) or manual_run_mins
+    _LOGGER.debug("Rachio run time is " + str(manual_run_mins) + " min")


This, among other things, is one of the many lint issues that CI is reporting that need to be fixed: https://travis-ci.org/home-assistant/home-assistant/jobs/233033184#L271

balloob · 2017-05-17T05:52:42Z

+    # Get access token
+    _LOGGER.debug("Getting Rachio access token...")
+    access_token = config.get(CONF_ACCESS_TOKEN)
+    if not access_token:


This is impossible, it's marked as required in your PLATFORM_SCHEMA

balloob · 2017-05-17T05:55:00Z

+
+    @property
+    def name(self):
+        return self._zone['name'] or "Zone " + self.number


If _zone['name'] does not exist, a KeyError will be raised. Use _zone.get('name') instead to get None back.

👍

Looks like this was unnecessary anyway. The API returns a placeholder name already, so I don't need to make one.

balloob · 2017-05-17T05:59:18Z

+        self._zone = self.r.zone.get(self._zone_id)[1]
+
+        # Possibly update device
+        if propagate:


Your entity is set to be polled for updates by Home Assistant (because you do not overwrite the should_poll property). Each entity will individually call _device.update(), which is fine because Throttle will take care of it only getting called once.

So you can remove the whole propagation of updates.

balloob · 2017-05-17T05:59:28Z

@@ -0,0 +1 @@
+[]


Please remove this file.

houndci-bot · 2017-05-18T22:18:08Z

+    def update(self):
+        """Pull updated device info from the Rachio API."""
+        self._device = self.rachio.device.get(self._device_id)[1]
+        self._running = self.rachio.device.getCurrentSchedule(self._device_id)[1]


line too long (81 > 79 characters)

balloob · 2017-05-23T22:17:24Z

Please remove the polymer submodule from your commits.

balloob

Ok to merge when polymer change has been removed from pr.

Klikini · 2017-05-24T00:23:15Z

And for everything not yet handled by this component (read: most things), the iframe feature works well with the Rachio webapp:

Klikini · 2017-05-24T23:28:38Z

~~I just remembered I made a change in my fork of RachioPy to get around an issue for testing and I need to figure out how to verify HTTP SSL certs in a thread before this merges.~~

DO NOT MERGE YET, it could create the opportunity for a man-in-the-middle attack between the HomeAssistant instance and Rachio's API.

fabaff · 2017-05-29T09:09:13Z

Docs: home-assistant/home-assistant.io#2723

Klikini · 2017-06-01T03:08:09Z

I know this is closed, but I wanted to write it here so I can remember.

The switch shows the correct state when a schedule is running:

But it doesn't detect manual runs (so it appears to switch off after being turned on)

brg468 · 2017-06-01T16:13:03Z

I found an issue with the way manual runs are handled from within HA. Your turn_on method is multiplying manual_run_secs by 60, but that number is already minutes multiplied by 60 during the init, which is causing the manual_run_min to be multiplied by 3600. Putting any number longer than 3 in for manual run wont work because its over the max of 10800 seconds (that's how I discovered the problem). Obviously removing one of those multiplications fixes the problem.
Glad you got this up and running!

 def __init__(self, rachio, device, zone_id, manual_run_mins):
        """Initialize a new Rachio Zone."""
        self.rachio = rachio
        self._device = device
        self._zone_id = zone_id
        self._zone = None
        self._manual_run_secs = manual_run_mins * 60

 def turn_on(self):
        """Start the zone."""
        # Convert minutes to seconds
        seconds = self._manual_run_secs * 60

        # Stop other zones first
        self.turn_off()

        _LOGGER.info("Watering %s for %d sec", self.name, seconds)
        self.rachio.zone.start(self.zone_id, seconds)

Klikini · 2017-06-01T22:41:14Z

f001724

scadagenius · 2017-06-03T16:55:45Z

Nice addition to HA. Hope in future we will have rain sensor input as well.

brg468 · 2017-06-04T02:45:30Z

You fixed the double convert, but in doing so you removed the 'seconds' variable, which is still trying to be called by the turn_on method. 😄

Klikini · 2017-06-04T17:56:26Z

@scadaguru At some point I need to break out the actual device/zone code into a platform, and then replace the switch code with calls to that. Once that's done, it'll be a easier to add sensors with it as well.

scadagenius · 2017-06-04T18:00:44Z

@Klikini that will be so nice. I am already enjoying now. Google Home will be watering my landscape now!
Thanks

Klikini · 2017-06-04T18:01:33Z

@brg468 https://github.com/Klikini/home-assistant/commit/b12b7b4bc82d5cbceb7e1acdfc88691a3f7240ea

mririgoyen · 2017-06-05T13:14:36Z

@Klikini FYI: https://community.home-assistant.io/t/rachio-in-0-46-0/19185

Looks like there are some pretty serious problems with this component.

Rachio platform started

2d78019

homeassistant added new-platform platform: switch.rachio labels May 15, 2017

homeassistant added the cla-needed label May 15, 2017

houndci-bot reviewed May 15, 2017

View reviewed changes

homeassistant added cla-signed and removed cla-needed labels May 15, 2017

balloob requested changes May 15, 2017

View reviewed changes

Rachio tests

461b8a5

homeassistant added the cla-signed label May 15, 2017

detect bad api token

8bd8590

balloob requested changes May 17, 2017

View reviewed changes

Documentation, Code cleanup

f5040cf

balloobbot added platform requirement labels May 18, 2017

Andy Castille added 2 commits May 18, 2017 17:16

Docstrings end with a period, log uses %

c398b66

Merge remote-tracking branch 'refs/remotes/home-assistant/dev' into dev

d2f7370

homeassistant added the cla-signed label May 18, 2017

houndci-bot reviewed May 18, 2017

View reviewed changes

Andy Castille added 2 commits May 18, 2017 17:41

Fix arguments, default run time is now 10 minutes

0e443bd

Fix typo, remove todo (GH issue exists)

78540d3

robbiet480 added the cla-recheck label May 20, 2017

homeassistant added cla-signed and removed cla-recheck labels May 20, 2017

balloob approved these changes May 23, 2017

View reviewed changes

Revert polymer submodule commit

8a9be6e

Andy Castille added 2 commits May 24, 2017 18:54

Use a RachioPy version with SSL cert validation

f3e4b69

Update requirements

0777cb3

fabaff mentioned this pull request May 29, 2017

Add initial Rachio docs home-assistant/home-assistant.io#2723

Merged

fabaff merged commit fc1bb58 into home-assistant:dev May 29, 2017

balloob mentioned this pull request Jun 2, 2017

0.46 #7866

Merged

Klikini mentioned this pull request Jun 5, 2017

Rachio update #7922

Closed

5 tasks

home-assistant locked and limited conversation to collaborators Sep 4, 2017

ghost added integration: rachio and removed platform: switch.rachio labels Mar 21, 2019


		_LOGGER = logging.getLogger(__name__)

		DOMAIN = 'switch'

Uh oh!

Conversation

Klikini commented May 15, 2017 • edited by fabaff Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml:

Checklist:

Uh oh!

homeassistant commented May 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Klikini commented May 15, 2017

Uh oh!

balloob left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Klikini May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob commented May 15, 2017

Uh oh!

balloob left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Klikini May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob commented May 23, 2017

Uh oh!

balloob left a comment

Choose a reason for hiding this comment

Uh oh!

Klikini commented May 24, 2017

Uh oh!

Klikini commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Klikini commented May 15, 2017 •

edited by fabaff

Loading

Example entry for `configuration.yaml`:

Klikini May 15, 2017 •

edited

Loading

Klikini May 17, 2017 •

edited

Loading

Klikini commented May 24, 2017 •

edited

Loading

brg468 commented Jun 4, 2017 •

edited

Loading