Make yield() overridable

[dok-net]

Hi @me-no-dev!
As I've a fully portable co-op scheduler running on ESP8266, ESP32 and Arduino (physically the ATmega 328P - Arduino Pro or Pro Mini) now [https://github.com/dok-net/CoopTask], here is another request to allow nice integration into the loop() with yield() (and, later, optionally, delay()). AVR Arduino et cetera have this, because yield() is overridable and delay() cyclically calls yield(), but as you know, the scheduling on the Espressif SOC is different.
I just hope that this very small adjustment is agreeable to you, even if you don't think that cooperative MT is useful on FreeRTOS, but users should implement OS tasks instead...

[me-no-dev]

I do not understand why you need those to be overwritten? Those are the proper integrations for ESP32 and FreeRTOS, no other delay is good idea.

[dok-net]

Per se, the weak linkage doesn't cause additional delay - if you are separately concerned about the loop_end(), I am willing to drop that from the patch if it helps get the main portion through?
Edit: I may have misunderstood you; you're probably talking about the delay() function, not additional delay added to execution times. I reiterate, I really don't believe forcing people to write completely different real-time multithreading software for the ESP32 than for Arduino or ESP8266 does the platform a great benefit. For many of them, the ESP32 is an affordable substitute for the ESP8266 with many more GPIOs; for others, it's an Arduino with on-board WiFi (and BT); I expect very few "makers" are able to get multi-threading right, in particular on real-time systems with thread/task priorities - think inversion of priority bugs.

[dok-net]

@me-no-dev Oh and juggling three different platforms just writing this library is crazy and full of surprises enough already. I forget that yield() is not a hook on ESP32 despite being so in "regular" Arduino, this patch includes that fix.

[me-no-dev]

I fully agree that moving to threading is not easy, but this is where things are going and I find it a bad practice to try and make esp32 be esp8266. Regardless how people see ESP32, it is not an ESP8266 with more pins :) I asked for the wek linkages because they are pointless for the functions you did it for. Delay on ESP32 will always be tied to vTaskDelay for example (RTOS API), yield is mostly pointless, but is there to keep 8266 folks happy.
Arduino for 8266 will be moving to RTOS as well. Once that is done, all those things you do here will be pointless and another set of work will have to happen to bring 8266 to esp32 :)

But my real question is why do you need those weak? what is te benefit?

[dok-net]

I'll gladly try to answer your question.
Making the bindings weak allows me to hook into those functions and insert additional behavior. The original code still gets called!
I can't prove it, but I am confident that there a loads of device libraries out there, that are portable in the sense of the Arduino single-thread API. Frankly, FreeRTOS doesn't look for the faint of heart, either.

In tools/sdk/include/freertos/freertos/FreeRTOSConfig.h, I find
#define configUSE_PREEMPTION 1
I imagine it's possible to specify a set of tasks that are non-preemptable among them, basically presenting a cooperative multithreading to each other, and it may be not that hard for someone who learns FreeRTOS to implement a user-friendly wrapper around that. Still. reusing what works on Arduino and ESP8266 seems not such a bad idea, and that's what my CoopTask does. For that, I really need the weak bindings, and I can just hope and very kindly ask you to make that tweak to the core, pleeeaase?

CoopTask Github repo

[dok-net]

@me-no-dev After reading the configuration options documentation and the actual settings in effect here, I find that Arduino ESP32 both preempts for the benefit of higher-priority tasks, and performs round-robin scheduling between same-priority tasks - I assume, also by preemption. For any so called bit banging among other things like atomic access and synchronization issues, this requires working with priorities. Please allow the weak bindings such that integration of any additional behavior at yield, delay and the loop() become available to library builders, just like on Arduino and ESP8266. It's absolutely not about making the ESP32 anything ESP8266-like.

[dok-net]

@me-no-dev First, only rebased, no changes.
Second, please consider this PR from the point-of-view of Arduino users, not as an ESP8266 emulation initiative.
Third, I may not have been completely clear on this: the weak functions are not about replacing the calls into Free RTOS, but they are there to shim some additional code that needs to run periodically. For (Mega AVR) Arduino code, this works for yield() and delay() for nearly 7 years now [69aead513d]

[me-no-dev]

My issue is that you want to alter the way that those functions will work. And I presume you think that others will use it too. As a user, I do not want those functions to do anything other than what they should do and I do not want that to change because I installed a library. It will mess up my other code.

If you need high prio task, be my guest and spin one ;) there is API for it. Does the official AVR arduino allow overwrite of those functions?

[dok-net]

@me-no-dev For the "The Official Arduino AVR core", the answer is yes, it does, see above [69aead513d].
Same for (official) Arduino Core for SAMD21 CPU, commit-ish 973ccc47d8.
Most likely, etc. etc. ;-)
A minor difference, delay() is implemented in terms of a busy-loop over yield() there, whereas my ESP32 patch does the appropriate thing on this platform.

About the loop() shim, AVR and SAMD cores have:

for (;;) {
	loop();
	if (serialEventRun) serialEventRun();
}

I think it's a mistake of [69aead513d] not to shim loop() in addition to yield() and thus implicitly delay() as well, because of that, the main loop() depends on calls to delay() and yield() for the (official) Arduino Scheduler library to run at all! Any sketch simply cycling in loop(), perhaps just using the synchronous delayMilliseconds(), blocks the scheduler altogether . We can do better than that :-)

[me-no-dev]

for yield I can accept. adding serialEventRun like here: https://github.com/arduino/ArduinoCore-samd/search?q=serialEventRun&unscoped_q=serialEventRun I can also accept. delay... not

[dok-net]

@me-no-dev I've done some further research. Would you be willing to merge if the delay() portion is temporarily reverted and split off into a future PR?
Because, at the end of the day, the way delay() is done now in ESP32 (ESP8266 too) is not quite Arduino compatible.
In official Arduino BSPs (AVR etc.), delay() iterates over yield() until the time is up.
vTaskDelay() on the other hand completely stops the Arduino world for x milliseconds.

[dok-net]

(updated PR subject line and introductory first comment)

[atanisoft]

@dok-net The reason the delay() call is implemented as a call to vTaskDelay() is because of the underlying env is using FreeRTOS functions to invoke setup() and loop() already. Having a busy loop approach calling yield as you suggest would leave the user with a watchdog timeout crash that they can't easily fix when they are using your library that replaces delay().

I'd suggest keep with just the yield() override that you have provided here and leave delay() as is.

[dok-net]

@atanisoft I assure you that without shimming delay(), too, in some way, the utility of any single-thread scheduling library is seriously limited. The other delay routine, delayMicroseconds(), is bad enough already, but sketches and libraries should be trusted that it's only ever used for high-precision delays, which are also very short delays, and therefore it is both without alternative and less hurtful. OTH, in a single milliseconds, my test sketch iterates over 30 times over a handful "tasks" and the loop(), blinking an LED, handling a button, dumping diagnostics periodically to serial, and running the webserver stack. Stopping the show takes all the advantage out of cooperative multitasking, and well, delay() isn't at all cooperative in its current implementation ;-)
CoopTask helps itself with pre-processor substitutions of delay() (and yield() where required) now, but that's prone to errors - calling direct CoopTaskBase::delay() falls victim to that double-substitution and the compiler chokes.
So, to be precise, invisibly iterating yield() in delay() is just a good enough fit on single-core MCUs that have nothing better or more power-conserving to do with their time, that's why at the end of the day, I guess I am asking for a way to override top-level delay() in order to do the right thing on a given platform, and the weak binding is the perfect way of doing that which doesn't hurt performance or anyone else for that matter if not used.

[atanisoft]

Co-operative scheduling doesn't work in a pre-emptive scheduled environment. Trying to shoehorn that together is an act of madness. Embrace the FreeRTOS support that the platform has and let it's scheduler manage the tasks for you!

Keep in mind that the esp32 is a dual core system and is not limited like the other environments your library may work in

[dok-net]

All I am asking for is less judgement against, and more freedom for the application and library developers. There is really no need to protect them from making choices, and, well yes, shoehorning FreeRTOS down everyone's throat and calling it Arduino doesn't fit every size.
Can you please explain what I must be missing, where is the detriment in weak binding delay(), which is only relevant to the Arduino API users at all?
While obviously I am trying to make my implementation of CoopTask work better, there can be all sorts of further uses in intercepting yield() and delay() in (existing) code, the serialEventRun() is an indication for that.
Besides, the Arduino world is by default pinned to core 1 on the ESP32.

[atanisoft]

By allowing the library to replace delay() it will lead to hard to trace watchdog timeouts for the end user. It will have other effects since the usage of vTaskDelay is critical to FreeRTOS on the esp32 in order to ensure all tasks are executing. It is not all about end user code, there are other housekeeping tasks that run as well.

serialEventRun is an interesting one, I haven't seen it in use so I can't really comment on it too much. But what I can say about it is that it looks like a nice shim for calling a function after loop() runs. It doesn't replace the core required functionality of the underlying system.

[dok-net]

You are right, there are ways to abuse a delay() shim, but I am sure there also many other ways to implement hard to track errors already :-)
As far as I am concerned, I only need to intercept the very fact that yield() or delay() were called, in order to do the specific state change, only in ADDITION to calling the underlying yield() or delay() implementation eventually. It's not about omitting or replacing that at all.
Reading some sensors periodically, writing to a display every x seconds, handling webserver requests, and responding to button presses, doesn't require multithreading or realtime programming expertise, if it's done from (stackful) coroutines, and as long as delay()s don't stop the world. I have seen sketches that are a work of horror, and additionally keep blocking activity like web requests while doing serial IO or I2C, just because delaying one task and doing other activity during that time is so hard on (novice) developers.

[atanisoft]

There are a little t of ways to break things, that is certain.

I'd suggest using the yield() hook alone in this case. If they call delay() and you have it in your library readme to avoid that API then it is on the end user. There also are a number of libraries that call delay() internally which could be broken by replacing the underlying delay call (if it is tiny dependent).

And you are right, some of the take you describe don't need to be run as multi-threaded and in some cases would be hindered by it (httpd). A FSM based scheduling system is perfect for some use cases. I use them in my projects running inside a FreeRTOS task with great success, a single task to implement httpd with concurrent connections as an example.

[dok-net]

OK, let me see, I am arguing off the assumption, contrary to your's, that there are delay() calls in off-the-shelf libraries that would benefit other tasks if transparently yielding to those during the blocking period, much more so than causing trouble. Keep in mind that the Arduino Scheduler library does exactly that, so one could claim that every library is already responsible for explaining to users whether it works with the Scheduler, where it does, where it doesn't, and what to do to get the greatest benefit from including the Scheduler library. No one is forced to use it.

Use of delay() in libs

Using delay() in init() or begin() seems common.

BMP280 / BME280 libs

A delay to let the hardware catch up in some "forced" mode.

BH1750

In multiple places.

DHT

Multiple times.

SDS011 particulate matter sensor on serial IO

Essential, the specified message rate it 1Hz.

E-Paper library

Multiple places in the code.

HP303B

Multiple places in measurement code.

These are just the few libs that I have git clones of.

[atanisoft]

delay() is definitely used in a number of libraries, which is why it is imperative not to break those libraries by replacing delay(). I'm also not certain that most of the usages of delay() in libraries is correct for all environments and most libraries out there flat out ignore error checking entirely (virtually every Wire usage is missing error checking!)

[dok-net]

Yes... let me add that my own shim checks the delay() call's context first, doesn't change the behavior (except the few CPU cycles for the check) at all if not called from a "task", therefore only ever has any noticeable side-effects in code that's in the user's sphere of responsibility anyway. I am really begging now :-)

[dok-net]

I've made up my mind, and I can only hope to convince you, that for an Arduino layer, which this is all about, both yield() and delay() must have a way of shimming functionality. Anyone programming against FreeRTOS isn't affected, default behavior isn't affected at all by weak bindings, suppressing this capability is not in the interest of the community, given that plain Arduino has it and has had it for many years now. Please agree.

[dok-net]

From Arduino reference on delay():

"More knowledgeable programmers usually avoid the use of delay() for timing of events longer than 10’s of milliseconds unless the Arduino sketch is very simple"

The take on this is twofold - one possible reading is that improving delay() by shimming it is a waste, because the "more knowledgeable" avoid it anyway, another reading is that making it more versatile is pretty welcome.

ESP32 specific, it seems that xTaskAbortDelay is not available, so one cannot break out of a delay, say, to respond to an interrupt. Is that on purpose?

(The FreeRTOS documentation (is this the right place to look?) is missing version information for the introduction of features, in Arduino-esp32 tools/sdk I find no trace of xTaskAbortDelay. This semaphore with block time example shows an alternative pattern. Both would at least open the way to an implementation of the CoopTask API in a FreeRTOS environment, if this is really useful or just a leaky abstraction, depends...)

[dok-net]

@me-no-dev @atanisoft I'm proposing #3227 as a way for me to port CoopTask to ESP32 (FreeRTOS in general?) in more palatable way, I hope. If that makes sense to you, I'd remove the delay() portion from this (#2991) PR, the remaining code passed your review, as far as I understand it?

[dok-net]

@me-no-dev @atanisoft I hope you'd be disappointed if I'd given up that easily ;-) So - I've read the docs some more, and think I've come up with a less intrusive way to do what I want to do.

If this PR is still OK (for merge) less the delay() shim, I just need one hopefully acceptable modification to delay() that I really can't do without, and that's a nod to AVR Arduino's delay() as well as ESP8266 Arduino master, both of which do something extra on delay:

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

The yield() after vTaskDelay() allows code to detect that a task was blocked.

Please, this is ESP32 Arduino, not the ESP32 IDF, allow some concessions.

[dok-net]

Additional suggestion, I could also do:

void delay(uint32_t ms) {
    vTaskDelay(ms / portTICK_PERIOD_MS);
    if (yield != __yield) yield(); // <---------------------------
}

This prevents the unnecessary vPortYield() after vTaskDelay() if yield() isn't overridden. I'm obviously not sure if vPortYield() in a NOP in Espressif IDF FreeRTOS on ESP32 :-)

[atanisoft]

I'm obviously not sure if vPortYield() in a NOP in Espressif IDF FreeRTOS on ESP32 :-)

vPortYield is not a NOP... https://github.com/espressif/esp-idf/blob/v3.2/components/freertos/portasm.S#L498

[dok-net]

Great, so the function pointer comparison is justified. Commited and pushed.

[dok-net]

@atanisoft @me-no-dev With what I hope is now a minor-sized PR here, I've also ported CoopTask to use FreeRTOS native tasks as substrate. This PR is essential for this to work. Is there now hope for this to pass review and get merged, is there anything else that I can to help it become acceptable? Thanks for bearing with me!

[me-no-dev]

uhmmm ... no! no changes to delay ;) in FreeRTOS delay does yield.

[dok-net]

@me-no-dev I am not even sure that using FreeRTOS tasks, that is, starting AND destroying them, is such a great idea with ESP32 Arduino, as my reading of the docs is that it takes the IDLE task to clean up destroyed tasks completely. Generally I don't see Arduino sketches letting the IDLE task run at all, unless *Yield defers to lower priority tasks?! But no one is required to use *Delay in a sketch, right?

Haven't I proved beyond reasonable doubt that AVR Arduino, as the gold standard, calls the OVERRIDABLE Arduino yield() as part of any Arduino delay()? Everyone is free to use FreeRTOS calls on ESP32, but Arduino libs and Arduino sketches don't and so they can rightfully expect Arduino compatibility, don't you think so, too?

I've learned from discussions here that there is such a thing as using Arduino-esp32 as a so called "component". There's already an incompatibility with that mode in the much lower tick frequency. So maybe, could your propose some #ifdef define for me to use around the above code change, such it will be disabled in the component mode?

I would also be quite willing to research into an even more transparent way of shimming vTaskDelay(), the issue being how to learn the xTicksToDelay value. I haven't found any.

TL;DR: vTaskDelay() doesn't do (Arduino) yield(), but puts the FreeRTOS task in an opaque blocking state.

[dok-net]

Dear @me-no-dev,
I've finally found a way to do what I want without doing anything you don't want in core - is this PR now up to standards, can you merge, or is there something I'm missing?

[me-no-dev]

yup :)

[dok-net]

Great - CoopTask 2.2.0 works out of the box on ESP32 now.