Closed lukiffer closed 6 years ago
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.
https://github.com/tchellomello/python-arlo/pull/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
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:The API also exposes pub/sub for the following actions:
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.
ArloBaseStation
andArloCamera
extend the new base class that has the subscribe/notify functionality.PyArlo
itself and manage these extra bits of sensor data and device RPC separate from the domain classes through a specificget_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!