Make update() non-blocking.

[blackketter]

When the network is unreliable or the NTP server is unavailable, the update() function can block for a substantial amount of time. This patch separates update request and response processing and allows for a single outstanding request. Requests are retransmitted every second if there's no response.

ForceUpdate() is still available and can still block.

[FWeinb]

I really like this approach! Thanks for this contribution.

@sandeepmistry Any objections?

[sandeepmistry]

I like the concept, however is this a breaking changing in behaviour?

If so, maybe we can add a new API for non-blocking behaviour.

[blackketter]

Under identical network conditions the behavior may change a bit. (i.e. A single call to update() on a slow network may not update the time.) If the use of update() is to call it multiple times (i.e. in loop() ) and that the application expects that it sometimes fails to update, then it should be fine.

[FWeinb]

Adding an API for the current time in a blocking way would be great to ensure that the time was synced at least once on startup. Otherwise I think that the behavior change is not that bad.

[blackketter]

forceUpdate() continues to behave with the with the same blocking behavior in my patch. Maybe that's what should be used?

Keep in mind the original behavior of update() was to block for only one second so on unreliable or slow networks it will continue to fail.

[sandeepmistry]

Thanks for the clarification. The suggested changes sound good to me.

[blackketter]

Those last three actually support #23 with a new method:

unsigned long long getEpochMillis();

[blackketter]

Any chance this request will be accepted?

[sandeepmistry]

@blackketter up to commit b68746a was good for me. Not sure why that wasn't merged then.

@FWeinb what do you think of the newer commits?

[NdK73]

Will this PR be accepted and included?
Async updates are always very useful, IMVHO. And making 'em synchronous is quite easy :)

[corneyl]

@blackketter When I use your fork, i often get a crash of the program. I've tried to decode the stacktrace, but it doesn't show line numbers:

Exception Cause: 0  [Illegal instruction]

0x4021099d: NTPClient::checkResponse() at ??:?
0x40210b08: NTPClient::update() at ??:?
0x4020d58d: Clock::update() at ??:?
0x402113b8: PubSubClient::connected() at ??:?
0x40211635: loop at ??:?
0x402020ad: esp_schedule at ??:?
0x402020d8: loop_wrapper() at core_esp8266_main.cpp:?
0x40203fec: cont_norm at cont.o:?

The exception is in checkResponse(), any clue what this can be?

Edit: I also got this exception:

Exception Cause: 2  [InstructionFetchError: Processor internal physical address or data error during instruction fetch]

0x4021099d: NTPClient::checkResponse() at ??:?
0x40210b08: NTPClient::update() at ??:?
0x4020d58d: Clock::update() at ??:?
0x40211634: loop at ??:?
0x402020ad: esp_schedule at ??:?
0x402020d8: loop_wrapper() at core_esp8266_main.cpp:?
0x40203fec: cont_norm at cont.o:?

[blackketter]

Hm, nothing stands out here. Does it happen every time or just occasionally? Is it possible to figure out approximately what line is failing with an objdump or by putting in some debugging printfs?

…

On Jan 16, 2018, at 12:48 PM, corneyl ***@***.***> wrote: @blackketter <https://github.com/blackketter> When I use your fork, i often get a crash of the program. I've tried to decode the stacktrace, but it doesn't show line numbers: Exception Cause: 0 [Illegal instruction] 0x4021099d: NTPClient::checkResponse() at ??:? 0x40210b08: NTPClient::update() at ??:? 0x4020d58d: Clock::update() at ??:? 0x402113b8: PubSubClient::connected() at ??:? 0x40211635: loop at ??:? 0x402020ad: esp_schedule at ??:? 0x402020d8: loop_wrapper() at core_esp8266_main.cpp:? 0x40203fec: cont_norm at cont.o:? The exception is in checkResponse(), any clue what this can be? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAICX7vll7ujAH4tUavtwREY2vHtKF66ks5tLQs2gaJpZM4KUI5c>.

[corneyl]

I think I found it: _updateCallback is not initialised.
I do not set a callback, but if I add a print statement to the callback check like this:

if (_updateCallback) {
   Serial.println("Calling callback...");
   _updateCallback(this);
}

it actually prints Calling callback... just before the stacktrace.

If I initialise _updateCallback like this: NTPUpdateCallbackFunction _updateCallback = NULL;, I do not get the exception.

[blackketter]

Good find!

…

On Jan 16, 2018, at 2:30 PM, corneyl ***@***.***> wrote: I think I found it: _updateCallback is not initialised. I do not set a callback, but if I add a print statement to the callback check like this: if (_updateCallback) { Serial.println("Calling callback..."); _updateCallback(this); } it actually prints Calling callback... just before the stacktrace. If I initialise _updateCallback like this: NTPUpdateCallbackFunction _updateCallback = NULL;, I do not get the exception. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAICXwFU4-9Bh2I0h_MDVjzPJU0zmTaXks5tLSMdgaJpZM4KUI5c>.

[connornishijima]

Is this library still being maintained?

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}