Understanding C++

In the process of making gadgets, I find myself wanting to blink a number of LED's separately and at different rates. From my understanding, if I declare an object 'Blinker' and construct multiple instances, e.g. Blinker myBlinker
Blinker hisBlinker
Blinker herBlinker

The compiler is supposed to create three copies of the Blinker code. I seem to get interference from one to the other. What am I missing? (Sketch Attached)

If I allow only one, by commenting out the others, it behaves as expected. Uncommenting makes LED stay on.

sketch_may24a.ino (1.64 KB)

Why not just post the code, in code tags?
It's not like it's going to bust any forum limits, is it?

  if (((millis ()) - OFFmillis) > T_off)

(What's) (with) (all) (the) (parentheses) (?)

They do NOT improve the readability of your code.

  static unsigned long ONmillis, OFFmillis;
  static unsigned char OnInit = LOW;
  static unsigned char OffInit = LOW;

You should read up on what the static keyword does, paying particular attention to when the static variable is part of a class. Which instances of the class share that variable?

Well, Paulie, if the brackets are the only piece of my code that is flawed for style, I'm sorry.

In regular "C", STATIC saves the value between calls. Looking it up as you suggested, I now don't understand how to declare the variables in such a way as to save the value between calls.

What's the correct way of declaring the variables and have them reside with the instance and having the value saved between calls?

I know what STATIC does, it saves the value between calls

That's one interpretation.

void setup() {
// put your setup code here, to run once:
Serial.begin(9600);
pinMode (10,OUTPUT); //D10
pinMode (16,OUTPUT); //D16
pinMode (14,OUTPUT); //D14
}

class Blinker {
public:
void Blink(unsigned long T_on, unsigned long T_off, unsigned char pin);
};

void Blinker::Blink(unsigned long T_on, unsigned long T_off, unsigned char pin)
{
static unsigned long ONmillis, OFFmillis;
static unsigned char OnInit = LOW;
static unsigned char OffInit = LOW;
T_on -= 1; T_off -= 1; // deduct for overhead of 1ms

if ((OnInit == LOW) && (OffInit == LOW))
{ // Run Once Code
ONmillis = (millis ()); // Snapshot of ON MILLIS count
OnInit = HIGH; // Reset flag once Present Count is set
digitalWrite (pin,HIGH); // Turn it ON
}
if (((millis ()) - ONmillis) > T_on)
{
OnInit = LOW; // Reset flag once Timed OUT
digitalWrite (pin,LOW); // LED Off if timed out
}
if ((OnInit == LOW) && (OffInit == LOW))
{ // Run Once Code
OFFmillis = (millis ()); // Snapshot of OFF MILLIS count
OffInit = HIGH; // Reset flag once Present Count is set
}
if (((millis ()) - OFFmillis) > T_off)
OffInit = LOW;
}//_________________________________________________________________________________

// Make instances of Blinkers
Blinker myBlinker;
Blinker hisBlinker;
Blinker herBlinker;

void loop() {
// myBlinker.Blink(200, 200, 10); // Blink output 10
hisBlinker.Blink(1000, 1000, 16); // Blink output 16
// herBlinker.Blink(50, 50, 14); // Blink output 14
}

rmetzner49:
In regular "C", STATIC saves the value between calls. Looking it up as you suggested, I now don't understand how to declare the variables in such a way as to save the value between calls.

static works exactly the same in c++ as it does in c, and "saving the value" is only part of what it does. Do some Googling, and understand its effect on variable scope, particularly as applied to member data of a class.

Regards,
Ray L.

The usual way of providing a class storage is to declare it in a private section.

Please remember to use code tags when posting code.

The other thing that got you all tangled up, was the lack of a constructor. Here is how it is supposed to be done (... stressing that I am fairly new to OO).

// Blink with OO
// 2015-05-24
// by Aarg

class blinker {
  public:
    blinker(unsigned long T_on, unsigned long T_off, unsigned char pin);
    void blink();
  private:
    unsigned long onTime;
    unsigned long offTime;
    unsigned long previousMillis;
    unsigned char outputPin;
    boolean pinState;
};

blinker::blinker(unsigned long T_on, unsigned long T_off, unsigned char pin)
{
  outputPin = pin;
  onTime = T_on;
  offTime = T_off;

  pinMode (pin, OUTPUT);
  previousMillis = millis();
  pinState = false;
}

void blinker::blink()
{
  if ( (millis() - previousMillis) >= (pinState ? onTime : offTime) )
  {
    digitalWrite (outputPin, pinState ? LOW : HIGH);
    pinState = !pinState;
    previousMillis = millis();
  }
}

// Make instances of blinkers
blinker b1(1000, 1000, 12);
blinker b2(50, 1300, 13);

void setup() { }

void loop() {
  b1.blink();    // blink output 12
  b2.blink();    // blink output 13
}

Here is how it is supposed to be done

Sorry, no. The constructor will, in OP's code, be called before the init() function is called, which is what gets the hardware ready. Calling pinMode(), in the constructor, before the hardware is ready is a waste of effort.

PaulS:
Sorry, no. The constructor will, in OP's code, be called before the init() function is called, which is what gets the hardware ready. Calling pinMode(), in the constructor, before the hardware is ready is a waste of effort.

I'm not sure what you mean. Do I have to do it the way the OP did it? This code is tested with real LEDs.

Of course, I could make an init() function. But why would it be necessary?

Here:

// Blink with OO
// 2015-05-24
// by Aarg

class blinker {
  public:
    blinker(unsigned long T_on, unsigned long T_off, unsigned char pin);
    void init();
    void blink();
  private:
    unsigned long onTime;
    unsigned long offTime;
    unsigned long previousMillis;
    unsigned char outputPin;
    boolean pinState;
};

blinker::blinker(unsigned long T_on, unsigned long T_off, unsigned char pin)
{
  outputPin = pin;
  onTime = T_on;
  offTime = T_off;
}

void blinker::init()
{
  pinMode (outputPin, OUTPUT);
  previousMillis = millis();
  pinState = false;
}

void blinker::blink()
{
  if ((millis() - previousMillis) >= (pinState ? onTime : offTime) )
  {
    digitalWrite (outputPin, pinState ? LOW : HIGH);
    pinState = !pinState;
    previousMillis = millis();
  }
}

// Make instances of blinkers
blinker b1(1000, 1000, 12);
blinker b2(50, 1300, 13);

void setup() {
  b1.init();    // initialize output on pin 12
  b2.init();    // initialize output on pin 13
}

void loop() {
  b1.blink();    // blink pin 12
  b2.blink();    // blink pin 13
}

Are you thoroughly happy now? :slight_smile:

Do I have to do it the way the OP did it?

No, you don't have to. You did, though.

Of course, I could make an init() function. But why would it be necessary?

Because the init() function that the Arduino code executes runs AFTER the constructor, so the pinMode() call in the constructor could get undone.

I would never rely on that not happening.

Are you thoroughly happy now?

Ecstatic. Sort of...

So it was just blind luck, okay. It's good to have an init() method anyway, because you might want to start timing later than when the constructor is called. It would be easy to have an enable/disable method too, but I was trying to keep things as simple as possible.

This code is tested with real LEDs.

As opposed to mine that use imaginary LEDs.

Put enough monkeys in a room, eventually they write Shakespeare. Every poster hinted at what to do, but nobody said "just do this". I did get enough hints to be able to work it, but I'll have to take a bit more time to understand why.

 void setup() {
  // put your setup code here, to run once:
Serial.begin(9600);
pinMode (10,OUTPUT);             //D10
pinMode (16,OUTPUT);             //D16
pinMode (14,OUTPUT);             //D14
}

class Blinker {
public:
  void Blink(unsigned long T_on, unsigned long T_off, unsigned char pin);

private:
  unsigned long ONmillis;
  unsigned long OFFmillis;
  unsigned char OnInit;
  unsigned char OffInit;
};

void Blinker::Blink(unsigned long T_on, unsigned long T_off, unsigned char pin)
{
  T_on -= 1;   T_off -= 1;         // deduct for overhead of 1ms
  
  if ((OnInit == LOW) && (OffInit == LOW))
    { // Run Once Code
    ONmillis = millis ();          // Snapshot of ON  MILLIS count
    OnInit = HIGH;                 // Reset flag once Present Count is set
    digitalWrite (pin,HIGH);       // Turn it ON
    }
  if ((millis () - ONmillis) > T_on)
    {
    OnInit = LOW;                  // Reset flag once Timed OUT
    digitalWrite (pin,LOW);        // LED Off if timed out
    }
  if ((OnInit == LOW) && (OffInit == LOW))
    { // Run Once Code
    OFFmillis = millis ();          // Snapshot of OFF  MILLIS count
    OffInit = HIGH;                 // Reset flag once Present Count is set
    }
  if ((millis () - OFFmillis) > T_off)
    OffInit = LOW;
  }//_________________________________________________________________________________
  
  // Make instances of Blinkers
    Blinker B_10; 
    Blinker B_16; 
    Blinker B_14; 

void loop() {
  B_10.Blink(200, 200, 10);      // Blink output 10
  B_16.Blink(1000, 1000, 16);    // Blink output 16
  B_14.Blink(50, 50, 14);        // Blink output 14
}

Thank you aarg, you provided me with insight although I wanted to call the Blinker with the times and the pin#.

I even shed some of the brackets for Paul.

rmetzner49:
Thank you aarg, you provided me with insight although I wanted to call the Blinker with the times and the pin#.

I even shed some of the brackets for Paul.

Well, you can create an additional method that passes the number of times to blink. It can have the same name, because of the overloading feature of C++.

It seems weird to me, to want to call the blink function with the pin number every time. I see no purpose to it, since any object that I can think of would always drive the same pin (as it is fixed in hardware). Hence there would be never any reason to change it in mid stream.

If all you need to do is change the blink parameters, you can easily add methods to do that.

Static variables are shared between all instances of the Class. So, hisBlinker & herBlinker will have the same value for the static variable. If you don't want that, it needs to be 'private' or public instance variable.

mistergreen:
Static variables are shared between all instances of the Class. So, hisBlinker & herBlinker will have the same value for the static variable. If you don't want that, it needs to be 'private' or public instance variable.

There is absolutely no need for static variables in this application. Unless, perhaps, you want to get fancy and keep track of which pins have already been assigned.

Every poster hinted at what to do, but nobody said "just do this".

Sometimes its important to get the project done. Sometimes its important to learn something. You did not make it clear whether you just wanted to get the project done, or whether you wanted to learn something.

The responses you get are quite different when the goal is to learn why the project isn't working vs. those you get when the goal is get the project done.

Developing a library is not a beginner project. We often assume that advanced projects are undertaken to learn. If you don't think that that is a valid assumption, you need to say that up front.

The responses you get are quite different when the goal is to learn why the project isn't working vs. those you get when the goal is get the project done.

Sorry if I came across as snippy, Paul. I've quite a bit of experience in 'C', but only one semester of C++ at the local community college some 10 years ago. I just found myself cutting and pasting a lot of code that is reusable in this one project and wanted to come up with an OO solution.

I guess the answer I was looking for was "The classifier 'static' is treated differently in C++ than in C. In C++ the term 'static' means that variable is shared between instances. To get a persistent variable in C++, simply declare them in the 'private' section of the class."

You are absolutely correct, it's non-trivial to develop libraries. With that said, I tend avoid just gluing together a bunch of pre-developed libraries. Many don't play well together and use conflicting hardware resources, many are gargantuan and I see a lot of posts on the forum where people use a library when they have absolutely no clue what it's supposed to accomplish.