💥 Support for general motion sensor

[NickM-27]

💥 BREAKING CHANGE 💥

• Renames entities previously known as binary_sensor.kitchen_person_motion to binary_sensor.kitchen_person_occupancy. The old name was a misnomer. All automations and templates that used the old entity_id must be updated manually.
• Introduces a new motion sensor entity (not associated with objects): binary_sensor.kitchen_motion. This entity is disabled by default.

Motion Sensor

Parent frigate PR: blakeblackshear/frigate#3152

This adds support for a true motion sensor.

Open Questions

1. I know (from other integrations / requests) that users will likely not work well with the direct data coming from frigate which will rapidly switch between True / False. Happy to provide specific examples, but it seems users will likely expect this motion sensor to behave in the fashion where it will be set to True on motion and then reset after no motion is seen for some period of time. I am open to suggestions on how to approach / implement this.
2. I'm not super sure about the naming, as the other sensors are already called "motion" despite not being directly related to motion. I think "{cam_name} General Motion" is decent but am open to suggestions.

[dermotduffy]

We'll be forever stuck in motion vs occupancy pain unless we bite the bullet here. Maybe better to get that over with? Don't really fancy being stuck with a new set of suboptimal names forever.

Option 1: Abandon old name

• Re-name the existing misnamed motion sensor from binary_sensor.kitchen_person_motion to binary_sensor.occupancy_kitchen_person.
• Have this PR use binary_sensor.motion_kitchen_person
• The old entity (binary_sensor.kitchen_person_motion) gets auto-removed by this PR.

Consequences:

• Breaks existing automations bluntly & directly through the removal of entity.
• Acceptable long-term naming.

Option 2: Re-use old name

• Re-name the existing misnamed motion sensor from binary_sensor.kitchen_person_motion to binary_sensor.kitchen_person_occupancy.
• Have this PR re-use the now free name binary_sensor.kitchen_person_motion.

Consequences:

• Breaks existing automations more subtley (entity still exists, but behaves entirely differently. May spam users through excessive triggering of 'true' motion sensor).
• Slightly [IMO] better naming for the long-term.

Option 3: Stick with poor naming, at least for now

• Use a name as the PR currently suggests, e.g. binary_sensor.kitchen_person_general_motion or binary_sensor.kitchen_person_real_motion, etc.

Consequences:

• No user breakage.
• Perpetual low-level of understable user confusion (example).
• Stuck with bad names for now, or forever.

I favor option 1, along with a major version number bump (v3): lets get the change over with. Either way it's a breaking change, so @blakeblackshear for opinions or alternatives.

[NickM-27]

@dermotduffy I agree, and am onboard with option 1. I'll outline some of my thoughts:

• I agree it needs to happen at some point and might as well be now with all the recent changes to bump to 3.0 and have this breaking change which will reduce future confusion.
• option 2 is difficult since the general motion is object agnostic, so unless we made specific redundant sensors that would be confusing (person_motion actives despite no person being there), it would not be that much better.

Separately, do you have any thoughts on the delayed motion sensor issue?

[dermotduffy]

option 2 is difficult since the general motion is object agnostic, so unless we made specific redundant sensors that would be confusing (person_motion actives despite no person being there), it would not be that much better.

Good point! That means there's an option 4 (current sensor binary_sensor.kitchen_person_motion):

• Motion: binary_sensor.kitchen_motion
• Occupancy: binary_sensor.kitchen_person_occupancy

I think that's slightly nicer naming than Option 1, and still doesn't re-use the entity name.

Separately, do you have any thoughts on the delayed motion sensor issue?

We should not implement a timer that "latches" motion for some period of time and then resets it -- we want to avoid creating/pretending state in HA if at all possible, if that state is not "real" as per the backend. So either, we should use an entity (like the PR currently does) if Frigate can detect both motion and "end of motion" (regardless of how frequently that fires), or (if Frigate cannot actually detect end of motion) we shouldn't use an entity at all and should instead generate an event (example) that can be used as an automation trigger. Users can generate their own latching sensors if they absolutely need them with something like this.

[blakeblackshear]

We can definitely determine the end of motion in frigate. You can wait until x frames or an amount of time before clearing the sensor. That could also be configurable.

[NickM-27]

Good point! That means there's an option 4 (current sensor binary_sensor.kitchen_person_motion):

• Motion: binary_sensor.kitchen_motion
• Occupancy: binary_sensor.kitchen_person_occupancy

I think that's slightly nicer naming than Option 1, and still doesn't re-use the entity name.

Interesting, how would the binary_sensor.kitchen_person_motion behavior work? Would it be the same as the occupancy sensor and same as it is today?

I think I'm still leaning towards option 1 just so it isn't too cluttered and the behavior fits the name as expected, but perhaps I am not 100% understanding how option 4 would work. Either way sounds like we're in favor of 1 or 4.

We can definitely determine the end of motion in frigate. You can wait until x frames or an amount of time before clearing the sensor. That could also be configurable.

Is that something that should be configured / implemented in this integration or on the Frigate side?

[blakeblackshear]

Is that something that should be configured / implemented in this integration or on the Frigate side?

On the frigate side, I think. Not all users of frigate use home assistant.

[dermotduffy]

Interesting, how would the binary_sensor.kitchen_person_motion behavior work? Would it be the same as the occupancy sensor and same as it is today?

There would be no binary_sensor.kitchen_person_motion. I think I expained it poorly.

Option 1:

• Real motion sensor (i.e. this PR): binary_sensor.motion_kitchen (no object here!)
• Real occupancy sensor (i.e. renamed version of current functionality): binary_sensor.occupancy_kitchen_person
• Entity name that would be auto-deleted: binary_sensor.kitchen_person_motion

Option 4:

• Real motion sensor (i.e. this PR): binary_sensor.kitchen_motion (no object here!)
• Real occupancy sensor (i.e. renamed version of current functionality): binary_sensor.kitchen_person_occupancy
• Entity name that would be auto-deleted: binary_sensor.kitchen_person_motion

(Only difference is whether motion / occupancy comes before or after the camera / object name).

On the frigate side, I think. Not all users of frigate use home assistant.

Agree. As mentioned above, it's also better to avoid Home Assistant from 'inventing' state that may not be real, i.e. it's better for Frigate to decide what constitutes "a camera with motion" and have the integration just faithfully use that state for the entity.

[NickM-27]

Ah I see, then I think option 4 makes more sense from a naming perspective. If we're good with that I can start updating this PR. I already updated the frigate PR to include configurable off delay 👍

[NickM-27]

It seems that there isn't a good way to test a sensor that starts as disabled as it can't await the entity registry call and doesn't seem to be updated in time when checking for the entity_state

otherwise, everything looks good with new names

[dermotduffy]

A nit: I was expecting these would be occupancy class rather than presence class sensors. In HA, presence is typically home/away, vs 'occupied' which I think is more in line with what we're doing here.

Reference

[NickM-27]

A nit: I was expecting these would be occupancy class rather than presence class sensors. In HA, presence is typically home/away, vs 'occupied' which I think is more in line with what we're doing here.

Reference

Gotcha yeah that makes sense, I was stuck between the two, will update

[NickM-27]

@dermotduffy Do you have any experience testing the entity which is disabled by default? Struggling to see how I could do both, but probably needs to be disabled by default unless we wait to release 3.0.0 to full release until frigate 0.11 is full release

I think in the long term this new motion sensor is definitely going to be wanted enabled by default

[NickM-27]

Parent PR has been merged

[NickM-27]

@dermotduffy I think I need help with this. Followed the reference but that only checked for the icon. To meet coverage I need to test that the mqtt message was received, but it seems despite the

async_fire_mqtt_message(hass, mqtt_topic, "ON")
await hass.async_block_till_done()

it is still unavailable

[NickM-27]

Ohhhh interesting, that makes sense

[codecov]

Codecov Report

Merging #248 (9f1bbb1) into master (aaf7e13) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #248   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines         1390      1431   +41     
=========================================
+ Hits          1390      1431   +41     

Impacted Files	Coverage Δ
custom_components/frigate/__init__.py	100.00% <100.00%> (ø)
custom_components/frigate/binary_sensor.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aaf7e13...9f1bbb1. Read the comment docs.

[dermotduffy]

Hi @NickM-27 , this is looking great! Very excited about this change (and have a usecase in mind in the card, too).

One thing this PR is currently missing is cleanup: We need to remove the old "motion" entity, otherwise it'll hang on forever, confuse people and cause support load. Here is an example of how to do this. You'll also obviously need to add a test to ensure the cleanup works -- here's an example you can probably extend.

[NickM-27]

Oh cool I didn't realize integrations could do that, I'll work on that 👍

[NickM-27]

Not sure why that test is making other tests fail?

[NickM-27]

Oh wow, thanks for fixing that. Is this ready to merge?

[dermotduffy]

Thanks @NickM-27 !

[acshef]

Sorry for resurfacing this already merged PR; I'm one of the folks that had misinterpreted the old "motion" sensors. Now that they're correctly named "occupancy" sensors, what's the difference between binary_sensor.<zone>_<object>_occupancy and {{ sensor.<zone>_<object>_count > 0 }} ? What are the use cases where a template around the *_count entities isn't sufficient? I'm trying to determine two things:

• Whether we can reduce the number of (redundant) Home Assistant entities
• Better understand the entities currently provided by the integration so I can submit my own PR to introduce the missing motion sensor that some of us thought existed: binary_sensor.<zone>_motion

[NickM-27]

@acshef

Whether we can reduce the number of (redundant) Home Assistant entities

Technically the occupancy sensor could be replaced by just the count sensor but I don't really see the problem. It can easily be disabled and especially as we just had a breaking change here I don't think it would be good to have another breaking change to remove this and force people to rewrite the automations yet again.

Also that the behavior of these could be adjusted to be different from each other in the future.

Better understand the entities currently provided by the integration so I can submit my own PR to introduce the missing motion sensor that some of us thought existed: binary_sensor._motion

We already have a binary_sensor.<camera>_motion in the latest release. Frigate does not expose zone based motion as zones only apply to objects.

[acshef]

@NickM-27

the behavior of these could be adjusted to be different from each other in the future

Agreed, that's a very good callout.

Frigate does not expose zone based motion as zones only apply to objects.

Challenge accepted, I have my new lead to follow. 😎
Thanks for the input Nick!

[NickM-27]

Challenge accepted, I have my new lead to follow. 😎

To be clear it's on purpose that this isn't the case. Motion is much more arbitrary than objects. Especially since it's the bottom center of object bounding boxes that are considered for object presence in zone, and motion typically isn't going to fit in in the same way.