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

Add support for expiring retained messages. #3082

Merged
merged 4 commits into from
Mar 12, 2020
Merged

Conversation

dustin
Copy link
Contributor

@dustin dustin commented Mar 7, 2020

For most of my environmental monitoring use cases, I want the readings
retained so I can pick them up from clients at any time, but if the
sensor (or zigbee2mqtt) fails, I want the readings to go away so I can
tell the difference between a stale reading and a missing reading.

This is easily accomplished in MQTTv5 using the "message expiry
interval" property. To add that to zigbee2mqtt, I added a 'version'
option to the mqtt section so I can specify to connect with version 5
and added a 'retention' property to devices allowing me to specify how
long items should be retained.

e.g.

mqtt:
  base_topic: site/zigbee2mqtt
  server: 'mqtt://myserver'
  user: zigbee
  version: 5
serial:
  port: /dev/ttyACM0
devices:
  '0x00358d00022308da':
    friendly_name: someroom
    retain: true
    retention: 900

@branch-switcher branch-switcher bot changed the base branch from master to dev March 7, 2020 18:52
@branch-switcher
Copy link

Hello @dustin. The base branch of this pull request has been updated to the dev branch. Please revisit the changes and make sure that there are no conflicts with the new base branch. Thank you for your contributions.

@dustin
Copy link
Contributor Author

dustin commented Mar 7, 2020

Updated for dev branch.

@dustin
Copy link
Contributor Author

dustin commented Mar 7, 2020

It seems the tests are very sensitive to properties, requiring they never be present even when empty.

@dustin
Copy link
Contributor Author

dustin commented Mar 7, 2020

Added tests for the various retention bits.

@dustin
Copy link
Contributor Author

dustin commented Mar 7, 2020

Oddly, the test that required mqtt v5 wasn't enough to make coverage happy on the line that configures the connection for mqtt v5. Fixed.

@Koenkk
Copy link
Owner

Koenkk commented Mar 8, 2020

Thanks for the PR looks good.

One question: what happens if the user set retention for a device without setting MQTT version to 5?

@dustin
Copy link
Contributor Author

dustin commented Mar 8, 2020 via email

@Koenkk
Copy link
Owner

Koenkk commented Mar 9, 2020

In that case please add a check here: https://github.com/Koenkk/zigbee2mqtt/blob/master/lib/util/settings.js#L291 to throw an exception when retention is set for a device and version != 5.

Please also add this option to the schema: https://github.com/Koenkk/zigbee2mqtt/blob/master/lib/util/settings.js#L120

@dustin dustin force-pushed the master branch 3 times, most recently from 611c654 to 9b61c52 Compare March 12, 2020 16:17
@dustin
Copy link
Contributor Author

dustin commented Mar 12, 2020

I added schema definitions and validation and subsequent required testing to keep coverage up.

I'd also attempted to rebase this to upstream dev, but those tests aren't passing for me.

@dustin
Copy link
Contributor Author

dustin commented Mar 12, 2020

The latest test failed on a line that isn't in my code...

For most of my environmental monitoring use cases, I want the readings
retained so I can pick them up from clients at any time, but if the
sensor (or zigbee2mqtt) fails, I want the readings to go away so I can
tell the difference between a stale reading and a missing reading.

This is easily accomplished in MQTTv5 using the "message expiry
interval" property.  To add that to zigbee2mqtt, I added a 'version'
option to the mqtt section so I can specify to connect with version 5
and added a 'retention' property to devices allowing me to specify how
long items should be retained.

e.g.

    mqtt:
      base_topic: site/zigbee2mqtt
      server: 'mqtt://myserver'
      user: zigbee
      version: 5
    serial:
      port: /dev/ttyACM0
    devices:
      '0x00358d00022308da':
        friendly_name: someroom
        retain: true
        retention: 900
@dustin
Copy link
Contributor Author

dustin commented Mar 12, 2020

(I'm still not entirely sure how this integrates, but I added a friendly name to my test case and it made the upstream happy)

Koenkk added a commit to Koenkk/zigbee2mqtt.io that referenced this pull request Mar 12, 2020
@Koenkk
Copy link
Owner

Koenkk commented Mar 12, 2020

Thanks!

@Koenkk Koenkk merged commit 97a4b6b into Koenkk:dev Mar 12, 2020
Koenkk added a commit to Koenkk/zigbee2mqtt.io that referenced this pull request Mar 15, 2020
@Zer0x00
Copy link

Zer0x00 commented Apr 7, 2020

What does the value of retention (here for example 900) stands for?
Seconds? Minutes? Days?

@dustin
Copy link
Contributor Author

dustin commented Apr 7, 2020

MQTT expiry intervals are expressed in seconds.

Minutes could be reasonable, but days wouldn't be, as the unit both lacks granularity and consistency in duration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants