delay() refactoring

[emelianov]

Refactoring delay to allow to run esp_schedule() and esp_yield()
periodically (not only on delay ends).
This will be helpfull in implementation of Ethernet dirver.

[d-a-v]

Per this detailed explanation, delay() is sometimes supposed to be interrupted by esp_schedule() before its end (like the timeout in ClientContext).

This is used in some places in the core, so we also need to

• rename current delay() to interruptible_delay()
• identify the delays() supposed to be interrupted and use interruptible_delay() instead
• be aware of all potential side effects

However this PR is very nice for recurrent scheduled functions to be called when some libraries or sketches call delay(some-long-delay);

(edit: link to ethernet thread)

[d-a-v]

This delay is supposed to be interrupted by esp_schedule() from wifi_dns_found_callback():

Arduino/libraries/ESP8266WiFi/src/ESP8266WiFiGeneric.cpp

Line 568 in 653f58e

delay(timeout_ms);

[devyte]

Are there really no other delays that are meant to be interrupted anywhere in the code? I thought some of the TCP timeouts would be like that (WiFiClient or ClientContext etc)

[emelianov]

Other delays (in ClientContext) updated during #6213

[dok-net]

@devyte Please, with all due respect, this PR right now is a horrible idea from my POV, given that it would, depending on how exactly it's fixed, either terribly break or at least work against the (cooperative multi-loop() multitasking/multithreading library CoopTask](https://github.com/dok-net/CoopTask).

I cannot be sure to claim that CoopTask is sufficiently evolved to handle what #6213 needs (believing it does so fine is another thing ;-) ), but letting other code run during delay()s is precisely one of the use cases for cooperative multitasking. I'm sorry for the pitch, but you can probably empathize that I'd rather have my implemention get more use rather than being locked out of the ESP8266 core libraries by ongoing work in the opposite direction. An alternate delay-like delay_interruptible() must not happen, please.

[d-a-v]

@dok-net interruptible_delay() can be a weak function that you'd override in your environment.

[dok-net]

@d-a-v @devyte I think it's not as simple as that, because it's not the delays, but the breaking from the delays that is troubling:

libraries/ESP8266WiFi/src/ESP8266WiFiGeneric.cpp:657:    esp_schedule(); // resume the hostByName function
libraries/ESP8266WiFi/src/ESP8266WiFiScan.cpp:326:        esp_schedule();
libraries/ESP8266WiFi/src/ESP8266WiFiSTA-WPS.cpp:110:    esp_schedule(); // resume the beginWPSConfig function
libraries/ESP8266WiFi/src/include/ClientContext.h:438:            esp_schedule(); // break current delay()
libraries/ESP8266WiFi/src/include/ClientContext.h:539:            esp_schedule(); // break current delay()
libraries/ESP8266WiFi/src/include/ClientContext.h:615:            esp_schedule(); // break current delay()

Once there are multiple concurrent delays in multitasking, the concept of "current delay()" no longer applies. Add on top that the esp_scheduler follows only a minimal 2 task design, sys and cont, and there is no off-the-shelf solution to this problem.
Right now,

// Following delay will be interrupted by connect callback
for (decltype(_timeout_ms) i = 0; _connect_pending && i < _timeout_ms; i++) {
    // Give scheduled functions a chance to run (e.g. Ethernet uses recurrent)
    delay(1);
}

would work without the esp_schedule() calls to cancel the 1ms delay immediately, just using _connect_pending = false; and letting the remaining part of the 1ms expire.
Luckily, also right now, it seems to me that esp_schedule() is not causing harm regardless that cont_yield() is overwritten by CoopTask. To be continued...

[d-a-v]

I think you don't understand @dok-net

We can modify the delay(1) loop that you cite above to call a function that does the same job, and make it weak.

So you can redefine it with whatever you think is good for your library. You know we can adapt, and we often do, so every use case has its chance to live and work.

We can do that for both function delay and delay_interruptible, both can be weak. So you don't need to be so much concerned about that. Ethernet is not yet merged and this PR will probably happen afterwards.

[dok-net]

@d-a-v Each time there is code that expects esp_schedule() a.k.a. ets_post() to cause a delay() to return prematurely, weak bindings of delay() and friends have no effect. esp_schedule() can't know which of many concurrent delays needs to be shortcut, and the whole work-stealing multitasking breaks down :-(
The loops in place right now that check a flag, and used delay(1) still work well, yes, at the expense of delaying up to 1ms even if the event handler notified that the condition is signaled. I am hoping that this is an acceptable delay, in comparison to the whole lot of user code that can execute in a multitasking setting during 1ms, even on ESP3266. I've measured cycle times under multitasking load (webserver, blinking LED, button reading, some text diagnostics to USB serial and a particular matter sensor on SoftwareSerial) of under 44 microseconds, if there are no uncooperative long-running delayMicrosecond() calls.
In an ideal world, we'd just port the ESP8266 two-tasking to RIOT OS or CoopTasks and use the task and ISR synchronization of those :-) In the meantime, please understand I'm trying to save what I am sure is a much-requested and very useful feature.

[dok-net]

I'd like to point out that the changes in this PR are contradictory and there's a lack of separation of concern. The change in ESP8266WiFiGeneric.cpp has nothing to do with the delay()/interruptable_delay() changes, as the use of delay(1) is simply made a bit less efficient due to the oneShotMs loop guard.

Also it's interruptible (with an I).

[infrafast]

Dear all, I am in early stage to use ethernet driver in my esp8266 and it appears that I faced some issue with the name resolution which takes much longer than via WIFI. Seems that the delay used in the "hostByName" function could be one of the cause. I propose to change it to a non blocking delay using the following patch:

diff --git a/libraries/ESP8266WiFi/src/ESP8266WiFiGeneric.cpp
b/libraries/ESP8266WiFi/src/ESP8266WiFiGeneric.cppba07e07 100644
+++ b/libraries/ESP8266WiFi/src/ESP8266WiFiGeneric.cpp
@@ -632,7 +632,8 @@ int ESP8266WiFiGenericClass::hostByName(const char*
aHostname, IPAddress& aResul
          aResult = IPAddress(&addr);
      } else if(err == ERR_INPROGRESS) {
          _dns_lookup_pending = true;
-        delay(timeout_ms);
+        for (int i = 0; i < timeout_ms && !aResult.isSet(); i++)
+            delay(1);
          // will resume on timeout or when wifi_dns_found_callback fires
          _dns_lookup_pending = false;
          // will return here when dns_found_callback fires

[MassiPi]

Dear all, I am in early stage to use ethernet driver in my esp8266 and it appears that I faced some issue with the name resolution which takes much longer than via WIFI. Seems that the delay used in the "hostByName" function could be one of the cause. I propose to change it to a non blocking delay using the following patch:

Hello @d-a-v ,
sad to see this specific modification was delayed to 3.1, since this totally breaks the ethernet usability..
Is it possible to expect this pull request to be implemented before 3.1 (which now is 3%), or should i keep manually modifying locally the source files?
Thanks a lot

[d-a-v]

@MassiPi I'm afraid we'll have to wait a bit more,
but you are right, now that new/experimental ethernet drivers are in the core, these delays have to be changed.
3.0.2 should be - I hope - the last regression-bugfix release after 3.0.0.

[mcspr]

Closing via #7148