Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mqtt binary sensor expire after #26058

Merged

Conversation

@KiLLeRRaT
Copy link
Contributor

commented Aug 19, 2019

First PR, so go easy on me. Not quite sure about the tasks in the below template that I haven't checked/ticked yet. Can someone please have a look and let me know what else I need to do?

Cheers,

Description:

Added expire_after functionality to mqtt binary_sensor. Basically copied the functionality from the standard mqtt sensor, except that we se availability, instead of just clearing out the state.

Pull request with documentation for home-assistant.io: home-assistant/home-assistant.io#10203

Example entry for Zigbee2MQTT configuration.yaml:

homeassistant: true
permit_join: true
mqtt:
  base_topic: zigbee2mqtt
  server: 'mqtt://192.168.111.249'
  user: dev
  password: devdev
serial:
  port: \\.\COM4
devices:
  '0x00158d0003973656':
    friendly_name: '0x00158d0003973656'
    retain: false
    homeassistant:
      unique_id: 'binary_sensor.front_door_contact'
      expire_after: 3

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@KiLLeRRaT KiLLeRRaT requested a review from home-assistant/core as a code owner Aug 19, 2019
@homeassistant

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

Hi @KiLLeRRaT,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@KiLLeRRaT

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

Hmm not quite sure what to do with the 3 issues shown above...?

@emontnemery

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

@KiLLeRRaT Can you explain the difference between the new option expire_after and the already existing off_delay?

@KiLLeRRaT

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

@KiLLeRRaT Can you explain the difference between the new option expire_after and the already existing off_delay?

Hi @emontnemery,

My situation:

I have a Xiaomi PIR, and a Xiaomi door open/close sensor. These are paired to a CC2530/ESP8266 which is linked to a zigbee2mqtt instance.

Each of these sensors have a heartbeat (I believe it's every 51 minutes) which sends a 'I'm here' back to the mqtt (and thus Home Assistant).

My problem was that if I lose the link between CC2530/ESP8266 and zigbee2mqtt, Home Assistant keeps displaying the same state that it has been, e.g. no motion, or door closed.

The workaround for this on the zigbee2mqtt website is to send an expire_after attribute from zigbee2mqtt, which means whatever value is displayed, will essentially be cleared out after this timeout.

The problem I faced is this attribute/setting is only available on the mqtt sensor, and not on the mqtt binary_sensor. That is what this implementation is doing.

So the way you'd use this is, you would set the timeout to say 110 minutes (two hearbeat cycles). If Home Assistant hasn't heard anything in that time, the state of the sensor is set to 'unavailable' which would show up on your dashboard, and you can then do something about it.

Without this, it will always just show 'no motion' and the only way you'd find out that something is funky, is to walk up to the PIR, and realise, oh, it's not triggering.

I don't believe the off_delay you mentioned will help, because it will still show a state of 'off' which means 'no motion' or 'door closed'. We need a third state which attacts attention.

This is what my PR is implementing: expire_after

If these is another way to handle this, without this PR, I'd also be quite happy, but this seems to be pretty simple and means people can easily set this up without much faffing around.

Here is the original feature request for this too:
https://community.home-assistant.io/t/expire-after-option-for-mqtt-binary-sensors/45722

Thanks for taking a look

@KiLLeRRaT

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

So what are the next steps for this guys? How can I sort out those CI errors? Not sure how or where to do that...

If someone could explain, (with example) OR clean it up, that'd be great.

Cheers,

@frenck frenck added the docs-missing label Aug 21, 2019
@KiLLeRRaT KiLLeRRaT referenced this pull request Aug 22, 2019
2 of 2 tasks complete
@pszafer

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

If I could have my word, I'd rather use automation for such simple things as disconnect instead of adding something like expire to binary sensor...
I have at least 3 ideas for this:

  • automation
{{ relative_time(states.binary_sensor.front_door_contact.last_updated) }}
  • create second template binary sensor with HeartBeat
  • create mqtt cover of all of it and then you have as many states as you want:
cover:
  - platform: mqtt
    name: "MQTT Cover"
    state_topic: "home-assistant/cover/state" -> STATE OF DOORS
    availability_topic: "home-assistant/cover/availability" -> AVAILABILITY OF ZIGBEE
    state_open: "ON"
    state_closed: "OFF"
    payload_available: "online"
    payload_not_available: "offline"
    optimistic: false
    device_class: door
@KiLLeRRaT

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

If I could have my word, I'd rather use automation for such simple things as disconnect instead of adding something like expire to binary sensor...
I have at least 3 ideas for this:

  • automation
{{ relative_time(states.binary_sensor.front_door_contact.last_updated) }}
  • create second template binary sensor with HeartBeat
  • create mqtt cover of all of it and then you have as many states as you want:
cover:
  - platform: mqtt
    name: "MQTT Cover"
    state_topic: "home-assistant/cover/state" -> STATE OF DOORS
    availability_topic: "home-assistant/cover/availability" -> AVAILABILITY OF ZIGBEE
    state_open: "ON"
    state_closed: "OFF"
    payload_available: "online"
    payload_not_available: "offline"
    optimistic: false
    device_class: door

Hi @pszafer,

Thanks for the ideas. I can try those out. Can you let me know how you set up that automation. I'm a little confused by that part. What are the trigger/conditions/action parts?

As for the functionality, I thought it is a very clean way of doing it, since the MQTT sensor already supports this, and the zigbee2mqtt program also supports this. It bring consistency, and a very clean way of setting this up. I think it makes things much simpler, especially for newcomers.

Thanks!
Albert

@pszafer

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

Maybe use 2 automations.

1 automation:

Mqtt trigger -  https://www.home-assistant.io/docs/automation/trigger/#mqtt-trigger + 
Delay with numeric state - https://www.home-assistant.io/docs/automation/trigger/#numeric-state-trigger
Fire some event.

2 automation:

Trigger on event above,
Check your condition (this time related)
Delay some time
Trigger event again, so you make a loop.

You might use as well Binary sensor template.
https://www.home-assistant.io/components/binary_sensor.template/

If you need more help ping me on community forum or contact on discord as issues and PR are not so much good place for help

@emontnemery

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

@KiLLeRRaT OK, your explanation for the functionality makes sense.

For the CI errors, it's due to failing MQTT test cases after your changes.

Instead of waiting for CI, you can run the tests locally, follow instructions here:
https://developers.home-assistant.io/docs/en/development_testing.html#running-single-tests-using-tox
In your case,
py.test tests/components/mqtt/test_binary_sensor.py

To find why CI is failing, click first here:
image
Then here:
image

And finally here:
image

@emontnemery

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

@Koenkk Does zigbee2mqtt already use expire_after in MQTT auto discovery message for sensors, it only seems to be used in some test case?

@Koenkk

This comment has been minimized.

Copy link

commented Aug 27, 2019

@emontnemery by default its not used, but as can been seen in the OP users can configure it.

@KiLLeRRaT

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

EDIT: Ignore this post, FINALLY got it working after getting latest dev, installing all the py requirements again, and comitting. Phew.

Hmm sorted my functionality/tests but an unrelated test is failing.

Anyone know about:

ERROR    homeassistant.config:config.py:437 Invalid config for [sensor.sma]: "sensors" should be a simple list from 0.99. Got {'platform': 'sma', 'host': '1.1.1.1', 'password': '', 'custom': {'my_sensor': {'key': '1234567890123', 'unit': 'V'}}, 'sensors': {'current_consumption': ['current_consumption']}}. (See ?, line ?). Please check the docs at https://home-assistant.io/components/sensor.sma/

I guess I will try to update my branch from the latest to see if I can get it going.

@KiLLeRRaT KiLLeRRaT force-pushed the KiLLeRRaT:mqtt_binary_sensor_expire_after branch 2 times, most recently from 923763c to 025e363 Aug 30, 2019
@KiLLeRRaT KiLLeRRaT force-pushed the KiLLeRRaT:mqtt_binary_sensor_expire_after branch from 025e363 to 38ab799 Aug 31, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

I'd like some opinions from members if this option is a good idea.

@project-bot project-bot bot moved this from Needs review to Second opinion wanted in Dev Sep 3, 2019
@balloob

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

It is already part of the MQTT sensor so I don't see harm in adding it to the MQTT binary sensor too.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

Ok. @emontnemery if you agree, can you review?

homeassistant/components/mqtt/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/mqtt/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/mqtt/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/mqtt/binary_sensor.py Outdated Show resolved Hide resolved
tests/components/mqtt/test_binary_sensor.py Outdated Show resolved Hide resolved
tests/components/mqtt/test_binary_sensor.py Outdated Show resolved Hide resolved
tests/components/mqtt/test_binary_sensor.py Outdated Show resolved Hide resolved
tests/components/mqtt/test_binary_sensor.py Outdated Show resolved Hide resolved
Dev automation moved this from Second opinion wanted to Review in progress Sep 4, 2019
@emontnemery

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

The new functionality is good as such, but I don't understand why the availability functionality needs to be changed.

@KiLLeRRaT KiLLeRRaT referenced this pull request Sep 8, 2019
5 of 7 tasks complete
@KiLLeRRaT

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

Hi @emontnemery and @MartinHjelmare, please take a look at the latest and let me know what your thoughts are.

This use case would be to specify an availability topic (even though it won't be used) to get the available functionality.

If we have the expire_after setting enabled, when we receive an MQTT message, the device state is updated to available. The reason for this is that it will not be updated any other way. (Zigbee2MQTT doesn't have device specific availability topics, only for the bridge itself).

Cheers,

@KiLLeRRaT KiLLeRRaT requested review from emontnemery and removed request for home-assistant/core Sep 15, 2019
@emontnemery

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

@KiLLeRRaT Yeah, this is better! Still a few additional things:

  • Don't touch self._available, add new member self._expired instead
  • Override available(self) with something like this:
    @property
    def available(self) -> bool:
        """Return true if the device is available and value has not expired."""
        expire_after = self._config.get(CONF_EXPIRE_AFTER)
        return MqttAvailability.available and (expire_after is None or not self._expired)
  • Add a version of test case test_setting_sensor_value_expires where availability topic is not set

Edit: Sorry for the slow feedback, please feel free to ping me on Discord instead

@KiLLeRRaT

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

@KiLLeRRaT Yeah, this is better! Still a few additional things:

  • Don't touch self._available, add new member self._expired instead
  • Override available(self) with something like this:
    @property
    def available(self) -> bool:
        """Return true if the device is available and value has not expired."""
        expire_after = self._config.get(CONF_EXPIRE_AFTER)
        return MqttAvailability.available and (expire_after is None or not self._expired)
  • Add a version of test case test_setting_sensor_value_expires where availability topic is not set

Edit: Sorry for the slow feedback, please feel free to ping me on Discord instead

Hey @emontnemery ,

I have tried what you suggested, but it seems like there is something fishy with the available property you suggested. I think there is possibly something else interfering. To test this, I reverted all my changes out, then I run the tests, everything passed. I then simply add in a super simple wrapper for the available property you gave me, less the expired stuff:

    @property
    def available(self) -> bool:
        """Return true if the device is available and value has not expired."""
        return MqttAvailability.available

I'd expect all the tests to still pass, but now they fail.... To me that is saying that there is another available property somewhere causing issues?

You can test this by grabbing any of the latest branches (without anything I did).
Run py.test -xo log_cli=true tests/components/mqtt/test_binary_sensor.py

They all pass.

Then add the below to mqtt/binary_sensor.py

    @property
    def available(self) -> bool:
        """Return true if the device is available and value has not expired."""
        return MqttAvailability.available

Now the tests fail.

Any ideas or pointers?

Sorry to make this so painful for you, it's the first time I've ever touched any Python, so I'm figuring it out as I'm going.

Cheers,

@emontnemery

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

You need to do like this instead to get the property's value:

    @property
    def available(self) -> bool:
        """Return true if the device is available and value has not expired."""
        return MqttAvailability.available.fget(self)
KiLLeRRaT added 2 commits Sep 19, 2019
…perty to check expired
@KiLLeRRaT

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

You need to do like this instead to get the property's value:

    @property
    def available(self) -> bool:
        """Return true if the device is available and value has not expired."""
        return MqttAvailability.available.fget(self)

Excellent! Thanks for the help @emontnemery , I think I got it this time around :)

Let me know if there is anything else to be done.

Cheers,

Dev automation moved this from Review in progress to Reviewer approved Sep 20, 2019
@emontnemery

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

Looks good, can you please open a documentation PR?

@KiLLeRRaT

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

Looks good, can you please open a documentation PR?

Hi @emontnemery, Hmm I already did that. Do I need to do it another way?

It's linked at the top:

home-assistant/home-assistant.io#10203

Cheers,

@emontnemery

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2019

@KiLLeRRaT I missed the link to the docs, thanks.

@emontnemery emontnemery merged commit e394be7 into home-assistant:dev Sep 21, 2019
11 checks passed
11 checks passed
CI Build #20190919.84 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 100% of diff hit (target 93.84%)
Details
codecov/project 94.05% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Sep 21, 2019
@lock lock bot locked and limited conversation to collaborators Sep 22, 2019
@KiLLeRRaT KiLLeRRaT deleted the KiLLeRRaT:mqtt_binary_sensor_expire_after branch Oct 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
8 participants
You can’t perform that action at this time.