can somebody tell me what`s wrong with my code, i am using it to calculate rpm using incremental encoder

long counter=0;
 long  rpm = 0, z =0;
 long  ini_time = 0;
long  end_time=0;

void setup(){
  attachInterrupt(0, A_RISE, RISING);


void loop(){


void A_RISE(){
  counter ++;
  if (counter == 1)
    ini_time = micros();
  else if (counter>470){ //470 is ticks per revolution
    z = counter;

    counter =0;

void rpm_counter(int x){


  • You need to tell us more about: what currently happens with your program, what you would expect instead.
    – jfpoilpret
    May 31 '15 at 6:19
  • it does not give any output in serial monitor @jfpoilpret
    – Adnan
    May 31 '15 at 6:24
  • Can you clarify your wiring: which Arduino pin did you wire your encoder to?
    – jfpoilpret
    May 31 '15 at 6:33
  • i attached encoder`s channel A to digital Pin 2 on arduino which is interrupt 0.
    – Adnan
    May 31 '15 at 6:34
  • 1
    Also, you could chnage the init order in your setup() to first Serial.begin(...), that would give time for serial init before triggering encoder interrupts.
    – jfpoilpret
    May 31 '15 at 6:56


Essentially any variable declared within global scope (anything you declare before you get to setup()) MUST have the volatile qualifier if you want to use it within an interrupt service routine otherwise it will not be correctly utilised/implemented/instantiated.

As your code stands, you need to add voltatile to all four of your global variables:

volatile int counter = 0;     // if you're stopping at 470, int is all you need
volatile long rpm = 0;
volatile long z = 0;          // this isn't necessary, see below!
volatile long ini_time = 0;
voltaile long end_time = 0;

Your ISR includes some wasteful instructions, which will increase the time it takes for the routine to complete. As @jfpoilpret suggested, you may be constantly interrupting loop(). Cutting the time down may solve this:

Firstly, z = counter; rpm_counter(z); . Pointless, if I'm honest. Why not just call rpm_counter with counter?

Secondly, rpm_counter() function... it doesn't use the arguement you sent! "x" is not used anywhere within the function, so why are you sending it?

Thirdly, even if my second point wasn't valid rpm_counter() doesn't really need to be a function in its own right. Unless you're going to be calling it from lots of different places, just inline the contents of the function to into A_RISE():

else if (counter>470)
{ //470 is ticks per revolution
    end_time = micros() - ini_time;
    rpm = (60*10^6) / end_time;
    counter = 0;

That could save a great deal of execute cycles.

  • “any variable declared within global scope [...] MUST have the volatile qualifier if you want to use it within an interrupt service routine”. This is incorrect. The volatile qualifier is needed if the variable is going to be used both in the ISR and in the main program. OTOH, if the variable is only used inside the ISR, it should be static local rather than global. Jun 1 '15 at 9:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.