raetha / wyzesense2mqtt

Configurable WyzeSense to MQTT Gateway intended for use with Home Assistant or other platforms that use MQTT discovery mechanisms.
MIT License
79 stars 22 forks source link

Better tracking of timestamps for sensor event #21

Closed dale3h closed 4 years ago

dale3h commented 4 years ago

Describe the Bug

I noticed that the timestamp attribute of the MQTT payloads is named device_class_timestamp. Was this intentional, or should this just be timestamp?

wyzesense2mqtt/777EFF2A {"available": true, "mac": "777EFF2A", "state": 0, "device_class": "opening", "device_class_timestamp": "2020-06-16T15:53:38.665000", "signal_strength": -64, "battery": 86}

Expected Behavior

Timestamp attribute to be named timestamp instead of device_class_timestamp. (Example below)

wyzesense2mqtt/777EFF2A {"available": true, "mac": "777EFF2A", "state": 0, "device_class": "opening", "timestamp": "2020-06-16T15:53:38.665000", "signal_strength": -64, "battery": 86}

Additional Context

Line of code containing the hardcoded attribute name: https://github.com/raetha/wyzesense2mqtt/blob/f754304b1e1b7e49a4b2100b986fca5e187cf71d/wyzesense2mqtt/wyzesense2mqtt.py#L353

raetha commented 4 years ago

@dale3h, that actually definition came from the project I originally referenced, and I never thought to check the HA specs on what it should be. I'll take a look and assuming you are right about the HA setup, will change this ASAP.

raetha commented 4 years ago

Ok, I can't actually find anything on the HA side specifying any standards for the state topic at all, other than the primary payload value based on the type of sensor. Other attributes all seem to be up to the individual integration. If you found anything otherwise, I'd love to see the reference, as I like to follow standards where possible.

Anyway, this makes sense to me, and without other guidance, I'll change the name of that attribute and commit to the dev branch and it will update with the next release, hopefully in a few days/week. Just been giving time for the last fix to see if that seemed to work as expected.

dale3h commented 4 years ago

If the timestamp is based on when the most recent payload was received from the sensor, it may make sense to rename this to last_seen to align with Zigbee2mqtt. If this route is chosen, I would also recommend changing the attribute value to match the same format, which is JavaScript's representation of a timestamp (the current number of milliseconds since the UNIX epoch). The current timestamp is definitely more human-friendly, but a second or millisecond-based timestamp would seem more appropriate as far as using the value in automations goes.

I assume it was a typo in the original, and that the original author meant to use just timestamp instead. I make this assumption based on 1) the timestamp having no direct relation to the configured device_class, and 2) it immediately follows the line that assigns device_class (i.e. a common copy/paste mistake).

raetha commented 4 years ago

From what I can tell this came all the way back from Kevin Vincent's HA-WyzeSense, so two projects back. I'm happy with the direction you've suggested.

I'll convert to it being named last_seen, and though I hate trying to read epoch time, it does make sense for automations. I'm leaning towards having both:

That would allow for both a quick check in the MQTT topic and HA sensor attributes, and also automations.

What do you think?

dale3h commented 4 years ago

I think that's a great idea! I hate reading epoch-based timestamps too, but converting arbitrary formats back to them is even worse. Having both will completely eliminate having to do either of those things.

raetha commented 4 years ago

I actually decided to have last_seen be the epoch, and last_seen_iso be the ISO version of the timestamp. That way last_seen is more consistent with the way this is done in other similar projects.