Arduino - Enum variable value change by itself

Question

I created an Arduino mega sketch to control LEDs channel to simulate simple sunset and sunrize. In my class LEDChannel declaration, i have an public enum to represent the state of the LED Channel and i have a private instance (CurrentState) of State enum in the class to keep track of this LEDChannel instance.

Everything compile well and is uploaded to my Arduino Mega. The problem is when i debug my code and check the value of CurrentState variable i get a value outside of enum range. Sometime i get value 7579 (actual value in my last test). Another day i can get a value of 612. The value should only be 0 to 4. I verified my code and any place i change the value of this variable, i use the enum. By example :

CurrentState = ManualSunset;

CurrentState variable receive the value NoAction in the default constructor of LEDChannel Class.

The class LEDChannel is part of a bigger project to make an Aquarium controller with other class to represent RTC time module, Bluetooth module, Fan and temperature sensor.

I developed my project with Visual Studio Community 2015 with vMicro plugin for Arduino. If you need the complete project, i will find a way to make it available.

Here is the LEDChannel Declaration/definition and main Arduino program .ino. This is a light version of my code. I removed all unnecessary code. To reproduce the problem, just comment line 13 in main program nNbLEDChannel = 1;. You should then get value ManualSunrise (3) for variable CurrentState. Take not that nNbLEDChannel is only used in main .ino file.

#pragma once

#if defined(ARDUINO) && ARDUINO >= 100
    #include "Arduino.h"
#else
    #include "WProgram.h"
#endif

//#include "DataTypes.h"
typedef byte duration;
class AquariumSimulator;

class LEDChannel
{
public:
    typedef enum State
    {
        AutomaticSunset = 0,
        AutomaticSunrise = 1,
        ManualSunset = 2,
        ManualSunrise = 3,
        NoAction = 4
    };

private:
    byte MaxBrightness;
    unsigned long lastTick;
    byte CurrentBrightness;
    LEDChannel::State CurrentState;
    byte DigitalPin;
    duration ManualSunsetDuration;// In secondes
    duration ManualSunriseDuration;// In secondes
    AquariumSimulator* pAquariumSim;

public:
    void StartManualSunrise(duration SecDuration);
    void PerformSunrise();
    LEDChannel();
    LEDChannel(byte DigitalPIN, AquariumSimulator* pSim);
private:
    void PerformManualSunrise();        
    void SetCurrentBrightness(byte value);
    void IncreaseCurrentBrightness(byte value = 1);
};

Here is the LEDChannel.cpp

#include "LEDChannel.h"
/*
Constuctor
*/
LEDChannel::LEDChannel(byte DigitalPIN, AquariumSimulator* pSim)
{
    LEDChannel();
    // Initialize default values
    pAquariumSim = pSim;
    pinMode(DigitalPIN, OUTPUT);// Configure Digital pin that control LED Channel
}

LEDChannel::LEDChannel() 
{
    CurrentState = NoAction;

}

void LEDChannel::PerformSunrise()
{

    switch (CurrentState)
    {
    case AutomaticSunrise:
        //PerformAutomaticSunrise();
        break;

    case ManualSunrise:
        PerformManualSunrise();
        break;
    }
}

void LEDChannel::PerformManualSunrise()
{

    unsigned long currentTick = millis();
    if (currentTick >= (lastTick + ManualSunriseDuration / MaxBrightness))
    {

        // If current brightness is at max brigthness value, stop sunset
        if (CurrentBrightness == MaxBrightness)
        {
            CurrentState = NoAction;
            lastTick = 0;
        }
        else
        {
            IncreaseCurrentBrightness();
            lastTick = currentTick;
        }
    }
}

void LEDChannel::SetCurrentBrightness(byte value)
{
    if (value > 255)
        CurrentBrightness = 255;
    else
        CurrentBrightness = value;
    analogWrite(DigitalPin, CurrentBrightness);

}

void LEDChannel::IncreaseCurrentBrightness(byte value)
{
    if ((CurrentBrightness + value) <= 255)
        CurrentBrightness += value;
    else
        CurrentBrightness = 255;
    SetCurrentBrightness(CurrentBrightness);
}

// Manual Sunrise for a duration in secondes
void LEDChannel::StartManualSunrise(duration SecDuration)
{

    switch (CurrentState)
    {
    case NoAction:
        CurrentState = ManualSunrise;
        ManualSunriseDuration = SecDuration;
        SetCurrentBrightness(0);
        break;
    }

}

And main program .ino

#include "LEDChannel.h"

LEDChannel** pLED;
byte nNbLEDChannel;

void setup()
{
    pLED = new LEDChannel*[1];
    pLED[0] = new LEDChannel(44, NULL);

    /*  If i put the next line as comment CurrentState get a valid value. 
    If i assigne a value to nNbLEDChannel I get invalid value for CurrentState*/
    nNbLEDChannel = 1;  

    pLED[0]->StartManualSunrise(10);
}

void loop()
{
    pLED[0]->PerformSunrise();
}

Answer 1

Have you guaranteed that loop() cannot be called before setup() ?

It is suspicious that setting a value immediately after your pointer array is the "source" of the problem. This leads me to suspect that either the pLED value is getting changed or you have some other indexing problem that is causing the memory where nNbLEDChannel resides to be interpreted as a LEDChannel object.

Answer 2

I didn't get out my Arduino UNO board, but I am pretty sure your main problem is the fact that you are trying to call the default constructor from within another constructor. Edit: The call to LEDChannel() you did actually constructed a new instance of the class which then never gets used or assigned. The version of the C++ standard being used by default by Arduino doesn't support that feature, see the following answer for more information (It also demonstraits the correct syntax for calling a constructor from another constructor if you are using a version of the standard which supports it). This means that the CurrentState variable is uninitialized and will be some random value as you are observing. Uncommenting out the line:

nNbLEDChannel = 1;

really doesn't have anything to do with the error and just rearranges the memory which apparently happens to put the CurrentState variable to a valid value.

Similar functionality, having common code within a constructor, can be achieved by moving your common initialization code out of the default constructor and into another function like this, which is then called by both constructors (don't forget to initialize all of the member variables of the class in this function to a default value and then do any other "special" initialization after it):

LEDChannel::LEDChannel(byte DigitalPIN, AquariumSimulator* pSim)
{
    // Initialize default values
    commonInitFunction();

    //Do other "special" initialization
    pAquariumSim = pSim;
    //pinMode(DigitalPIN, OUTPUT);// Configure Digital pin that control LED Channel
}

LEDChannel::LEDChannel()
{
    // Initialize default values
    commonInitFunction();

    //Do other "special" initialization
}

void LEDChannel::commonInitFunction()
{
    CurrentState = NoAction;
    //make sure all other member variables have been initialized to something either in here or
    //in your constructors to keep from having unexpected behavior
}

Although it is a good idea to initialize variables in an initializer list, as stated here, which would then change your constructors to be the following (Note: I just picked some random default values):

LEDChannel::LEDChannel(byte DigitalPIN, AquariumSimulator* pSim):
  MaxBrightness(255),
  lastTick(0),
  CurrentBrightness(0),
  CurrentState(NoAction),
  DigitalPin(DigitalPIN),
  ManualSunsetDuration(60),
  ManualSunriseDuration(60),
  pAquariumSim(pSim)
{
    //pinMode(DigitalPIN, OUTPUT);// Configure Digital pin that control LED Channel
}

LEDChannel::LEDChannel():
  MaxBrightness(255),
  lastTick(0),
  CurrentBrightness(0),
  CurrentState(NoAction),
  DigitalPin(0),
  ManualSunsetDuration(60),
  ManualSunriseDuration(60),
  pAquariumSim(NULL)
{
    CurrentState = NoAction;
}

This ensures all values are initialized (including in the proper order) for the instantiation of the class.

On a side note, I did notice a few other items in the code you provided which might cause you some unexpected behavior.

Since CurrentBrightness is a byte (i.e. uint8_t), in your void LEDChannel::IncreaseCurrentBrightness(byte value) method, your check to see if the CurrentBrightness + value is <= 255. This will only ever be false if the sum is equal to 255 because of overflow. Basically, if the CurrentBrightness is 250 and I add the value of 10 to it then I will get a value of 4 if I am not mistaken. This is probably not the desired behavior of this check.

The second is that the typedef keyword for your enum is not doing anything since you didn't specify a type. In C++, you don't need the typedef for an enumeration or struct if you only want to refer to it by the original name you gave it, like you did here. See some of the answers here, even though none of them were accepted the first few are