ezTime: Timezone date / time functions not working properly with time_t values
The TimeLib library allows using hour(now()) to get the current hour so it would seem reasonable that local.hour(local.now()) should work as well as hour(now()) (when the local timezone is set to be the default) , but it doesn’t.
Based on the above assumption that something like local.hour(local.now()), should work as it is passing in the time_t value from the local timezone object and simply attempting to get the hour in the local timezone, there appears to be an issue related to using the date / time functions like hour() minute() etc… when a time_t value is passed in. Example: local is not the default timezone. now() - works as expected and returns the standard unix time_t local.now() - returns a munged time_t value that is offset by the current local timezone offset. local.hour() - works as expected and returns the hour in the local timezone
local.hour(local.now()) - returns the hour in UTC timezone not the hour in the local timezone. local.hour(local.now(), LOCAL_TIME) - same as above These seem incorrect, as there appears to be no way to get from a time_t value returned by the local timezone object to a local time value.
local.hour(UTC.now(), UTC_TIME) - works as expected and returns the hour in the local timezone
If you set the default timezone to local by using local.setDefault() then: now() - returns a munged time_t value that is offset by the current local timezone offset. local.hour(now()) - returns the hour in UTC timezone not the hour in the local timezone hour(now()) - returns the hour in UTC timezone not the hour in the local timezone
While these examples are very simplistic actual code may do something more like:
t = now();
// other code
hourvalue = hour(t);
minvalue = minute(t);
// or
sprintf(buf, "%02d:%02d:%02d", hour(t), minute(t), second(t));
And that doesn’t work with eztime when using something other than the UTC timezone.
It seems that there should be a way to get from a time_t value returned by the timezone object to a local time value using the timezone methods.
I think much of these kinds of issues relate to the issues brought in in issue #10 where the time_t values returned by the timezone objects are not the actual time_t value but rather a munged value for the local timezone.
In this specific case it appears that tzTime() is always called by the various timeszone methods like hour() and when the local_or_utc value indicates the value is LOCAL_TIME, tzTime() flips it to UTC , the UTC time_t value is used and then the UTC time value is returned instead of the local timezone value. IMO, that is not the expected behavior.
While all this could be fixed to work, even when using munged time_t values, as the code is currently doing, local.hour(t) or local.hour(local.now()) could work when time_t value was talways he proper unix time_t value.
I think things would get much simpler and become consistent with the way time_t epoch values are supposed to work if all the now() functions always returned the unix time_t value which is the UTC time_t. Yes there would be some need for internally doing conversions when doing the local value conversions, but that wouldn’t need to be exposed to the users. Everything should still work and be compatible with the TimeLib API.
It would also make now() lightweight and very fast, as it should be. Currently tzTime() is called 3 or more times just to get a time_t when some thing like local.now() is called which definitely adds in quite a bit of overhead that would go away. makeTime() and breakTime() should be the expensive functions that deal with all the timezone issues and should be moved to be in the Timezone class which can handle the local timezone conversions. Then, all the timezone methods like hour() would first call breakTime() which would do the needed timezone offset conversion, then pick out the needed value and return it. users could call also local.breakTime() to get all the local time values at once if they desired. makeTime() and breakTime() could be left in the ezt class and call the default timezone version just like some of the other functions.
About this issue
- Original URL
- State: open
- Created 5 years ago
- Reactions: 3
- Comments: 22 (1 by maintainers)
I had some internet issues that took me offline for a bit, and some other personals things that consumed me. I’ll reach out to rop soon to see if we can figure out a way to get things moving.
Hey there,
Sorry to be a bit silent lately. I agree with @bperrybap about what needs to be done. A next major version is iin order, breaking corner cases is not a big deal. And I would help out getting it done quickly if it weren’t for me having to get some things done related to that which have to take priority.
Related but not urgent: timezoned also needs to be standardised into something that is futureproof and run on a few more machines.
How about we make ezTime a GitHub organisation and I make @bperrybap the other maintainer? I’ve just done this for M5ez, one of my other projects. We then use gitter.im to talk about things, and it works well.
@bperrybap: shall we video-conference about this soon? Please reach me at rop at rop dot nl
It sounds like the problem. The basic problem is that there is a design flaw in the library. The library doesn’t handle the timestamp and local conversions correctly. So depending on what calls you make and if you pass in a time_t timestamp to a function, you may or may not get the proper local time values. And ALL the calls using a time_t parameter like local.hour(t) or hour(t), fail to return the proper local time.
IMO, this makes the library completely unusable. Which is sad because I think it has the potential to be a really great and useful library.
I think the library would be great if it were refactored to use time_t values correctly as I noted previously and in issue #10 (timezones do not affect timestamps)
@teemue In terms of your specific example:
I’ve written about this a few times, here and in other issues. But here is a summary: Once ezTime is fixed use a proper time_t the the time_t value will never be a bastardized “local” time_t anymore. This means that makeTime() has to know if the values it is being passed are UTC value or local value. The easiest way to handle this would be to add the makeTime() method to the Timezone class so the user can explicitly specify which timezone the local values represent and the library code can deal with any necessary conversions accordingly. As a convenience, the library code could (should IMO) also make the global makeTime() function redirect to the appropriate Timezone class object makeTime() using defaultTZ just like so many of the other functions. So makeTime() would be redirected once setDefault() was called in the Timezone object. This offers the best of all worlds. The user can set up a Timezone object and set it as the default so all the calls work just like they do in TimeLib. and things like makeTime() “just work” since they know the local timezone and can do the any needed conversion to create the proper time_t value. And if a person wants to explicitly specify a conversion they can use the TimeZone object directly or specify UTC values using the built in UTC object. So for example once the user set up myTZ as the timezone, he calls myTZ.setDefault() Then all calls to makeTime() would assume values specified were local values for myTZ timezone. If he wants UTC he could use UTC.makeTime() likewise he could explicitly specify the timezone and use myTZ.makeTime() OR, he could even access or use other timezones. example:
You cannot do this with ezTime today as the code incorrectly handles the conversions.
Things can actually get much simpler and clearer when real time_t values are used for both the library code and the sketch code. This is especially true for applications that use a single timezone. From a sketch perspective, one that used to use TimeLib, all it has to do is a few upfront things, then all the exiting code should “just work”. Create the TimeZone object, initialize it, then call setDefault() to use it for all the TimeLib API functions. The ezTime code would then know it needs to convert from the time_t to get local broken down values, and that any local broken value specified is a local time value based on the default timezone. All while time_t is never modified even if timezones are changed.
With the current ezTime implementation, this is definitely not the case as there are many cases where ezTime returns the incorrect local time values.
Ran into this issue as well after spending hours struggling to understand what was going wrong while assuming the code actually did something. Might be a good idea to take the lib off public repo, or put a notice on the main repo page till a fix is in so that others don’t spend hours on something that inherently doesn’t work?
@bitboy85 that is a terrible solution as it creates a new API that is not compatible with TimeLib. Fixing the ezTime library code to work correctly (to work the same as the TimeLib API) doesn’t break compatibility with TimeLib API, it simply fixes the ezTime code to work the same as the TimeLib API. Creating new wrapper functions totally breaks compatibility with TimeLib as it means that users can’t use the existing TimeLib API functions they were previously using, which I believe was the goal of ezTime. And even if you were to create those new API functions, you still can’t completely work around the problem as the issue is in the underlying functions that these functions must call. There are also a few issues related to certain local time calculations which are based on calculations using the utc time_t vs the munged up local time_t so they are wrong when the local time is within the time zone offset of the transition.
Regardless of how the ezTime code is fixed, it needs to be fixed because “as is”, LOTS and LOTS of existing TimeLib code is broken when trying convert over to using this library since most of basic and commonly used API functions are broken. Simply put: The functions: now(), hour(), hourFormat12(), minute(), second(), day(), month(), isPM(), isAM(), isDST() don’t work correctly for anything but the utc timezone object. This is a HUGE problem for existing code that would like a simple solution to add timezone support which is the entire point of the ezTime library. IMO, the ezTime library is not usable until this is fixed since the core API functions to get the local time members do not work correctly when using something other than utc which is entire point of using a timezone library.
The real issue is down in the low level code. It makes some incorrect assumptions and does things backwards with respect to tracking time_t values. Things like returning the time_t values should be very fast. In this library they are slow as it does lots of expensive calculations - including DST and DOW calculations on every API call - even if just getting a time_t value. It will do these calculations more than once, including for a single call to get the time_t value. Also, some of the low level ezTime support routines make the assumption that a utc/local or local/utc flip conversion always needs to be done when they called and in some cases this is simply wrong. i.e. if you pass in local you want utc and if you pass in utc you want local. That is what is creating the problem describe in the issue above where the local time hour is wrong.
Here are two ways to fix the code work correctly.
method 1 Fixing the code to always use proper time_t values which would simplify the code and make it faster & leaner. This might break some existing TimeLib users code. But the vast majority or users and existing code would never know the difference. However the amount of existing sketch code that would break from this fix would be much less that amount of existing code that is currently broken because of the issues described earlier. For those case where using proper time_t values do happen to break existing sketch code trying to covert from TimeLib to ezTime, the sketch updates to fix it would be minimal since, for the most part, existing code was doing things that could have been done differently using the existing TimeLib API by calling a slightly different API function to do the work for them vs trying to do themselves. i.e. they were doing some things “the hard way” and could remove some of their code to instead call some TimeLib API functions to do that work instead. But again, the vast majority of existing code would never see any difference. i.e. much less existing sketch code would break from switching to actual time_t values than is currently broken by the issues in the current ezTime library.
method 2 In term of fixing the code to work correctly (i.e. fix the API to work the same as the TimeLib API works) without fixing the ezTime code to use proper time_t values, the issue is that some of the low level ezTime routines don’t look at or use the local_utc parameter properly and some make the assumption that you always want the opposite of what you pass in. THIS is what creates the problem noted in the issue above as a conversion from one to the other is not always needed and THAT is what breaks returning the proper local time as noted in the issue above. This is because the low level code is making an assumption that the time should be converted from utc/local or local/utc rather than converting the time to what the timezone object has been configured for. This is wrong.
I would love to use the eztime library for my clock projects. But “as is”, it is completely unusable since it cannot convert the time_t returned by now() to local elements by using functions like hour(t), min(t) etc… That is very basic functionality that simply does not work. And as pointed out above, it can fixed with or without converting the ezTime code to use proper time_t values.