scheduled functions: fixes

[d-a-v]

No description provided.

[dok-net]

@d-a-v Please also make changes to Schedule.h:

• don't advocate use of std::bind, which is primarily for currying and has readability issues (for humans), instead use lambda expressions
• the remark
  // Keep that in mind when binding functions to objects which may have short lifetime.
  is rather mysterious, anyone who has no clue of this better keep their hands of lambdas and bind ;-)
• wrt to run_scheduled_functions(), the comment has it that

// Use this function if your are not using `loop`, or `loop` does not return
// on a regular basis.

except, that yield() does the trick as stated above. Plus, does or does not delay(), or (in libraries) optimistic_yield() do the same? Please elaborate in comment, things are getting hard to do right.

• Oh, and yes maybe better give a hint that the scheduled functions shall not be long-running and must not yield() etc. themselves. Which is rather difficult to make sure when using libs, I guess (depending on delay() and optimistic_yield()'s behavior, though).

[hreintke]

@d-a-v @dok-net

Oh, and yes maybe better give a hint that the scheduled functions shall not be long-running and must not yield() etc. themselves

Is that only for the schedule_functions_us or also for the current schedule_functions ?
That would limit the possibility of usage. I use it also when there is library function that yields and I need to use that in AsyncWebserver callbacks.
Also longer running scheduled functions can be a valid use case.
The loop() can be completely empty and the scheduled functions the queue of actions to do in sequence.

[dok-net]

@hreintke @d-a-v Opinions on use cases vary widely, it seems. I personally tend to side with @hreintke - in fact, I am currently trying to concoct a test case and it just keeps crashing badly - I am not sure if this is exactly because yield() now causes an internal recursion :-( Hoping it's something else...

[dok-net]

@d-a-v The repeating doesn't work, the 2nd arg to schedule_function_us only specifies the initial delay, after that, the function is continually run without delays.

[d-a-v]

• yield() is now callable, recursion is prevented.
• comments are updated
• variables are renamed (I saw the request in another posts)

the function is continually run without delays

The following runs smoothly (polledTimeout self-resets)
Do you have another test failing ?

#include <Schedule.h>
void setup()
{
  Serial.begin(115200);
  Serial.println("\nHello");
  schedule_function_us([&](){ Serial.println("blah"); return true; }, 1000000);
}
void loop()
{
}

Opened for review

[hreintke]

@d-av :
Which branch/PR should I use to have the currently proposed schedule functionality.
There is also : #6039
For now I am using git master + locally added 6039.

What I do see that the schedule_function has an essential changed functionality because of also running them at yield();

In this

loop()
{
   do something
   delay(1000)
   do something else
   delay(1000)
  }

and :

void schedule()
{
	static int msi = 0;
	Serial.printf("%8d %3d mys\r\n",millis(),msi++);
	schedule_function(schedule);
}

The function is called three times per loop() instead of the current once

[d-a-v]

@hreintke #6039 was merged about 2 days ago. This commit just brings (final?) fixes.

run_schedule_functions() is indeed called three times in your example.
Each registered function is called only once per registration.
You can use the new function schedule_function_us() that will ensure regular calls (not realtime though, per comment in Schedule.h).

Does this break anything with your current use-case ?

[d-a-v]

Is that only for the schedule_functions_us or also for the current schedule_functions ?
That would limit the possibility of usage. I use it also when there is library function that yields and I need to use that in AsyncWebserver callbacks.
Also longer running scheduled functions can be a valid use case.
The loop() can be completely empty and the scheduled functions the queue of actions to do in sequence.

yield() can be called in scheduled functions.
scheduled functions may be long (they must call yield() or delay() then)
loop() can be completely empty like in the example above.

The only differences since #6039 are:

• scheduled functions are triggered at next yield()/delay()/loop() (only loop() before)
• the new schedule_function_us() (functions given to it can be seen as independant loop() called regularly) (that would make this core a "single stack multi-cooperative-threaded unreal-time OS").

edit: The mDNS loop could be called that way

[dok-net]

@d-a-v This below has somehow gotten lost between your fixes?
About polledTimeout, you're right, my handling of requeueing was conflicting, I've fixed this.

[hreintke]

@d-a-v
Running this sketch :

#include "InterruptTest.h"
#include "Arduino.h"
#include "FunctionalInterrupt.h"
#include "Schedule.h"
#include "Ticker.h"

Ticker t;

void setup()
{
	static int ai = 0;
	static int si = 0;
	Serial.begin(115200);
	t.attach_ms(300,
			[](){
		         Serial.printf("%8d %3d attach\r\n",millis(),ai++);
		         schedule_function(
		        		 [](){
		        	 	      Serial.printf("%8d %3d scheduled1\r\n",millis(),si++);
		        	 	      delay(100);
		        	 	      Serial.printf("%8d %3d scheduled2\r\n",millis(),si++);

		         	 	 });
				});
}
void loop()
{

	static int li = 0;
	static int li2 = 0;
	Serial.printf("%8d %3d loop1\r\n",millis(),li++);
	delay(1000);
	Serial.printf("%8d %3d loop2\r\n",millis(),li2++);
	delay(500);
}

The delay (100) in the scheduled function is not done, returns immediate.

   12099   8 loop1
   12361  40 attach
   12661  41 attach
   12961  42 attach
   13099  80 scheduled1
   13099  81 scheduled2
   13099  82 scheduled1
   13100  83 scheduled2
   13100  84 scheduled1
   13100  85 scheduled2
   13101   8 loop2
   13261  43 attach
   13561  44 attach
   13603  86 scheduled1
   13603  87 scheduled2
   13603  88 scheduled1
   13604  89 scheduled2
   13604   9 loop1
   13861  45 attach
   14161  46 attach
   14461  47 attach
   14604  90 scheduled1
   14604  91 scheduled2
   14605  92 scheduled1
   14605  93 scheduled2
   14605  94 scheduled1
   14605  95 scheduled2
   14606   9 loop2

[d-a-v]

@dok-net #6147, thanks!

@hreintke I said yield() and delay() are working. I should have said it is legal to call them.
In the delay() case, it is using a static variable that makes it non-reentrant.
You didn't have this issue before because scheduled function were only called at the end of loop().

One immediate solution would be to replace delay(100) with

    esp8266::polledTimeout::oneShotFastMs myDelay(100);
    while (!myDelay);

Another solution is an API change to allow calling non-recurrent function only on next loop() like it was before, or by adding a boolean to select when to call the function (yield/delay/loop or only loop).

A third solution is modifying delay() to allocate the structure instead of using a static one.
I wonder if it would work if this structure were in the stack.

I must ask for my curiosity why it is necessary to call delay when in a scheduled function.

[hreintke]

@d-a-v :
I knew that a static variable is used in delay(). That's why I made the sketch to see the behavior.

Although I can't find it now/remember which , some library called delay in a specific function,
I needed to get it out of ISR -> schedule.

The nasty issue is then that a very common and know function like delay() doesn't work as expected.

Because of the external libs, using the polledTimeout would require local patches.
API change looks a nice way around the issue.

If the wish list for functionality is still open 😄 I would like to have an option to remove a function from the list. Something like we can do in CallBackList, return a shared_ptr which can be reset or can go out of scope. (When I was working on CallBacklist, I had the idea to have the scheduled_functions just to be a specific case of callbacks)

Modifying delay() to allocate structure and allow reentrancy is probably quite challenging.
If you are moving to RTOS, think that also solves it.

[d-a-v]

If the wish list for functionality is still open 😄

It is. I need the functionality in yield and I found it useful to put the former one in yield too (the api was still marked "unstable" or so)
If it does not fit then we can change.

I think we agree fixing updating delay() is not a wise path.

We can revert to the old behaviour: scheduled functions only called at loop() with schedule_functions() but called from loop/yield/delay (no yield/delay calls allowed) with schedule_function_us(). That would make everybody fine with a not very consistent API.

To make it better can add a parameter with default value to keep previous behavior:

enum schedule_e { SCHEDULE_CAN_USE_DELAY, SCHEDULE_OFTEN_NO_YIELDELAYCALL };
bool schedule_function(const std::function<void(void)>& fn, schedule_e policy = SCHEDULE_CAN_USE_DELAY);
bool schedule_function_us(const std::function<bool(void)>& fn, uint32_t repeat_us, schedule_e policy = SCHEDULE_CAN_USE_DELAY);

It would be up to the user to ensure yield() nor delay() is called from inside the callback (with SCHEDULE_OFTEN_NO_YIELDELAYCALL)

[dok-net]

@d-a-v It's safe now to comment this in (per #6129)

[dok-net]

@d-a-v It's safe now to comment this in (per #6129)