timer problem

I have found my main issue seems to be timers. Cut down to basics code, took out lcd commands and substitued serial so I can see whats happening.
Edited shown here code:

#include <EEPROM.h>
#include "Arduino.h"
#include <nmea.h>    //include nmea library
#include <SMARTGPU.h>     //include the SMARTGPU library!


char RPM[5];   //char array for display
char TENTHS[6]; //char array tenths of hour
char HOURS[6]; //char array for display


//variables

int rpm = 200;
float mph = 0.0;
int hours;
int s = 0; // time in seconds
int t; //time in tenths of hour
unsigned long TimerA = 0;
unsigned long TimerB = 0;    

//pin assignments
int sensor1 = 2;


void setup() { 
  pinMode (sensor1, INPUT);
  Serial.begin(9600);

}


void loop(){

  
  itoa(rpm,RPM,10);
  
   
  if (rpm > 1){ // enter counter here
    TimerB = millis(); 
    if (TimerB >= 1000UL){                                 // s will wait untill timer hits first 1000 then begins incrementing per loop
      s++;}
     if (millis() - TimerB >= 1000UL){                   //if I take out the if statement and timer remains 0 forever
     TimerB = 0;}                                                // with this staement timer increments forever
     }
  if (rpm == 0){
    for (int i=0; i<6; i++){
      TENTHS[i] = EEPROM.read(i);} //read EEPROM data
   }
  if (s >= 360){
    t++;
    s = 0;
    if (t >999) {
      t = 0;}
    itoa(t,TENTHS,10); //changes number, into char, base10 number
      for (int i=0; i<6; i++){
         if (TENTHS[i] != EEPROM.read(i)) EEPROM.write(i, TENTHS[i]);}
  }
  
  hours = t /10;
  if (hours < 1){
    hours = 0;}
  itoa(hours,HOURS,DEC);
  //lcd.string(140,100,270,120,BLACK,FONT7,TRANS,HOURS);

Serial.write (HOURS);
Serial.print ("\t");
Serial.write (RPM);
Serial.print ("\t");
Serial.print (s);
Serial.print ("\t");
Serial.print (t);
Serial.print ("\t");
Serial.print (hours);
Serial.print ("\t");
Serial.print (rpm);
Serial.print ("\t");
Serial.print (TimerB);
Serial.print ("\t");
Serial.println ("char hours, char rpm, sec, tenths, hours, rpm, timerb");
}

I want the "s" to increment each time the timer hits 1000 ms. Then I either need the timer to reset to 0 and start over or make the "s" wait until timer hits 2000ms before it incements again.

  lcd.baudChange(2000000);

Is your LCD really that fast? 2000000 baud? That's 200,000 characters per second!

  if (rpm = 0){

That sets rpm to zero.

void readrpm(){
  pulse1 = 0;
  TimerA = 0;
  interrupts(); //enable
  if (TimerA >= 1000){
   noInterrupts(); //disable
   } 
   rpm = (pulse1 * 60); //assumes 1 pulse per revolution
  loop();
}

loop calls readrpm which calls loop (with interrupts off). You will use up available RAM in about 3 microseconds.

    (hours = hours + 0000.1);
    (t = 0);

What are the parentheses here for?

@PaulS
It looks like LISP ... think the compiler will complain a lot on those lines.

I also like the leading zero's ...

@JHEnt,
Can you tell us about the purpose (in plain english) of the sketch ?
What is the output you expect and what do you get?

cleaned up your code a bit ; as I don't have the HW & libs I can't compile :slight_smile:

#include <EEPROM.h>
#include "Arduino.h"
#include <nmea.h>    //include nmea library
#include <SMARTGPU.h>     //include the SMARTGPU library!

SMARTGPU lcd;             //create our object called LCD
NMEA gps(GPRMC);

int touch[2]; //array for x, y coord on touchscreen
char icon[3];  //char array that stores the name of the touched icon
char RPM[5];   //char array for display
char radar[5]; //char array for display
char HOURS[6]; //char array for display
char LAT[7];
char LON[7];

//variables
volatile int pulse1;
int rpm = 0;
float mph = 0;
float hours = 0;
float lon = 0;
float lat = 0;
int t = 0; // time in seconds
unsigned long TimerA;
unsigned long TimerB;    

//pin assignments
int sensor1 = 2;


void setup() 
{ 
  pinMode (sensor1, INPUT);

  lcd.init();  //configure the serial and pinout of arduino board for SMARTGPU support
  lcd.start(); //initialize the SMARTGPU processor
  delay (500);

  lcd.baudChange(2000000);

  lcd.imageSD(0,0,"splash"); //startup image
  delay (5000);
  lcd.setScreenBackground(BLUE);
  lcd.erase();
  TimerA = millis();

  attachInterrupt(0, engine, FALLING); //trigger engine function when signal drops
}

void engine()
{
  // rpm add function
  pulse1++;
}

void readrpm()
{
  pulse1 = 0;
  TimerA = 0;
  interrupts(); //enable
  if (TimerA >= 1000)
  {
    noInterrupts(); //disable
  } 
  rpm = (pulse1 * 60); //assumes 1 pulse per revolution
  loop();
}

void readhrs()
{
  if (hours >9999.9) 
  {
    hours = 0;
  }
  // read EEPROM
  if (rpm > 1)
  { 
    // enter counter here
    TimerB = millis();
    if (TimerB >= 1000)
    {
      t++;
      TimerB = 0;
    }
  }
  if (rpm == 0)
  {
    for (int i=0; i<6; i++)
    {
      HOURS[i] = EEPROM.read(i);  // you missed 4 here
    }
  }
  if (t >= 360)
  {
    hours = hours + 0.1;
    t = 0;
    itoa(hours, HOURS,10); //changes number, into char, base10 number
    for (int i=0; i<6; i++)
    {
      if (HOURS[i] != EEPROM.read(i)) EEPROM.write(i, HOURS[i]);
    }
  }
}

void loop()
{
  // read hours and place on screen
  readhrs();
  lcd.string(140,100,270,120,BLACK,FONT7,TRANS,HOURS);

  readrpm();
  if (rpm <1 )  // can rpm be negative???
  {
    rpm = 0;
  }
  itoa(rpm, RPM,10); //changes number, into char, base10 number
  lcd.string(140,20,270,40,BLACK,FONT7,TRANS, RPM); //need char array called rpm
}

Yes the smartgpu is that fast.

Primarily I want the display to show three things:
engine rpm, using interrupt on pin 2.
running hours to the 10th that get saved in EEPROM. Hoping to prevent EEPROM wear I wanted it only to save a digit when one changed.
3rd, speed from NMEA gps message on another serial port on a Mega or possibly on software serial on an UNO.

I had it working on several .bmp background screens prior to adding the engine rpm so I tried stripping it down to just whats here now.

What I am getting now is only charachters in the position of the hours on the screen and no other display.

I can see how the for statements can really shorten up my code, thanks for that.

I dug through old forum post and found my answer to the timer issue. I do wonder now if this timer resets millis() then will a second similar timer work?

void loop(){

  
  itoa(rpm,RPM,10);
  
   
  if (rpm > 1){ // enter counter here
    TimerB = ((millis() - startTime) /1000); 
    if (TimerB >= 1){
      s++;
      startTime = millis();}                              //resets timer each second
   }
     
  if (rpm == 0){
    for (int i=0; i<6; i++){
      TENTHS[i] = EEPROM.read(i);} //read EEPROM data
   }
  if (s >= 360){
    t++;
    s = 0;
    if (t >999) {
      t = 0;}
    itoa(t,TENTHS,10); //changes number, into char, base10 number
      for (int i=0; i<6; i++){
         if (TENTHS[i] != EEPROM.read(i)) EEPROM.write(i, TENTHS[i]);}
  }
  
  hours = t /10;
  if (hours < 1){
    hours = 0;}
  itoa(hours,HOURS,DEC);
  //lcd.string(140,100,270,120,BLACK,FONT7,TRANS,HOURS);

Serial.write (HOURS);
Serial.print ("\t");
Serial.write (RPM);
Serial.print ("\t");
Serial.print (s);
Serial.print ("\t");
Serial.print (t);
Serial.print ("\t");
Serial.print (hours);
Serial.print ("\t");
Serial.print (rpm);
Serial.print ("\t");
Serial.print (TimerB);
Serial.print ("\t");
Serial.println ("char hours, char rpm, sec, tenths, hours, rpm, timerb");
}

I have the timer working but still cannot get the interrupts to run. The program crashes when the counter rolls over the first second. If I // out the interrupts then the program runs but does not count rpm

#include <EEPROM.h>
#include "Arduino.h"
#include <nmea.h>    //include nmea library
#include <SMARTGPU.h>     //include the SMARTGPU library!

SMARTGPU lcd;             //create our object called LCD
NMEA gps(GPRMC);

int touch[2]; //array for x, y coord on touchscreen
char icon[3];  //char array that stores the name of the touched icon
char RPM[5];   //char array for display
char radar[5]; //char array for display
char HOURS[6]; //char array for display
char LAT[7];
char LON[7];
char TENTHS[6];

//variables
volatile int pulse1;
int rpm;
float mph = 14.5;
int hours;
float lon;
float lat;
unsigned long t; // tenths of hour
int s; // seconds
unsigned long TimerA;
unsigned long TimerB;    
unsigned long startTime = 0;

//pin assignments
int sensor1 = 2;


void setup() { 
  pinMode (sensor1, INPUT);
  Serial.begin(9600);
  for (int i=0; i<6; i++){
    t = EEPROM.read(i);
  }
  attachInterrupt(0, engine, FALLING); //trigger engine function when signal drops
}
void engine(){// rpm add function
  pulse1++;
}


void loop(){
  if (mph < 1){
    mph = 0;
  }
  itoa(mph,radar,10);
  //  lcd.string(140,60,270,80,BLACK,FONT7,TRANS,radar); //need char array called mph
  
    interrupts();
  TimerB = ((millis() - startTime) /1000); 
  if (TimerB >= 1){
    s++;
    startTime = millis();
    pulse1 = 0;
    noInterrupts(); 
  }
  rpm = (pulse1 * 60); //assumes 1 pulse per revolution
  itoa(rpm,RPM,10); //changes number, into char, base10 number
  //  lcd.string(140,20,270,40,BLACK,FONT7,TRANS,RPM); //need char array called rpm

  // hours
  if (s >= 360){
    t++;
    s = 0;
    if (t >99999) {
      t = 0;
    }
    for (int i=0; i<6; i++){
      if (t != EEPROM.read(i)) EEPROM.write(i,t);
    }
  }
  for (int i=0; i<6; i++){
    TENTHS[i] = EEPROM.read(i);
  } //read EEPROM data
  hours = t /10;
  if (hours < 1){
    hours = 0;
  }
  itoa(hours,HOURS,10);
  //  lcd.string(140,100,270,120,BLACK,FONT7,TRANS,HOURS);


  Serial.write (HOURS);
  Serial.print ("\t");
  Serial.write (RPM);
  Serial.print ("\t");
  Serial.print (s);
  Serial.print ("\t");
  Serial.print (t);
  Serial.print ("\t");
  Serial.print (hours);
  Serial.print ("\t");
  Serial.print (rpm);
  Serial.print ("\t");
  Serial.print (TimerB);
  Serial.print ("\t");
  Serial.println ("char hours, char rpm, sec, tenths, hours, rpm, timerb");
}
 interrupts();
  TimerB = ((millis() - startTime) /1000); 
  if (TimerB >= 1){
    s++;
    startTime = millis();
    pulse1 = 0;
    noInterrupts(); 
  }

When the timer rolls over you turn off interrupts. Why?

Later you Serial.print what looks to me like quite possibly more than 64 characters. In version 1.0+ of the IDE Serial.print relies on interrupts. It will put the first 64 characters into a buffer, and when that is full, sit back and wait for interrupts to pull them out and send them to the UART. But you have turned interrupts off.

I don't see why you are going to all that trouble in the first place. The time returned by millis () will take 50 days to wrap around. You can work out hours/minutes/seconds by simple modulus arithmetic and division. Are you planning to run this gadget for 50 days without stopping?

I appreciate the response.
I was under the impression that I needed to turn off the interrupts to make certain it was counting up pulses in 1 second exactly. I got this from reading a fan rpm example. I tired it out on a proto board. With a pull up resistor installed, when I manually pulse the pin2 to ground I do get rpm signal. If I am fairly consistant it seems to even out at a realistic level. If I am not shorting evenly enough the rpm will jump high and roll over into negatives. It may be working correctly as it is now.

I want it to read the EEPROM for hours to display if the engine is not running. When running I want it to count up. Ideally I want it to display hours run time to the tenth of an hour.

Thanks for the explaination of why my output would crash with the int turned off.

If you aren't planning to test your engine for 50 days I suggest keeping it simple. Just use millis (). That will give you a time more-or-less accurate to a millisecond. You don't need to yourself keep track of when the seconds tick over. That's like keeping an eye on your clock, and whenever the second hand passes 12 manually counting the minutes. You don't need to. The clock will do that for you.

Well like this it runs and when I short to ground my int pin I get rpm displayed on the serial monitor. When I take the same code and swap out the serial print command for the smartgpu display I do not get any rpm but zero. I also found that it will slow to a near crash when shorting the int if the lcd baud satys at 9600. If I let it run the serial at max it crashes as soon as I short the int a few times. I'm thinking I need to set up one processor to do the rpm counting and a second just to run the touchscreen.

I get your point about the clock but I am unsure how to implement it since I'm using the second counter to figure rpm and turn over the hourmeter.

#include <EEPROM.h>
#include "Arduino.h"
#include <nmea.h>    //include nmea library
#include <SMARTGPU.h>     //include the SMARTGPU library!

SMARTGPU lcd;             //create our object called LCD
NMEA gps(GPRMC);

int touch[2]; //array for x, y coord on touchscreen
char icon[3];  //char array that stores the name of the touched icon
char RPM[5];   //char array for display
char radar[5]; //char array for display
char HOURS[6]; //char array for display
char LAT[7];
char LON[7];
char TENTHS[6];

//variables
volatile int pulse1;
int rpm;
float mph;
float hours;
float lon;
float lat;
unsigned long t; // tenths of hour
int s; // seconds
unsigned long TimerA;
unsigned long TimerB;    
unsigned long startTime = 0;

//pin assignments
int sensor1 = 2;


void setup() { 
  pinMode (sensor1, INPUT);
  Serial.begin(9600);
  for (int i=0; i<6; i++){
    t = EEPROM.read(i);
  }
  attachInterrupt(0, engine, FALLING); //trigger engine function when signal drops
}
void engine(){// rpm add function
  pulse1++;
}


void loop(){
  if (mph < 1){
    mph = 0;
  }
  itoa(mph,radar,10);
  //  lcd.string(140,60,270,80,BLACK,FONT7,TRANS,radar); //need char array called mph
  
  TimerB = ((millis() - startTime) /1000); 
  if (TimerB >= 1){
    s++;
    startTime = millis();
    pulse1 = 0; 
  }
  
  rpm = pulse1; //assumes 1 pulse per revolution
  if (rpm > 4000){
    rpm = 4000;}
  itoa(rpm,RPM,10); //changes number, into char, base10 number
  //  lcd.string(140,20,270,40,BLACK,FONT7,TRANS,RPM); //need char array called rpm

  // hours
  if (s >= 360){
    if (rpm > 0){ // engine must run for tenth of hour to add to t
    t++;}
    s = 0;
    if (t >99999) {
      t = 0;
    }
    for (int i=0; i<6; i++){
      if (t != EEPROM.read(i)) EEPROM.write(i,t);
    }
  }
  
  for (int i=0; i<6; i++){
    TENTHS[i] = EEPROM.read(i);
  }
  hours = t/10.0;
  itoa(hours,HOURS,10);
  //  lcd.string(140,100,270,120,BLACK,FONT7,TRANS,HOURS);


  Serial.write (HOURS);
  Serial.print ("\t");
  Serial.write (RPM);
  Serial.print ("\t");
  Serial.print (s);
  Serial.print ("\t");
  Serial.print (t);
  Serial.print ("\t");
  Serial.print (hours);
  Serial.print ("\t");
  Serial.print (rpm);
  Serial.print ("\t");
  Serial.print (TimerB);
  Serial.print ("\t");
  Serial.println ("char hours, char rpm, sec, tenths, hours, rpm, timerb");
}

This sketch shows how you can calculate RPM without needing to have a second counter "to figure RPM".

volatile int pulse;
//pin assignments
int sensor1 = 2;

void setup() { 
  Serial.begin(9600);
  attachInterrupt(0, engine, FALLING); //trigger engine function when signal drops
} // end of setup

// ISR
void engine ()
{
  // rpm add function
  pulse++;
}  // end isr

unsigned long lastTime;

void loop ()
{
  // is a second up?
  if (millis () - lastTime > 1000)
    {
    // get interrupt data atomically
    noInterrupts ();
    int counts = pulse;
    unsigned long elapsed = millis () - lastTime;
    lastTime = millis ();
    pulse = 0;
    interrupts ();
   
    // display RPM
    float rpm = counts / (elapsed / 1000.0) * 60.0;
  
    Serial.print ("RPM = ");
    Serial.println (rpm);  
      
    }  // end of if

} // end of loop

Feeding in a 3KHz signal I see:

RPM = 179940.06
RPM = 180119.87
RPM = 179880.12
RPM = 180119.87
RPM = 179880.12
RPM = 180119.87
RPM = 179940.06

That's about right because 3000 revolutions a second is 180,000 per minute. So it can keep up with that alright.

What we do is calculate the number of revolutions we had in a time period (millis () - lastTime) divided by 1000 (to make them seconds) and multiplied by 60 (to give R/minute not R/second).