Fix for Time.zone issue.

[kennethlimcp]

This will fix https://github.com/spark/firmware/issues/280 for:

1.) Changed Get_UnixTime to factory in the time_zone_cache parameter

2.) Change the Time.zone() formula to multiple GMT_offset by 1800 instead 0f 3600 as tests showed that the original increase/decrease 2 hours per UTC

Sample code for test verification is here: https://gist.github.com/kennethlimcp/fe25df7bc1619654ff76

[bkobkobko]

Hi @kennethlimcp

Line 130 has syntax error--you cannot do += and return all in one line.

Can you explain the 3600 vs 1800 thing more? I am in the dark on this one.

[kennethlimcp]

Hey @bkobkobko,

I tested with the code above and the printouts show a +/- 2 hour per UTC change.

Opps. I didn't know about the return not being able to handle that. How can I change that?

I basically factored in the time zone cache when retrieving the unix time that is done in other functions of the Time functions but not this one I fixed.

[bkobkobko]

Maybe it is the caller that should be fixed since Get_UnixTime() seems to return the "raw" time but Get_CalendarTime() returns the time corrected in the way that I think you want. (I assume you are just calling Time.now().)

I have doubts about the 3600 vs 1800 change. Maybe @m-mcgowan can lend a eye here.

[andyw-lala]

Agreed - I was suspicious of that too.

On Fri, Oct 24, 2014 at 9:56 PM, Brian Ogilvie notifications@github.com
wrote:

Maybe it is the caller that should be fixed since Get_UnixTime() seems to
return the "raw" time but Get_CalendarTime() returns the time corrected in
the way that I think you want. (I assume you are just calling Time.now().)

I have doubts about the 3600 vs 1800 change. Maybe @m-mcgowan
https://github.com/m-mcgowan can lend a eye here.

—
Reply to this email directly or view it on GitHub
#334 (comment).

Andy

[technobly]

/* Get Unix/RTC time */
static time_t Get_UnixTime(void)
{
    time_t unix_time = (time_t)HAL_RTC_Get_Counter();
    unix_time += time_zone_cache;
    return unix_time;
}

I also question the change to 1800. 3600 is the number of seconds in an hour, so multiplied by the number of hours as an offset makes perfect sense when adjusting the offset of time in seconds.

Try different offsets besides +/- 2 and I think you'll find problems with 1800.

[kennethlimcp]

I'll try again tonight! Original test with unmodified code shows that there's a jump of +/- 2 hour even though the zone only change by +/- 1.

Sorry for the confusion. Just trying to contribute a fix with my limited skills :p

Where's a better place to fix it? Should getting unix time return the raw time the time synced with the server without UTC offfset?

[andyw-lala]

Unix time should be UTC, yes.

Always.

On Fri, Oct 24, 2014 at 11:07 PM, Kenneth Lim notifications@github.com
wrote:

I'll try again tonight! Original test with unmodified code shows that
there's a jump of +/- 2 hour even though the zone only change by +/- 1.

Sorry for the confusion. Just trying to contribute a fix with my limited
skills :p

Where's a better place to fix it? Should getting unix time return the raw
time the time synced with the server without UTC offfset?

—
Reply to this email directly or view it on GitHub
#334 (comment).

Andy

[m-mcgowan]

Just looked over this now - the changes are that the cache value is not checked and the time refreshed on each call. Can someone say what problem this fixes? I would like to see a unit test to illustrate the problem and prove the fix.

I don't see why we need to avoid the cache. Instead we can invalidate the cache when the timezone is changed, in addition to when the time changes, since the time and timezone make up the data used to generate the cached calendar value.

From inspection I see the original code (that uses the cache, but doesn't invalidate with a timezone change) has a bug:

int h = Time.hour();
Time.setTimezone(5);
int h2 = Time.hour();
assertEqual(h+5, h2);   // this will fail

It will fail because the cache isn't rebuilt when the timezone changes.

[m-mcgowan]

This issue has been recently raised again, and I fixed it in #508. Thanks for the contribution!