Using a class in a class

Hello guys,

I started to learn about the use of classes since my project code is getting a bit messy.
In my project I use 4 PID controllers (PI actually) for tempetature controlling etc. I use a PID library to do the PID calculations and some code around it to make it suitable for relays. Beside that I also have some digital outputs which are on a IO expander (MCP23008) so I use a library for that too.

My thoughts are that it would be nice if I can pack all this in one class (library) to clean up my project. Beside that is would be a good training.

My library seems to be working but I not sure if I do it correctly, so it would be nice if some people can
advise me.

For example, is the way I use the classes inside my class correct? Is er a way to do it better?
Do I need a destructor, or is not really necessary in an Arduino?

Any advise or comment would be helpfull.

Header file:

//////////////
//Header file
//////////////

#include <PI_v1.h>
#include <Adafruit_MCP23008.h>

class Actuator {
#define NORMAL_PIN 0
#define MCP_PIN 1

  public:
    //Actuator(int pin, double *pv,double *output,double *sp,double p,double i,int dir);//constructor
    Actuator(int, double*, double* , double, double, int, bool); //constructor
    ~Actuator(); //destructor
    void begin(int);
    void SetTunings(double, double, int);
    void SetOutputLimits(int, int);
    void SetMinSwitchTime(int);
    void on();
    void off();
    bool getStatus();
    void manualControl(double);
    void autoControl(double);

  private:
    int _pin;
    bool _pinType;
    bool _status;
    PInoD *_PID;
    Adafruit_MCP23008 *_mcp;
    double *_pv, *_sp, _output;
    unsigned long _windowSize;
    unsigned long _windowStartTime;
    int _cycle; //Duty cycle in seconds
    int _minSwitchTime; //Minimum switch time in seconds
};

CPP file:

//////////////
//CPP file
//////////////

#if ARDUINO >= 100
#include "Arduino.h"
#else
#include "WProgram.h"
#endif
#include "ActuatorClass.h"

//Contructor
Actuator::Actuator(int pin, double *pv, double *sp, double p, double i, int dir, bool pinType) { 
  _pin = pin;
  _pinType = pinType;
  _pv = pv;
  _sp = sp;
  _PID = new PInoD(_pv, &_output, _sp, p, i, dir);

  //Is this the right way?
  if (pinType == MCP_PIN) {
    _mcp = new Adafruit_MCP23008;
  }
  else {
    pinMode(_pin, OUTPUT);
  }
}

//Destructor (necessary or always good to do?)
Actuator::~Actuator() {
  delete(_PID);
  if (_pinType == MCP_PIN) { //Not sure if this is the right way
    delete(_mcp);
  }
}

void Actuator::begin(int pin) {
  _mcp->begin(pin); // use default address 7
  _mcp->pinMode(_pin, OUTPUT);
  _mcp->digitalWrite(_pin, LOW);
}

void Actuator::SetTunings(double p, double i, int cycle) {
  _PID->SetTunings(p, i);
  _cycle = cycle * 1000; //Convert to ms
}

void Actuator::SetOutputLimits(int minVal, int maxVal) {
  _PID->SetOutputLimits(minVal, maxVal);
}

//Set the minimum 
void Actuator::SetMinSwitchTime(int minSwitchTime) {
  _minSwitchTime = minSwitchTime * 1000;
}

//Set the pin to high
void Actuator::on() {
  if (_pinType == NORMAL_PIN) {
    digitalWrite(_pin, HIGH);
  }
  else {
    _mcp->digitalWrite(_pin, HIGH);
  }
  _status = HIGH;
}

//Set the pin to low
void Actuator::off() {
  if (_pinType == NORMAL_PIN) {
    digitalWrite(_pin, LOW);
  }
  else {
    _mcp->digitalWrite(_pin, LOW);
  }
  _status = LOW;
}

//Get status of output pin
bool Actuator::getStatus() {
  return _status;
}

//Manual control of the output value
void Actuator::manualControl(double output) {
  _PID->SetMode(MANUAL);
  _output = output;
}

//Automatic control of the output value
void Actuator::autoControl(double sp) {

  _PID->SetMode(AUTOMATIC);
  *_sp = sp;

  //Calculate pid
  _PID->Compute();

  //Convert output to time
  _windowSize = _output * _cycle / 100;

  /************************************************
   Limit minimum switch times when controlling
  ************************************************/  
  if (_output > 0) {
    //Minimum on time
    if (_windowSize < _minSwitchTime) {
      _windowSize = _minSwitchTime;
    }
    //Minimum off time
    if ((_cycle - _windowSize) < _minSwitchTime) {
      _windowSize = _cycle;
    }
  }
  /************************************************
    turn the output pin on/off based on pid output
  ************************************************/
  if (millis() - _windowStartTime > _cycle)
  { //time to shift the Relay Window
    _windowStartTime += _cycle;
  }

  if (_windowSize < millis() - _windowStartTime) {
    off();
  }
  else {
    on();
  }

  Serial.println(_output);
}

Seems like what you're doing is basically correct. Couple things to consider:

  • First, you're not "Using a class in a class" as the post's title says. That would imply that you're defining nested classes. You're actually just defining a class that contains members that are (pointers to) objects of other classes.

  • You should check for and properly handle the case when the "new" operator returns a null pointer.

  • You should initialize "Adafruit_MCP23008 *_mcp = nullptr;" That way if don't allocate an object in your constructor the pointer will be null.

  • I think it's vital and good form to define a proper constructor if your class does dynamic allocation. Will help prevent a memory leak. In your case just do:

Actuator::~Actuator() {
   delete(_PID);
   delete(_mcp);
}

Since _mcp will default to null, it's safe to "delete" the nullptr;

  • You should check for the nullptr in 'begin()' in case _mcp has not been assigned. Don't want to call a method on the nullptr!!!

  • An alternative method would have been to statically define a PInoD object as a member of your new class rather than doing it dynamically.

  • Since your new class always contains a PInoD object, maybe you could use C++ inheritance and have your new class derive from PInoD.

gfvalvo:

  • I think it's vital and good form to define a proper constructor if your class does dynamic allocation. Will help prevent a memory leak. In your case just do:
Actuator::~Actuator() {

delete(_PID);
  delete(_mcp);
}

You are talking about a proper destructor obviously.

Whandall:
You are talking about a proper destructor obviously.

Correct. Thank you.

Thanks for the advise!