Skip to content

NTP Timezone works the other way around #680

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

Closed
luc-github opened this issue Sep 29, 2017 · 21 comments
Closed

NTP Timezone works the other way around #680

luc-github opened this issue Sep 29, 2017 · 21 comments

Comments

@luc-github
Copy link
Contributor

Hi as reported alsohere #403 (comment)
I also confirm current behavior on ESP32 is for time zone +1 need to set -1
configTime(-3600, 0, "my.ntp.server", NULL, NULL);
and for time zone -1 need to set +1
configTime(3600, 0, "my.ntp.server", NULL, NULL);

on ESP8266 the behaviour is correct : for time zone +1 need to use +1
configTime(3600, 0, "my.ntp.server", NULL, NULL);

I did a script to compare same code on both platform : https://gist.github.com/luc-github/9511ed8e4bf8adb0f6cf40e81381aa53

@me-no-dev
Copy link
Member

cmon man :D:D:D make a PR to fix this :D

@luc-github
Copy link
Contributor Author

luc-github commented Sep 29, 2017

ok will do after testing your suggestion from gitter, to be sure which part to PR:relaxed:

@luc-github
Copy link
Contributor Author

luc-github commented Sep 29, 2017

Ok after reading several docs and man about TZ :

  • TZ needs to set it with opposite sign as time zone difference with UTC/GMT
    For China is UTC+8:00 => TZ=UTC-8:00
    So configTime function need to be called like :
    setTimeZone(-gmtOffset_sec, daylightOffset_sec);

  • CST is Central Standard Time / Central Time (Standard Time) which is UTC -6
    TZ variable use CST as reference
    so for Taipei the offset for CST is 14, hours but actually the correct result is get when using 8 hours difference which is UTC related. (taipei : UTC+8=> TZ=UTC-8)
    so I would suggest to avoid confusion to change CST by UTC and CTD (saving time flag related to CST) by DST as ESP32 TZ seems not care about letter meaning but only offset
    As results we should have:
    for Taipei : TZ = UTC-8
    for Paris : TZ= UTC-1DST-2

I may be wrong as it is first time I dig on this kind of topic but this is my understanding

If Ok I can do PR to change:
setTimeZone(-gmtOffset_sec, daylightOffset_sec);
and CST->UTC / CDT->DST

@MarkusAD
Copy link
Contributor

I'm wondering if this is actually a good idea because its the underlying sdk routines that imply the sense that a UTC offset east of GMT is negative and offsets west are positive. For example in esp-idf the TZ info for Italy is set by:

setenv("TZ", "CET-1", 1);

When actually Italy is 1 hour ahead of UTC or UTC+1. The current routines configTime() and configTzTime() agree with this, so in effect they are all in agreement as they are currently written in both esp-idf and in arduino core.

@luc-github
Copy link
Contributor Author

luc-github commented Sep 30, 2017

the oposite sign needed to TZ vs UTC is what I read here : https://knowledgebase.progress.com/articles/Article/000044163
and match my tests as well
you suggest the user need to put the offset negative ?- but in that case the ESP8266 is working differently for same function

@MarkusAD
Copy link
Contributor

Im not saying it doesn't look backwards to us. Yes, offsets west of UTC are usually negative. I'm saying it needs to be consistent across the 2 arduino-core TZ routines and the esp-idf TZ routines. If you want to fix it completely so that everyone agrees that offsets west are negative, it needs to be changed in esp-idf, since thats where the root cause of the described behavior starts.

@luc-github
Copy link
Contributor Author

luc-github commented Sep 30, 2017

Sorry, not sure to understand your meaning, setTimeZone function is not in IDF, only in arduino-esp32,
no need to change TZ behavior - just feed parameter properly, which is not currently the case in setTimeZone for what I can see

@MarkusAD
Copy link
Contributor

dude, if you change setTimeZone, then configTzTime() is still backwards and the way it works in esp-idf is still backwards. If youre gonna fix one of them, why not fix them all?

@luc-github
Copy link
Contributor Author

luc-github commented Sep 30, 2017

Hmmm Because for example china (GMT+8)
configTzTime("UTC-8:00","time.nist.gov", "0.pool.ntp.org", "1.pool.ntp.org"); is correct, so I do not see why need to change ?
this is what I see here : https://knowledgebase.progress.com/articles/Article/000044163
It looks I miss something

@MarkusAD
Copy link
Contributor

UTC-8 is not correct because China is UTC+8 (if it was correct you would be using "UTC8" or "UTC+8"... the configTzTime() routine suffers from the same problem as setTimeZone() does which should tell you the problem is not in configTzTime() nor in setTimeZone(), the problem starts in lower level routines in the sdk. Sure you can patch it by inverting the sense of the parameter passed to setTimeZone() but you still havent fixed the underlying problem.

@MarkusAD
Copy link
Contributor

Personally I don't see any problem with using it the way it is, even if it looks backwards. If you use the configTzTime() routine its just one extra "-", and it handles DST begin/end dates and times correctly which is definitely a win. As long as the clock is correct, it doesn't really matter how you get there.

@luc-github
Copy link
Contributor Author

Please forgive my ignorance as I am refering the only article I found and shared which said china (GMT+8 )for TZ need to use UTC-8. So I used as a reference and rule.
All others examples I found are using text and not UTC reference: like TZ='Asia/Kolkata'

Now if it is wrong, because you said using TZ UTC-8 is not correct because China is UTC+8 can you share some reference so I can learn ? I am not familiar with TZ and would be happy to improve

And as said it is consistency issue between ESP8266 and ESP32 configTime , parameter can be set negative when calling configTime,

@MarkusAD
Copy link
Contributor

MarkusAD commented Sep 30, 2017

My apologies, you are correct. TZ is backwards from the normal sense of +/- offset. I must be losing what few brain cells I have left, or I stayed up too long. So there is no problem with idf and since configTzTime() works, you are correct its only setTimeZone() that needs to invert the sense of offset. I stopped using configTime() since the clock was never correct. Perhaps you can just use the numeric offset and UTC for the zone for that.

@luc-github
Copy link
Contributor Author

Oki ^^ thanks for the confirmation - I was starting to get lost ^^

@MarkusAD
Copy link
Contributor

Me too apparently lol.

@MarkusAD
Copy link
Contributor

Drinking too many beers without eating is never a good idea. :\

@me-no-dev
Copy link
Member

entertaining though :D

@MarkusAD
Copy link
Contributor

Hush you, we're all entitled to one brain-freeze for each year over 50. I swear Im going senile, and good thing I only mess with low-voltage stuff now.

@me-no-dev
Copy link
Member

Hahahaha ... my brain is often in a dead loop and I am not even 40 ;) civilized arguing is always good to see :P

@luc-github
Copy link
Contributor Author

Ok PR done #685

@copercini
Copy link
Contributor

Closed due it was solved, Thanks @luc-github !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants