Adding expire_after to mqtt sensor to expire outdated values #6708

micw · 2017-03-19T20:37:31Z

Description:

Related issue (if applicable): fixes #6705

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2297

Example entry for `configuration.yaml` (if applicable):

sensor:
  - platform: mqtt
    state_topic: "sensor/value"
    unit_of_measurement: "°C"
    expire_after: 1800

Checklist:

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

Documentation added/updated in home-assistant.github.io

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

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
no new dependencies or files

I extended the sensors/mqtt_test.py to cover the new feature.

mention-bot · 2017-03-19T20:37:32Z

@micw, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sfam, @fabaff and @balloob to be potential reviewers.

homeassistant · 2017-03-19T20:37:33Z

Hi @micw,

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!

houndci-bot · 2017-03-19T20:37:38Z

tests/components/sensor/test_mqtt.py

+        """ Not yet expired """
+        state = self.hass.states.get('sensor.test')
+        self.assertEqual('100', state.state)
+


blank line contains whitespace

houndci-bot · 2017-03-19T20:37:38Z

tests/components/sensor/test_mqtt.py

+
+        state = self.hass.states.get('sensor.test')
+        self.assertEqual('100', state.state)
+


blank line contains whitespace

houndci-bot · 2017-03-19T20:37:39Z

tests/components/sensor/test_mqtt.py

@@ -37,11 +38,49 @@ def test_setting_sensor_value_via_mqtt_message(self):
        fire_mqtt_message(self.hass, 'test-topic', '100')
        self.hass.block_till_done()
        state = self.hass.states.get('sensor.test')
-
+        


blank line contains whitespace

balloob · 2017-03-19T21:44:03Z

homeassistant/components/sensor/mqtt.py

@@ -65,6 +71,8 @@ def __init__(self, name, state_topic, qos, unit_of_measurement,
        self._unit_of_measurement = unit_of_measurement
        self._force_update = force_update
        self._template = value_template
+        self._expire_after = int(expire_after or 0)


Please use the config schema to convert it to an integer and set default value

balloob · 2017-03-19T21:44:10Z

homeassistant/components/sensor/mqtt.py

@@ -31,6 +35,7 @@
    vol.Optional(CONF_FORCE_UPDATE, default=DEFAULT_FORCE_UPDATE): cv.boolean,
 })

+SCAN_INTERVAL = timedelta(seconds=1)


Remove this.

balloob · 2017-03-19T21:45:18Z

homeassistant/components/sensor/mqtt.py

@@ -73,6 +81,9 @@ def async_added_to_hass(self):
        """
        @callback
        def message_received(topic, payload, qos):
+            """Reset expiration time."""
+            self._value_expiration_at = time.time() + self._expire_after


Please use the event helper async_track_point_in_utc_time to call an async function to set the state to None. async_track_point_in_utc_time will return a function that can be called to remove the listener. So on message received, remove the last listener and add a new time listener.

balloob · 2017-03-19T21:45:33Z

homeassistant/components/sensor/mqtt.py

-        """No polling needed."""
-        return False
+        """Polling needed only for auto-expire."""
+        return self._expire_after > 0


This should not be implemented with polling

balloob · 2017-03-19T21:47:03Z

tests/components/sensor/test_mqtt.py

+        state = self.hass.states.get('sensor.test')
+        self.assertEqual('100', state.state)
+
+        time.sleep(1)


You are never allowed to call time.sleep in tests. Please use the mock_fire_time_changed helper.

balloob · 2017-03-19T21:47:19Z

tests/components/sensor/test_mqtt.py

+        time.sleep(1)
+        # FIXME: how to call update() on the sensor?
+
+        """ Not yet expired """


Comments in Python start with a #

balloob · 2017-03-19T21:47:26Z

tests/components/sensor/test_mqtt.py

+        state = self.hass.states.get('sensor.test')
+        self.assertEqual('100', state.state)
+
+        time.sleep(2)


balloob · 2017-03-19T21:47:32Z

tests/components/sensor/test_mqtt.py

+        """ Expired """
+        state = self.hass.states.get('sensor.test')
+        # FIXME: this will fail unless the fixmes above are fixed
+#        self.assertEqual('unknown', state.state)


stale comment.

micw · 2017-03-20T13:42:25Z

@balloob Thank you very much for the good feedback. I'm new to python and new to home assistant development as well, so I'm glad that you point me in the right direction. I'll revise my code later today.
Can you give me a hint about this config scheme (string to int) stuff?

Regards,
Michael.

…; lint

micw · 2017-03-20T19:46:10Z

@balloob I tried to change everything according your comments. Coding part was easy, the lint stuff was the really hard part. Hope it's good now.

balloob · 2017-03-21T03:39:39Z

homeassistant/components/sensor/mqtt.py

+                    dt_util.utcnow() +
+                    timedelta(seconds=self._expire_after)
+                    )
+                async_track_point_in_utc_time(self.hass,


async_track_point_in_utc_time returns a function that will unsubscribe the just subscribed listener. So instead of storing a timestamp when to expire, unsubscribe the old listener if it exists and subscribe a new one.

Then in the callback, set the unsubscribe function to None

Will this be thread-safe? E.g. no race-condition between unsubscribing a (probably in exactly in this moment executed callback) and a new message?

Flow with new code:

Callback is executed in the moment the message comes in

Unsubscribe is executed -> too late to have effect

new value is set

already running Callback resets the value

The more I think about it, the more I have the impression that I can also run in that with my current code:
Flow with my old code:

Callback is executed in the moment the message comes in

Callback checks time, detects that the value is to expire

new expire time and value is set

already running Callback resets the value

Any thoughts about the thread safety here?

To answer this question, the mqtt component is written using Python's asyncio library, which means that only one of these callbacks can be running at a time. If you do some searching for asyncio you can find some good literature.

balloob · 2017-03-21T03:40:18Z

homeassistant/components/sensor/mqtt.py

            if self._template is not None:
-                payload = self._template.async_render_with_possible_json_value(


why did you change this?

Line was 80 chars long (not my edit but wanted to have it fixed).

balloob · 2017-03-21T03:41:37Z

tests/components/sensor/test_mqtt.py

+        time = time + timedelta(seconds=1)
+        fire_time_changed(self.hass, time)
+
+        """ Not yet expired """


Please prefix comments with # . Only doc strings (first comment in a function) should contain triple quotes.

balloob · 2017-03-21T03:41:50Z

tests/components/sensor/test_mqtt.py

+
+        """ Expired """
+        state = self.hass.states.get('sensor.test')
+        # FIXME: this will fail unless the fixmes above are fixed


Soo, fixed now?

Oops, forgot that one ;) Yes, the test is fixed now.

…warning

houndci-bot · 2017-03-21T19:24:24Z

tests/components/sensor/test_mqtt.py

+
+        # Expired
+        state = self.hass.states.get('sensor.test')
+        # FIXME: I have no idea why this does not work. Got stuck here, help plz! :-(


line too long (85 > 79 characters)

emlove · 2017-03-23T19:56:23Z

homeassistant/components/sensor/mqtt.py

@@ -28,6 +32,7 @@
 PLATFORM_SCHEMA = mqtt.MQTT_RO_PLATFORM_SCHEMA.extend({
    vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
    vol.Optional(CONF_UNIT_OF_MEASUREMENT): cv.string,
+    vol.Optional(CONF_EXPIRE_AFTER, default=0): cv.positive_int,


Can we remove the default and just check if self._expire_after is not None: below?

I can but i'd still have to check 0 and None because with 0 it would cause strange behaviour otherwise.

Fair enough. Sounds good to me.

Which one? default or check 0 and None?

Sorry, you can keep it as is. keep the default zero.

emlove · 2017-03-23T20:00:53Z

homeassistant/components/sensor/mqtt.py

            if self._template is not None:
-                payload = self._template.async_render_with_possible_json_value(
+                template = self._template
+                payload = template.async_render_with_possible_json_value(


Why did this change?

Line was >79 chars long. I wonder how it passed the checks

It looked to me that it was exactly 79 characters long. It should be fine. If it was in there it is OK for length.

You're right, don't know why I got an error there. Maybe I accidently idented one more. Reverted.

Yeah, I figured it probably got indented while working on it, then reverted.

OK'd on gitter

emlove · 2017-03-23T21:56:06Z

Thanks for the contribution! 🎉

micw added 2 commits March 19, 2017 20:21

Adding expire_after to mqtt sensor to expire outdated values

7c62b42

Extending test case

45ee8b9

homeassistant added cla-needed platform: sensor.mqtt labels Mar 19, 2017

balloobbot added the platform label Mar 19, 2017

houndci-bot reviewed Mar 19, 2017

View reviewed changes

micw mentioned this pull request Mar 19, 2017

Adding documentation of expire_after for mqtt sensor to expire outdat… home-assistant/home-assistant.io#2297

Merged

homeassistant added cla-signed and removed cla-needed labels Mar 19, 2017

balloob requested changes Mar 19, 2017

View reviewed changes

mqtt: refactoring expire_after to use timed events instead of polling…

3413a9b

…; lint

homeassistant added the cla-signed label Mar 20, 2017

mnoorenberghe mentioned this pull request Mar 20, 2017

Option to expire sensor values that did not receive an updates #6705

Closed

balloob previously requested changes Mar 21, 2017

View reviewed changes

refactor to reset unused trigger

74afdb8

homeassistant added the cla-signed label Mar 21, 2017

micw added 2 commits March 21, 2017 20:21

Fix: handler must be set to None after execution or removal to avoid …

ae5e0e3

…warning

Commenting out non-working test

0dc47d7

houndci-bot reviewed Mar 21, 2017

View reviewed changes

micw added 4 commits March 21, 2017 20:59

Fix lint

4a25e4e

Commit to trigger new build

596fc2f

Commit to trigger new build

5d53175

Make testcase work

b48f8fa

emlove reviewed Mar 23, 2017

View reviewed changes

micw added 2 commits March 23, 2017 21:39

Undo unnecessary change

520a0b8

Remove default value, add extra check

c34a562

emlove approved these changes Mar 23, 2017

View reviewed changes

emlove merged commit 6c59898 into home-assistant:dev Mar 23, 2017

fabaff mentioned this pull request Apr 6, 2017

0.42 #6956

Merged

home-assistant locked and limited conversation to collaborators Jun 24, 2017

ghost added integration: mqtt and removed platform: sensor.mqtt labels Mar 21, 2019


		state = self.hass.states.get('sensor.test')
		self.assertEqual('100', state.state)

		if self._template is not None:
		payload = self._template.async_render_with_possible_json_value(

Adding expire_after to mqtt sensor to expire outdated values #6708

Adding expire_after to mqtt sensor to expire outdated values #6708

Conversation

micw commented Mar 19, 2017 • edited

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

mention-bot commented Mar 19, 2017

homeassistant commented Mar 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micw commented Mar 20, 2017

micw commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlove commented Mar 23, 2017

micw commented Mar 19, 2017 •

edited

Example entry for `configuration.yaml` (if applicable):