current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Arduino Stack Exchange is a question and answer site for developers of open-source hardware and software that is compatible with Arduino. It's 100% free, no registration required.
up vote -1 down vote favorite

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);

  Serial.begin(115200);
}//setup


void loop(){

    Serial.println(rpm);
}

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

    counter =0;
  }
}

void rpm_counter(int x){

  end_time=micros()-ini_time;
  rpm=(60*10^6)/end_time;

}
share|improve this question
    
You need to tell us more about: what currently happens with your program, what you would expect instead. – jfpoilpret May 31 at 6:19
    
it does not give any output in serial monitor @jfpoilpret – Adnan May 31 at 6:24
    
Can you clarify your wiring: which Arduino pin did you wire your encoder to? – jfpoilpret May 31 at 6:33
    
i attached encoder`s channel A to digital Pin 2 on arduino which is interrupt 0. – Adnan May 31 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 at 6:56

1 Answer 1

up vote 0 down vote

Volatile.

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.

share|improve this answer
    
“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. – Edgar Bonet Jun 1 at 9:52

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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