…

[d-a-v]

No description provided.

[TD-er]

I am interested if frameworks like Tasmota/EspEasy/Espurna/AnyOther could test this.

In ESPEasy the optimistic_yield is not used, so I don't see any reason why this should fail.

[TD-er]

@TD-er yield() has changed in this PR and is equivalent to current optimistic_yield(1000).

Ah OK, I got the impression it was the other way around.
I will later try to find a way to compile a version based on this PR....

[mcspr]

Similar here, but there's only a single place that optimistic yield is mentioned - debugging function that sometimes sends data to the network, and the only reason is that it avoids panic.

No specific preference for removal / keeping it, but perhaps methods should just change the prefix from esp_ to cont_?

[dok-net]

After needing some time to wrap my head around the existing namings; finding out which ones to shim, IIRC also PRing additional weak refs that got merged in the recent past; and building at least the CoopTask stackful coroutine multitasking lib on top of that work, plus devising the new esp_beak() in a separate, still pending PR; I am not particularly enthusiastic about this round of renamings and altering the contracts. I don't know that changing the semantics of yield() like in this PR is still Arduino AVR compatible enough. But I am rather seriously concerned that it breaks the multi-tasking hook semantics for AVR.
Besides, does this make the API even more dissimilar from the Arduino Core for ESP32, raising addional low-level portability issues?

[mcspr]

Ah, true, we do lose strong guarantee that stock yield() actually does something, which might be surprising...
edit: (and I wonder if Core libraries should actually use public api, when it might be overridden like that)

Afaik, esp32 simply noops optimistic_yield and yield is true thread yield
https://github.com/espressif/arduino-esp32/blob/af119215353b26d944b80d7126bed68e060e6697/cores/esp32/esp32-hal-misc.c#L49
https://github.com/espressif/arduino-esp32/blob/af119215353b26d944b80d7126bed68e060e6697/cores/esp32/esp32-hal.h#L43-L45

[mcspr]

Should the internals always stick to esp_... then? There's still yield() in some places.

Note that my quote is about esp32 ::
esp8266 implementation behaves sort-of like threads though, even if they don't really execute at the same time. and we still have multi-threading concurrency problems...

[dok-net]

I have a feeling that instead of essentially dropping all the uses of proper optimistic_yield(), based on the very low intervals in some calls, that those low intervals < 10000µs might be due to a misunderstanding of the workings of optimistic_yield().
In libraries/ESP8266WiFi/src/WiFiClient.cpp, libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp, and libraries/ESP8266WiFi/src/WiFiUdp.cpp, the 100µs interval is quite likely nothing less than a bug and I propose PR #7952.

There's a few places using 1000µs, which would probably be fine with 10000µs a well, and then there is WAIT_FOR_PIN_STATE in core_esp8266_wiring_pulse.cpp, which uses 5000µs, which I think will be fine/better off with 10000µs as well.

Rationale: low intervals for optimistic_yield provide more headroom for following code to not yield for a longer time, if the optimistic_yield is in the code path unconditionally. Generally optimistic_yield is great for available()-like functions, that may called in loops from sketches, in order to alleviate the user from having to keep an eye on total time spent in those loops to prevent WDTs. But 10000µs would be good enough for that, right?

Why is 1000µs considered a better default interval in this PR instead of the previous 10000µs, used in most places in the existing code?

[dok-net]

A general question, how would one go about distinguishing versions of the Arduino ESP8266 core pre and post this patch? I would like CoopTask to auto-detect this as much as possible, version conflicts that don't compile are a little hard on the intended user-base of the library.

[dok-net]

As I believe the inspiration for this PR came in part from #7148, which also was requested to be kept "online" be @d-a-v after initial reserverations from @devyte, I have now updated PR #7148.

I believe that #7148

• has more comprehensive and consistent renamings
• has fewer renamings where I personally see no previous issues
• fixes the 'delay(0)` issue
• maintains esp_yield() semantics of suspend and re-schedule
• solves compatibility issues with 3rd party MT libs by introducing the define HAVE_ESP_SUSPEND in coredecls.h

Not in here, and IMHO better suited to a separate follow-up PR:

• A minimal_yield(), though I disagree with 1000µs suiting all cases, in particular serial UART available() used to use 10000µs and that seem a better fit. Therefore, optimistic_yield() shall be maintained and the argument must not be disregarded.
• Please review PR Improve optimistic_yield intervals for performance gain in sketches #7952

Also, I don't know that

Don't panic() when yield() is called from SYS,
instead emit a warning on console in debug mode

is not too easily inviting new invalid calls to yield() from SYS, which can so simply be replaced by the proper esp_yield() instead.

[dok-net]

@d-a-v I'm truly sorry that you get upset, it's not my intention and I've given my writing a few iterations to express things calmly, and based on the facts. I would hope that you can see it this way: let's get the best of everyone's ideas together. Having this PR #7148 posted ahead of yours, seems like a reasonable strategy to reference for reading, reviewing, merging and extracting.

You said,

I don't see what you're talking about, neither why you advertise your PR in another one, apart from trying to show the world how clever you are.

Well there's nothing wrong with that, unless putting some else down and doing things exclusively for the purpose of showing off - which you can't really believe to be my intention. Mind you, I'm maintaining CoopTask and as such I hold some stake in stable APIs, isn't that sufficient reason to review and comment and propose changes to PRs that affect this?
In fact, I was surprised why you didn't review #7148 in detail and instead started a new one yourself.

Perhaps you felt my comment was trying to outline differences to your PR, no, that would be a misunderstanding, I was listing what it does, not claiming that this #7832 was doing worse overall. Sorry if that's what you thought I meant by it.

Where I said

disagree with 1000µs suiting all cases,

it doesn't mean that 1000µs is always wrong! I only find that there are other cases where 10000µs is better suited, so while your PR would have used 1000µs everywhere regardless of the argument to optimistic_yield(), I agree to introduce and patch for minimal_yield() (if that's the final name after review), but let's keep the ability to set 10000µs where it's suitable, like available() functions.

I trying not to think badly of this remark you made:

I'm sorry that I have to read so many non-sense.

I feel #7148 is changing enough for one PR now, please keep your work available such that those parts going beyond the former can be implemented by follow-ups.