Bug? Multiple class instances - Static variables overwriting each other

I've run up against a very peculiar problem, replicated with test code. When upgrading my project from single to multiple LED channel control (via creating instances of the LED control class I created), I noticed both LED channels were selecting the -exact- same random colour to fade between, every time, without fail.

what I discovered, is that:

  • when instance 0 chose a new colour, instance 1 would be expected to immediately choose a new one too, but it acts as if it already picked a colour, and begins following the identical colour of the first instance. This should be impossible, because the members in each class have their own private static variables which stores the led state data.

I replicated the problem with test code. Sorry for the length! It's as short as I can get it, while still replicating the programming style used. My gut feeling is a bug with the 'new' operator.

P.S. Defensive disclaimer: I'm not the best programmer of frameworks/structures in the world, but it works ;p I know there's a million and one moral crusades with regards to how certain things should be done, but in this case I'm working under the assumption the designers wouldn't leave broken features available if they knew they didn't work as intended. this is a stable release after all, perish the thought.

Tested under V1.0.3, V1.0.5, same issues.

Win 7, Arduino Mega 2560 Rev3

#include <Arduino.h>

class randomInstance {

  public:
    uint8_t id;
    
    randomInstance         (uint8_t instanceNum);
    void memberWithStatics (bool showVars = false);
};

randomInstance::randomInstance(uint8_t instanceNum){
  this->id = instanceNum;    
  this->memberWithStatics(); //Initialize the members with a first-run of colour selection.
};

void randomInstance::memberWithStatics(bool showVars){
  static uint32_t lastExecute = millis(), ticks=0;
  static uint32_t colour      = random(0x0, 0xFFFFFF);
  static uint32_t lastColour  = 0;
  
  if(showVars){
    Serial.print(F("memberWithStatics: lastColour ")); Serial.print(lastColour, HEX);
    Serial.print(F(" colour: "));      Serial.print(colour, HEX);
    Serial.print(F(" ticks: "));       Serial.print(ticks);
    Serial.print(F(" lastExecute: ")); Serial.print(lastExecute);
    Serial.print(F(" instanceID "));   Serial.println(this->id);
    return;
  };
  
  //If user didin't want variables dumped, assign new variables to everything
  lastColour  = colour;
  colour      = random(0x0, 0xFFFFFF);
  lastExecute = millis();
  ticks++;
};




randomInstance *instances[2]; //Pointer array for our class instances.

void setup(){
  Serial.begin(115200);
  
  //Allocate & initialize our instances
  instances[0] = new randomInstance(0);
  instances[1] = new randomInstance(1);
  
};

void loop(){
  Serial.println(F("Main: Dumping current data: "));
  instances[0]->memberWithStatics(true);
  instances[1]->memberWithStatics(true);  
  
  //Make instance 0 pick a new colour
  instances[0]->memberWithStatics();

  Serial.println(F("\nMain: Instance 0 called, new dump: "));
  instances[0]->memberWithStatics(true);
  instances[1]->memberWithStatics(true);  

  //Make instance 1 pick a new colour
  instances[1]->memberWithStatics();

  Serial.println(F("\nMain: Instance 1 called, new dump: "));
  instances[0]->memberWithStatics(true);
  instances[1]->memberWithStatics(true);  
  
  delay(10000);
};

Output:

Main: Dumping current data:
memberWithStatics: lastColour 6DAE4 colour: 58EDE ticks: 2 lastExecute: 1 instanceID 0
memberWithStatics: lastColour 6DAE4 colour: 58EDE ticks: 2 lastExecute: 1 instanceID 1

Main: Instance 0 called, new dump:
memberWithStatics: lastColour 58EDE colour: E50A54 ticks: 3 lastExecute: 12 instanceID 0
memberWithStatics: lastColour 58EDE colour: E50A54 ticks: 3 lastExecute: 12 instanceID 1

Main: Instance 1 called, new dump:
memberWithStatics: lastColour E50A54 colour: F32F99 ticks: 4 lastExecute: 30 instanceID 0
memberWithStatics: lastColour E50A54 colour: F32F99 ticks: 4 lastExecute: 30 instanceID 1

In a class, static means that all instances of the class share the variable. That hardly seems like what you want. Instead of static, the local variables in the method should be class fields:

class randomInstance
{
  public:
    uint8_t id;
    
    randomInstance         (uint8_t instanceNum);
    void memberWithStatics (bool showVars = false);

  private:
    int32_t lastExecute, ticks;
    int32_t colour;
    int32_t lastColour;
};

Provide the initial values in the constructor.

@PaulS - rats you got there first.

@Drae - in something as old as c/c++ You a (new user) are most unlikely to find a bug. All ways assume it your fault!

Mark

Really? o_o Ack, that's frustrating. It seems rather unclean to have it's accepted functionality be changed depending where it's used.

Will need a complete rewrite of the class, top to bottom if wanting to use multiple instances. I'm sad :frowning:

Each LED has about 5 operating modes, and each mode usually has about 10-15 static variables/defaults to track fade amounts, timing, synch, brightness calculations and stuff, depending on if it's a fade, pulse, jump, or synched to a song bpm set. Also so each mode keeps it's last state/setting data when a new mode is active. Dumping all that in the class scope will feel horrible. Might try a shared struct array between modes instead, each mode accessing the array index corresponding to the instance ID of the class.

Ick.

@ holmes4: You're right, though I was inclined to direct some animosity towards the new operator as implemented in the arduino. Little chance of finding a bug in C/C++ it's self these days, barring compiler quirks ;p

Drae:
Will need a complete rewrite of the class, top to bottom if wanting to use multiple instances. I'm sad :frowning:

Each LED has about 5 operating modes, and each mode usually has about 10-15 static variables/defaults to track fade amounts, timing, synch, brightness calculations and stuff, depending on if it's a fade, pulse, jump, or synched to a song bpm set. Also so each mode keeps it's last state/setting data when a new mode is active. Dumping all that in the class scope will feel horrible.

Well you can't have your cake and eat it too. You are complaining that the variables are shared, and then saying you want them to be shared.

Drae:
@ holmes4: You're right, though I was inclined to direct some animosity towards the new operator as implemented in the arduino.

In what way is that suspect?

If I thought that, then that would indeed be silly, which of course I don't ;p

I also didin't mention anything about wanting the variables to be shared, I don't. I want only the relevent mode's member function to have access to it, no other function, and no other instance. Every instance is responsible for it's own LED & various modes, and nothing else.

The normal meaning of static operates like a private variable store per function. It's static, persistent, and only accessible to that function. That same functionality is what I hoped for in the classes too. If I wanted it shared, I'd have specifically linked pointers to data stores between them, but it seems the language seems to do that for you, with no option of choosing the interpretation.

Anyway, seems I need to bite the bullet and rewrite. Thanks anyway. :slight_smile:

The normal meaning of static operates like a private variable store per function. It's static, persistent, and only accessible to that function. That same functionality is what I hoped for in the classes too. If I wanted it shared, I'd have specifically linked pointers to data stores between them, but it seems the language seems to do that for you, with no option of choosing the interpretation.

Ah, there is only one copy of the code of a class there are many copies of the data! Static means one copy only not one per instance.

Mark

holmes4:

The normal meaning of static operates like a private variable store per function. It's static, persistent, and only accessible to that function. That same functionality is what I hoped for in the classes too. If I wanted it shared, I'd have specifically linked pointers to data stores between them, but it seems the language seems to do that for you, with no option of choosing the interpretation.

Ah, there is only one copy of the code of a class there are many copies of the data! Static means one copy only not one per instance.

Mark

Seems I got to become intimately familliar with the intricacies of this ;p

I solved it by creating a data subclass to hold the variables for each mode, which only needed minimal code changes to each mode. existingVar to f.existingVar. It's clunky, but the lights are all happily following their own paths now, as intended by it's creator.

Drae:
I also didin't mention anything about wanting the variables to be shared, I don't. I want only the relevent mode's member function to have access to it, no other function, and no other instance.

OK, but by making the class variables static clearly it is referring to the class and not some function within the class. If you want to keep class variables private use the "private" keyword. That restricts their use to outside the class.

If you want to encapsulate some variables with some function you would have to make a sub-class. For example, if you use String (not that I recommend it) then each String variable would have its own private class variables.

But if you want to share variables between functions then they would be class variables.

Sorry to burst all of your misconceptions of static member variables in C++ but...
I got it to work just fine :wink: :wink: :wink: :wink:

Simply initialize the variable outside the class or at the top of the .cpp file.

Note to self: It is sometimes worth while to exclude "in arduino" from your searches on google instead use "in C++"

// Example of a properly used static variable used inside class, BUT INITIALIZED OUTSIDE OF CLASS! 
#define PrintArg(v){      \
  Serial.print(#v);       \
  Serial.print(" = ");    \
  Serial.println(v);      \
}

class Example{    
  public:
   int16_t ID;             // Member Variable
   static int16_t NextID;  
   
   Example(){              // Class contstructor
     ID = NextID++;       // reads static variable stores in id unique to each individual object
   }
   
   int16_t getNextID(){
      return(NextID);
   }
};

int16_t Example::NextID = 0;

Example Ex0, Ex1, Ex2;

void setup(){
   Serial.begin(115200);      // Start serial at 115200 baud rate
   while(!Serial);            // Wait for Serial Terminal to startup
   delay(300);                // Initial delay seems to work

   PrintArg(Ex0.ID);  // Prints:  "Ex0.ID = 0"  
   PrintArg(Ex1.ID);  //          "Ex1.ID = 1"
   PrintArg(Ex2.ID);  //          "Ex2.ID = 2"
   
   PrintArg(Ex0.getNextID());      // Prints: "3"
   PrintArg(Ex1.getNextID());      //         "3"
   PrintArg(Ex2.getNextID());      //         "3"
}
void loop(){ }