-
Notifications
You must be signed in to change notification settings - Fork 42
Add methods to start and stop detection #14
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just a few comments. I assume you've tested this on your own cameras?
Tested it with my doorbell. I bet the cameras would return different properties. Also, one interesting thing, switching to Home/Away in the app under one user wouldn't affect any properties as seen by another user (haven't checked with the same user). |
You're referring to the "schedule" tab? I can confirm it does update for the other user. Thanks for the PR! |
So, the arm/disarm feature is per user, not per device? Any ETA as to when to expect the change in PyPI? |
I'm not sure to be honest. All I know is i have one doorbell and 2 accounts. I currently enable/disable the motion sensing using the security tab. I switch between "home" and "disarmed." I don't know how this would play if i had accounts with different cameras on it, but with 2 accounts and a single shared camera when I change the setting on one it affects the other. I don't know how the enable/disable the actual motion detection setting would work. |
And in terms of PyPI, that's @bachya 's domain. If he's busy I can give it a whirl tonight but no promises. |
My current understanding is that the motion detection is the camera setting and affects all users, while Security tab in the app (with Home/Away/Disarmed/Schedule options) is per user. |
Hmm, that's not my experience but again I'm just dealing with a single camera so my results may be different |
@nonsleepr I neglected to notice this, but there aren't tests for your new code. I'm okay pushing 0.2.0 without them, but should you get the time, perhaps you could write some? |
Okay, 0.2.0 is published. |
My bad with the tests. Would spend more time on that in other PRs. |
Is |
Yes: https://pypi.org/project/python-eufy-security/ – I accidentally published to a different name. |
Will the HA PR be updated or can we simply alter the requirements in manifest.json and restart? Would like to help test and have another cam coming on Friday. :) |
I'm not sure about PR, I personally feel it's quite far from the state where it could have been merged into HA. You can use my version compatible with HACS though: https://github.com/nonsleepr/ha-eufy-security. |
FWIW, I have a draft HASS PR (home-assistant/core#28443), but I agree with @nonsleepr: we have a ways to go before this is ready to be un-drafted. @rpitera, FYI, your idea about adjusting |
It's adjusted in my repo. @Bacha, do you want to take ownership of it? Or maybe we could create a team? |
@nonsleepr - I've been using your repo manually; had no idea it was HACS compatible. I'll add the repo in HACS and use that. BTW guys, thanks for your hard work on this. I'm pretty happy with the results so far. Enough that I went out and ordered a third camera. |
@nonsleepr We shouldn’t have separate, disparate repos attacking the same problem. I’d be open to an organization if this repo and yours do fundamentally different things. |
I think for now while we're still ironing out issues, having @nonsleepr 's separate repo is convenient to easily update via HACS. Once we get it merged to HASS officially I think the repo won't be needed, but until then it might get more people testing as it's more convenient/easy. |
It's not the same problem: we have a client library and an integration library. |
Add methods
async_stop_detection
andasync_start_detection
to the camera object.