Resetting a coroutine

[FiftyToo]

A couple questions:

Is it possible to reset a coroutine so that it starts from the beginning? Is it possible to suspend all coroutines that are currently running, and then resume only the coroutines that were paused?

Very nice work with this library!

Jeff

[bxparks]

Back from vacation. Thanks for the positive feedback.

First question: No, because a coroutine is created statically, not dynamically. If you look at the finite state diagram in the README.md, there is no outgoing arrow from the Ending/Terminated state back to the Running state. But if you expand on you are trying to do, I may be able to suggest alternatives.

Second question: Confused about the question. If you suspend all coroutines, then aren't all of them paused?

[FiftyToo]

Hi Brian,

For my project, I have several coroutines controlling independent groups of LEDs. Some coroutines control the same group of LEDs using a different pattern. I do not want those coroutines running at the same time. I want to be able to stop one coroutine (suspend) and start another (resume). All of my coroutines are using COROUTINE_LOOP() and I am using the CoroutineScheduler.

1). When I resume the coroutine, I would like it to start at the beginning of the animation sequence, not where it left off when it was suspended. Since all of my coroutines are loops, none of them actually end

I added this to the public interface of the Coroutine class. have not yet had an opportunity to test:

    /** Reset the coroutine */
    void reset() { mJumpPoint = nullptr; } 

The idea is that when runCoroutine is called again, it will not jump to where it last yielded.

2). I want to be able to pause all of the coroutines that are running, and then resume them at a later time. Since not all of my coroutines are running when I pause, I would like to only resume the coroutines that were previously running when I did pause.

To accomplish this, my plan is to just mimic what you are doing with suspend/resume, but with new pause/unpause methods and a new Paused status. Then I will be able to distinguish between my coroutines that are stopped (suspended) and those that i want keep running (paused). I hope that explains it better.

Thanks again

[bxparks]

1) I think that could work, but it feels dirty to me because this is jumping to the start of the coroutine without resetting any internal variables to their initial state. How many YIELD() points does your coroutine have? If not too many, can you write it like:

static bool isReset = false;

COROUTINE(animate) {
  COROUTINE_LOOP() {
    beginLabel:
    // stuff
    COROUTINE_YIELD();
    if (isReset) {
      isReset = false;
      // perform any other initialization
      goto beginLabel:
    }
    // stuff
  }
}

// suspend the coroutine
animate.suspend();

// resume the coroutine, and reset it
isReset = true;
animate.resume();

If that works, then you could also move the isReset flag into the Coroutine class using the Custom Coroutine or Manual Coroutine facilities.

2) Yeah, it looks like you need to keep additional information about the previous state of the Coroutine just before it was paused, so you need to add additional member variables into the Coroutine object. I suspect the easiest way to do this is to subclass your own Coroutine class using the Manual Coroutine facility. Something like the following could work:

class ManualCoroutine : public Coroutine {
  public:
    void pause() {
      mWasRunning = !isSuspended();
      suspend(); // calls Coroutine::suspend()
    }

  void unpause() {
    if (mWasRunning) {
      resume();
    }
  }

  private:
    bool mWasRunning;
};

Then instead of calling ManualCoroutine::suspend(), you would call ManualCoroutine::pause().

[FiftyToo]

I think i will go the route of creating my own class like in your 2nd example. I am in the process of rewriting the coroutines to no longer use the COROUTINE() macro, so this will be a good fit.

As for my idea with reset(), what other internal variables would need to be set to their initial state? In my case, I will be suspending all coroutines, then reset and resume one or a few coroutines. Seems like that should take care of everything.

[bxparks]

With regards to reset(), I was looking at it from a more general perspective. It could work in your case if you do not have too many internal states. But it may do unexpected things in the general case. When a Coroutine is created, all static variables within the runRoutine() method, and the mJumpPoint and mStatus are in a known state. A general purpose reset() function would need to reset those variables to their initial state to be semantically correct. If Coroutines could be dynamically created and destroyed, this problem would disappear because we could let the old coroutine die and get reclaimed, then fire up a new one to "reset" from scratch.

[FiftyToo]

I understand what you mean about the static variables. I ended up changing these private variables to protected so that i can manipulate them:

  protected:
    uint8_t mDelayType;
    uint16_t mDelayStart; // millis or micros
    uint16_t mDelayDuration; // millis or micros
    void* mJumpPoint = nullptr;
    Status mStatus = kStatusSuspended;

I then created my own implementation and added play/stop/pause/unpause/reset functions:

class AnimationCoroutine : public Coroutine {
public:
    void pause() {
        mWasRunning = !isSuspended();
        suspend(); 
    }

    void unpause() {
        if (mWasRunning) {
            resume();
        }
    }

    void play() {
        mWasRunning = true;
        resume();
    }
    void stop() {
        mWasRunning = false;
        suspend();
        reset();
    }
    virtual void reset() {
        mJumpPoint = nullptr;
    }  

private:
    bool mWasRunning;
};

Then in my animation class, i override reset() and reset the static variables. I believe I can only ever have a single instance of my class because the static variables are now at the class scope and not the runCoroutine() scope (c++ is definitely not specialty), but that is not a problem for my needs.

namespace animations {
    namespace greatest_retro {

        const PROGMEM uint16_t leds[] = {
            GREATEST_G, GREATEST_R, GREATEST_E, GREATEST_A, GREATEST_T, GREATEST_E2, GREATEST_S, GREATEST_T2,
            STAR_BONUS_10000, STAR_BONUS_8000, STAR_BONUS_6000, STAR_BONUS_4000
        };

        class instance : public AnimationCoroutine {
            public:
                static uint8_t i;
                instance() { }

                int runCoroutine() override {
                    COROUTINE_LOOP()
                    {
                        for (i = 0; i < sizeof(leds) / 2; i++) {
                            strip.setPixelColor(pgm_read_byte_near(leds + i), strip.gamma32(COLOR_WHITE));
                            COROUTINE_DELAY(60);
                            strip.setPixelColor(pgm_read_byte_near(leds + i), strip.gamma32(COLOR_BLACK));
                        }
                        COROUTINE_DELAY(60);
                    }
                }
            protected:
                void reset() {
                    AnimationCoroutine::reset();
                    i = 0;
                }
        };
        uint8_t instance::i = 0;
    }
}

The last issue I am having is with suspend/resume. If a coroutine is being delayed when it is suspended, the elapsed time calculation is no longer accurate when the coroutine is resumed. It does not consider the amount of time it was suspended. This is causing me grief because

I think in order to change this, mDelayStart would need to be a uint32_t (doesn't seem like a uint16_t would work for long running processes anyways) and there would need to be another uint32_t variable to track the amount of time the coroutine has been suspended (resetting back to 0 after delay expires). This would provide enough information to change the calculation to consider the time suspended.

[bxparks]

(I'm doing a lot of traveling these days, thanks for your patience)

I probably don't understand your code completely, but I have a few comments:

1)) If you are creating your own subclass of Coroutine, you do not need to use a static int i variable. You can make this a private member variable of your Instance or AnimationCoroutine:

class instance : public AnimationCoroutine {
  public:
    int runCoroutine() override {
      COROUTINE_LOOP() {
        ...
      }
  private:
    uint8_t i;
};

So you can have as many instances of the instance class (maybe another name for this class would be less confusing?).

2)) With regards to the delay time, are you saying that you want the delay calculation to ignore the time that the coroutine spent in suspended state? That seems weird to me because the delay is meant to measure the time according to the wall clock. If the coroutine is suspended for an amount greater than the value of COROUTINE_DELAY(), then in the general case, it seems like the coroutine should start executing right away upon resume(), instead of waiting out the remainder of the delay. Whether the mDelayStart is uint16_t or uint32_t, we would still have an integer overflow problem. But I guess the difference is that uint32_t would overflow every 47 days instead of every 64 seconds. But see my proposed alternative (6) which makes this disagreement moot.

3)) nitpick: Recommend rewriting sizeof(leds) / 2 as sizeof(leds) / sizeof(uint16_t). I was confused several times.

4)) I'm quite hesitant to change the private variables into protected, because protected is visible to the end users. Once the internal implementation is exposed, then the variables become locked in as part of API of the library, and I can no longer make changes to the internal implementation.

5)) I'm also hesitant to add a reset() functionality, because I've never seen a Thread or Coroutine library that allows a "reset". Threads and Coroutines can be destroyed and recreated, but "reset" is not something I've seen.

6)) Alternate Solution: It seems like we can rewrite your code in a way that does not require invasive changes. If I understand your code (though it's likely that I've missed some behavior), would something like this work for you?

class Animation : public Coroutine {
  public:
    Animation() : index(0), state(kStateRun) {}

    int runCoroutine() override {
      COROUTINE_LOOP() {
        switch (state) {
          case kStateRun:
            strip.setPixelColor(... COLOR_WHITE));
            COROUTINE_DELAY(60);
            strip.setPixelColor(... COLOR_BLACK));
            if (state != kStateRun) break;

            index++;
            if (index >= kNumLed) { index = 0; }
            COROUTINE_YIELD();
            break;

          case kStatePause:
          case kStateStop:
            COROUTINE_YIELD();
            break;
        }
      }
    }

  void pause() { state = kStatePause; }

  void unpause() {
    if (state == kStatePause) state = kStateRun;
  }

  void stop() { state = kStateStop; }

  void resume() {
    if (state == kStateStop) index = 0;
    state = kStateRun;
  }

  private:
    const uint8_t kStateRun = 0;
    const uint8_t kStatePause = 1;
    const uint8_t kStateStop = 2;

    uint8_t index;
    uint8_t state;
};

The trick here is that you are no longer using the built-in suspend() and resume() functions. This Animation coroutine is always active, but uses its own state variable to manage its own internal state. It just calls COROUTINE_YIELD() if it's in the kStatePause or kStateStop states.

This solution is slightly less efficient compared to suspend() because the runCoroutine() will always be called, but I suspect that this is not a big deal in your case. The other nice thing about this implementation is that you no longer need to worry about mDelayStart overflow, since the coroutine is always active.

[FiftyToo]

1) I use separate namespaces for each animation, so giving each class a unique name makes for some redundant code (ie. animation1::animation1.start() ) Id rather use animation1::instance.start().

2) Comparing a coroutine to something more tangible like a record player, your implementation of suspend is similar to turning down the volume so that you can no longer hear the record while it continues to spin. I am more interested in stopping the turntable so that music stops playing. If you have multiple coroutines that are meant to be ran in tandem, then suspending them is not possible since the delays in each is no longer predictable. It just doesn't make sense to me to keep the timer running for the delay when everything else is suspended.

3) That's my lack of c++ skills shining brightly.

4) I get what you are saying, but I think its generally accepted that protected members (especially variables) should be handled with care.

5) Is it possible to destroy and recreate one of your coroutines? I was under the impression that that was not possible.

6) I am not sure how that would help in regards to the problem with COROUTINE_DELAY(). I am not really concerned about the 60ms delays that expire even with the coroutine suspended. I do not think it is correct behavior, but the problems it causes with timing are greatly reduced, especially when I now have the ability to reset the coroutines and have them start from the beginning.

My sketch has an additional coroutines that I am using to orchestrate all of these animation coroutines that I am writing. It is this coroutine that is causing me grief.

// orchestrator
COROUTINE(oc){
    COROUTINE_LOOP() {
        stopAllAnimations();
        retroMode(); // this just calls play() on several coroutines
        COROUTINE_DELAY_SECONDS(30);

        stopAllAnimations();
        all_flash.play();
        COROUTINE_DELAY_SECONDS(3);

        stopAllAnimations();
        all_color_rainbow.play();
        COROUTINE_DELAY_SECONDS(20);

        stopAllAnimations();
        all_flash.play();
        COROUTINE_DELAY_SECONDS(3);
    }
}

I have not yet migrated this to my own implementation of Coroutine, but I already know that it is not going to work as expected if I pause this orchestrator coroutine duing any one of the delays. Assuming i pause everything for a couple minutes during that first 30 second delay, when I unpause, it will immediately jump to the next line and switch the the all_flash animation.

The only other time I have used the concept of coroutines was with Unity, and that framework addressed the problem by controlling the rate at which time passes (if that makes sense). For example, if I am writing a coroutine to move a sprite across the screen, the distance I move would be a function of time. If i want that distance to be 0 (ie paused), then I would set timeScale to 0, which will have the effect of d = rt always being equal to 0 (t is multiplied by timeScale by the engine). Perhaps this could be a more elegant way to have the best of both worlds?

[bxparks]

Hi,

That is correct, AceRoutine does not support dynamic creation and destruction of coroutines. I have considered adding that for microcontrollers with enough heap memory.

If I understand you correctly, it sounds like your 60ms or 30s is measured according to how long the coroutine was "active", instead of being measure according to the "wall clock"? AceRoutine does not support the "active clock" concept. The library simply uses the built-in millis() and micros() methods provided by the Arduino framework, and it's not clear to me how to create a per-coroutine clock that ticks only when the coroutine is "active". (It would definitely take additional varables in the Coroutine class, and additional logic during context switching, and I'm not sure if it would actually work.)

The timeScale idea is very interesting. Though I'm not sure that it would help in this case. Seems like the problem is distinguishing between a coroutine's "active" time versus "suspended" time.

What is the accuracy that you need for your coroutine "active clock"? Can it have roughly a one-second accuracy? If so, instead of using COROUTINE_DELAY_SECONDS(), maybe you can use an explicit loop, and count the "active time" manually?

static uint16_t activeClock;
for (activeClock = 0; activeClock < 30; activeClock++) {
  COROUTINE_DELAY(1000);
}

You can use a macro to avoid repetition:

#define DELAY_ACTIVE_SECONDS(delay) \
  for (activeClock=0; activeClock < delay; activeClock++) { \
  COROUTINE_DELAY(1000); \
}

If you create your own Coroutine subclass, you would create activeClock as a private member variable of your subclass.

[bxparks]

Closing due to inactivity for almost a year. Hope you were able to accomplish what you wanted to do.