Simple OOP but millis timing not working: SOLVED

Using Arduino Programming Questions
#1

Struggling with OOP, I’m adapting some tutorial code from here to extend the capabilities of a simple LED class.

However it doesnt generate the correct delay when I try to use millis() instead of a delay.
(cribbed from blink without delay)

this is the bit that causes issues - I’ve attached the sketch & led.h, led.cpp files for completeness

here is the bit of the sketch that calls the flash(t) function

void loop() {
    led1.flash(100);
    led2.off();
  delay(5000);
  }

   void Led::flash(int t) {      //flash LED for time t

      unsigned long previousMillis; // will store last time LED was updated  

    // for (int i=2; i>0 ;i--){change(); delay(t);}  ***with delay the timing is correct

     for (int i=2; i>0 ;i--){
      unsigned long currentMillis = millis();   
      if (currentMillis - previousMillis >= t){ previousMillis = currentMillis; change(); }
    }
    }

I’m sure its a trivial mistake but I just cant see it. The millis code gives delay times of about 3 sec.

led.cpp (1.18 KB)

led.h (293 Bytes)

OOP_example.ino (411 Bytes)

#2 
  unsigned long previousMillis; // will store last time LED was updated

previousMillis is not being initialised so could be any value. Start by initialising it with a value then consider changing the whole principle of how the program works to eliminate all blocking code

#3

Try to make a minimal workable example that we can compile. At first look my understanding is that you are essentially using delays as you trap code in a for loop. Millis requires code to loop quickly so use of blocking code is likely to cause problems.

#4

Thanks - so its the delay() in the main loop thats causing issues. I'll have a look later.
I wonder if theres a way I can include a "wait" in the cpp that would let me avoid lots of millis() stuff in the main program?

#5

No, not just the delay() also the delay caused by using blocking code. Look up the tutorials in the stickies at the top of the section. A number of them are about doing multiple things at the same time and explain the use of millis.
In short Millis timing works by recording a time and then looping regularly through the code to check if a period has elapsed relative to this time. You check every time you loop. If you loop only once every 10 seconds because you have blocking code slowing your loop down then your resolution will be very poor and every timer less than 10 seconds will default to 10 or not be detected at all depending on how you write your code

#6

I wonder if theres a way I can include a "wait" in the cpp

Why would you do that ?

Add a function to your .cpp that checks whether the period has passed and if so, does whatever you need, ie changes the state of the LED, increments a counter etc. Call that function every time through loop() but do not put blocking code in loop() or anywhere else

#7

Have a look at how the code is organized in Several Things at a Time

Note how each function runs very briefly and returns to loop() so the next one can be called. None of the functions tries to complete a task in one call. And there may be dozens of calls to a function before it is actually time for it to do anything.

...R

#8

An example that I wrote for a query here some time ago

class pinToggle
{
  private:
    byte _pinNum;
    unsigned long _lowPeriod;
    unsigned long _highPeriod;
    unsigned long _currentPeriod;
    byte _currentState;
    unsigned long _startTime;
    unsigned long _currentTime;
    byte _currentSrate;
    boolean _toggling;
    byte _toggleCount;
    byte _count;
  public:
    pinToggle(byte pin)  //constructor
    {
      _pinNum = pin;
    }

    init(unsigned long lowPeriod, unsigned long highPeriod, byte toggleCount)
    {
      _currentState = LOW;
      _lowPeriod = lowPeriod;
      _highPeriod = highPeriod;
      _toggleCount = toggleCount;
      pinMode(_pinNum, OUTPUT);
      _currentPeriod = lowPeriod;
      digitalWrite(_pinNum, _currentState);
      _startTime = millis();
      _toggling = true;
      _count = 0;
    }

    update()  //check whether period has ended and change state if true
    {
      if (_toggling)
      {
        _currentTime = millis();
        if (_currentTime - _startTime >= _currentPeriod)
        {
          if (_currentState == LOW)
          {
            _currentPeriod = _highPeriod;
            _currentState = HIGH;
            _count++; //count transitions to HIGH
            Serial.println(_count);
          }
          else
          {
            _currentPeriod = _lowPeriod;
            _currentState = LOW;
          }
          _startTime = _currentTime;
          if (_toggleCount != 0)
          {
            if (_count == _toggleCount)
            {
              _toggling = false;
            } //counting done
          } //counting active
          digitalWrite(_pinNum, _currentState);
        } //end of period check
      } //toggling
    }
};

pinToggle led1(3);  //instance of an LED on pin 3

void setup()
{
  led1.init(500, 500, 5);  //LOW period, HIGH period, toggle count
  Serial.begin(115200);
}

void loop()
{
  led1.update();  //toggle the LED if it is time
}

The LED blinking could be started at any time by calling the init() function when a trigger action occurs

#9

You can refer to this output library on Github

#10

Thanks for all your help. I've learnt a lot. As you said, the problem lay not in the cpp file, but because the main sketch contained "blocking code"

I've formed the following conclusions:

1: for me, even a simple class library should not fail if the sketch its called from has blocking code. Also the sketch should not fail if the library contains blocking code.

2: so the library can only provide functions that arent time dependent - just very rapidly executed code.

Hence the flash and blink functions move to the main program, where their time dependence is manageable.

I discovered the way I was trying to implement a "change" (or toggle - thanks Helibob) didnt work.

 void Led::change() {  //whatever it was, change it

      //digitalWrite(pin, !digitalRead(pin)); //   ***********  doesnt work!

      //_currentState = (_currentState == LOW) ? HIGH : LOW;  //this works but why so complicated?

      _currentState = !_currentState;
      digitalWrite(pin, _currentState);
    }

So I needed to use a flag _currentState (declared as a private boolean variable in led.h) to record what the LED was set to.

I BELIEVE I've learnt that each instance of the led class gets its own copy of that variable - and I cant change it in the sketch. Is that right?

One puzzle - if I change pin to _pin throughout in .h and .cpp files it all compiles - but it doesnt work.

#11

1: for me, even a simple class library should not fail if the sketch its called from has blocking code.

I don't see how you can do what you want. If the sketch has blocking code then calling the class functions will be blocked

Also the sketch should not fail if the library contains blocking code.

The same problem in reverse

Hence the flash and blink functions move to the main program, where their time dependence is manageable.

Moving the flash and blink functions to the main program nullifies the usefulness of using OOP because you cannot create multiple instances of the object without extra code in the main program. Take my example. By adding just 3 new lines of code (the call to teh constructor, the init and update) you can add a second object. All of the timing and blinking etc is encapsulated in the class and you are safe in the knowledge that the operation of one object will not affect that of any other

I BELIEVE I've learnt that each instance of the led class gets its own copy of that variable - and I cant change it in the sketch. Is that right?

Yes, see my previous paragraph

if I change pin to _pin throughout in .h and .cpp files it all compiles - but it doesnt work.

_pin is a private variable accessible only to a specific instance of the object. By convention the names of private variables start with an underscore, but that in itself does not make them private. pin, however, would be a publicly available variable

digitalWrite(pin, !digitalRead(_pin));

Bearing in mind what I have explained above, _pin and pin are not the same variable

_currentState = (_currentState == LOW) ? HIGH : LOW;  //this works but why so complicated?

That is just a short form of

if (_currentState == LOW)
  {
    _currentState = HIGH;
  }
else
  {
    _currentState = LOW;
  }

Complexity is in the eye of the beholder

#12

johnerrington:
1: for me, even a simple class library should not fail if the sketch its called from has blocking code. Also the sketch should not fail if the library contains blocking code.

That is missing the most important point - programs should not have blocking code anywhere.

...R

#13

Thanks both -

programs should not have blocking code anywhere

Moving the flash and blink functions to the main program nullifies the usefulness of using OOP

Oh I see - because it means I then dont have seperate member variables.

I've changed a line above, sorry didnt spot the error because it was commented.

  //digitalWrite(pin, !digitalRead(pin)); //   ***********  doesnt work!

pin is a private variable accessible only to a specific instance of the object. By convention the names of private variables start with an underscore, but that in itself does not make them private. pin, however, would be a publicly available variable

I see.

pin, however, would be a publicly available variable

even if (as it was) declared private?

Thanks again: I'll need time to absorb all this and have a play with your "toggle" example.

#14

Hi,

digitalWrite(pin, !digitalRead(pin)); //   ***********  doesnt work!

What is it supposed to do?

Tom.... :slight_smile:

#15

pin, however, would be a publicly available variable

even if (as it was) declared private?

Which sketch are you referring to where pin is declared as private ?

#16

Hi TomGeorge - it was an attempt to toggle the state of the LED attached to “pin”.

UKHeliBob, it works like this, but when I changed all instances of “pin” to “_pin” in .cpp and .h it didnt. Maybe just a typo. I’ll try it again.

sketch and library here as they are short:

#include "led.h"
#define LED_1_PIN 9
#define LED_2_PIN 6
#define BUTTON_PIN 4

// Create your objects in the global scope so you can
// get access to them in the setup() and loop() functions
Led led1(LED_1_PIN);   //constructor initialises led pin & OFF
Led led2(LED_2_PIN);

void setup() {
  //pinMode(BUTTON_PIN, INPUT_PULLUP);
  led2.on();
}

void loop() {
  led1.change();
  led2.change(); 
  delay(200);
}

led.h

#ifndef MY_LED_H
#define MY_LED_H

#include <Arduino.h>

class Led {
  
  private:
    byte pin; 
    boolean _currentState;  //records whether led is on or off
    // leading underscore is a just convention to show this is a member variable for the class
    
  public:
    Led(byte pin);
    void init();
    void on();
    void off();
    void change();
};

#endif

led.cpp

#include "Led.h"

Led::Led(byte pin) {
  // Use 'this->' to make the difference between the
  // 'pin' attribute of the class and the 
  // local variable 'pin' created from the parameter.
  this->pin = pin;
  init();
}

void Led::init() {
  pinMode(pin, OUTPUT);
  // Always try to avoid duplicate code.
  // Instead of writing digitalWrite(pin, LOW) here,
  // call the function off() which already does that
  off();
}

void Led::on() {
  _currentState = LOW;
  //digitalWrite(pin, _currentState);
  digitalWrite(pin, LOW);
}

void Led::off() {
   _currentState = HIGH;
  //digitalWrite(pin, _currentState);
  digitalWrite(pin, HIGH);
}

  void Led::change() {
  //whatever it was, change it
      //digitalWrite(pin, !digitalRead(_pin)); //doesnt work!
      //_currentState = (_currentState == LOW) ? HIGH : LOW;  //this works but why so complicated?
      _currentState = !_currentState;
      digitalWrite(pin, _currentState);
    }
#17

it was an attempt to toggle the state of the LED attached to "pin".

I assume that you now know why it doesn't work

#18 
  private:
    byte pin;
    boolean _currentState;  //records whether led is on or off
    // leading underscore is a just convention to show this is a member variable for the class
    
  public:
    Led(byte pin);

Do yourself a favour and use different and distinctive names for private and public versions of variables. The convention of use of leading underscores for private variables is suggested for a reason

#19

Thanks UKHeliBob, yes I'll do that - I see the reasoning.

Can I mark this as solved?

#20

Can I mark this as solved?

Modify your first post and add [solved] to the title