0

I'm running into snags where class instances that I'm storing within a management class are not retaining their data. I wrote out the core concepts that I'm wrestling with into an example here: https://onlinegdb.com/KFqNSz2r6

I have an Action class that acts as a container for some arbitrary action. It's primary function is to act as a state machine to facilitate non-blocking code form a microcontroller project:

class Action {
public:
    Action(int interval ,funcp_t callback ) : _interval(interval) , _callback(callback)
    {
    }
    
    bool trigger(int now) {
        if (now - _lastCheck > _interval) {
            _lastCheck = now;
            return true;
        }
        cout << "now: " << now;
        cout << ", last: " << _lastCheck;
        cout << ", interval: " << _interval;
        cout << ", diff: " << (now - _lastCheck);
        cout << endl;
        return false;
    }
    
    void fire() {
         cout << "action fired. last check: " << _lastCheck << endl;
        _callback();
    }

    
private:
  int _interval = 0;
  int _lastCheck = 0;
  funcp_t _callback;
};

I also have an ActionManager class that acts as a container and orchestrator class for the actions. The tick method is called to advance the state machines for all actions:

class ActionManager {
public:
   void addAction(Action action) {
       _actions.push_back(action);
   }
   
   void tick() {
       int now = millis();
       
       
       for(auto action : _actions) {
           if(action.trigger(now)) {
               action.fire();
           }
       }
   }
   
private:
  vector<Action> _actions;
};

In the rest of my setup code I'm setting up the action manager, adding actions, and then starting an infinite loop (to emulate an arduino loop) to advance the ticks of the ActionManager:

void sayHi() {
    cout << "oh hi" << endl;
}
void sayHey() {
    cout << "hey there" << endl;
}

ActionManager manager = ActionManager();
Action hiAction = Action(5000, sayHi);
Action heyAction = Action(1000, sayHey);

int main()
{
    cout<<"--> Start <--" << endl;
    manager.addAction(hiAction);
    manager.addAction(heyAction);
    
    
    while(true){
        manager.tick();
        cout << endl;
    }
    
    return 0;
}

The problem I'm running into is that the _lastCheck property of the Action instances is never updated. The console is reading:

--> Start <--

now: 65, last: 0, interval: 5000, diff: 65

now: 65, last: 0, interval: 1000, diff: 65

now: 86, last: 0, interval: 5000, diff: 86

now: 86, last: 0, interval: 1000, diff: 86

now: 98, last: 0, interval: 5000, diff: 98

now: 98, last: 0, interval: 1000, diff: 98

now: 107, last: 0, interval: 5000, diff: 107

now: 107, last: 0, interval: 1000, diff: 107

now: 118, last: 0, interval: 5000, diff: 118

now: 118, last: 0, interval: 1000, diff: 118

Note that the last check value being printed out never changes.

I'm pretty sure that this issue is happening because when I call addAction, my actions are being passed by value instead of reference so tried storing the actions as a vector of pointers:

class ActionManager {
public:
   void addAction(Action *action) { // accept a pointer to an action
       _actions.push_back(action);
   }
   
   void tick() {
       int now = millis();
       
       
       for(auto action : _actions) {
           if(action->trigger(now)) {
               action->fire();
           }
       }
   }
   
private:
  vector<Action *> _actions; // store a vector of actions
};

...

manager.addAction(&hiAction); // add by passing in the address of the action
manager.addAction(&heyAction);
    

But I still get nothing stored in the last check.

I feel like I'm missing something simple and I've just been working around the problem for too long to see it. Any ideas?

Chris Schmitz
  • 20,160
  • 30
  • 81
  • 137
  • 1
    `for(auto action : _actions)` Make it `for(auto& action : _actions)`. As written, `action` is a copy of elements from `_actions`. Then `_lastCheck` is updated in that temporary copy, and remains unchanged in the original. – Igor Tandetnik Sep 25 '21 at 02:57
  • 1
    The version with pointers should work. I notice that your online example uses intervals of `500000` and `100000` - that is, over 8 min and over a minute. Perhaps you just aren't waiting long enough. – Igor Tandetnik Sep 25 '21 at 03:04
  • *"Note that the last check value being printed out never changes."* -- why would it change? You showed output with the difference going up to 118. Which line would assign a new value to `_lastCheck` before the difference hits 1000 (in your example)? – JaMiT Sep 25 '21 at 03:24
  • @IgorTandetnik ah yeah with the pointer version I was just being impatient. It was getting late and I was frustrated. with a clear head this morning and looking over the replies I see that it worked :) Thanks for the help! – Chris Schmitz Sep 25 '21 at 12:22

1 Answers1

1

In your original version, the problem is that when you iterate over the actions, you make copies of the actions instead of referencing them.
In for (auto action: _actions) the action is copied. Use for (auto&& action: _actions) instead.

The version using pointers, doesn't have that problem and once the difference between now and _lastCheck becomes greater than _interval, _lastCheck should be updated.

Toni Georgiev
  • 131
  • 1
  • 4
  • Awesome. Between your and @IgorTandetnik's replies I updated the code and saw it work. thanks! One followup question, what does the `&` between the type and variable name mean? I know & can mean "take the address of" when in front of a variable, but it seems like it would be different in this case b/c a variable storing an address is just a pointer. – Chris Schmitz Sep 25 '21 at 12:25
  • 2
    `&` makes `action` a reference; the loop changes from `for (Action action = ...)` to `for (Action& action = ...)`, which of course makes all the difference. Consider: `int x = 1; int y = x; y = 2;` (`x` is still 1). `int& z = x; z = 2;` (`x` is now 2) – Igor Tandetnik Sep 25 '21 at 12:27
  • Ahhh, ok that makes sense. Yeah I typed out that question and immediately thought "I'm sitting at a computer, just look it up :P" and found a nice explanation of all of the uses of `&` in c++ https://stackoverflow.com/a/8857890/1934903 which mirrors your reply. Thanks again! – Chris Schmitz Sep 25 '21 at 12:29
  • You may also take into account that there is a difference between `auto&` and `auto&&`. The later one is also known as 'universal reference' and it can bind to and extend the lifetime of values to the scope of the reference, similar to const references. – Toni Georgiev Sep 26 '21 at 07:23