Skip to content
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

Setting timezone with setenv doesn't work properly any more with V 2.7.1 #7353

Open
6 tasks done
ElToberino opened this issue Jun 6, 2020 · 13 comments
Open
6 tasks done

Comments

@ElToberino
Copy link

ElToberino commented Jun 6, 2020

EDIT:
Reference is in shipped examples, namely NTP-TZ-DST.ino.

Basic Infos

  • This issue complies with the issue POLICY doc.
  • I have read the documentation at readthedocs and the issue is not addressed there.
  • I have tested that the issue is present in current master branch (aka latest git).
  • I have searched the issue tracker for a similar issue.
  • If there is a stack dump, I have decoded it.
  • I have filled out all fields below.

Platform

  • Hardware: [ESP-12]
  • Core Version: [2.7.1]
  • Development Env: [Arduino IDE]
  • Operating System: [Windows]

Settings in IDE

  • Module: [Nodemcu]
  • Flash Mode: [qio|dio|other]
  • Flash Size: [4MB]
  • lwip Variant: [v2 Lower Memory]
  • Reset Method: [nodemcu]
  • Flash Frequency: [40Mhz]
  • CPU Frequency: [80Mhz]
  • Upload Using: [OTA AND SERIAL]
  • Upload Speed: [115200] (serial upload only)

Problem Description

The follwing two code snippets worked fine with V 2.6.3:

configTime(0, 0, "pool.ntp.org"); 
setenv("TZ", "CET-1CEST,M3.5.0/02,M10.5.0/03" , 1);

or, in changed order:

setenv("TZ", "CET-1CEST,M3.5.0/02,M10.5.0/03" , 1);
configTime(0, 0, "pool.ntp.org");

It always delivered the right local time.
With the current version, this seems to be broken - in a very strange way: Only the first call of the time function delivers the right local time, all following time server calls don't set the local time and the function delivers UTC time only. I read the changelog and searched the issues, but I coludn't find any explanation. Has there been a change regarding the definition of the local timezone or within the setenv function?

If required, here's the complete code of my time function which worked very well with V 2.6.3:

struct tm tm;
char timeshow[10];

void getTimeFromServer(){
  uint8_t  time_retry=0;                                // Counter retry counts time server
  setenv("TZ", "CET-1CEST,M3.5.0/02,M10.5.0/03", 1);
  struct tm initial;                                         // temp struct for checking if year==1970 
  initial.tm_year=70;
  
  while (initial.tm_year == 70 && time_retry < 15) {                 
    configTime(0, 0, "pool.ntp.org");                // get time from NTP server (ESP8266)
    delay(500);
    time_t now = time(&now);
    localtime_r(&now, &initial);
    Serial.print("Time Server connection attempt: ");
    Serial.println(time_retry + 1);
    Serial.print("current year: ");
    Serial.println(1900 + initial.tm_year);
    time_retry++;
  }

  if (time_retry >=15) {
      Serial.println("Connection to time server failed");
  } else {
    time_t now = time(&now);
    localtime_r(&now, &tm);
    strftime (timeshow, sizeof(timeshow), "%H:%M", &tm);
    Serial.print("Successfully requested current time from server: ");
    Serial.println(timeshow); 
    }
  }
}
@d-a-v
Copy link
Collaborator

d-a-v commented Jun 6, 2020

Check #6993 and #7109.
If you plan to work on 2.7.0+, you can

#include "TZ.h"
...
    configTime(TZ_Europe_Paris, "pool.ntp.org"); // check TZ.h, find your location
    // or
    configTime("CET-1CEST,M3.5.0/02,M10.5.0/03", "pool.ntp.org");

You can use otherwise

if (esp8266::coreVersionNumeric() >= 20700000)
{
    configTime("CET-1CEST,M3.5.0/02,M10.5.0/03", "pool.ntp.org"); // check TZ.h, find your location
}
else
{
    setenv("TZ", "CET-1CEST,M3.5.0/02,M10.5.0/03" , 1);
    configTime(0, 0, "pool.ntp.org");
}

@ElToberino
Copy link
Author

Great!
Thanks a lot for your quick response. That helped me much making my code compatible for both versions.

Best regards and thank you for your support!

@d-a-v
Copy link
Collaborator

d-a-v commented Jun 7, 2020

Thanks for the feedback @ElToberino

For backward compatibility, I think we should check in configTime(int tz, int dst, ...) that,
if tz==dst==0 and env var TZ is set, then call the other version (configTime(TZ,...) instead.

@devyte @earlephilhower I'll make a PR for 2.7.2

@ElToberino
Copy link
Author

I think that's a good idea.

Searching the web I found not so much information, though this way of getting local time is really simple and user friendly. I guess a lot of people even don't know that this great function is already implemented in the ESP8266 Arduino Core.

@d-a-v
Copy link
Collaborator

d-a-v commented Jun 7, 2020

Writing documentation is not our best skills... (at least speaking of me)
Help is always welcome !

@d-a-v
Copy link
Collaborator

d-a-v commented Jun 8, 2020

@ElToberino #7357 will not be merged.
However, I still wonder where you read this usage of configTime and setenv.

@ElToberino
Copy link
Author

OK, thanks for the information.

I found this simple solution on a German ESP-Site: https://fipsok.de/Esp8266-Webserver/ntp-zeit-esp8266.tab

This seemed to be a very easy way of getting and configuring local time and so I took the idea for my time function.
I'm not really an expert, so could you please tell me what is wrong or strange with my usage of configtime and setenv?

Thank you and best regards!

@cjastone
Copy link

cjastone commented Jun 9, 2020

Hi all - I've just returned to a project I was working on earlier in the year and have encountered the same issue as @ElToberino and it seems I'm using the same code as they are.

Here's my code:

#define NTP_SERVER "au.pool.ntp.org"
#define TZ_INFO "EST-10EDT-11,M10.5.0/02:00:00,M3.5.0/03:00:00" 

setenv("TZ", TZ_INFO, 1);
configTime(0, 0, NTP_SERVER);

I know I sourced a lot of my code from Andreas Spiess, who is quite prolific in the ESP community. His demo can be found here: https://github.com/SensorsIot/NTP-time-for-ESP8266-and-ESP32/blob/master/NTP_Example/NTP_Example.ino

I absolutely appreciate the work that you all do but I did want to add my weight to the argument that perhaps this should be considered a breaking change as I've seen that #7357 won't be merged.

In any case, thank you to all in this thread as it enables me to update my code to remain functional!

@d-a-v
Copy link
Collaborator

d-a-v commented Jun 9, 2020

@cjastone @ElToberino I don't know why tzset() is not called after setenv() but it should be,
it was in the initial example I reckon I didn't make much advertising. It is also in the current code.

Can you try with tzset() with your initial code and if it works, ask @SensorsIot and Jens Fleischer if they can update their examples.

@cjastone
Copy link

cjastone commented Jun 9, 2020

@d-a-v no luck with the tzset() I'm afraid, but thanks for replying so quick!

Honestly - I'm not mad about the change itself - it's a much cleaner and more readable solution, however I suspect that the code may be in much wider use.

If nothing else, the changelog was the first place I looked when my code broke, but I didn't find any relevant breaking changes listed. Maybe an update to the changelog is the easiest approach here?

@d-a-v
Copy link
Collaborator

d-a-v commented Jun 9, 2020

EDIT: this is not a good example - Reference are in shipped examples, namely NTP-TZ-DST.ino.

I just tried the snipped above, here's the complete sketch:


#include <ESP8266WiFi.h>

struct tm tm;
char timeshow[10];

void getTimeFromServer(){
  uint8_t  time_retry=0;                                // Counter retry counts time server
  setenv("TZ", "CET-1CEST,M3.5.0/02,M10.5.0/03", 1);
  struct tm initial;                                         // temp struct for checking if year==1970 
  initial.tm_year=70;
  
  while (initial.tm_year == 70 && time_retry < 15) {                 
    configTime(0, 0, "pool.ntp.org");                // get time from NTP server (ESP8266)
    delay(500);
    time_t now = time(&now);
    localtime_r(&now, &initial);
    Serial.print("Time Server connection attempt: ");
    Serial.println(time_retry + 1);
    Serial.print("current year: ");
    Serial.println(1900 + initial.tm_year);
    time_retry++;
  }

  if (time_retry >=15) {
      Serial.println("Connection to time server failed");
  } else {
    time_t now = time(&now);
    localtime_r(&now, &tm);
    strftime (timeshow, sizeof(timeshow), "%H:%M", &tm);
    Serial.print("Successfully requested current time from server: ");
    Serial.println(timeshow); 
  }
}

#include <PolledTimeout.h>
using namespace esp8266::polledTimeout;
periodicMs now(10000);

void setup ()
{
    Serial.begin(115200);
    WiFi.mode(WIFI_STA);
    WiFi.begin(STASSID, STAPSK);
    while (WiFi.status() != WL_CONNECTED) { Serial.printf("."); delay(100); }
    Serial.println(WiFi.localIP());
    getTimeFromServer();
    now.reset();
}

void loop ()
{
    if (now)
        getTimeFromServer();
}

If nothing else, the changelog was the first place I looked when my code broke, but I didn't find any relevant breaking changes listed. Maybe an update to the changelog is the easiest approach here?
and it gives correct time the first time then every 10s.

There is no doc about TZ, so we can't really say it's a breaking change. Only the NTP-TZ-DST example is up to date according to what's in the core.

I'd like to be able to reproduce you issue first.

@d-a-v
Copy link
Collaborator

d-a-v commented Jun 9, 2020

I've also recompiled the NTP-TZ-DST example from release 2.6.3 and it's still working with current master.

@ElToberino
Copy link
Author

Thanks for your work - it's really strange; perhaps it interferes with other tasks running simultaniously?
I use this time function in a really large sketch with a lot of other tasks running and call the time every ten minutes - as wroted in my initial post: (with V 2.7.1 and the original code above) only the first call works; all following calls only deliver UTC time.

But never mind - the solution you gave us is really fine and a nice and simple workaround.
As @cjastone wrote, I hope people with notice that. Even if it is not correct - this use of the time function seems to be widely spreaded.

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

Successfully merging a pull request may close this issue.

3 participants