tchellomello/python-arlo

Arlo Baby Support

Closed this issue · 6 comments

Creating this issue to gauge interest in the additional Arlo Baby functionality. Typically I would just jam out some code and submit a PR, but it appears that there would be a considerable refactor involved to keep the reusable code clean, and I would like to have open discussion with the maintainer @tchellomello to make sure this jives with his vision for the module and that downstream packages such as Home Assistant have an easy (if not transparent) migration path.

Why Refactor?

There's a big chunk of functionality in the ArloBaseStation class that maintains a server-side event stream socket to query certain bits of information about the base station. With the Arlo Baby camera API, it uses the same pattern (at a different endpoint) to get these bits of information:

  • Ambient air temperature
  • Ambient air quality (VOC detection)

The API also exposes pub/sub for the following actions:

  • Audio/music functionality (play, pause, skip, shuffle, volume control, timer, etc.)
  • Nightlight functionality (on, off, color, timer, etc.)
  • Alert configuration

Having these exposed to Home Assistant would be extremely helpful in that you could trigger HVAC events in response to air temperature alarms, play music when motion is detected, start your air purifier in response to air quality alarms, and more.

Proposal

There are a couple ways we could tackle this.

  1. Extract the SSE client into a base class and have both ArloBaseStation and ArloCamera extend the new base class that has the subscribe/notify functionality.
  2. Extract the SSE client into PyArlo itself and manage these extra bits of sensor data and device RPC separate from the domain classes through a specific get_sensor_data(sensor_type) (or similar) method.

In the former case, we could expose these items as properties (as they are now) and safeguard by checking to see if the camera is of the correct type. Alternatively, we could provide a map of available metrics/sensors on the ArloCamera class.

Again, I'm happy to contribute this, I just wanted community and maintainer input before actioning. Cheers!

Another thing occurred to me after thinking about it for a bit... we could abstract all of this up a layer, and preserve this module as-is, handling the compilation of sensor data directly from the ArloBaseStation class into a DTO in the Arlo Home Assistant code.

Hello @lukiffer,
Thanks for your post. I definitely agree with you that is time for a code refactoring in this library.
The work on this library started with the first Arlo camera products but rapidly Arlo released new products and as all the code is based on reverse engineering we will need to keep up the code as developers buy the devices to add support to it.

To agree with your plan and let's start a code refactoring for this. I've started something offline already but all means let's move this forward.

Thanks for your interest in collaborating and let's make this better.

@broox @jwillaz @chaddotson @viswa-swami @ryanwinter what do you guys think?
mmello

I think would be a good plan to get all the developers or users that want to help on this and create a product matrix so then we will who can test what and how. What are your thoughts on that?

@tchellomello I'm happy to test the Arlo Baby stuff, so feel free to add me to that matrix. This is my first foray into the Netgear/Arlo products (I've previously been a Nest fanboy).

As for the refactoring bits, after digging a bit deeper, I noticed that they treat the Arlo Baby as its own base station. So a refactor on the scale I was thinking before is unnecessary, we can simply return the Arlo Baby device as both an ArloCamera and ArloBaseStation. The night light controls were fairly straight forward. The audio playback features required a small refactor of the __run_action() method to accommodate actions like playTrack and pause where before we assumed it would only be one of get or set. I think there are more opportunities for refactoring here, but I just left them as TODO comments.

Ambient sensor data was a bit trickier. It's returned from their event in a base64-encoded, zlib-compressed byte array, and the format property of the event describes byte offsets for extracting fields. This is a little crazy, but keeps the payload size down I guess...

I'll get stuff tidied up and send over a PR.

#86

It wouldn't let me request a PR review, but let me know if anyone has feedback on the code and if it's good with everyone, I'll add the unit tests.

Fixed by #86