-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
Serious issues with timezone support in time functions #4637
Comments
nice to see someone else barking up that tree ...
and give correct values for gmtime() and localtime() with real dst, but only using core >= 2.4 and lwip2.0. |
@5chufti
It seems that there is some odd behavior in the timezone handling code that needs to be documented and, IMO, corrected. Your example works to correct gmtime() and localtime() when NTP is being used, so I had to experiment a bit for the RTC case. The sntp-lwip2.c code is a bit twisted up and appears to have some issues. 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. |
no sense in fixing things broken by design. |
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. 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. I have never seen a need to ever have an offset time_t value like what is being done in sntp-lwip2.c |
@5chufti do you mean the Arduino time handling design is broken in general, or our specific core code for handling time is broken? |
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. 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. 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. 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? |
@bperrybap newlib code used is here: https://github.com/igrr/newlib-xtensa/. The issue you describe is explained as follows:
|
Another thing, the time support in the core ( |
@igrr Thank you for the links. |
@bperrybap great! Thank you for picking this up. |
@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. |
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.
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.
And this really confused me.
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. |
@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).
It has to do with reading time with standard and implemented libc functions, as mentioned.
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 Also, as @5chufti told, the sketch (or another one) lacks the |
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. |
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) |
You are right. Look at the NTP-TZ-DST.ino example sketch. #define NTP0_OR_LOCAL1 1 // 0:use NTP 1:fake external RTC When set to local it uses |
Yeah I just moved that into a function
But my time(NULL) stays 0, do I need to be running anything other that 2.4.1 core? I checked with using the NTP version of the sketch, with configTime, and then it works. But I want to use the RTC/provide my own timestamp. |
Your function works for me. Are you sure you are passing it something other than 0? I'm using 2.4.1 on the computer I tested your function. |
Tried the following, but to no avail:
outputting:
And also not hitting the time_is_set callback I think. |
I took your code and ran it as is. This was my output.
|
I think I run into this using core 2.4.2, but I'm not sure if I'm doing something wrong or if it is a bug: I am using a DS3231 RTC to have a reliable timesource before NTP can take over. Inspired by the NTP-TZ-DST example, I do this as follows:
This seems to work fine, and the internal clock reports a correct time. Some time later, the network becomes available and the NTP client acquires time and sets the internal clock. When that happens, I periodically write the internal clock back to the RTC using the provided system callback function. Inside of it, I put:
Because of the call to time(), this should set the RTC to UTC, so on the next system boot the time is read from RTC, and time-zone adjusted to give correct internal clock... But in my case, the time is now ahead, until the NTP acquires the time again. As described by the OP, the call to time() gives a local time instead? So I now work around by calling gettimeofday(), which gives the correct UTC, and then get its time_t
I'm worried that I'm making a mess of the time handling... And next thing is verifying DST support... |
The timezone offset stuff is really broken and is not usable. Example code: RTC-Clock-MinExample.ino.zip NOTE/UPDATE |
That saved me a lot of time, thank you! I think your example should replace the one that is currently supplied with the Arduino core. |
Thanks for the solution.
Couldn't find the solution for Test Code: ntpTest.zip Environment:
|
fwiw - I found that some named time zones work better than others. I've had pretty good success with the ones here (e..g. "CET-1CEST-2" instead of "Europe/Paris") like this:
Note that I'm using ESP32, only adding the named timezone reference here that others may find useful. see also #4749 (comment) |
AFAIK using the timezone name or ID such as IBM has good article in explaining POSIX timezone format. Someone has created a GitHub project to maintain posix time zone strings DB. And @d-a-v has made a nice small utility for esp8266 based on the above project here. Personally I put the the DB file (zone.csv or zone.json) in SPIFFS and parse it manually if needed. On top of that, I think there is still an issue with the this setenv() and posix format stuffs: If I pass the posix timezone string in quoted form such as At the moment I workaround that either by removing all the quoted form timezone strings or pick the ones I need and edit them manually.
|
You mean, newer than the updated one proposed within the PR ? |
You said that the NTP-TZ-DST example was incorrectly using print() in the callback). Or any example that you feel would be better to show how to use the functions. A lot of us are not programmers, and we can use all the help we can get. It often isn't obvious by reading the libraries. |
Right, the output demo of the updated example using scheduled functions is now proposed in #6373 (comment). |
I tried to compile the new example code but the included file TZ.h is missing. Where can I get this file? I didn't find it in this repository. I looked at https://www.iana.org/time-zones but I didn't get any further. I think I found it. I finally realized that the code is only at the pull request stage. (after getting compile errors) I guess I will have to wait a while. |
To try a PR I use this script:
It works on any github repository.
and restart arduino IDE. |
Using sntp_stop() sntp_init() changed the behavior in 2.5.2 (stable) from integer multiples of 15 seconds for the next sntp update to (some random time within the next minute). I haven't beat that up too much to find out the worst-case update time. However, there's a different way to force an sntp update: every time I reconfigure/reconnect WiFi it always causes an sntp update, regardless of the setting of NTP_UPDATE_DELAY. I'd #defined that to 12 hours as I don't care if the system clock drifts a little. In my code, I do initial configurations in setup() (including sntp) and then inside loop() the first thing I do is wake up and reconfigure the modem, as I put it in modem sleep at the bottom of loop() and then delay(25 minutes) for lower power. By the time I need it (seconds after loop() begins), the system clock is always re-sync'd to NTP. I haven't figured out yet which part of the WiFi reconfiguration is causing the sntp update, but I've seen it happen on every 30 minute loop() overnight. This isn't a problem, merely an observation. :-) I do a full modem reconfigure + OTA setup in loop(), even though the modem was configured and connected in setup() without OTA when I got the first sntp update. |
SNTP default renew interval is one hour and it hasn't changed (can't be changed without lwIP recompilation). To remember: |
So maybe its time to finally support sntp_set_update_delay() ? :-) |
Looks like, from #5938, we still have d-a-v/esp82xx-nonos-linklayer#32 pending. |
Ahhh, you're absolutely correct. I missed the warning when I compiled it: (warning: "SNTP_UPDATE_DELAY" redefined [enabled by default]). I saw the #ifndef in sntp.c and thought I could override it, and didn't know lwip2 was a binary blob.
Adding extra SNTP updates when I reconnect WiFi or the DHCP lease renews doesn't cause me any trouble, I was only trying to minimize updates I didn't need... it's always a good idea to be a polite 'netizen when possible. :-) Thanks for your time and efforts! |
I am aware this is an old tread, but I have also noticed an error in the ESP8266 “time.h” functions related to the “_daylight” variable. (This with the latest “https://github.com/esp8266/Arduino” as at 22Feb2020.) - Update: My Bad - this was caused by an error on my part - I have detailed the actual behavour below for if anyone else is interested. "_daylight" appears to be a boolean variable used to tell "localtime_r" and "localtime" to add 1 hour to the time to allow for Daylight Saving (but "_daylight=true" is ONLY acted on between specific dates). In the ESP8266, it appears to always be set to true by "configTime(x,y,"pool.ntp.org",NULL,NULL);" (irrespective of what x and y are). If no valid "setenv(..); tzset();" is used, then this "_daylight=true" variable appears to cause "localtime_r" and "localtime" to add 1 hour onto the displayed time between 8March 31Oct(inclusive). (ie the US/Canada daylight saving dates). However, if a valid "setenv(..); tzset();" is set, then the "setenv/tzset" command will adjust the "_daylight" variable according to what the "TZ" string sets, AND seems to also adjust the dates between which "localtime_r" and "localtime" will act on a "_daylight=true" setting. Thus for the ESP8266, if you wanted the US/Canada daylight saving dates to be used (with for example an GMT+8hr shift), you could just use: However, if you want different daylight saving dates, then using a valid "setenv/tzset" command such as: (My error was using an incorrect "TZ" string which was therefore being ignored, which then caused the ESP8266 to revert to the US/Canada daylight saving dates. - If you have a correct "TZ" string, then manually changing "_daylight" is not required.) (For the ESP32, the "configTime(..)" command does not set "_daylight=true", so never causes a daylight saving hour to be added if no valid "TZ" string is available.) My updated test-program is as follows:
|
@Rob58329 that sounds like a new/different bug from this one. I would create a new issue for it. |
Using the Based on the suggestion in #4637 (comment), I changed the Changing it to use the new |
@osresearch Attached is an updated example that also has better comments and demonstrates how to use the local time functions with a TZ environment variable with: RTC only, NTP only, RTC and NTP. |
Hi Bill, thanks for posting the latest mini example code (3 October) It appears that code is for the ESP8266, can it be used on an ES32? Or, if not in its current form, what changes would be needed please? I have been trying to use the EZtime library but I cannot get it to work reliably on the ESP processors. The ESP example of simple time works well but doesn't include DST. |
From what I can read in @bperrybap 's example, only the edit |
@d-a-v actually |
That's right. |
duh... kind of weird to use compat wrapper in example, but it is what it is - |
Hi d-a-v and vortigont, thank you for the replies. I didn't understand wrappers and overloads, but if I use configTzTime on the ESP32 the rest of the mini example code should work ok, is that correct? |
No because the esp32 platform does not support setting a call back for when the time is set. --- bill |
I changed configTime to configTztime, code compiles but crashes the ESP32. Bill I am not bothered about when the time is set, so I'll try deleting that code. |
No joy I am afraid, it looks like the mini example needs some changes beyond me to figure out to run on the ESP32. It would be nice to have a similar example for the ESP32 at some point. |
vortigont , Thank you I will try your library later on. |
Platform
Settings in IDE
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
The text was updated successfully, but these errors were encountered: