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

Ongoing Schedule work: crashes in use case: nested schedule_recurrent_function_us() #6235

Closed
dok-net opened this issue Jun 27, 2019 · 7 comments

Comments

@dok-net
Copy link
Contributor

dok-net commented Jun 27, 2019

@d-a-v As discussed - I hope I understood you right and this is indeed supposed to work, otherwise forget about it - here's runnable code. Works out-of-the-box in #6139, crashes in master HEAD. Playing with different values for repeat_us, either crashes later, or goes it WDT reset.

Sketch:

#include <Schedule.h>
void setup()
{
	Serial.begin(115200);
	delay(500);
	Serial.println("Scheduler test");
	schedule_recurrent_function_us([]()
		{
			Serial.println("First function");
			schedule_recurrent_function_us([]()
				{
					Serial.println("Nested in first function");
					return false;
				}, 1);
			return false;
		}, 1);
}

void loop()
{
}

Stack decode:

   :Error:3 -> LoadStoreError: Attempt to read/write memory in a manner not supported by the hardware (for example, trying to read/write a byte to a memory area that only supports word accesses)
   0x401004b1 umm_assimilate_up
   : ?? ??:0
   0x401005a0 _umm_free
   0x401009ec free
   0x4020a64d operator delete(void*)
   0x402010a0 std
   0x40203068 std
   0x402017a6 delay
   0x402020d1 run_scheduled_recurrent_functions()
   0x40201156 setup
   0x40201038 std
   0x402012aa loop_wrapper()
   0x40100a49 cont_wrapper
   : ?? ??:0

d-a-v added a commit to d-a-v/Arduino that referenced this issue Jun 27, 2019
@d-a-v
Copy link
Collaborator

d-a-v commented Jun 27, 2019

@dok-net you're right. I updated the #6234 documentation about this fact, thanks.

About #6139 this is a huge PR with 25 files changed. So it will take time to be reviewed by maintainers.
I will not endorse alone such a big change.

If this feature is important, please explain what would be the use case for a recurrent scheduled function to create from inside another recurrent scheduled function.

@dok-net
Copy link
Contributor Author

dok-net commented Jun 27, 2019

If this feature is important, please explain what would be the use case for a recurrent scheduled function to create from inside another recurrent scheduled function.

I don't know, just general expectation, if it's documented as non-permissible, fine. Just making up a use case, is when recurrence is not desired, but delay is needed (return false on first call) - changing the "handler" and the next delay would be implemented as scheduling from inside such a function (?).

@d-a-v
Copy link
Collaborator

d-a-v commented Jun 27, 2019

got moderated/deleted by "esp8266" for some reason I don't know.

Nothing is deleted. Your message has just been moved to the right issue, reference is above.

edit: "I wonder why you didn't notice and nobody pointed it out for you" is that typical kind of message you are used to throw down at people which is not meant to keep everybody in good mood.

Just making up a use case, is when recurrence is not desired, but delay is needed (return false on first call) - changing the "handler" and the next delay would be implemented as scheduling from inside such a function (?).

That might be a use case, yes (?).

@dok-net
Copy link
Contributor Author

dok-net commented Jun 27, 2019

@d-a-v Thanks for moving the heads-up to the other "thread".
About #6139, I am not wedded to it, but I started that work out of conviction that a lock-free queue would be appropriate for the work at hand, I find it working in any situation that the linked-list masters, it still works when the linked-list fails, there are (perhaps theoretical) cases where it also should be more reliable (compiler aliasing of atomic values is obviously open for discussion, even with a single-threaded non-preemptive, except interrupts, model).
Sorry about PR #6139 seeming "huge". In fact, it really isn't that bad, most things are boilerplate code for the new libraries moved out of cores, some dependency fixes (good thing), reshuffling functions inside compilation units, whitespace that is a PITA (pls. suppress when reviewing), example sketches requested by reviewers (!), and the files moved from cores to libraries. I wish git had a different ideology about file moves and renames, but that's a different story :-)

@dok-net
Copy link
Contributor Author

dok-net commented Jun 28, 2019

edit: "I wonder why you didn't notice and nobody pointed it out for you" is that typical kind of message you are used to throw down at people which is not meant to keep everybody in good mood.

This is getting ahead of me, I am sorry. I really do mean to express that I am surprised by the fact and that I wonder if there was/is a reason. There is no cause to think that the other person didn't feel let down. Direct technical rhetoric is quite common everywhere else, so please stay calm.

That might be a use case, yes (?)

You don't sound convinced.

In other news, the performance of the LIFO is (surprisingly much) better than that of the (combined) FIFO. I extensively reviewed one of the (Arduino) Scheduler ports, which unfortunately didn't work on master of Esp8266 Arduino, I haven't tried the original release points to verify if it did before. It required patching most of the externals in core_esp8266_main.cpp et al to be weak references in order to override them. While such an API seems more natural at first sight, I believe the function scheduling is more appropriate on an embedded device with little stack space to begin with. I wonder what the implications of switching to Free RTOS will be on the ESP8266, sticking to a 2-task model seems sensible even then, and is less likely to cause irritations with backports of ongoing library etc. work for applications sticking to the NONOS-SDK.

A reminder from that, though, is that fair scheduling is not a given. Porting an increasing number of libraries to a scheduler, and sketches implementing FSMs and other asynchronous practices using it, could lead to starvation if it's not done right. This is just thought food, I do not state that the implementation does in fact suffer such a shortcoming.

@d-a-v
Copy link
Collaborator

d-a-v commented Jun 28, 2019

You don't sound convinced.

Were you ?

so please stay calm.

Well then, because I am not calm enough, I will let others answering to you.

@d-a-v d-a-v removed their assignment Jun 28, 2019
@dok-net
Copy link
Contributor Author

dok-net commented Oct 1, 2019

Fixed in PR #6485

@dok-net dok-net closed this as completed Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants