WebUI: NTP configuration is not obvious

[AlbertWeterings]

Bug description
A clear and concise description of what the bug is.
Loaded today's dev build on a SonOff Basic. Time handling is acting incorrect.

• In Status - Current time - The time is not changing if time zone is changed NTP tab
• NTP tab is not showing dropdown for time zone
• Enable/Disable DST is gone
• Set DST region is gone
• In dubug after typing NTP I get this:
  [655751] [NTP] Sync Time : 2020-08-19 07:58:20 (UTC)
  [655752] [NTP] UTC Time : 2020-08-19 08:05:41
  [655755] [NTP] Local Time : 2020-08-19 10:05:41 (UTC-02:00)
  If UTC = 08:05 UTC-2:00 can never be 10:05

Entering UTC 2:00 results in:
[026053] [NTP] Sync Time : 2020-08-19 08:09:30 (UTC)
[026054] [NTP] UTC Time : 2020-08-19 08:09:49
[026057] [NTP] Local Time : 2020-08-19 06:09:49 (UTC 02:00)
Should be 10:09

Expected behavior

• Status screen should show "Local time"
• Time zone setting doesn't respond to UTC/GMT/EST
• Time zone setting should count up on + or nothing and down on -
• Configuration in NTP should have DST
• Configuration in NTP should have DST region setting

Screenshots
If applicable, add screenshots to help explain your problem.

Tools used

• Desktop operating system
  Windows 10 2004
• Browser & version
  Firefox
• IDE & version
  Arduino IDE 1.8.13

[mcspr]

In Status - Current time - The time is not changing if time zone is changed NTP tab

See #2220
Old implementation sent already 'offset' timestamp, which we pretended to be in UTC and simply shown it hiding the -UTC suffix part.
If you have a better idea than sending timestamp string formatted by the device every ~seconds, please comment here / there.

Time zone setting doesn't respond to UTC/GMT/EST
Time zone setting should count up on + or nothing and down on -
Configuration in NTP should have DST
Configuration in NTP should have DST region setting

Not entirely correct, see:

espurna/code/espurna/config/general.h

Lines 1557 to 1566 in 0f11b66

	#ifndef NTP_TIMEZONE
	#define NTP_TIMEZONE TZ_Etc_UTC // POSIX TZ variable. Default to UTC from TZ.h (which is PSTR("UTC0"))
	// For the format documentation, see:
	// - https://www.gnu.org/software/libc/manual/html_node/TZ-Variable.html
	// ESP8266 Core provides human-readable aliases for POSIX format, see:
	// - Latest: https://github.com/esp8266/Arduino/blob/master/cores/esp8266/TZ.h
	// - PlatformIO: ~/.platformio/packages/framework-arduinoespressif8266/cores/esp8266/TZ.h
	// (or, possibly, c:\.platformio\... on Windows)
	// - Arduino IDE: depends on platform, see `/dist/arduino_ide/README.md`
	#endif

We do miss help-text on the WebPage though and should 'migrate' old NTP settings to the new format automatically, (<HOUR_OFFSET>,M<DST_ON>,M<DST_OFF>)

[AlbertWeterings]

Well I forced to compile with NTP_LEGACY_SUPPORT 1 by placing in ntp.h line 12:
#define NTP_LEGACY_SUPPORT 1

Compiled with Core 2.7.4, now current time in Status window is showing the correct local time and NTP tab is back to old also.
The difference is the use of:
Legacy using from external libraries:
#include <TimeLib.h>
#include <WiFiUdp.h>
#include <NtpClientLib.h>

time_t ntpLocal2UTC(time_t local);

And non Legacy using from core:
#include <lwip/apps/sntp.h>
#include <TZ.h>
#include "ntp_timelib.h"

The flash image size in legacy is decreased from 541KB to 537KB That makes the flash image using legacy significant smaller.

This raises the question to me why aren't we using legacy? I see nothing yet that is not working.

[mcspr]

For multiple reasons, actually * ‘Unix’ timestamp is plain wrong when using timezones – instead of UTC time, NtpClientLib adds offset to it, thus when we need UTC we need to subtract the offset to get the real value. It could’ve been possible to manage timezones ourselves, but we can easily avoid that by using new implementation, where the code is already written and working. * We support much more timezone settings. With legacy we are limited to hard-coded American or Europe offsets. E.g. we don’t have any support to southern hemisphere or 2nd American DST option, and much more exceptions to the basic implementation in the NtpClientLib. * Protocol support is much ‘simpler’, since we don’t take into an account ‘kill’ packets and happily accept garbage data sent to us by the NTP server, instead of waiting a bit and retrying. You can search for the 2038 year issue here, when offset is applied to the 0 timestamp we supposedly receive, device things time is sync’ed, despite this being an error. * You may notice the amount of code we have to work around the quirks of the NtpClientLib You can easily search for the city in the Tz.h and c/p the provided string into the timezone field, no need to force legacy configuration back. Unless, you really want webui to display the current time with timezone :/

[AlbertWeterings]

Ok, a lot makes sense. But why is it impossible to calculate the correct local time for the UI based on the string we put in the non legacy timezone field and UTC time? To me it looks like this is also being calculated when we type NTP in debug and when MQTT is sending the status.

For me it looks a bit confusing for most users if the scheduler is only working with UTC time or with the local time after you have added the correct timezone string and checking if your time is right in debug.

[mcspr]

It’s not, really, maybe I just have not researched the options very well. If remember correctly, unlike most generic PC libc implementations, esp’s libc is missing tm_gmtoff (aka utc offset) right in the time value structure, so we would need to re-implement it to actually ‘know’ the real offset / do something different with the values we have. We deal with POSIX tm struct, basic solution would be something like – get local tm struct via localtime_r(3), use mktime(3) to convert it back into time_t value, thus have the same ‘fake’ timestamp to show in the UI. Scheduler works based on the localized time, same as always. Time printing is also handled via libc functions, we present them with a localized tm struct via localtime_r(3).

[AlbertWeterings]

So time-t is the value printed in the UI?
time-t is set by a function called ntpLocal2UTC I think in legacy (line 21 in ntp.h) . I have searched if I could find how this function is working but was not able to find it. Maybe we can create a function to do the same but use UTC and the timezone variable?

Adding some information to the NTP page to guide user by setting the correct timezone value would make it perfect.

I was looking at the code. In non legacy we are using ntp_timelib.h instead of NtpClientLib.h in legacy. maybe this could be a suitable place to calculate local time for the UI

[mcspr]

Yep, we only send the number. time_t value is UTC, all tz modifications are expected to work with it and produce something else entirely e.g. print it, convert to tm struct to get day / month / hour and etc.

Main 'issue' is the incrementing counter, you may notice the seconds value changing :) I guess this could simply replace the existing webui keep-alive mechanism, sending text string with the ts every second when ntp is enabled. Barring the side-effect of having this data in the websocket queue and wasting (small, but nonetheless, some amount) memory.