Code Review // Proper timers & declarations

Hey guys, I'm building a small device based on the Internet of Things Printer from adafruit. It's basically a box which allows printing out a photo from the SD card. Printing is only allowed after X time has passed.

I'm having some issues with keeping time and correctly calculating times especially when the numbers get bigger. Overall my code works fine when I test it with smaller values but when I scale it up to hours/days it doesn't seem to work.

I can't post the entire code here because of the character limit but please check it on pastebin if you are inclined - http://pastebin.com/pP55fVZZ. It's quite short and properly commented.

Overall I'd really appreciate it if people gave it a look and point out any glaring issues especially with the way I'm declaring variables. Having no serious C training - and reading about the various schools of C thought :smiley: - I feel I might be missing some things which cause untraceable problems.

So the only real problem is that when I use longer times (1+ hours) to trigger printing it doesn't work. I haven't had the chance to find a 100% reproduction rule but otherwise the system works perfectly with smaller numbers.

I've extracted from my program everything related to the timing systems hoping it's easier to read. Please have a look and let me know if you see any problems.

//***** Set Variables
unsigned long     systemBoot;               // S since system boot
unsigned long     lastPrint;                // S since last print
int               printBias;                // S for random print times
const int         printInterval = 600;      // Minimum seconds between prints
const int         SDcardFileCount = 520;    // No of files on SD card
boolean           readyForPrint;            // If true, buttonpress will print

// SETUP
void setup() {
  printBias = 0;
}


// LOOP
void loop() {

 
  //***************************************************************************
  //***** Timer Managament
  systemBoot = millis() / 1000;
 
 
  if ((systemBoot - lastPrint) < (printInterval + printBias)) {
    readyForPrint = false;
  } else {
    readyForPrint = true;
  }
 
  //***** Button Management
  fCheckButton ();
}


//***** Button Press Function
int fCheckButton () {
  pinStatus = digitalRead(buttonPin);         // Read input value
 
  if (pinStatus == HIGH) {
  } else {
    if ( readyForPrint == true) {
      fPrintFile ();
    }
  }
 
}

//***************************************************************************
//***** Print a file
int fPrintFile () {
  //Compose random filename
  int randomNumber = random (1, SDcardFileCount + 1);
  String fileExt = ".bin";
  String sdFiName = randomNumber + fileExt;
 
  //Transform it to something usable for the printer
  const char * c = sdFiName.c_str();
 

  //Print actual image
  File sdCardFile = SD.open (c, FILE_READ);
 
  while (sdCardFile.available()) {
    printer.printBitmap(printWidth, printHeight, dynamic_cast<Stream*>(&sdCardFile));
  }
  sdCardFile.close();
 
  delay(1000);                                  // 1 sec delay
 
 
  //Finish printing task
  lastPrint = millis() / 1000;                  // Reset print timer
  printBias = random (1,120);                    // Set print bias
}

Cheers!

I am particularly worried about this excerpt from the millis documentation:

Please note that the return value for millis() is an unsigned long, logic errors may occur if a programmer tries to do arithmetic with smaller data types such as int's. Even signed long may encounter errors as its maximum value is half that of its unsigned counterpart.

Seeing how I am doing some (basic) arithmetic with this number, is it a good idea to keep it as unsigned long? Is there a general consensus on how to properly approach this?

Why are you diving the millis() value by 1000 ? You might just as well use the raw value

Because my intervals for allowing a print will be between 14 and 24 hours. It's easier to store them as seconds and it's easier to write 14 hours in seconds(50400) than in miliseconds(86400000).
The math is obviously still the same but in the end it's just easier to read.

It's easier to store them as seconds

Why should you care how difficult it is to store them?
A day's worth of seconds s going to need an unsigned long, so just store everything as unsigned long.

#define ONE_SECOND (1000UL)
#define ONE_MINUTE (60UL * ONE_SECOND)
#define ONE_HOUR   (60UL * ONE_MINUTE)
#define ONE_DAY      (24UL * ONE_HOUR)

Alright, that sounds legit. I've changed all timer related number as unsigned longs however I still can't print after more than ~30 minutes. Works just fine if it's under that time mark though.

The numbers I am using are most definitely within the range of their respective data type. Any ideas why it just won't print anymore if more than 30 min have passed?

however I still can't print after more than ~30 minutes

32 minutes and 46 seconds, probably.

That sounds like a very exact guess on your side so I assume I'll have to fish for an answer :slight_smile:

The time limit you specify translates to 1966000 miliseconds which is far less than what the unsigned long can hold - (2^32 - 1) = 4294967295 (i.e. 1000+hours)

I am not using any other type of number in my timer calculations so I don't see where your specific number plays a role in this bug.

32.767 minutes, is my guess, aka 32 minutes 46.02 seconds.

I know that 32,767 is the upper limit of Ints but I'm only using unsigned longs...

Unless some of my calculations are (unbeknownst to me) casting that number into an Int... but the only operations I'm doing to these variables are basic arithmetic operations like +,- and /. And all variables are of the same type.

Post your code.

I linked it on pastebin in my original post because I didn't want to dump it all in the message.

Here it goes, latest iteration:

/*
  Photomaton V0.95
*/





//-----------------------------------------------------------------------------
// Initialize Things
//-----------------------------------------------------------------------------


//*****************************************************************************
//***** Printer
#include "SoftwareSerial.h"                 // Serial comms library
#include "Adafruit_Thermal.h"               // Printer library
#define TX_PIN 6                            // Transmit  YELLOW WIRE
#define RX_PIN 5                            // Receive   GREEN WIRE
SoftwareSerial mySerial(RX_PIN, TX_PIN);    // Declare SoftwareSerial
Adafruit_Thermal printer(&mySerial);        // Pass addr to printer constructor


//*****************************************************************************
//***** Card Reader
#include <SPI.h>                            // SPI library
#include <SD.h>                             // SD card SPI library
const int sdPin = 8;                        // SD card reader pin


//*****************************************************************************
//***** Set Variables
unsigned long     systemBoot;               // S since system boot
unsigned long     lastPrint;                // S since last print
unsigned long     printBias;                // S for random print times
unsigned long     printInterval = 15;     // Minimum seconds between prints
const int         SDcardFileCount = 520;    // No of files on SD card
boolean           readyForPrint;            // If true, buttonpress will print

const int         buttonPin = 2;            // Connected button
const int         ledPin = 10;              // Led output
int               pinStatus = 0;            // Read pin status

const int         printWidth = 384;         //Width of images
const int         printHeight = 500;        //Height of images





//-----------------------------------------------------------------------------
// SETUP
void setup() {
  //---------------------------------------------------------------------------


  //***************************************************************************
  //***** Button Configuration
  pinMode (ledPin, OUTPUT);
  pinMode (buttonPin, INPUT_PULLUP);


  //***************************************************************************
  //***** Printer
  mySerial.begin(19200);                    // Init software serial
  printer.begin();                          // Init printer
  lastPrint = 0;


  //***************************************************************************
  //***** Card Reader
  pinMode (sdPin, OUTPUT);                  // Init card reader

  if (!SD.begin(sdPin)) {                   // Notify if card has issues
    printer.doubleHeightOn();
    printer.println(F("OH NOES!!!"));
    printer.doubleHeightOff();
    printer.println ("SD can't be read. Maybe ...");
    printer.feed(1);
    printer.println ("... the card is missing?");
    printer.println ("... the reader is damaged?");
    printer.feed(2);
    fResetPrinter();
    return;
  }
  printer.feed(1);

  randomSeed(analogRead(0));                // Unused analog as seed
  printBias = 1;
}





//-----------------------------------------------------------------------------
// LOOP
void loop() {
  //---------------------------------------------------------------------------


  //***************************************************************************
  //***** Timer Managament
  systemBoot = millis() / 1000;


  if ((systemBoot - lastPrint) < printInterval) {
    readyForPrint = false;
    digitalWrite(ledPin, LOW);
  } else {
    readyForPrint = true;
    digitalWrite(ledPin, HIGH);
  }

  //***************************************************************************
  //***** Button Management
  fCheckButton ();
}





//-----------------------------------------------------------------------------
// OTHER
//-----------------------------------------------------------------------------


//***************************************************************************
//***** Button Press Function
int fCheckButton () {
  pinStatus = digitalRead(buttonPin);         // Read input value

  if (pinStatus == HIGH) {
  } else {
    if ( readyForPrint == true) {
      fPrintFile ();
    }
  }

}


//***************************************************************************
//***** Print a file
int fPrintFile () {
  //Compose random filename
  int randomNumber = random (1, SDcardFileCount + 1);
  String fileExt = ".bin";
  String sdFiName = randomNumber + fileExt;

  //Transform it to something usable for the printer
  const char * c = sdFiName.c_str();

  //Print image header
  fPrintSeparator ();
  printer.println();

  //Print actual image
  File sdCardFile = SD.open (c, FILE_READ);

  while (sdCardFile.available()) {
    printer.printBitmap(printWidth, printHeight, dynamic_cast<Stream*>(&sdCardFile));
  }
  sdCardFile.close();

  delay(1000);                                  // 1 sec delay

  //Print image footer
  fPrintSeparator ();
  printer.justify('C');
  printer.println("- " + String(randomNumber, DEC) + " -");
  fPrintSeparator ();

  //Finish printing task
  printer.feed(4);                              // Breathing room
  lastPrint = millis() / 1000;                  // Reset print timer
//  printBias = random (1,600);                   // Set print bias
  fResetPrinter();                              // Reset printer
}


//***************************************************************************
//***** Reset the printer
int fResetPrinter () {
  printer.sleep();                              // Go to sleep
  delay(3000L);                                 // Wait 3 seconds
  printer.wake();                               // wake() before new print
  printer.setDefault();                         // Restore defaults
}

//***************************************************************************
//***** Print separator
int fPrintSeparator () {
  for (int i = 0; i < 32; i++) {
    printer.write(0xB0);
  }
}

printInterval has a smaller value for testing reasons at the moment.

Eventually these are the values I want to have in the final sketch:
printInterval = 50400000 miliseconds
printBias = a random value between 1 and 36000000

I use printBias to add some randomness to the printInterval - in order to change it at every button press for the next cycle between a total of 14 and 24 hours. I realize as I am typing this that I could just initialize printInterval with a value and then randomize it between the values I want to have in the end without the need for another variable :-X .

I noticed through testing that printInterval doesn't really matter once 30 min have passed. Nothing prints anymore.

I've managed to do a few more tests and I no longer believe the problem lies within the way I am storing/declaring.

I've updated the code to use millis directly (instead of transforming to seconds) and removed the printBias variable - generating a new (random) time at every print.

So far with more testing I've noticed that:

The LED lights up at the proper time showing that the print condition is checked and met. However the button does not call the print function when pressed (as is the normal behavior)

It doesn't print after the 30 minutes mark but if I leave it alone and come back later - tested with approx 3 hours - the print works just fine.

Tested with a 12 hours period as well and even though the LED lit up when it was supposed to buttonpress didn't initiate the print function. Pressed it again after a few more minutes and it worked!?

So my new hypothesis is that the problem could lie within the time check/button management functions between lines 102-139. The code is not doing much, I don't see if there's anything wrong with it.

void loop() {
  //***************************************************************************
  //***** Timer Managament
  systemBoot = millis();


  if ((systemBoot - lastPrint) < printInterval) {
    readyForPrint = false;
    digitalWrite(ledPin, LOW);
  } else {
    readyForPrint = true;
    digitalWrite(ledPin, HIGH);
  }

  //***************************************************************************
  //***** Button Management
  fCheckButton ();
}




//***************************************************************************
//***** Button Press Function
int fCheckButton () {
  pinStatus = digitalRead(buttonPin);         // Read input value

  if (pinStatus == HIGH) {
  } else {
    if ( readyForPrint == true) {
      fPrintFile ();
    }
  }

}

I don't think the problem could be with fPrintFile () as I've tested that quite extensively and it worked every time. I am also ruling out bad wiring as the button works 100% when the program runs with smaller time values.