Feedback on my first attempt to write a library for Arduino + troubleshooting

Okay, so I decided to try learning to write libraries by writing a library that does the "blink without delay" - routine, where I set the pins to use and on/off-times. It does work to a certain degree, however when looking at it with an oscilloscope there timings are off by approx +10% and I of course have no idea why. When attempting to run several leds simultaneously the on times seem to be much shorter (maybe half) occationally, but it still seems to work.

Maybe some of you guys are able to see the things I've overlooked or done wrong?

The header-file:

// Blinks LED without the use of delay().

#ifndef BlinkWithoutDelay_h
#define BlinkWithoutDelay_h

#include "Arduino.h"

class BlinkWithoutDelay
{
    public:
    BlinkWithoutDelay(int blinkPin);
    void BLINK(int _offTime, int _onTime);

    private:
    int _blinkPin;
    int _currentTime;
    int _previousTime;
    int _pinState;
    int _offTime;
    int _onTime;
};
#endif

The source file:

// Blinks LED without the use of delay().

#include "Arduino.h"
#include "BlinkWithoutDelay.h"


BlinkWithoutDelay::BlinkWithoutDelay(int blinkPin)
{
    pinMode(blinkPin, OUTPUT);
    _blinkPin = blinkPin;
}

void BlinkWithoutDelay::BLINK(int _offTime, int _onTime){
   _currentTime = millis();               //
 int  _pinState = digitalRead(_blinkPin);              // Check the state of pin, on or off
  if ((_pinState == 0) && (_currentTime - _offTime > _previousTime)){ // If it's already "off" check if it has reached the set "offtime", otherwise ignore.
    _previousTime = _currentTime; // Reset the timer
    _pinState = 1; // Change the pinState to "on".
  }
  if ((_pinState == 1) && (_currentTime - _onTime > _previousTime)){ // Check if pin is already "on", check if it has reached the set "ontime", otherwise ignore
    _previousTime = _currentTime; // reset the timer
    _pinState = 0; // Change the pinState to "off".
  }
  digitalWrite(_blinkPin, _pinState); // Set pin according to "pinState".
}

A simple sketch to blink one or more LEDs:

void setup(){
}

void loop(){
  BLINK(5,10, 11); // offtime, ontime, pin
}

unsigned int previousTime = 0;                       // To use with timer.

void BLINK(int offTime, int onTime, int blinkPin){
  pinMode(blinkPin, OUTPUT);                         // Set blinkPin as output (should perhaps be set outside of function?
  unsigned int currentTime = millis();               // 
  int pinState = digitalRead(blinkPin);              // Check the state of pin, on or off
  if ((pinState == 0) && (currentTime - offTime > previousTime)){ // If it's already "off" check if it has reached the set "offtime", otherwise ignore.
    previousTime = currentTime; // Reset the timer
    pinState = 1; // Change the pinState to "on".
  }
  if ((pinState == 1) && (currentTime - onTime > previousTime)){ // Check if pin is already "on", check if it has reached the set "ontime", otherwise ignore
    previousTime = currentTime; // reset the timer
    pinState = 0; // Change the pinState to "off".
  }
  digitalWrite(blinkPin, pinState); // Set pin according to "pinState".
}
(_currentTime - _offTime >= _previousTime)

you also need to pay more attention to datatypes:

    int _blinkPin;
    int _currentTime;
    int _previousTime;
    int _pinState;
    int _offTime;
    int _onTime;

Your example sketch does not use your library. How about posting the code that doesn't work?

It is a lot of code that might cause the delay. If I were to write such a libray I would write a method to set/modify the blink and in the loop call a method blink(); In that blink method I would: check the timer first. Is not past the set time just exit. else: - flip the led - set the timer and exit. You could even ad a method to switch the blink on and off. As for accuracy, there is always the possibility that small differences will be found. You are working with the millis() functions. So if your routine is called at 10.1 millissecond or 10.999 millisecond it is still 10 milliseconds.

BlinkWithoutDelay::BlinkWithoutDelay(int blinkPin)
{
    pinMode(blinkPin, OUTPUT);

The hardware is not ready when your constructor is called. Setting the pin mode in the constructor is wrong.

  int pinState = digitalRead(blinkPin);              // Check the state of pin, on or off

I'm nearly certain that the Arduino could remember what it set the pin to. Other code, outside of the library, should NOT be diddling with the pin.

Sorry. What I posted was the sketch I wrote before trying to make the library. :slight_smile:

Here:

#include <BlinkWithoutDelay.h>

BlinkWithoutDelay LEDA(11);
/*
BlinkWithoutDelay LEDB(10);
BlinkWithoutDelay LEDC(9);
BlinkWithoutDelay LEDD(8);
*/

void setup() {
  

}

void loop() {
  LEDA.BLINK(5,10);
/*  
  LEDB.BLINK(10,15);
  LEDC.BLINK(15,20);
  LEDD.BLINK(20,25);
*/
}

AWOL: (_currentTime - _offTime >= _previousTime)

you also need to pay more attention to datatypes:

  int _blinkPin;
    int _currentTime;
    int _previousTime;
    int _pinState;
    int _offTime;
    int _onTime;

Yes. I just did it for simplicity, I should use short or unsigned I guess.

Thanks.

PaulS: BlinkWithoutDelay::BlinkWithoutDelay(int blinkPin) { pinMode(blinkPin, OUTPUT);

The hardware is not ready when your constructor is called. Setting the pin mode in the constructor is wrong.

  int pinState = digitalRead(blinkPin);              // Check the state of pin, on or off

I'm nearly certain that the Arduino could remember what it set the pin to. Other code, outside of the library, should NOT be diddling with the pin.

Thanks. Should I set the pinMode inside "void BlinkWithoutDelay::BLINK(int _offTime, int _onTime){}" instead?

All caps is reserved for constant names. What's with the leading underscores everywhere?

int _currentTime;
int _previousTime;

Times should be unsigned long. This won't work for very long.

[quote author=Nick Gammon date=1422143519 link=msg=2058667] All caps is reserved for constant names. What's with the leading underscores everywhere? [/quote]

I was looking through some of the libraries that comes with the Arduino IDE and I noticed they often used leading underscores for variables and thought it was some kind of common practice with source files.

One of your problems (apart from what AWOL spotted) is that millis has "creep" as described here.

Changing to micros() avoids this and seems to give better results. Oh, and I moved the underscores around for you. :P

Plus I made the times unsigned long.

Sketch:

#include "BlinkWithoutDelay.h"

BlinkWithoutDelay LEDA(11);

BlinkWithoutDelay LEDB(10);
BlinkWithoutDelay LEDC(9);
BlinkWithoutDelay LEDD(8);


void setup() {
  

}

void loop() {
  LEDA.BLINK(5000,10000);
  LEDB.BLINK(10000,15000);
  LEDC.BLINK(15000,20000);
  LEDD.BLINK(20000,25000);

}

BlinkWithoutDelay.cpp:

// Blinks LED without the use of delay().

#include "Arduino.h"
#include "BlinkWithoutDelay.h"


BlinkWithoutDelay::BlinkWithoutDelay(const byte blinkPin)
{
  pinMode(blinkPin, OUTPUT);
  blinkPin_ = blinkPin;
}

void BlinkWithoutDelay::BLINK(const unsigned long offTime, const unsigned long onTime){
  unsigned long currentTime_ = micros();               //
  int  pinState = digitalRead(blinkPin_);              // Check the state of pin, on or off
  if ((pinState == LOW) && (currentTime_ - offTime >= previousTime_)){ // If it's already "off" check if it has reached the set "offtime", otherwise ignore.
    previousTime_ = currentTime_; // Reset the timer
    pinState = HIGH; // Change the pinState to "on".
  }
  if ((pinState == HIGH) && (currentTime_ - onTime >= previousTime_)){ // Check if pin is already "on", check if it has reached the set "ontime", otherwise ignore
    previousTime_ = currentTime_; // reset the timer
    pinState = LOW; // Change the pinState to "off".
  }
  digitalWrite(blinkPin_, pinState); // Set pin according to "pinState".
}

BlinkWithoutDelay.h:

// Blinks LED without the use of delay().

#ifndef BlinkWithoutDelayh_
#define BlinkWithoutDelayh_

#include "Arduino.h"

class BlinkWithoutDelay
{
 public:
 BlinkWithoutDelay(const byte blinkPin);
 void BLINK(const unsigned long offTime, const unsigned long onTime);

 private:
 byte blinkPin_;
 unsigned long previousTime_;
};
#endif

Lars81:
I was looking through some of the libraries that comes with the Arduino IDE and I noticed they often used leading underscores for variables and thought it was some kind of common practice with source files.

Leading underscores, followed by a capital, are reserved names. It is probably best to avoid leading underscores altogether.

Adding a trailing underscore is fairly commonly done for class variables, not just variables in general. That way you can quickly spot the difference between local variables (or global variables) and ones which belong to a class.

PaulS: The hardware is not ready when your constructor is called. Setting the pin mode in the constructor is wrong.

Plus what PaulS said. You could conceivably set the pin to output every time you change it. Not a great solution. Or have a "begin" method which does it.

    int _offTime;
    int _onTime;
};
#endif

You never use those variables. Or _pinState, which you redeclare in the function. May as well get rid of them. And currentTime may as well be a local variable, you hardly need to save it.

(Changes made to my suggested code above).

nicoverduin:
It is a lot of code that might cause the delay. If I were to write such a libray I would write a method to set/modify the blink and in the loop call a method blink();
In that blink method I would:
check the timer first. Is not past the set time just exit. else:

  • flip the led
  • set the timer and exit.
    You could even ad a method to switch the blink on and off.
    As for accuracy, there is always the possibility that small differences will be found. You are working with the millis() functions. So if your routine is called at 10.1 millissecond or 10.999 millisecond it is still 10 milliseconds.

Thanks! It sounds interesting, but I don’t fully understand. Could you perhaps explain/write it in pseudocode?

I changed it to use micros() instead of millis() and that gave better accuracy.

Okay. I tried to implement the changes as you suggested and it seems to work a lot better now. The huge flickering is gone (probably due to wrong data types?). The durations seem to be about 1-2% higher than stated though (e.g, onTime of 1000us is 1020us on the oscillocope). Could that be caused by the time the microcontroller uses to execute the commands?

Also I added a begin() function with pinMode to be used within “void setup()”, and I removed the unused variables.

Sketch:

#include <BlinkWithoutDelay.h>

BlinkWithoutDelay LEDA(11);
BlinkWithoutDelay LEDB(12);


void setup() {
  LEDA.Begin();
  LEDB.Begin();

}

void loop() {
  LEDA.Blink(1000,500);
  LEDB.Blink(2000,1000);
}

Header:

// Blinks LED without the use of delay().

#ifndef BlinkWithoutDelay_h
#define BlinkWithoutDelay_h

#include "Arduino.h"

class BlinkWithoutDelay
{
 public:
 BlinkWithoutDelay(unsigned char blinkPin);
 void Begin();
 void Blink(unsigned long offTime_, unsigned long onTime_);

 private:
 unsigned char blinkPin_;
 unsigned char pinState_;
 unsigned long previousTime_;

};
#endif

Source file:

// Blinks LED without the use of delay().

#include "Arduino.h"
#include "BlinkWithoutDelay.h"


BlinkWithoutDelay::BlinkWithoutDelay(unsigned char blinkPin)
{
blinkPin_ = blinkPin;
}


void BlinkWithoutDelay::Begin(){
 pinMode(blinkPin_, OUTPUT);
}


void BlinkWithoutDelay::Blink(unsigned long offTime_, unsigned long onTime_){
   unsigned long currentTime_ = micros();
   if ((pinState_ == 0) && (currentTime_ - offTime_ > previousTime_)){ // If it's already "off" check if it has reached the set "offtime", otherwise ignore.
    previousTime_ = currentTime_; // Reset the timer
    pinState_ = 1; // Change the pinState to "on".
  }
  if ((pinState_ == 1) && (currentTime_ - onTime_ > previousTime_)){ // Check if pin is already "on", check if it has reached the set "ontime", otherwise ignore
    previousTime_ = currentTime_; // reset the timer
    pinState_ = 0; // Change the pinState to "off".
  }
  digitalWrite(blinkPin_, pinState_); // Set pin according to "pinState".
}

Whether it is time to toggle the state of the pin has NOTHING to do with the current state of the pin. Your method is WAY too complex. If it's time, toggle the pin. Assign the new value to the pin. There is no need to check the current state of the pin.

pinState_ = !pinState_;

PS. The trailing underscores look just as dumb as the leading underscores. Get rid of them.

Not quite as dumb, Paul.

See: Google C++ Style Guide

Class Data Members

Data members of classes, both static and non-static, are named like ordinary nonmember variables, but with a trailing underscore.

PaulS: Whether it is time to toggle the state of the pin has NOTHING to do with the current state of the pin. Your method is WAY too complex. If it's time, toggle the pin. Assign the new value to the pin. There is no need to check the current state of the pin.

pinState_ = !pinState_;

PS. The trailing underscores look just as dumb as the leading underscores. Get rid of them.

void BlinkWithoutDelay::Blink(unsigned long offTime_, unsigned long onTime_){ unsigned long currentTime_ = micros(); if (currentTime_ - offTime_ > previousTime_){ // If it's already "off" check if it has reached the set "offtime", otherwise ignore. previousTime_ = currentTime_; // Reset the timer pinState_ = !pinState_; } if (currentTime_ - onTime_ > previousTime_){ // Check if pin is already "on", check if it has reached the set "ontime", otherwise ignore previousTime_ = currentTime_; // reset the timer pinState_ = !pinState_; } digitalWrite(blinkPin_, pinState_); // Set pin according to "pinState". }

You mean like this? Won't that just cause it to mix the onTimes and offTimes? I will try it when I get home from work.

I understand that this would work if the onTime and offTime was the same, but I want the option to have different durations for on and off.

I kept the underscores just to make it simpler for myself to differ between class variables and global variables.