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

[SYS] Add utc & unix timestamp options to SYStoMQTT and Sensor messages #1533

Merged
merged 17 commits into from
Mar 17, 2023

Conversation

ilgrank
Copy link
Contributor

@ilgrank ilgrank commented Mar 14, 2023

Description:

(sorry, I removed previous pull request by mistake)

Sync to the defined NTP server and add unix timestamp to MQTTtoSYS topic and to all sensor messages.
(As discussed in #1404 )

In certain scenarios, (e.g. without an external controller and a database) it is impossible to know when a sensor reported a reading. (especially with retained messages)
Since most battery-powered sensors lack time information, the Gateway is the only point that can provide this data.

Checklist:

  • The pull request is done against the latest development branch
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • I accept the DCO.

@ilgrank ilgrank changed the title Sync to NTP and add unix timestamps to SYStoMQTT and Sensor messages Add unix timestamp to SYStoMQTT and Sensor messages Mar 15, 2023
@1technophile
Copy link
Owner

unixtime seems to be the same as uptime from my tests:

{"uptime":188,"unixtime":188,"version":"Mar 14 2023","discovery":true,"env":"esp32dev-ble","freemem":133276,"mqttport":"1883","mqttsecure":false,"tempc":48.33333,"freestack":2372,"rssi":-31,"SSID":"","BSSID":"","ip":"192.168.1.","mac":"","lowpowermode":0,"interval":100,"intervalcnct":3600000,"scnct":107,"modules":["ONOFF","BT"]}

Also I think we should call the NTP less frequently.

@ilgrank
Copy link
Contributor Author

ilgrank commented Mar 15, 2023

unixtime seems to be the same as uptime from my tests:

{"uptime":188,"unixtime":188,"version":"Mar 14 2023","discovery":true,"env":"esp32dev-ble","freemem":133276,"mqttport":"1883","mqttsecure":false,"tempc":48.33333,"freestack":2372,"rssi":-31,"SSID":"","BSSID":"","ip":"192.168.1.","mac":"","lowpowermode":0,"interval":100,"intervalcnct":3600000,"scnct":107,"modules":["ONOFF","BT"]}

That's strange.. it's working for me:

{
  "uptime": 1567,
  "unixtime": 1678842830,

is the NTP server reachable by your ESP?
Also, please try the latest push, as I've made some small optimization on that side.

Also I think we should call the NTP less frequently.

Totally agree on this point
I'm still testing ESP timekeeping reliability tho, and so far even on an 'underpowered' C3 the additional load seems negligible. (Since this is code is running just on ESP8266 and ESP32 for now, should not be a big issue)

That said, are there already less frequently called routines?
Ideally, NTP Sync should happen at boot, but then it will likely fail since the ESP will likely still be connecting.
At that point it should be retried every.. e.g. 120 seconds until succeeds, and then every... e.g. 3600 seconds? (of course the timings could me made configurable)

@1technophile
Copy link
Owner

That said, are there already less frequently called routines?

We don't have, but I'm interested in adding a second one for OTA update checks.

Also if the timestamp is for people without a controller, should we publish a human readable format instead of ms.

@DigiH
Copy link
Collaborator

DigiH commented Mar 15, 2023

Wouldn't be a legible time format UTC, without having to set timezone and dst offsets, be more helpful as an immediately recognisable time stamp, without requiring the help of a time converter, especially without the help of a controller?

@ilgrank
Copy link
Contributor Author

ilgrank commented Mar 15, 2023

We don't have, but I'm interested in adding a second one for OTA update checks.

That seems a good scenario with a comparable order of magnitude (even more so if it will be user-configurable)

Also if the timestamp is for people without a controller, should we publish a human readable format instead of ms.

Wouldn't be a legible time format UTC, without having to set timezone and dst offsets, be more helpful as an immediately recognisable time stamp, without requiring the help of a time converter, especially without the help of a controller?

Well.. I get your point, and I frankly I'm still undecided too, as similar projects are still divided on what's best.
Tasmota, as an example, uses human-readable time in ISO8601 format:

{
  "Time": "2023-03-15T02:43:50",
  "ENERGY": {
    "TotalStartTime": "2020-04-10T18:33:07",

while Shelly uses Unix time:

  "time": "16:35",
  "unixtime": 1678462538,
  "serial": 1,

maybe the best approach could be to make the timestamp user-selectable, both for enable/disable and unix/ISO8601
e.g.:

message_Timestamp=true
timestamp_Format="ISO8601"

what do you think? (@1technophile and @DigiH )

@DigiH
Copy link
Collaborator

DigiH commented Mar 15, 2023

"2020-04-10T18:33:07"

👍 from me!

Best to also prefix a UTC, just to be totally clear.

I don't think it would even have to be made user-selectable. The above is clearly legible by anyone at first sight, and can just as easily be converted to any other time format by a controller, as would unixtime.

And users being able to read and understand unixtime on the spot - is it one minute ago, one hour, one day - are very few and far between ;)

@1technophile
Copy link
Owner

"2020-04-10T18:33:07"

I like this also, and I don't think we need it to be changed by the user

@1technophile
Copy link
Owner

@ilgrank, you could add the NTP sync in this routine #1538

Add optional timestamps to the SYStoMQTT topic
Also, the sync with the NTP time server now occurs every hour instead of every 2 minutes (can be defined with TimeBetweenCheckingSYS in UserConfig.h)
@ilgrank
Copy link
Contributor Author

ilgrank commented Mar 17, 2023

@1technophile : synced with dev branch and pushed the updated timestamp code, I commented the features in UserConfig.h, let me know if there's any other place where I should better explain (well, not that there's a lot to explain in a boolean option anyways :)

@DigiH : to maintain compatibility with ISO8601 standard and still warn users that the time is UTC, the time string now ends with a capital Z, indicating that the time is UTC:

Z = special UTC designator ("Z") (optional)

(reference here)

@DigiH
Copy link
Collaborator

DigiH commented Mar 17, 2023

to maintain compatibility with ISO8601 standard and still warn users that the time is UTC, the time string now ends with a capital Z, indicating that the time is UTC:

I already watched, and the Zulu time zone indicator is perfect and standard to mark the time stamp as UTC. The only possible other addition could be to rename the key to "utctime" (in coexistence with "unixtime") to also make it immediately clear for non-Z aware users, and to avoid future queries about it.

@ilgrank
Copy link
Contributor Author

ilgrank commented Mar 17, 2023

[...]other addition could be to rename the key to "utctime" (in coexistence with "unixtime") to also make it immediately clear for non-Z aware users, and to avoid future queries about it.

that's actually a great idea! Committing it now :)

@DigiH DigiH changed the title Add unix timestamp to SYStoMQTT and Sensor messages Add utc & unix timestamp options to SYStoMQTT and Sensor messages Mar 17, 2023
@1technophile 1technophile changed the title Add utc & unix timestamp options to SYStoMQTT and Sensor messages [SYS] Add utc & unix timestamp options to SYStoMQTT and Sensor messages Mar 17, 2023
@ilgrank
Copy link
Contributor Author

ilgrank commented Mar 17, 2023

also renamed the DEFINEs for better readability

@1technophile
Copy link
Owner

Thanks

@1technophile 1technophile merged commit c8510f2 into 1technophile:development Mar 17, 2023
@ilgrank ilgrank deleted the Timestamps branch March 17, 2023 16:10
@ilgrank
Copy link
Contributor Author

ilgrank commented Mar 17, 2023

Thanks

Thank you for OMQTTGateway, I'm happy to have had a chance to contribute :)

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.

3 participants