Add RainMachine switch platform #8827

bachya · 2017-08-04T18:44:15Z

Description:

Adds support for RainMachine sprinkler controllers (via the Switch component).

Related issue (if applicable):

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

Example entry for `configuration.yaml` (if applicable):

switch:
  platform: rainmachine
  ip_address: 192.168.1.100
  password: my_password_123

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 (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.

MartinHjelmare

Overall looks good. Some minor comments.

MartinHjelmare · 2017-08-07T13:07:25Z

.coveragerc

@@ -143,6 +143,9 @@ omit =
    homeassistant/components/rachio.py
    homeassistant/components/*/rachio.py

+    homeassistant/components/rainmachine.py


Don't add this line since you don't add a component only a platform.

MartinHjelmare · 2017-08-07T13:08:13Z

.coveragerc

@@ -143,6 +143,9 @@ omit =
    homeassistant/components/rachio.py
    homeassistant/components/*/rachio.py

+    homeassistant/components/rainmachine.py
+    homeassistant/components/*/rainmachine.py


Replace this with homeassistant/components/switch/rainmachine.py and move the line to the switch group in alphabetical order.

MartinHjelmare · 2017-08-07T13:11:54Z

homeassistant/components/switch/rainmachine.py

+
+
+@asyncio.coroutine
+def async_setup_platform(hass, config, async_add_devices, discovery_info=None):


Move this function above the class definitions.

MartinHjelmare · 2017-08-07T13:26:34Z

homeassistant/components/switch/rainmachine.py

+            _LOGGER.error('Neither IP address nor email address given')
+            return False
+    except rm.exceptions.HTTPError as exec_info:
+        _LOGGER.error('HTTP error during authentication: %s', str(exec_info))


You should not have to convert exec_info into a string. The base Exception class in Python defines __str__.

MartinHjelmare · 2017-08-07T13:28:18Z

homeassistant/components/switch/rainmachine.py

+
+        async_add_devices(entities)
+    except rm.exceptions.HTTPError as exec_info:
+        _LOGGER.error('Request failed: %s', str(exec_info))


MartinHjelmare · 2017-08-07T13:56:12Z

homeassistant/components/switch/rainmachine.py

+
+    def __init__(self, client, zone_json, **kwargs):
+        """Initialize a RainMachine zone."""
+        super(RainMachineZone, self).__init__(client, zone_json, **kwargs)


super().__init__(client, zone_json, **kwargs)

MartinHjelmare · 2017-08-07T13:58:09Z

homeassistant/components/switch/rainmachine.py

+        except exceptions.HTTPError as exc_info:
+            _LOGGER.error('Unable to turn off zone "%s"',
+                          self.rainmachine_id)
+            _LOGGER.debug(str(exc_info))


Remove str conversion.

MartinHjelmare · 2017-08-07T13:58:22Z

homeassistant/components/switch/rainmachine.py

+        except exceptions.HTTPError as exc_info:
+            _LOGGER.error('Unable to turn on zone "%s"',
+                          self.rainmachine_id)
+            _LOGGER.debug(str(exc_info))


MartinHjelmare · 2017-08-07T15:48:34Z

homeassistant/components/switch/rainmachine.py

+MIN_SCAN_TIME_FORCED = timedelta(milliseconds=100)
+
+PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
+    vol.Optional(CONF_IP_ADDRESS):


Require at least one of the auth keys and make them exclusive, something like this:

PLATFORM_SCHEMA = vol.Schema(vol.All( cv.has_at_least_one_key(CONF_IP_ADDRESS, CONF_EMAIL), { vol.Required(CONF_PLATFORM): cv.string, vol.Optional(CONF_SCAN_INTERVAL): cv.time_period, vol.Exclusive(CONF_IP_ADDRESS, 'auth'): cv.string, vol.Exclusive(CONF_EMAIL, 'auth'): vol.Email, vol.Required(CONF_PASSWORD): cv.string, vol.Optional(CONF_ZONE_RUN_TIME, default=DEFAULT_ZONE_RUN_SECONDS): cv.positive_int, vol.Optional(CONF_HIDE_DISABLED_ENTITIES, default=True): cv.boolean } ), extra=vol.ALLOW_EXTRA)

Note that you can't extend the platform schema when you want to use a validator around the dict.

~~This is great, but I'm confused on how to utilize it? Without any extra changes, I get exceptions about my ip_address and email_address variables (set in async_setup_platform()) being functions.~~

Never mind, got it. 😄

MartinHjelmare · 2017-08-07T15:49:35Z

homeassistant/components/switch/rainmachine.py

+            _LOGGER.debug('Configuring remote API...')
+            auth = rm.Authenticator.create_remote(email_address, password)
+        else:
+            _LOGGER.error('Neither IP address nor email address given')


If you add the more extensive platform validation schema you can remove this.

houndci-bot · 2017-08-07T18:19:33Z

homeassistant/components/switch/rainmachine.py

+MIN_SCAN_TIME_REMOTE = timedelta(seconds=5)
+MIN_SCAN_TIME_FORCED = timedelta(milliseconds=100)
+
+PLATFORM_SCHEMA = vol.Schema(


redefinition of unused 'PLATFORM_SCHEMA' from line 10

bachya · 2017-08-07T18:22:47Z

Thanks, @MartinHjelmare – I've made your requested changes.

MartinHjelmare · 2017-08-07T18:41:33Z

I think the branch got jacked. 😜
If you know your git wushu, you can rebase and remove the stray commits that shouldn't be there. Otherwise there's always:
https://xkcd.com/1597/

Edit: Take for habit to work in a separate feature branch of your own, not directly on dev.

bachya · 2017-08-07T19:05:21Z

@MartinHjelmare: That's what I had done prior to this PR; not sure what happened. I'll fix shortly.

Edit: Alright, we should be in business now (once CI passes). 👍

MartinHjelmare · 2017-08-07T19:21:51Z

homeassistant/components/switch/rainmachine.py

-        super(RainMachineZone, self).__init__(client, zone_json, **kwargs)
-        self._run_time = kwargs.get(CONF_ZONE_RUN_TIME)
+        super().__init__(client, zone_json, **kwargs)
+        self._run_time = kwargs.get(zone_run_time)


The argument is already available now.

This change is already pushed:

Are you not seeing that? Wondering if I messed up the branch further...

That looks like what I have. But I think this line should be changed to this:

self._run_time = zone_run_time

Cause zone_run_time is now available as a positional argument.

Ah, great point. Updating shortly.

bachya · 2017-08-07T22:36:02Z

@MartinHjelmare: The lint CI step is failing with this error:

lint runtests: commands[1] | pylint homeassistant
************* Module homeassistant.components.switch.rainmachine
E: 41,21: No value for argument 'v' in function call (no-value-for-parameter)
ERROR: InvocationError: '/home/travis/build/home-assistant/home-assistant/.tox/lint/bin/pylint homeassistant'

That isn't anywhere in my code; guessing it might be in the platform schema definition. Any advice for me?

bachya · 2017-08-08T00:49:18Z

Found this: pylint-dev/pylint#818 – key point:

Yeah, this happens because we don't have currently a proper understanding of decorators, which voluptuous seems to use, but it's on our road map. Unfortunately, the only solution that I have for now is to recommend you disabling the error locally or in pylintrc.

Going to tell pylint to ignore that issue, then re-push. Holding tight...

Edit: Victory!

MartinHjelmare

Nice!

* Add RainMachine switch platform * Updated requirements_all.txt * Cleaning up CI and coverage results * Small update to deal with older pylint * Fixed small indentation-based error * Added some more defensive try/except logic around calls * I'm not a fan of importing a library multiple times :) * Making PR-requested changes * Fixed ref to positional parameter * Attempting to fix broken linting * Ignoring no-value-for-parameter pylint error

homeassistant added cla-signed new-platform platform: switch.rainmachine labels Aug 4, 2017

balloobbot added core platform requirement labels Aug 4, 2017

bachya mentioned this pull request Aug 4, 2017

Add documentation for the RainMachine switch platform home-assistant/home-assistant.io#3135

Merged

homeassistant added cla-signed labels Aug 4, 2017

MartinHjelmare requested changes Aug 7, 2017

View reviewed changes

bachya added 7 commits August 7, 2017 09:57

Add RainMachine switch platform

23268ca

Updated requirements_all.txt

b516697

Cleaning up CI and coverage results

a661193

Small update to deal with older pylint

3f0ff2d

Fixed small indentation-based error

1325d90

Added some more defensive try/except logic around calls

bef1758

I'm not a fan of importing a library multiple times :)

e070e54

balloobbot added the component label Aug 7, 2017

houndci-bot reviewed Aug 7, 2017

View reviewed changes

homeassistant added the cla-signed label Aug 7, 2017

Making PR-requested changes

4842642

homeassistant added the cla-signed label Aug 7, 2017

balloobbot removed the component label Aug 7, 2017

MartinHjelmare requested changes Aug 7, 2017

View reviewed changes

Fixed ref to positional parameter

26dd97c

homeassistant added the cla-signed label Aug 7, 2017

Attempting to fix broken linting

c7fa93c

Ignoring no-value-for-parameter pylint error

f1cc640

homeassistant added the cla-signed label Aug 8, 2017

MartinHjelmare approved these changes Aug 8, 2017

View reviewed changes

MartinHjelmare merged commit 289c88f into home-assistant:dev Aug 8, 2017

fabaff mentioned this pull request Aug 12, 2017

0.51 #8919

Merged

home-assistant locked and limited conversation to collaborators Dec 11, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RainMachine switch platform #8827

Add RainMachine switch platform #8827

bachya commented Aug 4, 2017 •

edited

Loading

MartinHjelmare left a comment

MartinHjelmare Aug 7, 2017

MartinHjelmare Aug 7, 2017

MartinHjelmare Aug 7, 2017

MartinHjelmare Aug 7, 2017

MartinHjelmare Aug 7, 2017

MartinHjelmare Aug 7, 2017

MartinHjelmare Aug 7, 2017

MartinHjelmare Aug 7, 2017

MartinHjelmare Aug 7, 2017 •

edited

Loading

bachya Aug 7, 2017 •

edited

Loading

MartinHjelmare Aug 7, 2017

houndci-bot Aug 7, 2017

bachya commented Aug 7, 2017

MartinHjelmare commented Aug 7, 2017 •

edited

Loading

bachya commented Aug 7, 2017 •

edited

Loading

MartinHjelmare Aug 7, 2017

bachya Aug 7, 2017

MartinHjelmare Aug 7, 2017

bachya Aug 7, 2017

bachya commented Aug 7, 2017

bachya commented Aug 8, 2017 •

edited

Loading

MartinHjelmare left a comment



		@asyncio.coroutine
		def async_setup_platform(hass, config, async_add_devices, discovery_info=None):

Add RainMachine switch platform #8827

Add RainMachine switch platform #8827

Conversation

bachya commented Aug 4, 2017 • edited Loading

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

MartinHjelmare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinHjelmare Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

bachya Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bachya commented Aug 7, 2017

MartinHjelmare commented Aug 7, 2017 • edited Loading

bachya commented Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bachya commented Aug 7, 2017

bachya commented Aug 8, 2017 • edited Loading

MartinHjelmare left a comment

Choose a reason for hiding this comment

bachya commented Aug 4, 2017 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

MartinHjelmare Aug 7, 2017 •

edited

Loading

bachya Aug 7, 2017 •

edited

Loading

MartinHjelmare commented Aug 7, 2017 •

edited

Loading

bachya commented Aug 7, 2017 •

edited

Loading

bachya commented Aug 8, 2017 •

edited

Loading