NotificationsYou must be signed in to change notification settings
Fork13.3k
Star16.5k

Delay(0) is an obfuscation of yield() or esp_schedule(); esp_yield()#7146

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

dok-net wants to merge2 commits intoesp8266:masterfromdok-net:delay0isyield

Closed

Delay(0) is an obfuscation of yield() or esp_schedule(); esp_yield()#7146

dok-net wants to merge2 commits intoesp8266:masterfromdok-net:delay0isyield

Conversation

Copy link

Contributor

dok-net commentedMar 10, 2020•
edited
Loading

Let's unroll the code:
yield() =>

if (can_yield()) {  ets_post(LOOP_TASK_PRIORITY, 0, 0); // esp_schedule();  esp_yield_within_cont();}else {  panic();}

delay(0): =>

ets_post(LOOP_TASK_PRIORITY, 0, 0); // esp_schedule();if (can_yield()) {  esp_yield_within_cont();}

So, in detail, there is a difference. Yield() panics in ISRs (and SYS?), delay(0) posts CONT from ISR and SYS just fine, but doesn't actually yield unless in CONT.
I don't think using delay(0), requiring an extra literal 0 argument passed, is more expressive in SYS or ISRs to express a restart for suspend-yielded CONT.

For compatibility expectations, in AVR Arduino core, delay(0) is basically a no-op, it does not yield.

On the ESP32:

void delay(uint32_t ms) {  vTaskDelay(ms / portTICK_PERIOD_MS);}

This obviously is illegal to call from ISRs.

The take-away, as far as I am concerned, is that for the legal uses, yield() succinctly expresses what delay(0) does, and is slightly more efficient and safer.

In ClientContext.h, thedelay(0) is probably unintentional, theesp_schedule() direct call is used in a few places already, and is functionally identical in this place, while also more expressive and saves resources.

dok-net force-pushed thedelay0isyield branch from7790741 to6c2eff4Compare

March 10, 2020 09:14

Copy link

Collaborator

d-a-v commentedMar 10, 2020

panic() is called whenyield() is called from SYS,
but not whendelay(0) is called from SYS.

This is history, I don't know the exact reason for this. It may be improved.

can_yield():

bool ICACHE_RAM_ATTR cont_can_yield(cont_t* cont) {    return !ETS_INTR_WITHINISR() &&           cont->pc_ret != 0 && cont->pc_yield == 0;}

Copy link

ContributorAuthor

dok-net commentedMar 10, 2020•
edited
Loading

@d-a-v Yes, I implied that. Are you thus approving of what I call a cleanup?

d-a-v reviewed

Mar 10, 2020

View reviewed changes

libraries/ESP8266WiFi/src/include/ClientContext.h OutdatedShow resolvedHide resolved

d-a-v approved these changes

Mar 10, 2020

View reviewed changes

mcspr reviewed

Mar 10, 2020

View reviewed changes

cores/esp8266/uart.cpp Outdated

Copy link

Collaborator

mcsprMar 10, 2020

Choose a reason for hiding this comment

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

Looking atuart_write_char_delay, it is used by theuart{0,1}_write_char and those specific write funcs are installed asets_install_putc1(...) when debug mode is set. Does this mean that SYS calls this part, not CONT?

Copy link

Collaborator

d-a-vMar 10, 2020•
edited
Loading

Choose a reason for hiding this comment

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

This is right,esp_schedule() + esp_yield() should replacedelay(0) here.
Souart{n}_write_char() should be in IRAM right ?
Anduart_write_char_delay() is inline, maybe the attribute__attribute__((always_inline)) should be added ?

Maybe adding a functionyield_from_isr() would be useful to help through reading the code, for when we need to migrate to another SDK ?

Copy link

Collaborator

mcsprMar 10, 2020

Choose a reason for hiding this comment

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

re IRAM and inline. It seems to be inlined anyway, no symbol is created for any functions down the chain besides delay(). Is it a concern for sdk code in any way?
Other q i think is answered below.

Copy link

Collaborator

d-a-vMar 11, 2020

Choose a reason for hiding this comment

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

@mcspr
inline is an indication (likeregister which is deprecated in latest gcc version I believe) and compiler is free to not honor this request, which is fair.
But in that case, the function needs to be in iram because it can be called from an ISR.
Furthermore we can't declare a function to be at the same time inline and in iram.
So the proposal above is to force the compiler to always use this function inline.

d-a-v self-requested a review

March 10, 2020 10:55

devyte requested changes

Mar 10, 2020

View reviewed changes

Copy link

Collaborator

devyte left a comment

Choose a reason for hiding this comment

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

I am against this change. I agree that there is an issue of expressibility, i. e. the functionality should get better function names or whatever, but this proposal is changing core behavior and imposing restrictions. At a glance I can tell that not even my own app would work with this.
The argument about compatibility is not valid, because our nonos core is the only one with dual contexts. Not even the esp32 has to handle that.
The expressibility issue has been discussed before (a long time ago shortly after I arrived). It was deemed valid, but fixing it would require just about everyone out there to change code as well, so not worth the trouble at the time. We could fix it for a major release, but due to its high impact it would require a migration doc and equivalence table with specific details explaining the differences. It is not a minor undertaking, and should be designed and agreed upon by the maintainers before even making a PR.

dok-net changed the title~~Delay(0) is just a convoluted way of saying yield()~~Delay(0) is an obfuscation of yield() or esp_schedule(); esp_yield();

Mar 10, 2020

dok-net changed the title~~Delay(0) is an obfuscation of yield() or esp_schedule(); esp_yield();~~Delay(0) is an obfuscation of yield() or esp_schedule(); esp_yield()

Mar 10, 2020

Delay(0) is just a convoluted way of saying yield()

ba73d82

Copy link

ContributorAuthor

dok-net commentedMar 10, 2020•
edited
Loading

@devyte One step at a time, a change in core and the included libs doesn't mean 3rd parties have to change alongside, if what's there before still works afterward.
That said, and having reviewed everything more deeply - here I find PRs are just better suited to code reviewing and discussions based on the code than mere issues, please be forgiving if you feel otherwise - I come to this conclusion:

The panic() in yield() doesn't strike me as in anyone's particular best interest. But without the panic() branch, yield() can - and I suggest should - be restated as the rolled out version ofdelay(0), doing in each context what's the correct thing to do:

extern"C"void__yield() {esp_schedule();if (can_yield()) {esp_yield_within_cont();    }}

This has the benefit of allowing us to return toyield(); fromesp_schedule(); esp_yield();.
I take it that you've voted against this PR in the previous form, so I'm modifying it to this formulation.
It also is in the spirit of the expectation, that something that looks like regular Arduinoyield() will resume, whereas for indefinite suspension,esp_yield() exists on ESP8266.
esp_schedule(); is, and always was, unconditional indelay(0);, if this question comes up.
And in any place whereyield() before was synomymous topanic(), well, the code wouldn't run, and usingyield() as metaphor forpanic() is even more weird thandelay(0) instead ofyield() :-)
@d-a-v please take notice.

dok-net force-pushed thedelay0isyield branch from8b6ddc4 toe3bfbf5Compare

March 10, 2020 14:20

dok-net requested a review fromdevyte

March 10, 2020 14:21

yield() without panic() can do the right thing in CONT and SYS alike.

cfde64b

dok-net force-pushed thedelay0isyield branch frome3bfbf5 tocfde64bCompare

March 10, 2020 14:52

dok-net closed this

Mar 10, 2020

dok-net deleted the delay0isyield branch

March 10, 2020 17:37

Copy link

ContributorAuthor

dok-net commentedMar 10, 2020

Concluding this:
It has become obvious that in current master, theesp_yield() performs something that would be more aptly namedesp_suspend(). Commonly,yield() is expected to return from the scheduler on the next round, but foresp_yield(), it only does so after beingesp_schedule()d before or elsewhere.
This can and will be tackled in another timeline.

d-a-v reviewed

Mar 11, 2020

View reviewed changes

Copy link

Collaborator

d-a-v left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Approving this PR again after a re-self-request (also withthis that I was also mentioning privately),
because I still think these changes are good for core api abstraction and help further sdk changes.

dok-net mentioned this pull request

Mar 11, 2020

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

Merged

Copy link

ContributorAuthor

dok-net commentedMar 11, 2020•
edited
Loading

Resuscitated in#7148 with a unique wrapper foresp_schedule(); esp_yield() =>esp_resume().
Also added the ICACHE_RAM_ATTR and inlining fixes to uart.cpp.

mcspr pushed a commit that referenced this pull request

Oct 16, 2021

Implement esp_yield() as a replacement for delay(0)

c312a2e

esp_yield() now also calls esp_schedule(), original esp_yield() function renamed to esp_suspend().Don't use delay(0) in the Core internals, libraries and examples. Use yield() when the code issupposed to be called from CONT, use esp_yield() when the code can be called from either CONT or SYS.Clean-up esp_yield() and esp_schedule() declarations across the code and use coredecls.h instead.Implement helper functions for libraries that were previously using esp_yield(), esp_schedule() andesp_delay() directly to wait for certain SYS context tasks to complete. Correctly use esp_delay()for timeouts, make sure scheduled functions have a chance to run (e.g. LwIP_Ethernet uses recurrent)Related issues:-#6107 - discussion about the esp_yield() and esp_delay() usage in ClientContext-#6212 - discussion about replacing delay() with a blocking loop-#6680 - pull request introducing LwIP-based Ethernet-#7146 - discussion that originated UART code changes-#7969 - proposal to remove delay(0) from the example code-#8291 - discussion related to the run_scheduled_recurrent_functions() usage in LwIP Ethernet-#8317 - yieldUntil() implementation, similar to the esp_delay() overload with a timeout and a 0 interval

hasenradball pushed a commit to hasenradball/Arduino that referenced this pull request

Nov 18, 2024

Implement esp_yield() as a replacement for delay(0)

e154f4d

esp_yield() now also calls esp_schedule(), original esp_yield() function renamed to esp_suspend().Don't use delay(0) in the Core internals, libraries and examples. Use yield() when the code issupposed to be called from CONT, use esp_yield() when the code can be called from either CONT or SYS.Clean-up esp_yield() and esp_schedule() declarations across the code and use coredecls.h instead.Implement helper functions for libraries that were previously using esp_yield(), esp_schedule() andesp_delay() directly to wait for certain SYS context tasks to complete. Correctly use esp_delay()for timeouts, make sure scheduled functions have a chance to run (e.g. LwIP_Ethernet uses recurrent)Related issues:-esp8266#6107 - discussion about the esp_yield() and esp_delay() usage in ClientContext-esp8266#6212 - discussion about replacing delay() with a blocking loop-esp8266#6680 - pull request introducing LwIP-based Ethernet-esp8266#7146 - discussion that originated UART code changes-esp8266#7969 - proposal to remove delay(0) from the example code-esp8266#8291 - discussion related to the run_scheduled_recurrent_functions() usage in LwIP Ethernet-esp8266#8317 - yieldUntil() implementation, similar to the esp_delay() overload with a timeout and a 0 interval

Labels

None yet

4 participants

Movatterモバイル変換

Delay(0) is an obfuscation of yield() or esp_schedule(); esp_yield()#7146

Delay(0) is an obfuscation of yield() or esp_schedule(); esp_yield()#7146

Uh oh!

Conversation

dok-net commentedMar 10, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

d-a-v commentedMar 10, 2020

Uh oh!

dok-net commentedMar 10, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mcsprMar 10, 2020

Choose a reason for hiding this comment

Uh oh!

d-a-vMar 10, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcsprMar 10, 2020

Choose a reason for hiding this comment

Uh oh!

d-a-vMar 11, 2020

Choose a reason for hiding this comment

Uh oh!

devyte left a comment

Choose a reason for hiding this comment

Uh oh!

dok-net commentedMar 10, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

dok-net commentedMar 10, 2020

Uh oh!

d-a-v left a comment• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dok-net commentedMar 11, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dok-net commentedMar 10, 2020•
edited
Loading

dok-net commentedMar 10, 2020•
edited
Loading

d-a-vMar 10, 2020•
edited
Loading

dok-net commentedMar 10, 2020•
edited
Loading

d-a-v left a comment•
edited
Loading

dok-net commentedMar 11, 2020•
edited
Loading