last_run never updates when timer is disabled, re-enabled

[morganrallen]

In my use case I'm using a single Thread to run multiple sequences of varying times. The basic flow is.

• Enable PIN
• Set timer
• ....
• onRun
• Disable PIN
• Set timer (different time)

But as last_run doesn't get updated during this sequence the timer ends up firing immediately. My single solution was to move last_run = millis from Thread::Thread to Thread::setInterval

Let me know if you'd like a PR but it's a pretty straight forward change.

[CAHEK7]

last_run updated in runned() function, did you call it somehow (directly or indirectly through run())? It's not reflected in the basic flow.

[morganrallen]

Indirectly yes, I'm using a ThreadController to run my Threads, so runned is being called. But the issue still persists that a span of time passes between runned being call and me calling setInterval and re-enabling putting last_run already in the past.

[morganrallen]

Another possible solution would be to provide a function that sets enabled = true and updates last_run to current time.

[CAHEK7]

But you still can call runned() directly. It actually updates last_run with millis, so you can use it. Kind of

t1.runned();
t1.setInterval(100500);

[ivanseidel]

I think the question here is: should setInterval reset the timer?

Setting last_run to millis() on construction is not a good idea indeed, as the behavior is not predictable because we cannot be sure Threads are starting when they are created (usually, I create all of them on global scope and then setup and run).

However, resetting it every time you setInterval(..) can lead to a problem. What if you are updating the interval of a function multiple times. For instance, let's say you are controlling the period of a flashing lamp. If setInterval resets the timer automatically, every time you setInterval it will make it delay the execution (and never execute).

That way, I think It would be best if you call runned directly (think of runned as reset)

[morganrallen]

@CAHEK7 As far as I can tell, runned cannot be called directly.

Thread.h:53:7: error: 'void Thread::runned()' is protected

Though I am not very good with C++ so I could be missing something.

[CAHEK7]

I forgot about it. Yes, it should be moved to public.

[morganrallen]

@ivanseidel I'm not sure I'm following the problem here. If I were to setInterval before onRun fires I would have an expectation that the timer resets. But if the interval is change after onRun it should fire like normal.

[ivanseidel]

Perhaps, keep it private but create a new public method called reset that calls runned.

@morganrallen If we reset it everytime setInterval is called, it will not permit people to make tasks run with variable interval time relative to when they were run, not reset. Every update to the interval, even if it's the same value, would trigger a reset on the timer. Usually, it's best to keep the logic as it is to keep compatibility, but provide documentation about how to use the new reset method.

[morganrallen]

@ivanseidel Yeah, I was going to suggest that also.

Still don't get the issue here but doesn't matter, reset will suit my needs fine.

Thanks, I'm getting some good use out of this lib. More suggestions to come.

[ivanseidel]

Great @morganrallen! I'm glad you liked it and is contributing.

Just to clarify the use case for not reseting it: Imagine you have a potentiometer connected to an arduino and a led as well. You create this code:

void loop(){
  ledBlinkThread.setInterval(analogRead(A1));
  threads.run();
}

That way, the ledBlinkThread would never be fired (unless analog reads 0), because it would never allow the timer to trigger since it's being reseted all the time..

Can you add a public virtual method reset that calls runned? that way we can make it work for you and even improve docs on that matter.

[morganrallen]

Ah yes, didn't think of setting based on external input. Gotcha!

I'll get a PR together tomorrow.