Serious issues with timezone support in time functions

[bperrybap]

Platform

• Hardware: [ESP-12]
• Core Version: [2.4.1]
• Development Env: [Arduino IDE]
• Operating System: [Linux Mint 17.3]

Settings in IDE

• Module: [Wemos D1]
• Flash Mode: [qio|dio|other]
• Flash Size: [4MB/1MB]
• lwip Variant: [v1.4|v2 Lower Memory|Higher Bandwidth]
• Reset Method: [ck|nodemcu]
• Flash Frequency: [40Mhz]
• CPU Frequency: [80Mhz|160MHz]
• Upload Using: [OTA|SERIAL]
• Upload Speed: [115200|other] (serial upload only)

Problem Description

The are some serious issues with how timezone offsets are being handled in various time functions.
There was mention of some issues in the discussion about the
NTP-TZ-DST.ino demo sketch in issue #3835
That issue mentioned issues with gmtime() and localtime() but the issues are much more serious.
Here is the behavior I've seen - using the NTP-TZ-DST.ino demo sketch with a few tweaks to see the behavior of gettimeofday() as well.
The results were the same regardless of whether NTP was used or the "fake" RTC mode was used.

• The time_t returned by time() does not equal the tv_sec value returned by gettimeofday()
  They should always be the same value.

• gettimeofday() in the tv_sec member returned, is always supposed to return the number of seconds since the epoch without any consideration or alteration based on local timezone.
  This is working correctly.

• time() is always supposed to return the number of seconds since the epoch without any consideration or alteration based on local timezone.
  This is not working correctly
  time() is returning a time_t that has been adjusted/modified and is offset by the specified timezone information.
  This is incorrect. time() has modified that actual time_t timestamp it returns. time_t timestamp values are not be modified for the local time. Any correction for local time should be handled by localtime() or ctime()

• gmtime() and localtime() are currently generating the same broken down time elements.
  This is incorrect.
  They both take the time_t value and break down it into its components based on the time_t value provided with no adjustments for timezone.
  The issue is actually with localtime()
  localtime() is behaving just like gmtime() and is not adjusting the time for the timezone offsets.
  This is easily be demonstrated by handing both localtime() and gmtime() a zero value time_t.
  Both will generate the identical GMT time at the epoch.

These issues have likely been masked/hidden since many users that are setting timezone information are then calling localtime() with the time_t value from time(). While the fields created by localtime() when handed a time_t from time() will be correct, the time adjustment is being done incorrectly since the time_t was adjusted by time() rather than localtime() doing the timezone adjustment.

localtime() will fail to work correctly when handed the tv_sec value from gettimeofday() (it will show GMT)
gmtime() will fail to work correctly when handed the time_t from time() (it will show local time since time() is returning an incorrect time_t value)

---

I'm not sure how this can be fixed since it appears to be an issue in code outside of this source tree.
It seems like until it is fixed, users using the time code with timezone offsets need to know and understand the issues.
Until it gets fixed, perhaps some notes and big warnings could be placed in the comments in the
NTP-TZ-DST.ino demo sketch

[5chufti]

nice to see someone else barking up that tree ...
using it like this should get you going

  configTime(0, 0, "pool.ntp.org");
  setenv("TZ", "CET-1CEST,M3.5.0,M10.5.0/3", 3);
  tzset();

and give correct values for gmtime() and localtime() with real dst, but only using core >= 2.4 and lwip2.0.
For the sake of backward compatibility they ..... you allready found out.

[bperrybap]

@5chufti
Not sure what you mean by this:

For the sake of backward compatibility they ..... you allready found out.

It seems that there is some odd behavior in the timezone handling code that needs to be documented and, IMO, corrected.
And the NTP-TZ-DST.ino demo sketch disparately needs some updates to properly handle timezones (which it isn't doing now) and to deal with the existing code along with some clear comments about explaining how to properly use the functions for timezones and how to avoid using the misbehaving functions and to avoid modifying/corrupting the actual timestamp.
A big issue is that the example sketch is not doing things correctly and it also needs lots of information in the comments to help people understand how the library code works and in some cases how it works incorrectly.
(I'd be happy to submit a PR to update this sketch, with an updated version that actually works, with lots of comments explaining what works and what is broken, if people agree it would be useful)

Your example works to correct gmtime() and localtime() when NTP is being used, so I had to experiment a bit for the RTC case.
And I really dug into the code in time.c and sntp-lwip2.c
The "fake RTC" build of the example sketch (not using ntp) can also be made to work properly as well by not using the timezone fields and using the TZ variable.

The sntp-lwip2.c code is a bit twisted up and appears to have some issues.
The main one being that it appears that the way sntp-lwip2.c is using timezone information is breaking some of the libc functions like time(), gmtime(), localtime(), and gettimeofday() by adjusting the actual time_t timestamp by the timezone offset.
It seems like libc and and code in sntp-lwip2.c are both handling timezone offsets, but the code in sntp-lwip2.c is not handling timezone offsets correctly in all cases.

Also, if you call gettimeofday() and pass in a timezone pointer to get the current timezone offset, it will always be zero even when using the TZ timezone variable which is not correct.

It does appear that if you avoid setting/using any of the timezone offset information used by settimeofday(), configtime(), and sntp_set_timezone() to keep sntp-lwip2.c from messing with the timestamp, things mostly work, other than gettimeofday() can't get the current timezone offset.

Not sure how to get this stuff fixed.

[5chufti]

no sense in fixing things broken by design.
the two values for timezone and dst are just offsets that are senslessly applied to the system time, without any TZ rules. So I ask: if one has to do the dst desicion before handing over the offset, why have two offsets at all?
"system time" should allways be utc, so no need for providing offsets, just provide function to set ntp servers and/or initial time from hw-clock; the rest should be done via standard c functions.

[bperrybap]

I don't follow. I couldn't tell if you were referring to the esp8266 Arduino core code, or the unix time functions dealing with timezones and dst in general.

The system time stamp returned as a time_t by functions like time() should always be utc with no offset or adjustment of the local timezone - if it isn't utc, then something is broken or something is being done/used incorrectly.

You also have to take into consideration that setting the time from an RTC may not be just a one time thing. It may need be periodic, particularly in an embedded environment as the RTC may be much more accurate at time keeping than the local system that has no network connectivity.

IMO, timezone rules and the current timezone offset/dst are not the same thing and serve very different purposes.
A timezone rule is optional and is how you can programmatically determine the current timezone offset and dst setting.
Once timezone and dst are configured, those two can be easily (and always) be applied to the utc time_t "system-time" time stamp whenever local time information in broken down form is requested/needed.
If there is no need or use of local time values, then timezone rules, offsets, dst, don't matter as they won't come into play.
i.e. a timezone rule may be expensive while simple offsets and dst adjustments are simple.
It could be that the timezone rule is only run periodically like on the hour to re-calculate current offset and dst setting. Or perhaps the timezone is manually set by an operator and the dst setting is set by a cron job that runs twice a year to set it appropriately - in that case no timezone rule is used and no calculation is done, dst is set/un-set when necessary.

There can be an issue initially setting the system time from an RTC clock, since the RTC may not be set to utc but rather local time.
In that situation a conversion from broken down local time to a utc time_t has to be done somewhere.
But for that situation, I tend to use mktime() to create a time_t which has a local time offset, then offset it by the proper amount to get to a proper utc time_t and then use it.

I have never seen a need to ever have an offset time_t value like what is being done in sntp-lwip2.c
although I have seen many people incorrectly use time_t values offset to local time instead of using proper timeezone offset and dst settings especially in cases on embedded systems where the time libraries didn't include timezone support - like the popular Arduino Timelib library.

[devyte]

@5chufti do you mean the Arduino time handling design is broken in general, or our specific core code for handling time is broken?
You both seem to have a firm grasp of how the time and timezone code should behave. How about investigating a rework and making a PR?

[bperrybap]

I don't believe the design is broken - at least not the way the unix/BSD/linux time function APIs are defined. It does seem like some of the code in time.c and sntp-lwip2.c is not behaving properly.

What I've seen is that some code attempts to adjust for timezone offsets by mucking with the actual time_t timestamp. It shouldn't be done that way. While that can make things like localtime() appear to work, in reality it is doing the local time correction incorrectly so that something else breaks.
For example, the 3rd party Arduino Time/TimeLib library (which is not this code) has no support for timezone at all. So many users just set the internal time_t time to represent the local time rather than utc. i.e. they create a time_t value that represents their local time rather than utc. For most users that works just fine since the time_t is never used for anything but working locally with local time. However, it is not using the correct time_t timestamp, so if the time_t value from were used as a timestamp or used in anyway with anything external to the device, it can create problems.

In the case of the sntp-lwip2.c code, when using a timezone offset, it modifies the actual timestamp. The issue then becomes that various time API calls return and use different timestamp values - that should all be the same. Like in the case of time() returning a different timestamp than gettimofday() - which is wrong. The C library functions have no knowledge of this timezone offset information in sntp-lwip2.c so they think there is no timezone offset which causes gmtime() and locatltime() to show the same time.
Since the atual timestamp was modified by the timezone offset, gmtime() shows local time rather than utc.
And if using the TZ variable, the libC functions will know about the proper offset, but time() in time.c doesn't since it getting the timestamp from the sntp-lwip2.c code which has its own timezone offset information and has mucked with the timestamp to offset it by the timezone offset.

I'd be more than happy to propose a re-work, but to make sure it is all correct, I also need to see the libC library code for the time functions. Is that source code available somewhere? All I saw in the espressif github tree I found were .a archive files and no actual source code.

I think if done carefully it could preserve compatibility with existing code that uses the timezone offsets provided by the various functions sntp-lwip2.c.
This is because I believe that most users will be setting the timezone and then using the time_t value from time() to pass to localtime() not ever realizing that once you set timezone information in the sntp-lwip2.c functions, the time_t values you are getting from time() are incorrect since they are getting proper looking information from locatltime().

There does seem to be a bit of issue with the way the dst offset is handled, and I'd have to look much closer at that in sntp-lwip2.c along with the libC code make sure they all play nice together.

The main thing is does anybody know where I can find the source code to the libC time functions supplied in the esp8266 core?

[igrr]

@bperrybap newlib code used is here: https://github.com/igrr/newlib-xtensa/.

The issue you describe is explained as follows:

time function is implemented inside the core here, which is incorrect. time defined in newlib should be used. The core should provide implementation of _gettimeofday_r, and use time implementation from newlib (which calls _gettimeofday_r (source here)). However, time implementation in newlib has been disabled here (yes, at that time NonOS SDK provided its own time function, so i didn't have a better solution).
The fix is pretty obvious — revert change in newlib, rebuild it, remove time implementation from the core. Result: time and gettimeofday will agree.

[igrr]

Another thing, the time support in the core (_gettimeofday_r) should not be calling into sntp_ functions to ask for the current time. The core should be doing timekeeping on its own, and sntp library should call settimeofday or adjtime when needed to adjust the time.

[bperrybap]

@igrr Thank you for the links.
I believe that there are other issues related to tm_isdst and dst offset handling in the sntp-lwip2.c code.
I'll go through all the code in detail and see if I can figure that out as well.

[devyte]

@bperrybap great! Thank you for picking this up.
You may also want to look at the example, in case you haven't done so.

[bperrybap]

@devyte The NTP-TZ-DST.ino example is the one I've been playing with and I've mentioned a few times that needs a bit of polishing and cleaning up and additional informational comments added.

A how it gets updated will depend on the final updates done to the time.c , sntp-lwip2.c code, and core library code since if those updates fix the issues, the example sketch won't need work arounds and added comments describing the issues and how to work around them.

[RudyFiero]

I had struggled with the NTP-TZ-DST.ino. It took me some time to figure out that most of it has nothing to do with setting the time. Some things that seem to be relevant but no explanation as to the need or purpose.
ESP.eraseConfig(); I have no idea why this is there. So do I

This example shows how to read and set time,

So why is the following included? It has nothing to do with time that can be set. Sure it is good to know, as most things are, but it isn't relevant to the stated purpose.

// for testing purpose:
extern "C" int clock_gettime(clockid_t unused, struct timespec *tp);

And this really confused me.

  // human readable
  Serial.print(" ctime:(UTC+");
  //  **Serial.print((uint32_t)(TZ * 60 + DST_MN));**  // TZ*60 + DST_minutes
  //  Serial.print("mn) ");
  Serial.print(ctime(&now));

I couldn't understand what the meaning of the large number. Large number? Yes, if TZ is negative (-6 for me), and using (uint32_t) produces a large positive number. It should have been (int32_t).

When I saw the example program I felt it was exactly what I needed. I want to have a hardware real time clock as a backup when the internet is not available. No external time libraries needed, perfect. And I wanted sub-second timestamps, and it is all here. But it has been a lot of work trying to figure out what in the example that I don't need. (I'm not a programmer)

Okay, rant over. I'm thrilled that this is going to be looked at. I can't wait to see the result.

[d-a-v]

@RudyFiero I originally made this "exhaustive" example to check time functions along with updated in NTP code (lwip2's sntp) and several fixes where brought (not only by me).

This example shows how to read and set time,

So why is the following included? It has nothing to do with time that can be set
extern "C" int clock_gettime(clockid_t unused, struct timespec *tp);

It has to do with reading time with standard and implemented libc functions, as mentioned.

and using (uint32_t) produces a large positive number. It should have been (int32_t).

Very true. I live in UTC+1 and I missed this, thanks for spotting.

@5chufti We could have patched lwip1.4 to get the same behaviour. lwip1.4 is already a patched version from espressif and I was not sure it be wise to go that risky way at that time. Risky because lwip1.4 is supposed to get updates from espressif.

@bperrybap Thanks for getting into this. Any help to improve / comment this example sketch is welcome. Maybe this sketch needs to be splitted into several pieces for clarity. The core deserves to become more accurate with time handling. The hard points will be at least to synchronize with newlib's time() fix, to keep backward compatibility with configTime() and maybe with lwip1.4's way of handling time.

Also, as @5chufti told, the sketch (or another one) lacks the tzset() example which is very handy.
I use for instance "CET-1CEST,M3.5.0,M10.5.0/3" (which is CEST) and it accurately follows TZ. We should have a script that import these data to some defines and show them to users in an example
as @igrr suggested in #1679(comment) (source of all these changes).

[RudyFiero]

The problem I had was that I was not familiar with time routines in general. Millis of course. But date/time I was not very familiar with. I didn't know what belonged with who, or what belonged together. It took a while before I figured out there were independent methods for getting time. But I still couldn't figure out who was in charge. Some more comments would have helped.

I'm not a programmer. I design electronics for a living, and have mostly had other people who did the firmware/software. But now I'm trying to do my own stuff at home. And my brain ain't as young as it used to be. Its harder now to deal with a lot of abstraction and complexity.

[Booli]

How do you set the time manually with the new time core?

Scenario: I connect to a MQTT server, which sends me a unixtime stamp every so often. I want to set the "internal clock" to that time stamp, but I can't figure out how to do it. Must be simple right?

I would guess something like settimeofday(unixtimestamp)