arkhipenko / TaskScheduler

Cooperative multitasking for Arduino, ESPx, STM32, nRF and other microcontrollers
http://playground.arduino.cc/Code/TaskScheduler
BSD 3-Clause "New" or "Revised" License
1.22k stars 224 forks source link

[help] Implement Async Delays #83

Closed ricardojlrufino closed 4 years ago

ricardojlrufino commented 4 years ago

I am trying to implement a LED effect without delays, but the code is is becoming unnecessarily complex. I would like to know if you can implement this something like (pseudo-code):

for(int i=0; i<NUMPIXELS; i++) { // For each pixel...
    pixels.setPixelColor(i, pixels.Color(0, 150, 0));
    pixels.show();   
    delay(50); // Pause before next pass through loop
  }

// reset after 3 secons 
delay(3000);
resetLeds(); 

something like (pseudo-code)

// Declare the tasks
class LedTask : public Task {
    public:
        virtual void run(){
            for(int i=0; i<NUMPIXELS; i++) { // For each pixel...
               // ...
               scheduler.delay(50);
            }

            // Use Scheduler.delay to stop a execution
            scheduler.delay(3000);
            // ,,,,
        };
};

There a easy way without creating multiple tasks ?? and with clean code?

EDIT: I have added several implementation templates below

ricardojlrufino commented 4 years ago

This my implementation:

HUGEEEE

class UpdateLedsTask : public Task {

    const uint32_t DEFAULT_COLOR = strip.Color(255, 255, 0);
    const uint32_t INTERVAL = 100;

    public:
        UpdateLedsTask(Scheduler* aScheduler, int aPin /*, Calculator* aC*/) : Task(INTERVAL, TASK_FOREVER, aScheduler, false){
          // aScheduler->addTask(resetLedsTaks);
        };

        bool Callback(){

            // Inicializa todos na cor padrão sem nenhum efeito.
            if(delayResetNext){
               resetLeds();
               disable();
               return true;
            }

            uint8_t i = getRunCounter();            
            if(i == LED_COUNT){
                Serial.println("Stop leds...");
                disable();
            }

            strip.setPixelColor(i, color);         //  Set pixel's color (in RAM)
            strip.show();                          //  Update strip to match

            return true;
        };

        bool OnEnable() {
            Serial.print("OnEnable:");
            if(delayResetNext){
              Serial.println("delayReset");  
            }else{
              Serial.println("normal");  

            }
            return true;
        }

        void OnDisable(){

            // Change time, color and re-enable
            if(delayReset){
                Serial.println("delayReset, wait for 3sex");
//                setInterval(3000); // reset after 1 sec
                delayResetNext = true;
                delayReset = false;
                restartDelayed(3000);
            }  

        }

        void setColor(uint32_t _color, bool _delayReset){
          color = _color;
          delayReset = _delayReset;
          delayResetNext = false;
          enable();
        }

        /* Estado inicial dos leds.*/
        void resetLeds(){
          for(int i=0; i<LED_COUNT; i++) { // For each pixel...
            strip.setPixelColor(i, DEFAULT_COLOR);
           }
          strip.show();

          disable();
          delayReset = false;
          setInterval(INTERVAL);
        }

    private: 

        uint32_t color;
        bool delayReset;
        bool delayResetNext;
};
ricardojlrufino commented 4 years ago

Attempt 2: More clean, but many classes and to much code

class ResetLedsTask : public Task {

    const uint32_t DEFAULT_COLOR = strip.Color(255, 255, 0);

    public:
        ResetLedsTask(Scheduler* aScheduler) : Task(3000, TASK_FOREVER, aScheduler, false){
        };

        bool Callback(){
          Serial.println("Stop leds...");
          for(int i=0; i<LED_COUNT; i++) { // For each pixel...
            strip.setPixelColor(i, DEFAULT_COLOR);
           }
           strip.show();
           disable();

           return true;
        };
};

class UpdateLedsTask : public Task {

    const uint32_t INTERVAL = 100;

    public:
        UpdateLedsTask(Scheduler* aScheduler, int aPin, ResetLedsTask* _resetTask) : Task(INTERVAL, TASK_FOREVER, aScheduler, false){
          // aScheduler->addTask(resetLedsTaks);
          resetTask = _resetTask;
        };

        bool Callback(){

            uint8_t i = getRunCounter();            
            if(i == LED_COUNT){
                disable();
            }

            strip.setPixelColor(i, color);         //  Set pixel's color (in RAM)
            strip.show();                          //  Update strip to match

            return true;
        };

        void OnDisable(){
          resetTask->enableDelayed(); 

        }

        void setColor(uint32_t _color, bool _delayReset){
          color = _color;
          enable();
        }

    private: 
        uint32_t color;
        ResetLedsTask* resetTask;

};
ricardojlrufino commented 4 years ago

The working version

class UpdateLedsTask : public Task {

    const uint32_t INTERVAL = 10000;
    const uint32_t DEFAULT_COLOR = strip.Color(255, 255, 0);

    public:
        UpdateLedsTask(Scheduler* _aScheduler, int aPin) : Task(INTERVAL, TASK_FOREVER, _aScheduler, false){
          aScheduler = _aScheduler;
        };

        bool Callback(){

            show(color, 100);

            aScheduler->delay(3000);

            show(DEFAULT_COLOR, 1);

            return true;
        };

        void setColor(uint32_t _color, bool _delayReset){
          color = _color;
          enable();
        }

        void show(uint32_t _color, int adelay){
          for(int i=0; i<LED_COUNT; i++) { // For each pixel...
            strip.setPixelColor(i, _color);
            aScheduler->delay(adelay);
            strip.show(); 
           } 
        }

    private: 
        uint32_t color;
        Scheduler* aScheduler;

};

Need add this code to lib:

void Scheduler::delay(int interval) {
    unsigned long currentMillis = millis();
    currentTask().disable();
    while(millis() - currentMillis < interval){
       execute(); // continue execution.
    } 
    // currentTask().enable();
}

But since I don't know the library in depth, I don't know where it could be a problem. It's giving an error when I try to re-enable the task, arduino resets.

arkhipenko commented 4 years ago

If I understand what you are trying to do, this could be accomplished with two tasks in a relatively straightforward way:

Task tRinseAndRepeat(3000, TASK_FOREVER, &rrCB, &ts, true); 
Task tLed(50, LED_COUNT, &ledCB, &ts);

void setup() {

} 

void loop() {
ts.execute();
}

void ledCB() {
    strip.setPixelColor(tLed.getRunCounter()-1, COLOR);
    strip.show(); 
}

void rrCB() {

    resetLeds();
    tLed.restart();
}

No changes to the library are necessary.

NB: Based on your examples I do not think you fully understand how this library works (no offense).

ricardojlrufino commented 4 years ago

I am looking for a way to use, through object orientation, and without using global variables.

For this simple task, it seems to be better this way, but for other more complex situations I don't like having a lot of callback mixed with the main code.

What would be the best design for this situation?

PS:' im a java guy

arkhipenko commented 4 years ago

Then you derive two new classes from the class Task:

  1. RinseAndRepeat - you might want to include an additional parameter to the constructor to pass on the reset interval (so 3000 millis is not hardcoded).

Overload its Callback() method with something like this:

bool  RinseAndRepeat::Callback() {
    resetLeds();
    Led->restart(); // if Led is a pointer to the Led control class 
        return true;
}
  1. Led - you might want to include a number of LEDs as a constructor parameter (or keep it as a constant) Overload its Callback() method with something like this:
    bool  Led::Callback() {
    strip.setPixelColor(self->getRunCounter()-1, COLOR);
    strip.show(); 
    return false;
    }

    and that's it. You basically will have to create 2 files per new class (.cpp and .h) and a sketch file, total of 5 Example 21 explains how to structure your code.

Please note that compilation parameters must be used in all files, not just .ino

arkhipenko commented 4 years ago

I am closing this issue as no changes to the library required.

ricardojlrufino commented 4 years ago

Didn't you find it interesting that the library has a delay function?

arkhipenko commented 4 years ago

If you are talking about Task::delay() then it has nothing to do with the Arduino delay(). Task::delay is a non-blocking method that just reschedules task invocation to a later time and returns immediately. Arduino delay() is blocking and prevents other tasks from being executed. The difference is fundamental.


From: Ricardo JL Rufino notifications@github.com Sent: Wednesday, January 15, 2020 10:15 AM To: arkhipenko/TaskScheduler TaskScheduler@noreply.github.com Cc: Anatoli Arkhipenko arkhipenko@hotmail.com; State change state_change@noreply.github.com Subject: Re: [arkhipenko/TaskScheduler] [help] Implement Async Delays (#83)

Didn't you find it interesting that the library has a delay function?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHubhttps://github.com/arkhipenko/TaskScheduler/issues/83?email_source=notifications&email_token=AACMMTIFI7T5YNHMQRNGBWDQ54SCDA5CNFSM4KFQMEYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJAU7NY#issuecomment-574705591, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AACMMTMLLUB3OQF6AIY5E6LQ54SCDANCNFSM4KFQMEYA.

ricardojlrufino commented 4 years ago

not current delay implementation, this proposal:

void Scheduler::delay(int interval) {
    unsigned long currentMillis = millis();
    currentTask().disable();
    while(millis() - currentMillis < interval){
       execute(); // continue execution.
    } 
    // currentTask().enable();
}

This will avoids create 2 tasks, using like:

            show(color, 100);

            aScheduler->delay(3000);

            show(DEFAULT_COLOR, 1);

is a more intuitive way to use delays, something close to arduino delay

ricardojlrufino commented 4 years ago

maybe create a method: Task :: delayNow (), might be better to avoid confusion

arkhipenko commented 4 years ago

No, this would not work. I am conceptually against disabling tasks for the purpose of delay until the next iteration. What you are trying to achieve could be done by other means (switching callback methods, using flags, etc.). Execute() belongs to the loop() and nowhere else. In your example, the task is already inside the execute() loop - you are suggesting calling execute again in another loop - I am not even sure what that is going to do. What if there are 20 tasks with delays? You cannot delay without switching context, and this is a cooperative library, so there are only one thread, one CPU and one process. So no delays.

You might want to have a look at FreeRTOS - I believe being a preemptive OS there you can delay as much as you like.

ricardojlrufino commented 4 years ago

I got your point

thanks for the feedback