Skip to content

sntp broken in git commit (Nov 22, 2017) #3905

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
whyameye opened this issue Dec 3, 2017 · 9 comments · Fixed by #4000
Closed

sntp broken in git commit (Nov 22, 2017) #3905

whyameye opened this issue Dec 3, 2017 · 9 comments · Fixed by #4000
Labels
waiting for feedback Waiting on additional info. If it's not received, the issue may be closed.

Comments

@whyameye
Copy link

whyameye commented Dec 3, 2017

Basic Infos

Hardware

Hardware: ESP-12F
Core Version: git commit 7b09ae5

Description

in example sketch HTTPSRequestCACert output from git commit 7b09ae5 pulled on Nov 22, 2017

Setting time using SNTP
Current time: Thu Jan  1 08:00:00 1970

same sketch, output from 2.4.0-rc2:

Setting time using SNTP.
Current time: Mon Dec  4 03:38:13 2017
@whyameye whyameye changed the title sntp broken in git commit sntp broken in git commit (Nov 22, 2017) Dec 3, 2017
@5chufti
Copy link
Contributor

5chufti commented Dec 3, 2017

try moving the sntp initialisation part [configTime()] before the WiFi initialisation, this should help.
new sntp implementation gets synced with the first dhcpd offer, then every 60min.
Or force a sync as explained here: #1679
So it is more likely a not up-to-date example than a broken sntp.

@d-a-v
Copy link
Collaborator

d-a-v commented Dec 3, 2017

In this sketch, the while (now < 1000) should be while (now < 30000) since now is initialized to 28800=8*3600 (0 with lwip1.4's sntp implementation), then jumps to far away in 1970's future once ntp timestamp is received.
That way it works with both version of sntp.

d-a-v added a commit to d-a-v/Arduino that referenced this issue Dec 4, 2017
@devyte
Copy link
Collaborator

devyte commented Dec 4, 2017

Can someone please make a PR with the needed changes to the HTTPSRequestCACert.ino example?

@d-a-v
Copy link
Collaborator

d-a-v commented Dec 4, 2017

I have it part of a future PR if it is not urgent. Anyone feel free to fix it if this can't wait.

@devyte
Copy link
Collaborator

devyte commented Dec 5, 2017

@whyameye does @d-a-v 's code change solve your issue?

@devyte devyte added the waiting for feedback Waiting on additional info. If it's not received, the issue may be closed. label Dec 5, 2017
@whyameye
Copy link
Author

whyameye commented Dec 5, 2017

yes! Thank you. :-)

@devyte
Copy link
Collaborator

devyte commented Dec 5, 2017

@d-a-v if you make a PR with that, I'll merge it.

@d-a-v
Copy link
Collaborator

d-a-v commented Dec 5, 2017

What this sketch is doing is waiting for time to be set by NTP server before doing the good stuff.
What do you think of either:

  • a user callback from settimeodday() (upon ntp timestamp received) as proposed in the # 1679 ntp thread (here (1.b.) and there)
  • or a function that would return true once, once ntp is set, then false until next time,
  • or a variable set to true on sync (that the user would have to reset once read)
  • or nothing to not break the API :]

@devyte @igrr

We could also have an always working settimeofday() if we can patch lwip1.4 (there is a static to remove) but I don't know the pain level of this in the core integration process.

@d-a-v
Copy link
Collaborator

d-a-v commented Dec 20, 2017

@devyte #4000 is the simple fix

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
waiting for feedback Waiting on additional info. If it's not received, the issue may be closed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants