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

Renamings for that esp_yield is really suspend, replacing delay(0), shimmable suspend-CONT API for async esp_suspend() / esp_schedule() #7148

Merged
merged 22 commits into from
Oct 16, 2021

Conversation

dok-net
Copy link
Contributor

@dok-net dok-net commented Mar 11, 2020

The main API change here concerns the renaming of esp_yield() to esp_suspend(). This can be detected by 3rd party libraries through the new HAVE_ESP_SUSPEND define.
Lots of internal renamings to make the difference between suspending and yielding, the latter meaning suspend and resume at the next opportunity, more succinct.

Hint: This PR now contains, due to sizeable merge efforts each time, and they are related, everything that's in PR #6782 as well.

Fixes #7969

Related items:
#6212, here is a more complete replacement of delay(0) usage across the Core
#8317 here, implement delay on a lower level
#7969 since we deal with a bunch of weak funcs, here internal libraries reference the exact functionality they want
#7146 discussion originated the UART changes, as noticed with the delay(0) vs. yield() usage

@dok-net
Copy link
Contributor Author

dok-net commented Mar 11, 2020

CI bombed again, this time for MacOS :-(

@dok-net dok-net force-pushed the delay0isyield branch 2 times, most recently from 0513ce5 to c32b24a Compare March 26, 2020 16:32
esp_schedule();
esp_yield();
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about commenting the choice of this name (is it interrupting tasks calling (delay()) from cont stack) and also that it is equivalent to delay(0) ?

Copy link
Collaborator

@d-a-v d-a-v Jan 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If new function names are to be added, with restrictions on usage, like "use esp_break() if code is called from SYS", I suggest adding _from_sys in its name to make it clear.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yield() historically panics on purpose when it is called from SYS.
esp_break() is exactly yield() without panic(), callable from both cont and sys.
Considering that the yield()'s panic() is avoided by delay(0) or its new flavour esp_break(), then we may just update yield() to not panic no ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what happened, but you must not have available the current sources somehow.
The actual comment now states, I think I adapted it after an earlier discussion:

// Replacement for delay(0). In CONT, same as yield(). Whereas yield() panics
// in SYS, esp_break() is safe to call and only schedules CONT. Use yield()
// whereever only called from CONT, use esp_break() if code is called from SYS
// or both CONT and SYS.

Meaning, esp_break() is intended for both SYS and CONT.
With regard to yield() panicking in SYS, whereas delay(0) does not and esp_break() of course does neither, I think this was discussed and it was stated that yield() shall intentionally continue panicking when called from anywhere else but CONT.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to disable my previous answer before the last one. Sorry that this confused you.

So you propose esp_break() for both sys and cont because yield() is for cont only, while they both do the same job, like delay(0) which currently is used to replace yield() everywhere where it is needed for sys and cont.
Then, you replace delay(0) by the new sys+cont yield() taste.
So we have now yield(), delay(0), esp_break() ?
Why not simply yield() with a comment ?

If the earlier discussion you refer to is that one, it also says:

We could fix it for a major release

@devyte maybe you can elaborate on this:

The expressibility issue has been discussed before (a long time ago shortly after I arrived). It was deemed valid

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@d-a-v Thanks, one can never look too many times at the code. I'm going to explain next that it's a bit different than you think, but that wasn't helped by me replacing delay(0) by yield() on time too often - instead of by esp_break() (5c39094).

Then, you replace delay(0) by the new sys+cont yield() taste.
So we have now yield(), delay(0), esp_break() ?
Why not simply yield() with a comment ?

Delay(0) is either pointless, in those places where the code runs only ever from CONT, so for final clarity and to let the runtime panic to express that contract, I'm replacing it by yield(). Again, only in those places.
Wherever code may run from SYS (and perhaps from CONT, too), the new esp_break() must be used to replace delay(0). This is to make the intention clear not to panic in SYS, which I find obfuscated by a zero time delay(0) call.
As you can see, yield() (intentional panic in SYS) is not the same as esp_break(), and my reservations about delay(0) I've explained above.

cores/esp8266/uart.cpp Outdated Show resolved Hide resolved
@d-a-v d-a-v self-requested a review March 27, 2020 10:38
@dok-net dok-net force-pushed the delay0isyield branch 2 times, most recently from 48a4cb2 to 401670e Compare January 4, 2021 21:18
@dok-net dok-net force-pushed the delay0isyield branch 2 times, most recently from 4976bc6 to 5c39094 Compare January 10, 2021 12:39
@dok-net dok-net force-pushed the delay0isyield branch 2 times, most recently from 5f22bea to cd03fa9 Compare March 15, 2021 10:48
@dok-net dok-net force-pushed the delay0isyield branch 3 times, most recently from 24806f8 to 8e92256 Compare April 8, 2021 10:53
@dok-net dok-net changed the title Delay(0) is an obfuscation of yield() or esp_schedule(); esp_yield() Fix for: esp_yield really suspends, delay(0) is an obfuscation of yield() for CONT and SYS Apr 8, 2021
@dok-net dok-net mentioned this pull request Apr 8, 2021
@dok-net dok-net force-pushed the delay0isyield branch 2 times, most recently from a3d08f1 to 2a39360 Compare April 9, 2021 14:33
@d-a-v
Copy link
Collaborator

d-a-v commented Oct 8, 2021

Strangely enough, I can't find any use of yieldUntil in the libraries/lwIP_Ethernet code itself, which we are told it is presented for.

Should we understand that you're in the need for a personalized explanation of why it is useful for ethernet ?

@dok-net
Copy link
Contributor Author

dok-net commented Oct 8, 2021

Not really... I've commented at times before about "separation of concern" issues, but let us not go deeper into that. Also, no need to get personal at all.
On the technical side, one, there is a template in PolledTimeout that includes the yield, using that would make the PR more terse. I could contribute the code change, if I didn't believe that the use of PolledTimeout, especially fast, wasn't a bad idea for slow networking code. Yes, you could rightfully claim that the PR I am pitching has a separation of concern problem in its own right, but alas, it has fixed those trouble spots that the yieldUntil addresses many moons earlier, so I stand by it, hoping to be able to field the right review questions and get your approval on technical grounds. Please read the issue that it solves, too.

@mcspr
Copy link
Collaborator

mcspr commented Oct 8, 2021

About template vs. std::function, my first attempt in this direction shows much worse memory efficiency

Strange, I was observing rom going down from 317933 before and 317853 after. Ram remained the same.

https://gist.github.com/mcspr/50e7c8ee2bc9c27bc8e83dd0e7aae111
With the general idea to move shared code externally, keeping the bare minimum inline

@dok-net
Copy link
Contributor Author

dok-net commented Oct 8, 2021

@mcspr Thank you for your contribution. I may not have taken the time to analyze further otherwise. Yes, your change shaves 160 bytes off the IROM usage - that's for the pristine WiFiScan.ino example sketch. I'm committing your change, with a small refactoring of the const references on uint32_t that I think were unintended.
EDIT: I had to fix a subtle bug you introduced because of your thinking being influenced by yieldUntil. Please understand that in this PR, delays may become canceled by an asynchronous esp_resume, and the "blocked" predicate is there to test for some condition that may preempt delaying again until the timeout is fullfilled.

Copy link
Collaborator

@mcspr mcspr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm
besides the comments and the commit & title could be improved for squashing?

some historical reference

libraries/ESP8266WiFi/src/ESP8266WiFiGeneric.cpp Outdated Show resolved Hide resolved
libraries/ESP8266WiFi/src/ESP8266WiFiMulti.cpp Outdated Show resolved Hide resolved
cores/esp8266/core_esp8266_waveform_phase.cpp Outdated Show resolved Hide resolved
cores/esp8266/coredecls.h Outdated Show resolved Hide resolved
cores/esp8266/uart.cpp Show resolved Hide resolved
libraries/ESP8266WiFi/src/ESP8266WiFiGeneric.cpp Outdated Show resolved Hide resolved
cores/esp8266/core_esp8266_waveform_pwm.cpp Outdated Show resolved Hide resolved
cores/esp8266/core_esp8266_waveform_pwm.cpp Outdated Show resolved Hide resolved
cores/esp8266/Schedule.cpp Show resolved Hide resolved
cores/esp8266/core_esp8266_main.cpp Outdated Show resolved Hide resolved
@dok-net dok-net changed the title Fix for: esp_yield really suspends, delay(0) is an obfuscation of yield() for CONT and SYS Renamings for that esp_yield is really suspend, replacing delay(0), shimmable suspend-CONT API for async esp_suspend() / esp_schedule() Oct 13, 2021
@dok-net dok-net requested review from mcspr and removed request for d-a-v October 13, 2021 06:43
cores/esp8266/Schedule.cpp Show resolved Hide resolved
cores/esp8266/core_esp8266_waveform_phase.cpp Outdated Show resolved Hide resolved
tests/host/common/Arduino.cpp Outdated Show resolved Hide resolved
…ve microoptimization that

could break on inlining.
Fix in-source specification for the main `esp_try_delay`.
@mcspr
Copy link
Collaborator

mcspr commented Oct 16, 2021

@dok-net can you merge master branch here or unlock the modifications by the maintainers on the right column?

@dok-net
Copy link
Contributor Author

dok-net commented Oct 16, 2021

@mcspr Done

@mcspr mcspr merged commit c312a2e into esp8266:master Oct 16, 2021
@mcspr mcspr added this to the 3.1 milestone Oct 16, 2021
@dok-net dok-net deleted the delay0isyield branch October 16, 2021 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WiFiScan.ino ported to CoopTask libraries doesn't find any networks
3 participants