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

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

Closed
wants to merge 2 commits into from

Conversation

dok-net
Copy link
Contributor

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

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, the delay(0) is probably unintentional, the esp_schedule() direct call is used in a few places already, and is functionally identical in this place, while also more expressive and saves resources.

@d-a-v
Copy link
Collaborator

d-a-v commented Mar 10, 2020

panic() is called when yield() is called from SYS,
but not when delay(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;
}

@dok-net
Copy link
Contributor Author

dok-net commented Mar 10, 2020

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

@@ -892,7 +892,7 @@ inline void
uart_write_char_delay(const int uart_nr, char c)
{
while(uart_tx_fifo_full(uart_nr))
delay(0);
yield();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at uart_write_char_delay, it is used by the uart{0,1}_write_char and those specific write funcs are installed as ets_install_putc1(...) when debug mode is set. Does this mean that SYS calls this part, not CONT?

Copy link
Collaborator

@d-a-v d-a-v Mar 10, 2020

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 replace delay(0) here.
So uart{n}_write_char() should be in IRAM right ?
And uart_write_char_delay() is inline, maybe the attribute __attribute__((always_inline)) should be added ?

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

Copy link
Collaborator

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

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 (like register 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 d-a-v self-requested a review March 10, 2020 10:55
Copy link
Collaborator

@devyte 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 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 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
@dok-net
Copy link
Contributor Author

dok-net commented Mar 10, 2020

@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 of delay(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 to yield(); from esp_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 Arduino yield() will resume, whereas for indefinite suspension, esp_yield() exists on ESP8266.
esp_schedule(); is, and always was, unconditional in delay(0);, if this question comes up.
And in any place where yield() before was synomymous to panic(), well, the code wouldn't run, and using yield() as metaphor for panic() is even more weird than delay(0) instead of yield() :-)
@d-a-v please take notice.

@dok-net dok-net closed this Mar 10, 2020
@dok-net dok-net deleted the delay0isyield branch March 10, 2020 17:37
@dok-net
Copy link
Contributor Author

dok-net commented Mar 10, 2020

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

Copy link
Collaborator

@d-a-v d-a-v left a comment

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 with this 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
Copy link
Contributor Author

dok-net commented Mar 11, 2020

Resuscitated in #7148 with a unique wrapper for esp_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
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 is
supposed 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() and
esp_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
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.

4 participants