Update PIR triggered from sensor to binary sensor #1490
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1490 +/- ##
=======================================
Coverage 92.66% 92.66%
=======================================
Files 150 150
Lines 9538 9538
Branches 974 974
=======================================
Hits 8838 8838
Misses 499 499
Partials 201 201 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Sounds indeed reasonable, thanks for the PR @alams154! I marked this initially as a breaking change, but removed the label as 1) we were already returning a boolean and 2) this was not ever exposed in homeassistant users, where we would have an issue.
@ryenitcher what do you think?
|
If we’re thinking about HA this should really be an |
Will this be a change in the type or require a change somewhere else? My motivation for this change was to add a corresponding change in binary_sensor.py so we could see the motion detection in HA using a KS200M. Right now I'm using the Kasa app for motion detection and migrating all the functionality of the switch to HA would be awesome. |
|
See this HA architecture discussion regarding this. It seems the question in HA is around whether the duration of motion is known or not to determine whether something should be an event or a binary_sensor. Quite how we would model an event-like feature in python-kasa I'm not too sure at the moment but probably requires registering some kind of callback via the feature. |
|
Probably best to close the PR then? |
|
Maybe let's mark it as draft until we've figured out the best api for events. |
I’m not certain I know enough about HA to have any valid input on this. However, I will note that the PIR state reading feature works via low rate polling of the switch, so it’s not exactly event driven. So it’s not reporting an event, per se, but rather a snapshot of a state that may, by chance, contain an event. |
|
As we are polling currently every 5s, and based on my understanding that this state stays keeps reported for a while, I think we should merge this and worry about events when we are ready for that. This way users can already use this, or what so you think, @sdb9696? |
|
I think we should wait a bit until we’ve thrashed out the event api a bit more. We should incorporate this into that discussion, but I’d envisaged the motion query running automatically every second once a listener is started. That’s thinking based around the |
For the two Kasa motion switches I have access to, the ADC value reported by the API is not latching, and reports real-time values only. For continuous motion events, low rate polling will likely pick up the event, so long as the that motion triggering the event is generated continuously. However, for short lived motion events, low rate polling will more oft than not miss the event entirely. Further, it is worth noting that there is no mechanism exposed within the API itself to distinguish a long motion event from multiple short motion events in close temporal proximity. |
Is there an issue somewhere that has this discussion? I'd like to follow any updates on that discussion and if there's anything I could do to help. |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Binary sensor may better fit this entity since Home Assistant classifies motion into the following device class BinarySensorDeviceClass.MOTION