Bug in Agenda::update() about agenda HOT 3 CLOSED

Hi there, thank you for your support.
I will start saying that I never considered the necessity to use update or delay inside a task function call, but this could not be stupid or useless, but I see a bug (or a typo) in your test code;

delay() function parameter should be expressed in milliseconds.
In this case you are asking to execute every 1000 microseconds the task and delaying for 100 milliseconds, so for sure the function is called every delay call also more than once.

In any case, because of the order I selected here in the update function:
https://github.com/gioblu/Agenda/blob/master/Agenda.cpp#L91

Actually happens exactly what you are saying if delay() or update() are called in a task function because timing value and task removal is executed after, and not before as should be, the task execution.So the second level call of update will find the task ready to be executed still not cleared by the first level call.

Thank you for your support. I will soon propose you a bugfix.

from agenda.

You are right there is typo in test code but it actually doesn't matter what parameters I use because any parameters could trigger the bug. The example code should be:

void some_task() {
    sched.delay_microseconds(100); // <-- 100 us
}
void setup() {
    sched.insert(some_task,1000); // <-- 1 ms
}

It's nonsense to try schedule some task to run every 1ms when task run time is at least 100ms.

update() should not called in any task function and delay() function calls in task function should not crash whole program. Also allowing delay() call in task function may simplify some tasks and lead less spaghetti code.

In update() function doing timing calculation just before task execution removes stack overflow crash. But task timing might still be wrong when delay() is used in task but that is other issue and solving it that might be much harder. So you should update documentation also that it's not recomended to use delay() in task because it might lead longer delays than programmer has meant to.

By the way I think that more accurate timing calculation would be

_tasks[i].registration += _tasks[i].timing;

from agenda.

Hi! I added in readme what I think about the point of the delay inside task function and I have moved the line as we said to fix the stackoverflow of update in task function.

Agenda is not using interrupts, so executing a really long task inside a delay, in general could lead to drastically overshoot the originally requested delay value.

With your comment on the registration update line, you let me focus on another quite big problem here. With a long function execution before another you have the time variable not at all correlated with the actual time so I will add time = micros() after task execution to bring back time variable coherent. I am now not sure to add your proposed line because I would discuss with you more in particular the possible risks of the variable overflow with this addition, and this what potentially could lead to.

Will follow modification as explained. Thanks again.

from agenda.