Understanding Code, not working and working

Hi every one

As I'm almost new to Arduino but have some experience in programming both C++ and Python although it isn't much but it is enough at least to help me understand most of the code. But I ending up from time to time not understanding why it not working and for that matter even working :confused:

For the moment I'm testing code to read RPM signals from Computer fans and used the code in Arduino playground, worked well but here's my problem. I'm trying to move the code to a function that is only called every 4th second... Well i don't get it right the values I get from the function isn't correct. Including code...

perhaps a pair of fresh eyes can see what I'm doing wrong :frowning:

// read RPM
 // Include these 
 #include <SPI.h>
 #include <LiquidCrystal.h>
 
// initialize the library with the number of the sspin 
//(or the latch pin of the 74HC595) and all variables
 LiquidCrystal lcd(10);
 String strOne, strTwo;
 int half_revolutions = 0;
 int rpm = 0;
 unsigned long lastmillis = 0;


 void setup()
 {
   lcd.begin(16, 2);
   lcd.print("Fanspeed: FAN 1");
   attachInterrupt(0, rpm_fan, FALLING);
   strOne = String();
   strTwo = String(" V/min   ");
 }

 void loop()
 {
   showTemp();
   delay(4000);
   fanRPM();
   delay(4000);
   
 }
 
 void showTemp()
 {
   lcd.setCursor(0, 0);
   lcd.print("Showing Temp    ");
   lcd.setCursor(0, 1);
   lcd.print("45.34C water in");
 }
 
 void fanRPM()
 {
   if(millis() - lastmillis == 1000)
   {
     detachInterrupt(0);
     rpm = half_revolutions * 30 / 4;
     strOne = rpm + strTwo;
     lcd.setCursor(0, 0);
     lcd.print("Showing Fan 1");
     lcd.setCursor(0, 1);
     lcd.print(strOne);
     half_revolutions = 0;
     lastmillis = millis();
     attachInterrupt(0, rpm_fan, FALLING);
   }
   
 }

 // this code will be executed every time the interrupt 0 (pin2) gets low.

 void rpm_fan()
 {
   half_revolutions++;
 }
 int half_revolutions = 0;

This variable is referenced in the ISR and in other functions. Therefore, it needs to be volatile.

A few things to look at...
half_revolutions should be volatile.

No need to detach/attach interrupt handler every time. Just disable interrupts while reading half_revolutions then enable them again.

Why are you messing around with Strings (String strOne, strTwo;) to concatenate text when it can just as easily be printed without.

rpm = half_revolutions * 30 / 4; needs explaining.

if(millis() - lastmillis == 1000) means the code may miss being called once and will never be called again. Use if(millis() - lastmillis >= 1000) instead.

Changed half_revolution to Volatile = Works :slight_smile:

removed strOne = rpm + strTwo, Changed lcd.print(rpm + strTwo); work perfect

rpm = half_revolutions * 30 / 4; needs explaining. , Well here's one problem still, when I executed the original code for the rpm meter it shows correct rpm of my fan. But when I moved it to a function it's value went of the roof and is now over 17000rpm I know it spins at 2250rpm give or take a few 100rpm. The 4 was an attempt to make it correct...

I still can't get it to show correct value of the fan without adding a number to be divided with

Disable/enable attach, not quite sure where or how I would change the coding for that :frowning:

The code is untested but you should get the idea.
rpm = half_revolutions * 30 / 4; would work if you called the fanRPM routine every 4 seconds but your main loop is only calling it every 8+ seconds and with the if(millis() - lastmillis == 1000) it will only get run once a second exactly.

// read RPM
// Include these 
#include <SPI.h>
#include <LiquidCrystal.h>

// initialize the library with the number of the sspin 
//(or the latch pin of the 74HC595) and all variables
LiquidCrystal lcd(10);

volatile int half_revolutions = 0;
volatile unsigned long lastmillis = 0;
int rpm = 0;


void setup()
{
  lcd.begin(16, 2);
  lcd.print("Fanspeed: FAN 1");
  attachInterrupt(0, rpm_fan, FALLING);
}

void loop()
{
  showTemp();
  delay(4000);
  fanRPM();
  delay(4000);
  
}

void showTemp()
{
  lcd.setCursor(0, 0);
  lcd.print("Showing Temp    ");
  lcd.setCursor(0, 1);
  lcd.print("45.34C water in");
}

void fanRPM()
{
  if(millis() - lastmillis > 1000)
  {
    noInterrupts();
    rpm = half_revolutions;
    half_revolutions = 0;
    interrupts();
    rpm = ((millis() - lastmillis) / 1000) * (rpm / 2);
    lcd.setCursor(0, 0);
    lcd.print("Showing Fan 1");
    lcd.setCursor(0, 1);
    lcd.print(rpm);
    lcd.print(" V/min   ");
    lastmillis = millis();
  }
  
}

// this code will be executed every time the interrupt 0 (pin2) gets low.

void rpm_fan()
{
  half_revolutions++;
}

fanRPM already determines how often to do work (albeit incorrectly), so you
should simply call it everytime round loop().

The basic model is everything done in or called from loop() should test whether
something needs doing and if so do it and then return immediately.

Try not to use large delays anywhere in your code, its not the way to write anything
but the simplest sketches.