esp8266 / Arduino

ESP8266 core for Arduino
GNU Lesser General Public License v2.1
15.96k stars 13.34k forks source link

time() does not work without active NTP server #1679

Closed av1024 closed 6 years ago

av1024 commented 8 years ago

Basic Infos

Hardware

Hardware: ESP-12E Core Version: 2.1.0-rc2

Description

"Internal" time.c functions always return 0 /1970-01-01/ if not synced via NTP.

I want to initialize internal clock via constant or from RTC for log timestamping (so I don't need precise time on startup, but valid measured intervals)

Settings in IDE

Module: Generic ESP8266 Module Flash Size: 4MB CPU Frequency: 80Mhz Flash Mode: qio Flash Frequency: 80Mhz Upload Using: SERIAL Reset Method: ck

Sketch


#include <Arduino.h>
#include <time.h>

void setup() {
 // WiFi.begin(); // wifi disabled for example
 // configTime(3*3600, 0, "not.avail.ntp"); 
 Serial.begin(115200);
}

void loop() {
  time_t now = time(nullptr);
 Serial.println(ctime(&now));
 delay(1000);
}

Debug Messages

Thu Jan 01 00:00:00 1970

Thu Jan 01 00:00:00 1970

...

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

everslick commented 8 years ago

I use this workaround for now:

static uint32_t starttime = millis() / 1000;
static uint32_t localtime = starttime;

void time_set(uint32_t time) {
  starttime = millis() / 1000;
  localtime = time;
}

uint32_t time_get(void) {
  return (millis() / 1000 - starttime + localtime);
} 
miky2k commented 8 years ago

I m looking for full software rtc in core libraries too.

av1024 commented 8 years ago

Thanks, @everslick! I have my implementations RTC and/or NTP sync from my "raw C" projects on AVR, but I want to use "embedded" code because it already exists.

@far5893 There is widely known Time library from arduino.cc, but it is not compiled with 2.1.0-rc2 for me. And of course, this is not core lib

@igrr Please take attention to string handling in sntp_xxx code. I have got exception 28 (or sometimes 9) with sntp_asctime[_r] and sntp_sync_xxx approx after 22-24 hours uptime. there is only ctime(&now) calls in my code. One of saved traces:

Decoding 17 resultsException code: 28: LoadProhibited: A load referenced a page mapped with an attribute that does not permit loads
0x402265f9: sntp_asctime_r at ??:?
0x402265f9: sntp_asctime_r at ??:?
0x4020d6cc: TwoWire::requestFrom(unsigned char, unsigned int, bool) at /home/av/.arduino15/packages/esp8266/hardware/esp8266/2.1.0-rc2/libraries/Wire/Wire.cpp:240
0x40226647: sntp_asctime at ??:?
0x4020d6f8: TwoWire::requestFrom(unsigned char, unsigned char) at /home/av/.arduino15/packages/esp8266/hardware/esp8266/2.1.0-rc2/libraries/Wire/Wire.cpp:240
0x40201b44: asctime at /home/av/.arduino15/packages/esp8266/hardware/esp8266/2.1.0-rc2/cores/esp8266/time.c:110
0x4020df5e: Adafruit_BMP280::read24(unsigned char) at /home/av/Arduino/libraries/Adafruit_BMP280_Library/Adafruit_BMP280.cpp:210
0x40209742: check_float(float, float, float&) at /home/av/bin/arduino-1.6.5/WebConfig.ino:311
0x4020986e: readSensors() at /home/av/bin/arduino-1.6.5/WebConfig.ino:311
0x40105c2e: ets_timer_setfn at ??:?
0x4020a534: ESP8266WiFiSTAClass::status() at /home/av/.arduino15/packages/esp8266/hardware/esp8266/2.1.0-rc2/libraries/ESP8266WiFi/src/ESP8266WiFiSTA.cpp:635
0x4020ad5c: WiFiClient::operator=(WiFiClient const&) at /home/av/.arduino15/packages/esp8266/hardware/esp8266/2.1.0-rc2/libraries/ESP8266WiFi/src/WiFiClient.cpp:362
0x40209e08: every_s() at /home/av/bin/arduino-1.6.5/WebConfig.ino:311
0x40209eb8: handle_ms() at /home/av/bin/arduino-1.6.5/WebConfig.ino:323
0x40209ef3: loop at /home/av/bin/arduino-1.6.5/WebConfig.ino:337
0x4020f6f8: loop_wrapper at /home/av/.arduino15/packages/esp8266/hardware/esp8266/2.1.0-rc2/cores/esp8266/core_esp8266_main.cpp:43
0x40100114: cont_norm at /home/av/.arduino15/packages/esp8266/hardware/esp8266/2.1.0-rc2/cores/esp8266/cont.S:109
igrr commented 8 years ago

sntp_xxx functions come from espressif SDK, as far as I can tell.

On Sun, Feb 28, 2016, 13:04 Alexander Voronin notifications@github.com wrote:

Thanks, @everslick https://github.com/everslick! I have my implementations RTC and/or NTP sync from my "raw C" projects on AVR, but I want to use "embedded" code because it already exists.

@far5893 https://github.com/far5893 There is widely known Time library from arduino.cc, but it is not compiled with 2.1.0-rc2 for me. And of course, this is not core lib

@igrr https://github.com/igrr Please take attention to string handling in sntp_xxx code. I have got exception 28 (or sometimes 9) with sntp_asctime[_r] and sntp_sync_xxx approx after 22-24 hours uptime. there is only ctime(&now) calls in my code. One of saved traces:

Decoding 17 resultsException code: 28: LoadProhibited: A load referenced a page mapped with an attribute that does not permit loads 0x402265f9: sntp_asctime_r at ??:? 0x402265f9: sntp_asctime_r at ??:? 0x4020d6cc: TwoWire::requestFrom(unsigned char, unsigned int, bool) at /home/av/.arduino15/packages/esp8266/hardware/esp8266/2.1.0-rc2/libraries/Wire/Wire.cpp:240 0x40226647: sntp_asctime at ??:? 0x4020d6f8: TwoWire::requestFrom(unsigned char, unsigned char) at /home/av/.arduino15/packages/esp8266/hardware/esp8266/2.1.0-rc2/libraries/Wire/Wire.cpp:240 0x40201b44: asctime at /home/av/.arduino15/packages/esp8266/hardware/esp8266/2.1.0-rc2/cores/esp8266/time.c:110 0x4020df5e: Adafruit_BMP280::read24(unsigned char) at /home/av/Arduino/libraries/Adafruit_BMP280_Library/Adafruit_BMP280.cpp:210 0x40209742: check_float(float, float, float&) at /home/av/bin/arduino-1.6.5/WebConfig.ino:311 0x4020986e: readSensors() at /home/av/bin/arduino-1.6.5/WebConfig.ino:311 0x40105c2e: ets_timer_setfn at ??:? 0x4020a534: ESP8266WiFiSTAClass::status() at /home/av/.arduino15/packages/esp8266/hardware/esp8266/2.1.0-rc2/libraries/ESP8266WiFi/src/ESP8266WiFiSTA.cpp:635 0x4020ad5c: WiFiClient::operator=(WiFiClient const&) at /home/av/.arduino15/packages/esp8266/hardware/esp8266/2.1.0-rc2/libraries/ESP8266WiFi/src/WiFiClient.cpp:362 0x40209e08: every_s() at /home/av/bin/arduino-1.6.5/WebConfig.ino:311 0x40209eb8: handle_ms() at /home/av/bin/arduino-1.6.5/WebConfig.ino:323 0x40209ef3: loop at /home/av/bin/arduino-1.6.5/WebConfig.ino:337 0x4020f6f8: loop_wrapper at /home/av/.arduino15/packages/esp8266/hardware/esp8266/2.1.0-rc2/cores/esp8266/core_esp8266_main.cpp:43 0x40100114: cont_norm at /home/av/.arduino15/packages/esp8266/hardware/esp8266/2.1.0-rc2/cores/esp8266/cont.S:109

— Reply to this email directly or view it on GitHub https://github.com/esp8266/Arduino/issues/1679#issuecomment-189833248.

av1024 commented 8 years ago

@igrr Huh :( May be some try/catch over sntp_xxx in esp8266/time.c ? But I'm not sure this may help

Juppit commented 8 years ago

Maybe you will have a look at this library: https://github.com/Juppit/esp8266-SNTPClock

It was developed from a version with ticker library without network access. Therefore uncommenting the network stuff it should still give you time functions.

Am 28.02.2016 um 11:30 schrieb Alexander Voronin:

@igrr https://github.com/igrr Huh :( May be some try/catch over sntp_xxx in esp8266/time.c ? But I'm not sure this may help

— Reply to this email directly or view it on GitHub https://github.com/esp8266/Arduino/issues/1679#issuecomment-189837221.

Juppit commented 8 years ago

Wrong thread, I guess. There was someone looking for time finctions without network.

av1024 commented 8 years ago

@Juppit Your version have same issue as this - system snmp_xxx functions does not provide correct time if no ntp sync was made prior. Another bug is in SNTPClock::begin - while(!m_secsSince1900) loop never ends if no network/success ntp sync made

igrr commented 8 years ago

These are not c++ exceptions, these are CPU exceptions, so try/catch won't help here. Do you have a test case which is failing? I'd rather try to reproduce it and report to Espressif.

av1024 commented 8 years ago

@igrr I don't know how to reproduce it a bit stable. I'll try write minimal code only for timesync/print just now, but I have no idea how to speed up testing - 20+ hours before exception.. may be wifi connection issue while sync, may be some string formatting...

igrr commented 8 years ago

20 hours is not that bad, I can probably hook it up to gdb and let it run until it crashes.

On Sun, Feb 28, 2016, 14:50 Alexander Voronin notifications@github.com wrote:

@igrr https://github.com/igrr I don't know how to reproduce it a bit stable. I'll try write minimal code only for timesync/print just now, but I have no idea how to speed up testing - 20+ hours before exception.. may be wifi connection issue while sync, may be some string formatting...

— Reply to this email directly or view it on GitHub https://github.com/esp8266/Arduino/issues/1679#issuecomment-189851050.

kaeferfreund commented 8 years ago

hey, if I got this right you are looking for some RTC capabilities. I have used the "TimeLib.h" for that. To initialize the time, I send a head request to a google server. I sync it every night and it's running since 2 weeks now...

av1024 commented 8 years ago

@kaeferfreund Nope. I am looking for system capability to work w/o internet/NTP server. I know about TimeLib (distributed now as arduino.cc's Time.h). My use case is sensor, that may be sometimes offline and may go online w/o internet/NTP access. I plan to use DS3231 as backup RTC in my v3.1 PCB design but I just don't want to use third party lib where an system implementation exists.

@igrr, 5hrs. Still working. Neither blocking port 123 on router, nor disabling wifi access does not reproduce fail. But for now I know time intervals of internal time sync )) - if NTP accessible, then sunc performed every hour, if not - every approx 33seconds. PS: I am not familiar with gdb, but try tomorrow at work (not sure it will work on cubietruck, sot first try on PC)

av1024 commented 8 years ago

@igrr Three day test - all ok, no crash. May be an issue with printing asctime(nullptr) - this call immediately throws exception 28. But I have no code with this call, only commented out from old exercises.

I'll try with more complex code and open separate issue if found something.

everslick commented 8 years ago

FYI: I have the feeling, that stack growth has to do with Exception(28). I could not find out yet, how to measure stack usage. I guess the stack collides with heap occasionally. I could prevent Exception(28 by moving stack variables to the heap.

On Wed, Mar 2, 2016 at 4:32 PM, Alexander Voronin notifications@github.com wrote:

@igrr https://github.com/igrr Three day test - all ok, no crash. May be an issue with printing asctime(nullptr) - this call immediately throws exception 28. But I have no code with this call, only commented out from old exercises.

I'll try with more complex code and open separate issue if found something.

— Reply to this email directly or view it on GitHub https://github.com/esp8266/Arduino/issues/1679#issuecomment-191288577.

igrr commented 8 years ago

You can measure stack usage for the code which runs on Arduino task (i.e. called from setup and loop). See https://github.com/esp8266/Arduino/blob/master/cores/esp8266/cont.h#L65. For code which runs on other tasks, there is no way to check this at the moment.

Edit: the cont_t structure which has to be passed into this function is not currently exposed to the user. It is declared here: https://github.com/esp8266/Arduino/blob/master/cores/esp8266/core_esp8266_main.cpp#L64. I think you can declare it as extern in your sketch as a workaround.

av1024 commented 8 years ago

In my case Exception 28 usually indicate incorrect usage of pointers - asctime(NULL) as above and mostly print(PSTR/FPSTR/F(...)). In most cases I use something like print(PSTR("debug")) instead of print(F("debug")) and got error as should.

UPD: tested with cont_get_free_stack(). Got about 3136-3144 of 4096 bytes, looks ok )) and 35200 bytes of heap (I2C, Web Server, 5x100words sensor logs,1500bytes String in web server handler)

everslick commented 8 years ago

@Ivan: thanx for the hint. but i'm not sure, if i do it right:

i create a cont_t object:

static cont_t stack;

in setup() i call: cont_init(&stack);

in loop() i do: LogPrint(" free stack: %i bytes\n", cont_get_free_stack(&stack)); LogPrint(" stack guard: %s\n", cont_check(&stack) ? "CORRUPTED" : "OK");

every two seconds.

but the log always reads: free stack: 4100 bytes

do i have to spawn a separate main loop with my cont_t object using cont_run() ?

On Wed, Mar 2, 2016 at 6:13 PM, Alexander Voronin notifications@github.com wrote:

In my case Exception 28 usually indicate incorrect usage of pointers - asctime(NULL) as above and mostly print(PSTR/FPSTR/F(...)). In most cases I use something like print(PSTR("debug")) instead of print(F("debug")) and got error as should.

— Reply to this email directly or view it on GitHub https://github.com/esp8266/Arduino/issues/1679#issuecomment-191329715.

igrr commented 8 years ago

Not exactly, you need to use the existing object.

extern "C" cont_t g_cont;

loop() {
  size_t free = cont_get_free_stack(&g_cont);
  // etc
}

P.S. for further discussion please visit our gitter chat or open a separate issue, let's not hijack the original issue :)

everslick commented 8 years ago

On Thu, Mar 3, 2016 at 4:32 PM, Ivan Grokhotkov notifications@github.com wrote:

extern "C" cont_t g_cont;

ahh, thx. without calling cont_init() on it i guess ?

hariprasad-d commented 7 years ago

HI, Is this issue still open or closed? Having the same problem here, time() api returns "Thu Jan 01 00:00:00 1970". looks like sntp.c never gets compiled from my Arduino IDE. Had inserted asserts under sntp_request and also enabled LWIP_DEBUGF debug logs. No core or logs seen.

joaohenrifranco commented 6 years ago

Same problem here. Any workarounds?

5chufti commented 6 years ago

it looks like a) s_bootTime only gets set on first use of _gettimeofday_r() what seems to be some helper function for the newlib libc. b) s_bootTime is only regarded via libc functions. time() definitely does not care about s_bootTime.

So no changes when using bare metal sntp functions ...

to me it would somehow make sense if the status of sntp_init() in configTime() would be stored as flag and subsequent requests for a timestamp would return either an actual fresh timestamp or the (s_bootTime +) millis()/1000 depending on this flag. Optional an input_timestamp (from some rtc_chip) could be provided as parameter in configTime() to be used to calculate s_bootTime if sntp_init() fails.

d-a-v commented 6 years ago

With current master, the above sketch works

extern "C" void sntp_set_system_time(uint32_t);

and in setup():

sntp_set_system_time(0);

Similar could be done with lwip1.4 but not without patching source and recompiling, just because all functions and variables are statically linked in sntp.c (which has public sntp_get_current_timestamp() used by ctime()).

edit: The main goal here, in both case, is to install the os_timer_setfn(sntp_time_inc) function.

5chufti commented 6 years ago

somehow the whole time() thing looks pretty uncared of and broken ... best to just use bare metall functions and 3rd party time(zone)lib.

P.S. with sdk sntp functions I can't connect to timeservice on M$servers, so I have to use "hand coded" timeservice after all.

P.P.S.: I am using current master and get the same result as the OP (lwip variant set to V2)

d-a-v commented 6 years ago

With example sketches, we can try and help.

PS: How do you use your ntp servers ?

PPS: The OP has to be modified with the two lines I showed. We could propose a PR to make this automatic (without these two lines). sntp timer would be initialized at boot and reinitialized when dhcp or user gives a ntp server. Note that lwipv2 automatically uses dhcp-advertised ntp servers. Initially for me the OP sketch was working unmodified with current NTP time, and I had to ESP.eraseConfig() to go back 50 years ago.

5chufti commented 6 years ago

ok, I can confirm that sntp_set_system_time(0); helps a lot. I modified time.c / configTime()and inserted this after sntp_set_timezone() and now the call to configTime() starts the "fake rtc" and - if WiFi is working and the server responds - eventually the time will be correct even if WiFi drops later on. Still an optional parameter to configTime() to set time to a value taken from rtc chip would be nice to have. After this simple single initialisation all time operations could be done via standard functions.

now someone would have to set up a fake ntp server to check what dalightsaving rules espressif have implemented ..

P.S.: I use the M$ server exactly the same: just use its name instead of pool.ntp.org. With hand coded udp requests I get an answer, sdk functions don't ... Unfortunately I can't use wireshark, my IT-dept ...

#include <ESP8266WiFi.h>
#include <time.h>

#define ssid "your_ssid"
#define wpwd "your_secret"

// extern "C" void sntp_set_system_time(uint32_t);

void setup() {
  Serial.begin(74880);
  WiFi.persistent(false);
  WiFi.mode(WIFI_STA);
  WiFi.begin(ssid, wpwd);

  configTime(3600, 3600, "pool.ntp.org");

//  sntp_set_system_time(0);
}

void loop() {
  time_t tnow = time(nullptr);
  Serial.println(ctime(&tnow));
  delay(1000);

  if (millis()==30000) {
    WiFi.disconnect();
    WiFi.mode(WIFI_OFF);
    Serial.println("WiFi off");
  }
}
d-a-v commented 6 years ago

You can fake using an external RTC with sntp_set_system_time(secs-since-epoch). I fixed lwip2 so:

a PR is soon on its way

5chufti commented 6 years ago

ok, so I see that the issue will be fixed with bells-and-whistles. To spare you of further issues like this I suggest to do at least some rudimentary documentation on how to use the core time.c and the necessary preconditions.

5chufti commented 6 years ago

@d-a-v Hi, had finally time to check your changes (used your fork) and time now starts w/o prior contact to ntp server, BUT a) I was not able to use sntp_set_system_time(0); without prior declaration b) TZ is somehow wrong.

if you test my small sketch with master, you will get following output: with correct offset for MEZ before & after sync with ntp server as done on Sun Nov 12 12:09:06 2017 (shortly after noon). edit: BUT even with "sntp_set_system_time(1499893466)" (a timestamp during DST period) it shows only localtime as MEZ not MESZ (ignoring dst offset), so proofing #2505 as valid.

 ets Jan  8 2013,rst cause:2, boot mode:(3,6)

load 0x4010f000, len 1384, room 16 
tail 8
chksum 0x2d
csum 0x2d
v00000000
~ld
Thu Jan  1 01:00:00 1970
Thu Jan  1 01:00:01 1970
Thu Jan  1 01:00:02 1970
Thu Jan  1 01:00:03 1970
Thu Jan  1 01:00:04 1970
Thu Jan  1 01:00:05 1970
Thu Jan  1 01:00:06 1970
Sun Nov 12 12:09:06 2017
Sun Nov 12 12:09:07 2017
Sun Nov 12 12:09:08 2017

with your git it gives (with / without sntp_set_system_time(0))

 ets Jan  8 2013,rst cause:2, boot mode:(3,6)

load 0x4010f000, len 1384, room 16 
tail 8
chksum 0x2d
csum 0x2d
v00000000
~ld
Thu Jan  1 02:00:00 1970
Thu Jan  1 02:00:01 1970
Thu Jan  1 02:00:02 1970
Thu Jan  1 02:00:03 1970
Thu Jan  1 02:00:04 1970
Thu Jan  1 02:00:05 1970
Thu Jan  1 02:00:06 1970
Sun Nov 12 13:13:14 2017
Sun Nov 12 13:13:15 2017
******************************************************

 ets Jan  8 2013,rst cause:2, boot mode:(3,7)

load 0x4010f000, len 1384, room 16 
tail 8
chksum 0x2d
csum 0x2d
v00000000
~ld
Thu Jan  1 08:00:00 1970
Thu Jan  1 08:00:01 1970
Thu Jan  1 08:00:02 1970
Thu Jan  1 08:00:03 1970
Thu Jan  1 08:00:04 1970
Thu Jan  1 08:00:05 1970
Thu Jan  1 08:00:06 1970
Sun Nov 12 13:14:18 2017
Sun Nov 12 13:14:19 2017

as you can see in first case ds offset is wrongly applied in second case TZ seems to remain standard sdk value first, correct TZ only after sync with ntp server but ds offset again wrongly applied

d-a-v commented 6 years ago

sntp_set_system_time() is exposed in lwip include files. You must include <arch/cc.h> or anything from lwip2 to have it. I don't know if I should make it exposed for lwip1.4 too...

About DST, I just tried it again: It is now Mon Nov 13 01:39:01 CET 2017 on my laptop and my wall-clock (though the latter does not tell me TZ, and DST was sadly set about 2 weeks ago by hand... but agrees with my computers). With winter time configTime(3600,0,"ntp.pool.org") which is MEZ (and CET), it esp-sketchly is Mon Nov 13 01:42:02 2017. With summer time configuration configTime(3600,3600,"ntp.pool.org") which is MESZ (and CEST too) esp says Mon Nov 13 02:42:48 2017.

In your two last examples, what where configTime() parameters ? How could you verify the validity of the patch with 1499893466 ? I guess you have the matching local time. It was in July'17, did you use it with (TZ,DST) = (3600,0)=(edit)MESZMEZ or (3600,3600)=MEZMESZ ?

5chufti commented 6 years ago

HI, ok there is not much sense in just adding dst-offset if it is present and not according to a dst rule. If I have to decide if dst applies or not prior to call of configTime() I can just add it to the TZ value...

So to sum it up: as long as (underlying sdk) function does not use dst-rule for applying dst-offset (and this is plausible explanation for observed problems) it is of no use

d-a-v commented 6 years ago

DST is a difficult problem which seems not to be properly solved though I am not a specialist. There are two main cases, when you can access to internet, or when you can't. Check this arduino lib and also this. If someone comes with a useful library, it might be worth a PR.

Juppit commented 6 years ago

Take a look at this.

So what about timestamps in periods with other rules? I have just learned from it that e.g. in China was no DST after 1991.

5chufti commented 6 years ago

I would have been "too good to be true" to have correct dst rules in sdk time functions. Without major changes (forget about TZ as integer, string needed) there is no decent implementation. With only integer TZ=XY you still could be in an area with different offset (0, 1800, 3600) or with different start/end dates, therefor textual TZ is needed. So the achievable optimum is reliably start time() on startup with TZoffset from configTime() to get UTC, have sntp_set_system_time() properly exposed (including cc.h is even worse than have declaration). Either place it in time.c or have ESP.time(), ESP.configTime() and ESP.sntp_set_system_time() Then it is up to the user to have correct TZdst handling via some TZ lib.

@Juppit unix-like "timestamps" are allways in UTC (ok, not really, no leapseconds) and have to be "translated" to local time. "DateTime" timestamps should never be used w/o postfix giving actual offset to UTC.

d-a-v commented 6 years ago

@igrr (@devyte ?) What do you think of exposing sntp_set_system_time() globally (in #3808) along with configTime() in Arduino.h ? or with a better arduinishCapsed name like setTime()/setEpoch() ?

igrr commented 6 years ago

How about overriding settimeofday to call sntp_set_system_time? That would at least be posix-y and also compatible with the ESP32, where settimeofday/gettimeofday work as one would expect.

On Mon, Nov 13, 2017 at 9:55 PM, david gauchard notifications@github.com wrote:

@igrr https://github.com/igrr (@devyte https://github.com/devyte ?) What do you think of exposing sntp_set_system_time() globally (in #3808 https://github.com/esp8266/Arduino/pull/3808) along with configTime() in Arduino.h ? or with a better arduinishCapsed name like setTime()/setEpoch() ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/esp8266/Arduino/issues/1679#issuecomment-343925688, or mute the thread https://github.com/notifications/unsubscribe-auth/AEJceoRUxdUGcDGW6L9FsPpc3sdsT-_0ks5s2EpugaJpZM4Hiela .

d-a-v commented 6 years ago

@5chufti I put a library example esp8266/NTP-TZ-DST. @igrr I did not try to fix gettimeofday @BrandonLWhite is working on it

BrandonLWhite commented 6 years ago

I think implementing settimeofday as @igrr suggests sounds like the right approach. I try to prefer use of existing CRT or POSIX APIs vs. introducing new stuff like low-level lwip functions.

I completed the work for getting gettimeofday functional just now, here in a new squashed PR: #3830.

So what I'd recommend doing for a settimeofday implementation would be to extract my code

s_bootTime_us = now_s * 1000000ULL - micros64();
s_bootTimeSet = true;

into a new method, then call it for both ensureBootTimeIsSet as well as settimeofday.

d-a-v commented 6 years ago

I guess @igrr should take your PR first, then I'll adapt mine to yours.

BrandonLWhite commented 6 years ago

@5chufti

I would have been "too good to be true" to have correct dst rules in sdk time functions.

I agree. I like to think of timezones, DST, localtime as a user interface display concern. Logic and event/data logging should stick to UTC. You can certainly convey what the timezone is for the device, so that other systems can be aware and format accordingly. As has been mentioned, you should use a textual ID such as an Olson ID. A simple UTC offset is generally not good enough.

So, I agree that timezone and DST support is not within the scope of the core lib. If someone really wants or needs to build localized times in their firmware that should be accomplished with libraries. That would have to pull in all of the tz database and logic and that's quite some baggage! That's why I'd avoid it in an MCU. Even if you are serving a web app straight from the ESP8266, you should use a JS client lib like moment.js to format time displays in a human format. That can even be done with regard to future changes to DST rules without having to update your device firmware!

Thanks, just my 2 cents on the matter.

igrr commented 6 years ago

Merged #3818, thanks!

igrr commented 6 years ago

One data point the topic of timezone/dst support. Newlib (C library which we use) knows how to handle timezones and DST. Application needs to supply the timezone description (which may include DST rule), and newlib will take it into account when e.g. localtime_r is called. So an application can use it like this: https://github.com/espressif/esp-idf/blob/master/examples/protocols/sntp/main/sntp_example_main.c#L76-L87

Obviously the application needs to store the list of timezone descriptions somewhere (possibly mapped to user-friendly names of regions or major cities in each timezone). That, indeed, should be application's responsibility. But there is a project which can help generating TZ strings from the timezone database: https://github.com/nayarsystems/posix_tz_db.

5chufti commented 6 years ago

@BrandonLWhite Hi, maybe I am wrong but isn't the whole "save bootTime" thing the wrong way around? With your patch (in progress) we allways will have the "bootTime" for at least as long as int64 µs is worth (AFAIK more than a lifespan).

how about for time() just:

°) get real timestamp via sdk
°) if valid (t != 0) store diff to micros64() for later and return 
      tv_sec=timestamp
      tv_usec=micros64() % 1000000
°)  if not successful just return 
      tv_sec=(micros64() + diff)/1000000
      tv_usec=micros64() % 1000000 

this should return time from boot until ntp sync and +/- UTC after first sync?

reminder: we still need the correction of hardwired ntp servers from d-a-v patch. edit: for use with rtc chip, sntp_set_system_time() would just update "diff".

BrandonLWhite commented 6 years ago

@igrr

One data point the topic of timezone/dst support. Newlib (C library which we use) knows how to handle timezones and DST. Application needs to supply the timezone description (which may include DST rule), and newlib will take it into account when e.g. localtime_r is called.

Ah, yes of course, I completely forgot about that! So this is still not a fully capable local time implementation, but it is good enough for getting local time pretty much "right now" and light duty UTC-localtime conversions. The reason is that the TZ string that is used only supports a single DST forward/back rule. So therefore all calculations must assume that is the rule for all of time. While that is not correct for a great number of locales, it is acceptable for most applications.

You are right. This capability exists in the runtime and people can and should use it if it meets their needs.

BrandonLWhite commented 6 years ago

@5chufti 2^64 us is about 584,554 years, so there is no danger there.

The time() CRT function only returns a 32-bit seconds-since-epoch value. So there is no sub-second component.

Regarding your example, I would be hesitant to mix values from different unrelated timing sources on each call. There is no observable relationship between micros64 (monotonic) and sntp_get_current_timestamp().

isn't the whole "save bootTime" thing the wrong way around?

It isn't ideal, but I think it's the best that can be achieved given what is available from the SDK. Since the SDK can only yield 32-bit seconds, it is limited in terms of precision and Y2038. It is good enough for use with the CRT time() function, but is not suitable for gettimeofday. So for gettimeofday we can use the SDK's 32-bit value as a reference point, and then achieve precision and longevity by combining it with a 64-bit monotonic clock. This would be unnecessary if the SDK provided a 64-bit way to get the current real time, with either us or ms precision.

5chufti commented 6 years ago

@BrandonLWhite I took your micros64() implementation and the attached time.c

Ofcourse it would be also possible to do sync only once and subsequently only if configTime() was called again if you have doubts on monotonicity. But the rest should be as monotonic as your micros64(). I did test it for general function and it works on old and new lwip.

replacement.zip

d-a-v commented 6 years ago

In #3835, there is still an issue with gettimeofday() if it is called for the first time before sntp-date is set by ntp. It works well after, or with settimeofday(). A solution would be to reset s_bootTimeSet when ntp is set (or adjusted?) Can it be reset anytime with no harm ?

(note that this PR is now on my ntp branch if you try that way)

d-a-v commented 6 years ago

With the latest #3835 commit (not yet merged, or my ntp branch), the main bug is gettimeofday() which is always wrong if it is called first time before a ntp date is set. A good way to fix this would be to be able to re-set s_bootTimeSet and al. everytime ntp receive a new timestamp. It is done though sntp_set_system_time() called from both settimeofday() and sntp from inside lwip through an lwip-api-#define.

sntp_set_system_time() is defined in lwip2, because it was defined inside lwip1.4. @igrr I think this code, which was introduced by espressif inside lwip - where it does not belong to - should be moved into cores/esp8266, but that would introduce a change in lwip1.4 too (we could move it anyway from lwip2 with attribute((weak)) and no change in lwip1.4)

In either case, sntp_set_system_time() can take one (epoch) or two (epoch, usec) arguments (it's a choice, it currently is one).

Do you think time.c can be changed to be able to be updated/reset/restarted through a call from sntp_set_system_time(epoch,usec) (ie. from external RTC or SNTP) ?

igrr commented 6 years ago

I very much agree about removing the time keeping functions from LwIP SNTP module. In my mind, SNTP should just call settimeofday (or adjtime, if we can implement it) when new timestamp is received. All timekeeping should be done in the core. All timezone management should be done in newlib. I think that having LwIP SNTP responsible for timekeeping is crazy, but apparently many of our customers already use these extra SNTP module functions, so they are not going away in the ESP8266 SDK. But since we are moving towards using upstream LwIP in this project, i think we should nuke these additions and do things the right way.