Arduino class hierarchy, Strings and memory leak

Question

Good afternoon , i'm starting a new Arduino Project 1.6.10 IDE ver. but i'm encountering some problems of memory leak when i use a class based structure.

I post my code first and then i'll point the place when the memory leak seems to appear.

mainSketchFile.

#include <Ethernet.h>
#include <MemoryFree.h>
#include "Constants.h"
#include "State.h"



StateFactory CurrentStateFactory;

void setup() {

  pinMode(BUZZER,OUTPUT);
  Serial.begin(9600);
  Serial.println("START");
  delay(1000);

}

void loop() {

  Serial.print(F("Free RAM = ")); 
  Serial.println(freeMemory(), DEC);  // print how much RAM is available.
  CurrentStateFactory.changeStatus(1);
  Serial.println(CurrentStateFactory.getCurrentState()->getNumber());
  CurrentStateFactory.changeStatus(2);
  Serial.println(CurrentStateFactory.getCurrentState()->getNumber());
}

The problem seems to be in State.h i marked the point in the comments

#ifndef State_h
#define State_h

/////////////////// STATE/////////////////////////

class MachineState{
  public: 
    virtual int getNumber();
  protected:

};

/////////////////////ACTIVE FULL/////////////////////////////////
class ActiveFull : public MachineState
{
  public:
      ActiveFull();
      virtual int getNumber();
  private:
      String statusName; //<----- PROBLRM SEEMS TO BE HERE WHEN COMMENTED NO MEMORY LEAK APPEN
      int number;
};

ActiveFull::ActiveFull(){
  this->number=1;
};


int ActiveFull::getNumber(){
  return this->number;
}

////////////////////////////// ACTIVE EMPTY ////////////////////
class ActiveEmpty : public MachineState
{
  public:
      ActiveEmpty();
      virtual int getNumber();
  protected:
      String statusName;//<----- PROBLRM SEEMS TO BE HERE WHEN COMMENTED NO MEMORY LEAK APPEN
      int number;
};

ActiveEmpty::ActiveEmpty(){
   this->number=2;
};

int ActiveEmpty::getNumber(){
  return this->number;
}


//////////////////FACTORY/////////////////////////////


class StateFactory{
    private:
      MachineState *currentState;
    public: 
      StateFactory();
      void *changeStatus(int choice); // factory
      MachineState *getCurrentState();
  };

StateFactory::StateFactory(){
  MachineState *var1=new ActiveFull();
  this->currentState=var1; 
}

MachineState *StateFactory::getCurrentState(){
  return this->currentState; 
 }


void *StateFactory::changeStatus(int choice)
{
 delete  this->currentState;  // to prevent memory leak
  if (choice == 1){
      MachineState *var1=new ActiveFull();
      this->currentState=var1;
    }
  else if (choice == 2){
      MachineState *var1=new ActiveEmpty;
      this->currentState=var1;
    }
  else{
      MachineState *var1=new ActiveEmpty;
      this->currentState=var1;
    }
}

#endif

i use the library to track the memory usage and this is the output of the sketch:

No memory Leak (String statusName commented)

Free RAM = 7897
1
2
Free RAM = 7897
1
2
Free RAM = 7897
1
2
Free RAM = 7897
1
2
Free RAM = 7897
1
2
Free RAM = 7897
1
2
Free RAM = 7897
1
2
Free RAM = 7897
1
2
Free RAM = 7897
1
2

Memory leak when property String statusName is uncommented

Free RAM = 6567
1
2
Free RAM = 6559
1
2
Free RAM = 6551
1
2
Free RAM = 6543
1
2
Free RAM = 6535
1
2
Free RAM = 6527
1
2

Thanks in advise for your Time. Hope you can help me.

Answer 1

it seems like a destructor's problem,

I post a an implementation based on your code ..

#ifndef State_h
#define State_h


/* MachineState Class */
class MachineState{
  public:
  virtual void test() = 0;
     MachineState(){
        number = 0;
        statusName = "NULL";
     }
     virtual ~MachineState(){
      Serial.println("Destroy base");
     }
     void setNumber(int n){
      number =  n;
     }
     void setStatusName(String some){
      statusName = some;
     }
     String getStatusName(){
      return statusName;
     }
     int getNumber(){
      return number;
     }
     virtual void print()const{
      Serial.println("Class MS");
     }
  protected:
      String statusName;
      int number;

};


/* ActiveFull Class */
class ActiveFull : public MachineState{
  public:
      ActiveFull(){
        x = "Class AF";
        setNumber(1);
      }
      void print()const{
        Serial.println("Class AF"); 
      }
      void test(){}
      ~ActiveFull(){
       Serial.println("Destroy AF");
      }
  private:
    String x;
};


/* ActiveEmpty Class */
class ActiveEmpty : public MachineState
{
  public:
      void print()const{
        Serial.println("Class EE"); 
      }
      ActiveEmpty(){
        x = "Class EE";
        setNumber(2);
      }
      void test(){}
      ~ActiveEmpty(){
          Serial.println("Destroy EE");
      }
  private:
    String x;
};

/* StateFactory Class */
class StateFactory{
    private:
      MachineState *currentState;
    public: 
      StateFactory();
      ~StateFactory(){
        Serial.println("Ho distrutto StateFactory");
      }
      void changeStatus(int choice); // factory
      MachineState *getCurrentState();
  };

StateFactory::StateFactory(){
  this->currentState=new ActiveFull(); 
}

MachineState *StateFactory::getCurrentState(){
  return this->currentState; 
 }


void StateFactory::changeStatus(int choice){
  if(this->currenState)
     delete  this->currentState;
  if (choice == 1){
      currentState = new ActiveFull();
    }
  else if (choice == 2){
      currentState = new ActiveEmpty();
    }
  else{
      currentState = new ActiveEmpty();
    }
}

#endif

This is my result with your main:

...

2
Class EE
Free RAM = 7751
Destroy EE
Destroy base
1
Class AF
Destroy AF
Destroy base
2
Class EE
Free RAM = 7751
Destroy EE
Destroy base
1
Class AF
Destroy AF
Destroy base

...

Answer 2

DISCLAIMER: I wanted to post this as a comment, not an answer, because in my opinion it does not solve the problem but just gives advices. Then I needed some code blocks, so I needed the answer features.

Well, your code IMHO needs some improvements (or maybe that's just because you reduced it, but anyway I'll post them for you)

1. Don't put the function implementation in the header file: use a cpp file to store the functions implementation and the header file to store the prototypes
2. The purpose of the inheritance is to reuse the majority of the code you already have in common. So it's meaningless to have a lot of different variables; much better to declare them differently.

For instance you can use it like this:

/* File State.h */

class MachineState{
    public: 
        int getNumber();
    protected:
        String statusName;
        int number;
};

/////////////////////ACTIVE FULL/////////////////////////////////
class ActiveFull : public MachineState
{
    public:
        ActiveFull();
};

////////////////////////////// ACTIVE EMPTY ////////////////////
class ActiveEmpty : public MachineState
{
    public:
        ActiveEmpty();
};

/* File State.cpp */

int MachineState::getNumber(){
    return this->number;
}

ActiveEmpty::ActiveEmpty(){
    this->number=1;
};

ActiveEmpty::ActiveEmpty(){
    this->number=2;
};

or, if you don't have to change the value of number (and so you don't need a real variable)

/* File State.h */

class MachineState{
    public: 
        virtual int getNumber() = 0;
    protected:
        String statusName;
};

/////////////////////ACTIVE FULL/////////////////////////////////
class ActiveFull : public MachineState
{
    public:
        virtual int getNumber();
};

////////////////////////////// ACTIVE EMPTY ////////////////////
class ActiveEmpty : public MachineState
{
    public:
        virtual int getNumber();
};

/* File State.cpp */

int ActiveEmpty::getNumber(){
    return 1;
};

int ActiveEmpty::getNumber(){
    return 2;
};

Then there is a small problem with the deallocation: if the new fails, you will get problems at the next delete. To solve this, you can do something like (and I've also shortened your code a bit)

void *StateFactory::changeStatus(int choice)
{
    if (this->currentState) // If it was correctly allocated
        delete this->currentState;  // to prevent memory leak
    switch (choice)
    {
    case 1:
        this->currentState = new ActiveFull();
        break;
    case 2: // case 2 can be removed since it is identical to default
        this->currentState = new ActiveEmpty();
        break;
    default:
        this->currentState = new ActiveEmpty();
        break;
    }
}

That said... Well, I'd modify the loop this way:

void printCurrentStateNumber()
{
    if (CurrentStateFactory.getCurrentState())
        Serial.println(CurrentStateFactory.getCurrentState()->getNumber());
    else
        Serial.println("No more memory");
}

void loop() {
    Serial.print(F("Free RAM = ")); 
    Serial.println(freeMemory(), DEC);  // print how much RAM is available.
    CurrentStateFactory.changeStatus(1);
    printCurrentStateNumber();
    CurrentStateFactory.changeStatus(2);
    printCurrentStateNumber();
}

This is to test if the state was successfully created.

As for your explicit problem, I don't know how the library function works. Before starting to understand why there is this leak, I'd try to figure out if this is really a leak. So start the modified program (with the test before delete and the print of the no more memory string) and let it run until the library tells you that it run out of memory. If it stabilizes or it reaches 0 and does not print that, it's a library problem. On the other hand if the program halts printing the string it's a leak.

One sidenote: it's not a good habit to let a small microcontroller perform allocations and deallocations too often, since it has a limited memory. Do the test, because if there is a real leak maybe it should be investigated more, but for your application I suggest you to think about permanently allocating two instances of the object and then just use them according to the value you passed it before - obviously if there are only a couple of derived classes), like this:

/* In the header file */
#define NUM_OF_STATES 2

class StateFactory{
private:
    MachineState states[NUM_OF_STATES];
public: 
    StateFactory();
    void changeStatus(int choice); // factory
    MachineState *getCurrentState();
private:
    int currentIdx;
};

/* In the source file */

StateFactory::StateFactory()
{
    states[0] = new ActiveFull();
    states[1] = new ActiveEmpty();
    this->currentIdx = 0;
}

MachineState *StateFactory::getCurrentState(){
    return states[this->currentIdx];
}

void StateFactory::changeStatus(int choice)
{
    switch (choice)
    {
    case 1:
        this->currentIdx = 0;
        break;
    case 2: // case 2 can be removed since it is identical to default
        this->currentIdx = 1;
        break;
    default:
        this->currentIdx = 1;
        break;
    }
}

FINAL NOTE: Revising the answer I found that your changeStatus function returns a void * instead of a void. You should definitely fix that, and MAYBE things will get fixed (in fact you are returning a pointer instead of nothing). But I'm not sure about that.