Skip to content
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 webhook support for Netatmo Cameras #20755

Merged
merged 11 commits into from Feb 17, 2019

Conversation

@danielperna84
Copy link
Contributor

danielperna84 commented Feb 4, 2019

Description:

This PR updates the netatmo dependency (pyatmo) to version 1.8. I actually don't know what else has been changed, but I added helper methods to allow support for using webhooks for cameras. Update to 1.8 has already been done here. Webhook support is also what I have added in the component.
In its current form the webhook support only fires events on the bus like such:

Movement:

DEBUG (MainThread) [homeassistant.core] Bus:Handling <Event netatmo_webhook_received[L]: home_name=myhome, event_type=movement, webhook_id=mysupersecretwebhookid, camera_id=00:11:22:33:44:55>

Person:

DEBUG (MainThread) [homeassistant.core] Bus:Handling <Event netatmo_webhook_received[L]: home_name=myhome, message=John seen, event_type=person, id=aaaaaaaa-bbbb-1111-a1a1a1-abcdef123456, webhook_id=mysupersecretwebhookid, name=John, camera_id=00:11:22:33:44:55, is_known=True>

I chose not to interact with the sensors this component (optionally) generates because there are no off-events for when motion stops or a person hat not been seen for a while. Besides that we also need periodic updates for the required refresh token to be refreshed (pyatmo does that on every update).

I have tested this with a Netatmo Welcome camera, and the existing functionality has not been broken. I also have the weather station with it's various sensors, and they seem to keep working like before as well. For other device types I can't make any statements. If anything breaks, that's due to changes in pyatmo I'm not aware of. But I doubt that there will be any trouble.

I'll take care of the documentation as soon as this PR generally gets accepted.

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

Example entry for configuration.yaml (if applicable):

netatmo:
  api_key: YOUR_CLIENT_ID
  secret_key: YOUR_CLIENT_SECRET
  username: YOUR_USERNAME
  password: YOUR_PASSWORD
  webhook_url: https://hass.example.com/api/webhook/mysupersecretwebhookid

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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.
Show resolved Hide resolved homeassistant/components/netatmo/__init__.py Outdated

danielperna84 added some commits Feb 5, 2019

@danielperna84

This comment has been minimized.

Copy link
Contributor Author

danielperna84 commented Feb 6, 2019

I'll probably also change the types of events that are being fired. They should be different depending on what type of event actually happened in my opinion because that also affects what data is available.

@andersonshatch

This comment has been minimized.

Copy link
Contributor

andersonshatch commented Feb 6, 2019

This will only support the Welcome camera event types, Presence will send event types of movement, human, vehicle, and animal.
Let me know if it will be helpful to provide the body to any of those, I can certainly do that.

@danielperna84

This comment has been minimized.

Copy link
Contributor Author

danielperna84 commented Feb 6, 2019

Sure, that would be great.

@andersonshatch

This comment has been minimized.

@danielperna84

This comment has been minimized.

Copy link
Contributor Author

danielperna84 commented Feb 8, 2019

@andersonshatch
I have added the events. The possible events on the bus are now:

  • netatmo_person
  • netatmo_movement
  • netatmo_human
  • netatmo_animal
  • netatmo_vehicle
  • netatmo_other

Except from the netatmo_person event all events contain the following data:

  • event_type
  • home_name
  • camera_id
  • message
  • snapshot_url (missing for person detection)
  • vignette_url (missing for person detection)

For netatmo_person there will be an event for each person with the additional data:

  • id
  • name
  • is_known
  • face_url

The netatmo_other event will be used for all other possible events we might not know of yet or will be added without our knowledge. I have seen topology_changed in the past, which happened when I took it off the power and plugged it back in. There may be other interesting events, and with this solution we are able to monitor them, and eventually add the important ones here as dedicated events.

@danielperna84

This comment has been minimized.

Copy link
Contributor Author

danielperna84 commented Feb 10, 2019

I have now also added services to add and drop the webhooks. Adding webhooks is particulary useful because they can get banned. Being able to (re)add the webhook via service removes the need to restart Home Assistant in case the webhook has been banned.
It's also possible to define an URL with the addwebhook service. The catch is, that the handle_webhook callback won't be subscribed during setup. So people who manually define a webhook as a workaround have to manually work with the received data.

@danielperna84

This comment has been minimized.

Copy link
Contributor Author

danielperna84 commented Feb 10, 2019

@andersonshatch
Could you please try out my latest changes? The automation code I have posted above (now removed) did not work for some reason. I have to admit that I have copied an automation that did something similar and just adjusted it without properly testing it.

What I thought would work (but does not):

- alias: "Daniel Welcome"
  trigger:
    platform: event
    event_type: netatmo_person
    event_data:
      name: Daniel

I do get the matching event. But for some reason the automation won't fire. 😕

EDIT:
I probably was just stupid because I had a condition that prevents sending me Telegram notifications during the night. Which is a problem when developing at night. 😆

This did work:

- alias: "Welcome Movement"
  trigger:
    platform: event
    event_type: netatmo_movement

danielperna84 added some commits Feb 10, 2019

@andersonshatch

This comment has been minimized.

Copy link
Contributor

andersonshatch commented Feb 10, 2019

Other than the one set of changes I annotated, it seems to work okay, though I only tested so far with a dummy event of one type.
I'm confused as to whether I would need to call netatmo.add_webhook when I have:

netatmo:
  ...
  webhooks: true

Is it expected that the webhook is not registered until the service is called? So I would need to create an automation to call this service after hass startup?

@danielperna84

This comment has been minimized.

Copy link
Contributor Author

danielperna84 commented Feb 10, 2019

No, you don't need to call the service. I first added it to be able add and drop during runtime while testing this. In practice it will be irrelevant for me, except if the webhook got banned. In that case you have to go to the dev-panel from Netatmo and unban the app. Afterwards you would need to restart Home Assistant to subscribe again. Using the service lets you do that without restarting Home Assistant.
On top of that the webhooks: true only really works with a generic exposed setup. It probably won't work for people with special reverse proxy setups where the auto-generates URL is incorrect. These users then could manually create a working webhook they can work with and add that one by using the service. So essentially it can be ignored by the mayjority of users, but a few wouldn't be excluded just because they expose HASS as usual.

I'll take care of the documentation now.

@danielperna84

This comment has been minimized.

Copy link
Contributor Author

danielperna84 commented Feb 10, 2019

@andersonshatch

Could you please give me a description of what snapshot_url and vignette_url are for the documentation?

@danielperna84 danielperna84 referenced this pull request Feb 10, 2019

Merged

Add documentation for Netatmo webhooks #8463

2 of 2 tasks complete
@andersonshatch

This comment has been minimized.

Copy link
Contributor

andersonshatch commented Feb 10, 2019

Sure!
Snapshot_url points to the full frame image captured at the time of that event, whereas vignette_url points to a cropped+zoomed image that focuses on the thing in that event, so if it was an animal event for example, the vignette image will show you just the part the camera believed was an animal.

@danielperna84

This comment has been minimized.

Copy link
Contributor Author

danielperna84 commented Feb 11, 2019

@balloob
To me this seems fine now. Do you agree? Documentation has been taken care of as well.

@danielperna84 danielperna84 changed the title Update Netatmo, add webhook support Add webhook support for Netatmo Cameras Feb 13, 2019

@danielperna84 danielperna84 merged commit 847711d into home-assistant:dev Feb 17, 2019

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 93.344%
Details

@wafflebot wafflebot bot removed the in progress label Feb 17, 2019

@danielperna84 danielperna84 deleted the danielperna84:netatmo branch Feb 17, 2019

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Please open a new PR where we can address the comments.

@@ -61,9 +106,88 @@ def setup(hass, config):
for component in 'camera', 'sensor', 'binary_sensor', 'climate':
discovery.load_platform(hass, component, DOMAIN, {}, config)

if config[DOMAIN][CONF_WEBHOOKS]:
webhook_id = hass.components.webhook.async_generate_id()

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 17, 2019

Member

We shouldn't call an async function from a sync context. Probably use hass.add_job to schedule the async function.

@@ -61,9 +106,88 @@ def setup(hass, config):
for component in 'camera', 'sensor', 'binary_sensor', 'climate':
discovery.load_platform(hass, component, DOMAIN, {}, config)

if config[DOMAIN][CONF_WEBHOOKS]:
webhook_id = hass.components.webhook.async_generate_id()
NETATMO_WEBHOOK_URL = hass.components.webhook.async_generate_url(

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 17, 2019

Member

See above.

webhook_id = hass.components.webhook.async_generate_id()
NETATMO_WEBHOOK_URL = hass.components.webhook.async_generate_url(
webhook_id)
hass.components.webhook.async_register(

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 17, 2019

Member

See above.

"""Handle webhook callback."""
body = await request.text()
try:
data = json.loads(body) if body else {}

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 17, 2019

Member

Why not use:

data = await request.json()

def setup(hass, config):
"""Set up the Netatmo devices."""
import pyatmo

global NETATMO_AUTH
global NETATMO_AUTH, NETATMO_WEBHOOK_URL

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 17, 2019

Member

We should not use global. If this needs to be accessed from another module, use hass.data to store it.

@@ -106,6 +230,12 @@ def get_camera_type(self, camera=None, home=None, cid=None):
home=home, cid=cid)
return self.camera_type

def get_persons(self):
"""Gather person data for webhooks."""
global NETATMO_PERSONS

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 17, 2019

Member

Don't use global.

@danielperna84 danielperna84 referenced this pull request Feb 17, 2019

Merged

Netatmo, address comments from #20755 #21157

3 of 3 tasks complete

danielperna84 added a commit that referenced this pull request Feb 19, 2019

Netatmo, address comments from #20755 (#21157)
Netatmo component cleanup

@balloob balloob referenced this pull request Mar 6, 2019

Merged

0.89.0 #21712

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.