Measure RPM and then speed

I am trying to count pulse with a Hall Sensor (working) and then convert that to RPM and then speed.

No matter what I do the program works for 30 seconds or so and then won’t reenter the loop.

I have tried waiting till I have a certain number of counts and get the same behavior.

I have looked at countless code samples and can’t see what is going wrong here.

#include <Wire.h>   // Wire Library for i2c parts
#include <LiquidCrystal_I2C.h>    // LCD Library
LiquidCrystal_I2C lcd(0x3F,16,2);  // Address 0x3F 16x2 (x,y) zero based index

int ledPin = 13;
int countPin = 2;          // Interrupt Pin 2 or 3 on Uno
volatile int counts;
int currentCounts = 0;
int lastCountTime = 0;
long int rpm = 0;
int oneRevDistMeter = 15;
int speedKmh = 0;

void setup() 
{
  lcd.init();
  lcd.backlight();
  lcd.print("Cruise System On");
  lcd.setCursor(0,1);
  lcd.print("Waiting . . . . . ");
  delay(2000);
  pinMode(ledPin, OUTPUT);  //  
  pinMode(countPin, INPUT_PULLUP);
  attachInterrupt(digitalPinToInterrupt(countPin), countISR, RISING);
  Serial.begin(9600);
}

void loop() 
{
  Serial.println(counts);
  digitalWrite(ledPin,LOW); // turn off count LED
  
if (millis() >= lastCountTime+2000)
{
  detachInterrupt(2);
  currentCounts=counts;
  rpm = (6000*currentCounts)/(millis()-lastCountTime);
  lastCountTime = millis();
  counts = 0;
  speedKmh = rpm*oneRevDistMeter*6/1000;
  lcd.setCursor(0,1);
  lcd.print("Speed =      kmh");
  lcd.setCursor(8,1);
  lcd.print(speedKmh);
  attachInterrupt(digitalPinToInterrupt(countPin), countISR, RISING);
}
}
void countISR() 
{ 
  counts++; //  Counts the magnetic sensor
  digitalWrite(ledPin, HIGH); // flash led on sensor input
}

Perhaps your debug prints at the bottom of loop would show the program is never leaving loop, so it never has a chance to start at the top of loop. Your challenge is to add lines to print values at possible error locations in loop until you see the logic problem.

Paul

Detaching and attaching your interrupt handler is not the usual thing to do. Usually, one disables all interrupts, copies/resets the needed variables, and then enables all interrupts.

  noInterrupts();
  currentCounts=counts;
  counts = 0;
  interrupts();

You want all, or your, interrupt handler(s) disabled for the shortest possible time. Calculating RPM and speed and printing to the LCD take too long to be done with all, or your, interrupt handler(s) disabled.

if (millis() >= lastCountTime+2000)

Addition involving unsigned long time values is NOT a good idea. Subtraction is guaranteed to work (unless the interval exceeds 49+ days). Addition is NOT.

The value stored in lastCountTime is NOT the last time a count happened. You need a better name.

Variables involving time should be unsigned long, NOT int.

  rpm = (6000*currentCounts)/(millis()-lastCountTime);

What value is in currentCounts? 6000 is an int. currentCounts is an int. If currentCounts holds a value of 6 or more, 6000*currentCounts will NOT fit in an int (register).

6000UL is NOT an int, so 6000UL * currentCounts (see how much easier that is to read with some spaces?) will not be stuffed into an int register. It will fit in an unsigned long register, which is what will be used to hold the intermediate result.

Why is rpm a signed value? Are you expecting to measure -3569 RPM?

Thanks a bunch. Will try this all out in the AM. You are dead on that I am outside the useable range for the variables!!!

Will followup with corrected and working code. Thanks again.

PaulS:
Detaching and attaching your interrupt handler is not the usual thing to do. Usually, one disables all interrupts, copies/resets the needed variables, and then enables all interrupts.

  noInterrupts();

currentCounts=counts;
  counts = 0;
  interrupts();



You want all, or your, interrupt handler(s) disabled for the shortest possible time. Calculating RPM and speed and printing to the LCD take too long to be done with all, or your, interrupt handler(s) disabled.



if (millis() >= lastCountTime+2000)



Addition involving unsigned long time values is NOT a good idea. Subtraction is guaranteed to work (unless the interval exceeds 49+ days). Addition is NOT.

The value stored in lastCountTime is NOT the last time a count happened. You need a better name.

Variables involving time should be unsigned long, NOT int.



rpm = (6000*currentCounts)/(millis()-lastCountTime);



What value is in currentCounts? 6000 is an int. currentCounts is an int. If currentCounts holds a value of 6 or more, 6000*currentCounts will NOT fit in an int (register).

6000UL is NOT an int, so 6000UL * currentCounts (see how much easier that is to read with some spaces?) will not be stuffed into an int register. It will fit in an unsigned long register, which is what will be used to hold the intermediate result.

Why is rpm a signed value? Are you expecting to measure -3569 RPM?