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

Use asynchronous UDP library to get non blocking time update #60

Closed
gmag11 opened this issue Mar 16, 2018 · 29 comments
Closed

Use asynchronous UDP library to get non blocking time update #60

gmag11 opened this issue Mar 16, 2018 · 29 comments
Assignees

Comments

@gmag11
Copy link
Owner

gmag11 commented Mar 16, 2018

Now that I have the control of NTP sync procedure, it would be a very nice feature to add non blocking NTP request. This is possible by using ESPAsyncUDP library.

This have a side effect. Using this library would make next versions of NTPClientLib compatible only with ESP8266 and ESP32, as there is no equivalent library for MKR1000 and ArduinoWiFi.

Please add your thoughts and ideas in this issue.

@gmag11
Copy link
Owner Author

gmag11 commented Mar 18, 2018

Initial support of asynchronous UDP communication is ready in AsyncUDP branch. There can be many bugs but it is compiling and working in my local setup.
There is no timeout processing yet, so issues may happen in case of no response from server.
This version is and will be only compatible with ESP32 and ESP8266 so if you are working with Arduino boards keep version 2.5.0.
Please test it and comment here. @AlbertWeterings @xoseperez

@gmag11
Copy link
Owner Author

gmag11 commented Mar 19, 2018

Notice that for some time, I've moved NTPClientLib.h and .cpp to ESP8266 example folder. I need it so that Visual Studio Intellisense works properly. Move them to src folder for testing if you prefer to.

@gmag11
Copy link
Owner Author

gmag11 commented Mar 19, 2018

Timeout processing is working now, using Ticker library. I've chosen this lib better than timer interrupts because it is available both on ESP8266 and ESP32 Arduino framework.

@gmag11 gmag11 self-assigned this Mar 19, 2018
@mcspr
Copy link
Contributor

mcspr commented Mar 23, 2018

It should be noted that hostByName uses delay internaly, and that just blocks if there is any error.

int error = WiFi.hostByName (getNtpServerName ().c_str (), timeServerIP);

For example, ESPAsyncTCP client code directly uses sdk dns resolution

What I am thinking about is maybe do the same? Instead of hostByName use dns_gethostbyname and allow the caller not wait for broken dns server. Internal resolver will then just call given NtpClient callback and client will either connect or just cache the ip.

Edit: Plus, I was running this since yesterday without any issues. Disabled dns server, wrong ntp server ip, wrong dns address - client recovered and is ok so far.

@gmag11
Copy link
Owner Author

gmag11 commented Mar 23, 2018

Good point! I did not think about this. I'll do that. Thanx

@Misiu
Copy link

Misiu commented Sep 5, 2018

@gmag11 thank You for such an awesome library.
I'd like to know if Async version is working and is it released? The latest release is 2.5.1 and I'm not sure if it includes async code.
I'm using ESP8266, so I guess that this example should be my base code.

@gmag11
Copy link
Owner Author

gmag11 commented Sep 10, 2018

Hi. No, it is not released in master branch. But you can use it by selecting AsyncUDP branch. https://github.com/gmag11/NtpClient/tree/AsyncUDP
Notice it may contain some bug. At least for me, it was working fine. DNS async resolution is still missing so it is better if you configure NTP server IP address instead of server name.
Please let me know your experience with it.

@Misiu
Copy link

Misiu commented Sep 10, 2018

@gmag11 I'll try that version today. Hopefully, I'll create something working :)
@mcspr maybe You can create PR with Your suggested change? This way we'll be one step closer to releasing this into the master branch.

@mcspr
Copy link
Contributor

mcspr commented Sep 10, 2018

@Misiu i'll take a look at it again this week. plus fixing minor esp32 example errors, as code paths will be the same for both esp8266 and esp32.

By the way... sdk itself does provide sntp (and timelib analogue) support and arduino for both esp8266/32 expose it. have you tried it?
esp8266/arduino entrypoint for ntp config
esp8266/arduino usage example (exact same api for esp32) and imagine 'delay' is replaced with timestamp checker somewhere in loop

@Misiu
Copy link

Misiu commented Sep 11, 2018

@mcspr I never used the built-in sntp functions (I didn't know they existed till now 😄 )
For my next project, I need wifi, mqtt and sntp. I want all of those to connect async to not complicate my loop code. The best scenario (everything connects and never disconnects) is simple, but if something disconnects I want to reconnect as easy as possible.
Do you know if configTime has a callback? I want to get time at start and every hour I want to resync it. Without any callback it will be hard to know that sync failed.

@gmag11
Copy link
Owner Author

gmag11 commented Sep 11, 2018

I used them in older versions. But it was a mess to mantain different code for each platform. By that time esp32 did not include same sntp functions in its sdk, so I decided to get that code out of this library.

@Misiu
Copy link

Misiu commented Sep 11, 2018

@gmag11 so right now I could use configTime from SDK. What about callbacks, are there any available (ntp sync fsuccess, ntp sync fail)? I can't test it right now, but I found this example and this one, is it a good start?
I currently target ESP8266, so I don't mind if something is missing in ESP32 SDK :)

@mcspr
Copy link
Contributor

mcspr commented Sep 11, 2018

(assuming recent esp8266/Arduino + lwip2 is used. my understanding is old sdk/lwip1.4 required manual sync because of some bugs)

Quick look at how sntp/time works there, because I was interested how it compared to ntpclient/timelib combo. Initial success is determined by checking if sntp_get_current_timestamp() returns >0, as examples show via time() usage. After that:
If sntp successfuly retrieves timestamp it will schedule next sync based on SNTP_UPDATE_DELAY
Which is set by arduino core to be 1 hour
sntp will relentlessy retry each server until it gets time
Core sets SNTP_RETRY_TIMEOUT and SNTP_RETRY_TIMEOUT_MAX (3 and 30 seconds respectively)

And it will autotrigger sync when network settings change, so i believe it is set-and-forget. No error state to check though, besides initial sync success.

edit in the example that @Misiu mentioned, there is cb when time is set:
https://github.com/esp8266/Arduino/blob/91519309d000c65c283c6721ae58d4bf708bf719/libraries/esp8266/examples/NTP-TZ-DST/NTP-TZ-DST.ino#L39-L49

@gmag11
Copy link
Owner Author

gmag11 commented Sep 12, 2018

Callback functions are available. Check included examples

@mcspr
Copy link
Contributor

mcspr commented Sep 15, 2018

So I did real device test of dns resolver with these changes: AsyncUDP...mcspr:async-dns

On ESP32 it is broken - if dns_found_callback calls async_getTime, system will just hang on udp->connect. Seems to be some kind of threading issue specific to esp32. Doing similar changes to these me-no-dev/AsyncTCP@a1a184a directly to core's AsyncUDP does help (replacing all _udp_xxx calls with udp_xxx when relevant).

ESP8266 is ok, server on the lan works fine when accessed via hostname.

@hostmit
Copy link

hostmit commented Sep 20, 2018

Can I get a feedback? Is async branch is relatively safe to use on esp8266?

@hostmit
Copy link

hostmit commented Sep 20, 2018

Btw not sure why would u waste time for ESP32? It has built in FreeRtos support. So anyone can put sync ntp in a task and dont bother with blocking thread.

@hostmit
Copy link

hostmit commented Sep 20, 2018

Is there working sample to use?

@mcspr
Copy link
Contributor

mcspr commented Sep 21, 2018

@hostmit Like I said, esp8266 works fine for me (knock on wood):
https://github.com/gmag11/NtpClient/tree/AsyncUDP/examples/NTPClientESP32
https://github.com/gmag11/NtpClient/tree/AsyncUDP/examples/NTPClientESP8266

So if there is no reason to use asyncudp for esp32, ntpclient should have it's own task to run network requests in the background?

@hostmit
Copy link

hostmit commented Sep 21, 2018

Thanks. The DNS resolution still blocks the loop :(

As for ESP32, I have ntp client updating only once. And yes, in vTask. Since ESP32 has RTC, no need to update more then once.

@gmag11
Copy link
Owner Author

gmag11 commented Jan 4, 2019

AsyncUDP is now included on ESP32 core. I've included this platform on current AsyncNTP code and merged into develop branch. This will be part of future 3.0.0 version. As async DNS is missing it is not ready to be merged on master but you can consider this version as beta if you use IP address for NTP server.

I'll start including async DNS resolution now.

@gmag11
Copy link
Owner Author

gmag11 commented Jan 8, 2019

So I did real device test of dns resolver with these changes: AsyncUDP...mcspr:async-dns

On ESP32 it is broken - if dns_found_callback calls async_getTime, system will just hang on udp->connect. Seems to be some kind of threading issue specific to esp32. Doing similar changes to these me-no-dev/AsyncTCP@a1a184a directly to core's AsyncUDP does help (replacing all _udp_xxx calls with udp_xxx when relevant).

ESP8266 is ok, server on the lan works fine when accessed via hostname.

You are completely right. Async dns resolve works on ESP8266 and so does async NTP. With ESP32 async DNS gets the correct IP address but breaks something into lwip library and UDP connect causes code to freeze.

Anyway, it seems that as @hostmit said on #60 (comment) , for ESP32, lwip runs on a separate thread so turn DNS solving asynchronous is not so important. I've confirmed this by reducing loop() period. All lines are written to Serial at correct time.

Then, finally I will leave ESP32 code as it is on develop branch and ESP8266 code as of AsyncDNS branch. I only have to clean code and add precompiler directives to separate achitectures.

@gmag11
Copy link
Owner Author

gmag11 commented Apr 20, 2019

I've published Async version as 3.0.0-beta

@gmag11 gmag11 closed this as completed Apr 20, 2019
@mcspr
Copy link
Contributor

mcspr commented Apr 20, 2019

Slightly late comment 😴

I wonder if it is still worth adding additional dependency. ESP8266 client introduce couple of new things:

  1. Stop sync in begin (setSyncProvider will call it immediately)
  2. Option to use asynchronous sync provider, setting a flag instead of calling NtpClient::getTime() immediately.
  3. NtpClient::loop() to call inside loop if we want to use async option.

@tokra
Copy link

tokra commented Nov 8, 2020

I've published Async version as 3.0.0-beta

Hey @gmag11 , Is there any example how to use it ? Thank you in advance

@gmag11
Copy link
Owner Author

gmag11 commented Nov 10, 2020

@tokra Maybe you find this video useful https://youtu.be/r2UAmBLBBRM

Basically you need to call something like configTime (1 * 3600, 0, "pool.ntp.org", "time.nist.gov")

Definition is here

After that you can get time with time_t now = time(nullptr) and convert to char* with ctime(&now)

@tokra
Copy link

tokra commented Nov 10, 2020

@gmag11 thanks for the link but thats not what i meant. I thought, how to use it with Async UDP..

The reason for ASYNC UDP, is that i am using AsyncMQTT client,
and when i receive certain data from topic (asynchronously), i want to send some data with actual time as well. I cannot use example like this https://raw.githubusercontent.com/SensorsIot/NTP-time-for-ESP8266-and-ESP32/master/NTP_Example/NTP_Example.ino because its using delay().

For now i am using AsyncHttpRequest to worldtimeapi.org (non-NTP solution).

But when i saw this I thought I can also use this Async NTP client.

@gmag11
Copy link
Owner Author

gmag11 commented Nov 10, 2020

You do not need to update time every time. If you use my advise on inmediately previous message, ntp will not interfere your code.

@tokra
Copy link

tokra commented Nov 13, 2020

Got it thanks !

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

No branches or pull requests

5 participants