add regular scheduled functions, now also callable on yield()

[d-a-v]

added bool schedule_function_us(std::function<bool(void)> fn, uint32_t repeat_us)
lambda must return true to be not removed from the schedule function list
if repeat_us is 0, then the function is called only once.

Legacy schedule_function() is preserved

Linked list management is simplified

This addition allows network drivers like ethernet chips on lwIP to be regularly called

• even if some user code loops on receiving data without getting out from main loop
  (callable from yield())
• without the need to call the driver handling function
  (transparent)

This may be also applicable with common libraries (mDNS, Webserver, )

[earlephilhower]

This may be more of a scheduled_functions general question, but what about interrupt safety? One use I see for this is to put a notice into a queue in an IRQ (instead of actually doing work there). But you could also have things like repeated ones, now, in the main app. So isn't there a race condition in the list update stage (or if, say, an IRQ happens while parsing through the existing list)?

[d-a-v]

but what about interrupt safety?

You are right.
Per #2218 (comment), critical sections are now protected.

edit: still not sure this is the right way
edit2: is it better (per Arduino.h comment) ?

uint32_t savedPS = xt_rsil(15);
// do work here
xt_wsr_ps(savedPS); // restore the state

[earlephilhower]

That looks good and seems like it will preserve the last IRQ level which is what is needed.

I think you also need locking around get_fn. It's not likely (so would be ugly to debug) but you could get an IRQ in the middle of it and a call to schedule_fcn()...which will call get_fn() again and your linked list operations will go wonky.

[dok-net]

May I bring the existence of cores/esp8266/interrupts.h to your attention? That actually gets included in Esp.cpp already.
About the use of volatile, I have some reserverations - IIRC, memory fences must be handled differently.

[dok-net]

I've grepped across the source tree, and interrupts are disabled in one form or the other in any of these places:

libraries/EEPROM/EEPROM.cpp: noInterrupts();
libraries/ESP8266SdFat/src/SdCard/SdioTeensy.cpp: noInterrupts();
libraries/ESP8266SdFat/src/SpiDriver/DigitalPin.h: cli();
tools/sdk/lwip2/builder/glue-esp/lwip-esp.c: ets_intr_lock();
cores/esp8266/core_esp8266_timer.cpp: uint32_t savedPS = xt_rsil(15); // stop other interrupts
cores/esp8266/core_esp8266_wiring_digital.cpp: uint32_t savedPS = xt_rsil(15); // stop other interrupts
cores/esp8266/interrupts.h: _state = xt_rsil(15);

I gather, that timer and GPIO interrupt handlers by design decision ("AVR compatibility" gets mentioned) always run with interrupts disabled, reducing the changes that an ISR gets interrupted to about zero.

I think tools/sdk/lwip2/builder/glue-esp/lwip-esp.c maintains a linked list a lot like the one that's being discussed here, and stands out as having a rather large block of code during which interrupts are disabled.
I intend to take some measurements as to the effect disabling interrupts has on the receive timings of EspSoftwareSerial and report these soon, just be be on the safe side, please mind my word of caution that too much interrupt blocking adversely affects the real-time characteristics of the platform. Also, there is a chance that the data structure being used for the schedule queue is non-optimal for this case, and there might a problem with atomic operations / memory fences even in the case where interrupts get disabled. Is anyone knowledgeable of what the compiler and CPU do to register aliases during interrupts, i.e., do they get written to RAM and restored from RAM upon interrupt entry and return? I doubt that.

Now, late at night, all I can contribute toward a solution is this article, containing supposedly lock-free code for a queue:
https://stackoverflow.com/questions/871234/circular-lock-free-buffer

I will need to look into that for EspSoftwareSerial, the ISR buffer interacts with user code and has all the possibility of issues that I suggest this PR might have.

[d-a-v]

May I bring the existence of cores/esp8266/interrupts.h to your attention? That actually gets included in Esp.cpp already.

Sure, and thanks for it. I wasn't aware (!)

About the use of volatile, I have some reserverations - IIRC, memory fences must be handled differently.

This volatile is unnecessary, I forgot to remove it.

please mind my word of caution that too much interrupt blocking adversely affects the real-time characteristics of the platform

Sure but they are necessary to avoid races.
Lock-free queue structure is very interesting.
Maybe a new issue would have to be opened to discuss about your comment about locking:

• use the best/correct API everywhere
• consider lock-free linked lists
• your results about measurements on the receive timings of EspSoftwareSerial when disabling interrupts

Is anyone knowledgeable of what the compiler and CPU do to register aliases during interrupts, i.e., do they get written to RAM and restored from RAM upon interrupt entry and return? I doubt that.

Registers used in a function (or all registers) are pushed on the stack and restored before the "reti".
I've seen that with gcc on other archs. I believe this is always mandatory when dealing with interrupts.

[dok-net]

Registers used in a function (or all registers) are pushed on the stack and restored

That's what I believe and that's the reason to use std::atomic load and store - otherwise all interrupt locking may not be much help. Or am I completely missing something?

[earlephilhower]

@dok-net, what specifically are you worried about?

The SDK blob IRQ wrapper stores all the registers in use before calling an IRQ function and then restores then before returning from interrupt. I don't think there's any particular concern there, it's a simple and common operation.

On function entry in main code you call IRQ-disable as the first state changing operation. That either finishes uninterrupted, or an IRQ gets called. That IRQ may call the same function, (and can't be interrupted) so will run to completion changing the linked list). On return from interrupt the main app's locking code completes and it has full access to the updated list (there is no chance of the list being cached incoherently anywhere in the machine) and can do its own work...

[dok-net]

@d-a-v @earlephilhower Is there a quick explanation why linked lists are used? IIRC linked lists are discouraged for use where lock-free programming is an advantage. I just too a perfunctory glance at the code spots and couldn't figure out why linked links are used - if all the reason should be to keep entries in for repeated execution, pop and re-push should do the trick, too. I've implemented a lock-free (well, on ESP32, on single-threaded ESP8266 it disable IRQs after all ;-) ) ring buffer / circular queue for EspSoftwareSerial, which I am still testing. It's multi-producer, single-consumer capable, and could be a nice basis to #6039...

[earlephilhower]

@dok-net I didn't do any of the coding on this, but I imagine linked lists were used to minimize heap usage when only a few (or common case: 0) delayed functions are in play. There's no atomic TAS operations on the chip, though, so even simple things like mutexes aren't doable safely w/o stopping IRQs. I would not want to guarantee that std::atomic (if available, I don't remember if it compiled or not) works as-expected with interrupts on this chip, actually.

In any case, I'm still trying to understand the concern here. Logically, I think it's fine. Are you concerned about performance (i.e. you have a use where you have to add items to the queue at high frequency, etc.? W/only 32 slots even that's a pretty bounded problem) or something else?

[dok-net]

Due to multiple changes, I've left a PR with explanations - d-a-v#6

[dok-net]

IMHO: This linked-list implementation is not - probably never was - preemption safe, generally a compiler will keep the values of all the pointers in registers, even on the single-core ESP8266 an IRQ will not flush the registers but just push them to the stack and restore them, therefore any IRQ that's scheduling functions fails during an ongoing run_scheduled_functions(). Blocking IRQs during the complete execution of run_scheduled_functions makes it thread/IRQ safe, but I don't think this is permissible from an IRQ performance POV.

[d-a-v]

Good point.
We don't want to lock while executing the scheduled function themselvses.
One solution is to tag variables with volatile.
Another one is to build a local copy of the list while being locked, then unlock and run that list.

[d-a-v]

I think I must step back on this.
Locking IRQ is the right thing to do when the value of a variable can be modified in a TAS block.
Even if there are registers holding some variables, they will not be changed while in a locked block because an IRQ won't occur in that block, regardless whether the variable is cached in a register.

I think the compiler will not / must not optimize a variable into registers when is it not declared locally.

[dok-net]

@d-a-v What happens if a scheduled function/task calls yield() etc. itself? Calling schedule_function() recursively seems safe, but what about this?
I am beginning to think that loop() should be just another (initially) scheduled function, returning true forever, what do you think? That said, wasn't there a scheduler for adding loops() in place all the time...?

https://www.arduino.cc/en/Reference/SchedulerStartLoop
https://github.com/arduino-libraries/Scheduler/

[d-a-v]

What happens if a scheduled function/task calls yield() etc. itself?

My thinking is these scheduled functions should be thought as interrupt functions (that are artificially shifted out from sys stack).
Only the minimal should be done in them, no delay no yield.

If it happens we need to be calling other functions that themselves call yield, then we can add a boolean fence set up in run_schedule_functions that will be checked in yield family functions.

I am beginning to think that loop() should be just another (initially) scheduled function

That can be something we think about when we will move from nonos-sdk to rtos-sdk which will happen sometimes soon enough.

That said, wasn't there a scheduler for adding loops() in place all the time...?

check loop_task() and loop_wrapper()

[dok-net]

@d-a-v please revisit my edited comment above regarding https://github.com/arduino-libraries/Scheduler/
I would love to stick to the Arduino idea in ESP8266/ESP32 "Arduino" :-) :-)
What seems really interesting is the stack modifications or using the same "loop" stack for all "functions". The use of assembly language in the Arduino Scheduler is holding me back - on the other hand, loop_task() and loop_wrappper() are pretty low-level and I have too little understanding of the ESP8266 Arduino internals to participate in this discussion beyond asking some obvious questions.

[d-a-v]

(edited: task->stack)

These schedulers are multitasking schedulers with each its own task.
When we switch to rtos-sdk, we already have that with FreeRTOS pretty much like in arduino-esp32 which you know well because of EspSoftwareSerial compatible with esp32. On esp8266 we have limited ram so maybe it's not a good idea to have a separate stack for each task (as opposed to scheduled functions running on the same stack).

[dok-net]

@d-a-v I've rebased d-a-v#6 - there's still something to do, one critical bug and performance related nullptr and rvalue refs.

[d-a-v]

one critical bug

Which is it ?

[dok-net]

Null pointer access in last line of get_fn_unsafe

[dok-net]

@d-a-v Big oops, something we've all missed:

if (item->callNow)
{
    if (item->mFunc())
    {
         lastRecurring = item;
    }

lastRecurring should get updated if (!item->callNow) :

while (toCall)
{
    scheduled_fn_t* item = toCall;
    toCall = toCall->mNext;
    if (!item->callNow || item->mFunc())
    {
        lastRecurring = item;
    }
    else
    {
        InterruptLock lockAllInterruptsInThisScope;

        if (sFirst == item)
            sFirst = sFirst->mNext;
        else if (lastRecurring)
            lastRecurring->mNext = item->mNext;

        if (sLast == item)
            sLast = lastRecurring;

        recycle_fn_unsafe(item);
        }
    }

[dok-net]

@d-a-v Could we maybe agree to rename toCall as next? I've really a deep expectation in my mind that toCall is the current, to call, item, and it's making the piece of code very hard to reason about.

[devyte]

In that case:
toCall => nextItem
item => currentItem or currItem
or something along those lines

[d-a-v]

Already updated in #6137