Skip to content

Rachio webhooks#15111

Merged
balloob merged 2 commits intohome-assistant:devfrom
Klikini:rachio-webhooks
Jul 1, 2018
Merged

Rachio webhooks#15111
balloob merged 2 commits intohome-assistant:devfrom
Klikini:rachio-webhooks

Conversation

@Klikini
Copy link
Copy Markdown
Contributor

@Klikini Klikini commented Jun 23, 2018

Description:

  • The Rachio component, previously only a switch component, is now broken up into a rachio component as well as switch and binary_sensor components.
  • Rather than polling the server every few seconds and hitting the newly-imposed rate limit, this implementation uses webhooks for instant updates with only a few API calls at startup and when manually toggling switches.
  • It is now much easier to continue expanding with new sensors and actions (schedules, anyone?) and possibly some neat UI features to remove the need for using the official app completely.
  • Note that this will require adding rachio to the configuration and moving the access_token option (renamed to api_key) to the new section, leaving manual_run_mins in the switch section. You may also need to specify hass_url_override if yoru instance is unaware of its actual web location (base_url). Port-forwarding or proxying may be needed in order for the Rachio API to be able to call [your instance]:8123/api/rachio.

Related issue (if applicable): fixes #12639

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

Example entry for configuration.yaml (if applicable):

rachio:
  access_token: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
  hass_url_override: http://home.robiotic.net:8123

switch:
  - platform: rachio
    manual_run_mins: 10

binary_sensor:
  - platform: rachio

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please rename the DOMAIN import: DOMAIN as DOMAIN_RACHIO

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, instead of \ you can wrap all values in (…)

Copy link
Copy Markdown
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.

Set should_poll property to False.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use our dispatch helper instead of the event bus to communicate between component and platforms.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please do not use the event bus for this. Use the dispatcher instead.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is weird. Why not control this by passing True as second parameter to add_devices instead. This will cause the update() method to be called, which is meant for polling an update

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

With _poll_update in the constructor, I can pass data that I already fetched in an earlier call. When HA calls update, it would have to fetch that data and use an API call (which now have a rate limit). After instantiation, updates are provided via webhooks only.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please have this entity set should_poll to False to avoid having Home Assistant try to update it regularly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How is this used?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure exactly what "this" is (and I'm not even writing JS...)

  1. The custom URL can be set in the config.
  2. If set, it overrides the base_url. If not, it stores the base_url.
  3. (Now replaced with generate_secret.) This is passed to the Rachio API and then provided with incoming webhooks to authenticate the connections.
  4. This assembles the hostname and path. The combine URL is then given to the Rachio API to call when events occur.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should under no circumstance send our API password to Rachio. Instead, generate a random token and have your web view implement that as authentication.

You can generate a token by using homeassistant.auth.generate_secret

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When would a custom url be necessary ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For cases where the base_url will not be accessible by Rachio's server, or is incorrect. I don't know how commonly this will happen, but I figured it should be possible after there were similar issues with another component.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These are kinda overkill. Just make the instance name self.username ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be .debug(…)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How does this work. How long will it send things over this webhook? Are we registering on each restart of Home Assistant ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It will send things over this webhook until it is deleted on shutdown. Yes, a new webhook is created on each restart, as this accounts for port/hostname changes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What happens if this callback does not get called? Is there a way we can query existing webhooks and make sure there is only 1 configured?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have added a line to remove all existing HA webhooks on startup. It should only be 1 extra startup API call if they were removed correctly on shutdown. In my testing, they have always been removed correctly at shutdown (even sending a series of ^C!)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

? Do you not just mean a 200?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, 204. The page is blank.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please don't override this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Implement should_poll: false

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove the polling option and send True as second param to add_devices so it will call the update method.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same explanation as #15111 (review)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doesn't every controller have 3 switches?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For now, yes. But I want it to be easy to add more switchable options in the future.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Dispatcher.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be .debug

@balloob balloob added this to the 0.73 milestone Jul 1, 2018
@balloob
Copy link
Copy Markdown
Member

balloob commented Jul 1, 2018

Awesome.

@balloob balloob merged commit 9db8759 into home-assistant:dev Jul 1, 2018
@ghost ghost removed the in progress label Jul 1, 2018
balloob pushed a commit that referenced this pull request Jul 1, 2018
* Make fewer requests to the Rachio API

* BREAKING: Rewrite Rachio component
else:
self._state = None

dispatcher_connect(hass, SIGNAL_RACHIO_CONTROLLER_UPDATE,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's safer to connect the state update handler in async_added_to_hass which will be scheduled after the device has been added to home assistant. If the signal is sent before the entity has been added to home assistant the handler will error since we connect before the entity has been added to home assistant.

name = url[1:].replace('/', ':')

# pylint: disable=no-self-use
@asyncio.coroutine
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Remove what, specifically?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@asyncio.coroutine

@balloob balloob mentioned this pull request Jul 6, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 2018
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.

After being online for a few days Rachio errors appear in the logs and component stops working

4 participants