0

I'm trying to keep track of a timer, and the internal variables seem to be overrun by one another.

I just want two instances of the Output class, which can keep track of the _elapsed_time so I can check if the time elapsed. It's a poor-man's real-time for a game. I know there are libraries available and I'm planning to use them, but I would really appreciate knowing what's going on here: I want a buzzer pin, and status led pin to 'blink' at different intervals. They both work individually, but when I have them both uncommented they work somewhat serially...

#include "Arduino.h"

#include "MTC.h"

#include <SoftwareSerial.h>   //Software Serial Port

//HARDWARE SETTINGS
#define PIN_STATUS                  13
#define PIN_BUZZER                  A2

Output *STATUS(0);
Output *BUZZER;

void setup()
{
    Serial.begin(9600);
    STATUS = new Output();
    STATUS->Begin(PIN_STATUS, STATE_OFF);
    BUZZER = new (Output) Output(3);
    BUZZER->Begin(PIN_BUZZER, STATE_OFF);
}

void loop()
{
    static bool run_once = true;

    if (run_once == true)
    {
        run_once = false;
        Serial.print("Running");
        Serial.flush();
    }
    delay(250);

    //Serial.println("still running");

    STATUS->SetStrobe(3000);

    //BUZZER->SetStrobe(4000);

}

and class:

#ifndef MTC_h
#define MTC_H


typedef int OUTPUT_STATE;
OUTPUT_STATE STATE_OFF = 0;
OUTPUT_STATE STATE_ON = 1;
OUTPUT_STATE STATE_STROBE = 2;

class Output
{
private:
    int _pin;
    OUTPUT_STATE _last_state;
    unsigned long _cycle_duration;
        unsigned long _last_elapsed;

public:
    Output();                                           //constructor: set pin data store
    void Begin(int pin, OUTPUT_STATE state);                                //set hardware to output and defaul state
    void Set(OUTPUT_STATE state);                               //toggle operability
    void SetStrobe(unsigned long duration);                     //strobe operability
};

Output::Output()                                    //constructor: set pin data store
{

}

void Output::Begin(int pin, OUTPUT_STATE state)
{
    _pin = pin;
        _last_elapsed = millis();
    pinMode(_pin, OUTPUT);
    digitalWrite(_pin, state);
    _last_state = state;
}

void Output::Set(OUTPUT_STATE state)
{
    if (_last_state != state)
    {
        digitalWrite(_pin, state);
        _last_state = state;
    }
}

void Output::SetStrobe(unsigned long duration)
{
    static OUTPUT_STATE _last_strobe_state = STATE_OFF;

    unsigned long elapsed = millis() - _last_elapsed;
    if (elapsed >= duration)
    {
        if (_last_strobe_state == STATE_OFF)
        {
            digitalWrite(_pin, STATE_ON);
            _last_strobe_state = STATE_ON;
        }
        else
        {
            digitalWrite(_pin, STATE_OFF);
            _last_strobe_state = STATE_OFF;
        }
        _last_state = STATE_STROBE;
        _last_elapsed = millis();
    }
}
#endif
1

Your Output::SetStrobe() is wrong, it uses a static variable (_lasr_strobe_state) that is thus shared between all instances of OUTPUT, which is NOT what you want.

You have to change it into a non-static member of class Output instead:

class Output
{
private:
    int _pin;
    OUTPUT_STATE _last_state;
    // NEW member here!!!
    OUTPUT_STATE _last_strobe_state;
    unsigned long _cycle_duration;
        unsigned long _last_elapsed;

public:
    Output();     //constructor: set pin data store
    void Begin(int pin, OUTPUT_STATE state);//set hardware to output and defaul state
    void Set(OUTPUT_STATE state);            //toggle operability
    void SetStrobe(unsigned long duration);  //strobe operability
};

Then you initialize it in the constructor:

Output::Output():_last_strobe_state(STATE_OFF)
{

}

And you remove it (static declaration) from method SetProbe (the rest of the method does not change):

void Output::SetStrobe(unsigned long duration)
{
    unsigned long elapsed = millis() - _last_elapsed;
    if (elapsed >= duration)
    {
        if (_last_strobe_state == STATE_OFF)
        {
            digitalWrite(_pin, STATE_ON);
            _last_strobe_state = STATE_ON;
        }
        else
        {
            digitalWrite(_pin, STATE_OFF);
            _last_strobe_state = STATE_OFF;
        }
        _last_state = STATE_STROBE;
        _last_elapsed = millis();
    }
}
1

This is not a real answer, as @jfpoilpret already gave a perfectly valid one. Just a few observations and suggestions from looking at your code. I write is as an answer only because it is too long to fit in a comment.

  1. The member variable _cycle_duration is not used. You could remove it and save a couple of bytes per Output instance.
  2. if (run_once == true) is equivalent to if ((run_once == true) == true). They are both considered bad style. Preferred style is to just write if (run_once).
  3. On the Arduino, it is generally good practice to avoid dynamic allocation whenever possible. Of course, it is not always possible, but in this instance it is quite easy: declare STATUS and BUZZER as objects instead of pointers (as in Output STATUS;), then replace -> by a single dot (.).

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.