Help with RPM Calculation

Go To Last Post
16 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I have working code but just wanted to ask if anyone knows of a more elgant way to handle a problem I've noticed & had to write special code to handle...

I'm using INT0 to capture governor trigger events from a hall-effect switch... I have 16-bit Timer1 running with a prescaler of 1... I increment an OVF_Counter variable whenever Timer1 rolls over, and clear it to zero within the INT0 ISR.. where I save the Timer1 value at each event. I then calculate an elapsed number of Timer1 counts based on the time deltas as well as the number of OVF events that have occurred.

The problem I'm having, however, is dealing with the rare case in which the governor event happens at the exact same moment as the Timer1 rollover... and thus I enter the INT0 ISR code and grab a very low Timer1 value before the OVF Counter has had a chance to increment. I put in logic to try to catch this situation & compensate for it... but it just seems like a clumsy patch... and was wondering if anyone had any alternate suggestions to deal with this?

Here's a look at my current code: I enable ISR nesting with an sei() call in the middle of the code because the events happen so frequently that I run the risk of miss-timing other ISR events not shown here otherwise. I was going to take that part out so as not to distract from my main question... but just left it for completeness.

volatile signed char Timer1_OVF_Counter;

ISR(INT0_vect)
{
	// Locals
	static unsigned int CurrentTime=0, LastTime=0;
	signed char Timer1_OVF_Counter_Flag;

	// Extract 16-bit Timer1 Value
	CurrentTime = TCNT1;
	
	// Get OVF Count before enabling nested ISR's below
	Timer1_OVF_Counter_Flag = Timer1_OVF_Counter;
	
	// Reset Governor OVF Counter
	Timer1_OVF_Counter = 0;
	
	// Disable this Individual ISR to Prevent Recursion
	ClrBit(EIMSK,INT0);
	
	// Enable Global ISR's Allowing ISR Nesting
	sei();

	// Handle Rare Occurence where Timer1 OVF's at Same Instant as INT0 Event & OVF_Counter Not Yet Incremented
	if ((Timer1_OVF_Counter_Flag==0) && (CurrentTime < LastTime))
	{
		// Calculate Elapsed Timer Counts Assuming 1 Timer1OVF Must Have Occurred even though Value was Zero
		Gov.ETimeCounts = (unsigned long) CurrentTime - (unsigned long) LastTime + 0x10000;
		
		// Set OVF Counter back to zero since it would've been incremented to 1 after the sei() call above
		Timer1_OVF_Counter = 0;
	}
	// Normal Path
	else
	{
		// Calculate Elapsed Timer Counts
		Gov.ETimeCounts = (unsigned long) CurrentTime - (unsigned long) LastTime + (unsigned long) Timer1_OVF_Counter_Flag * 0x10000; 
	}
	
	// Set Current Values to Previous for Next Entry
	LastTime = CurrentTime;
		
	// Now Re-Enable this Individual ISR
	cli();
	SetBit(EIMSK,INT0);
}

ISR(TIMER1_OVF_vect)
{
	// Increment Timer1 OVF Counter
	Timer1_OVF_Counter++;
}

Thanks,
James

Last Edited: Thu. Nov 6, 2008 - 12:54 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

If all cases can be covered in the range of the timer 1 count why not just keep resetting it to 0 in the ISR?

This also saves doing the Current - Last calculation (as Last is always 0)

Cliff

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

They can't though... they can stretch out for 1 1/2 or 2 1/2 timer1 spans for lower RPM's... I'm also using Timer1 for other things that would get messed up if I reset it to zero just for this...

James

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Do not enable interrupts inside an ISR -- you won't like the results. The trick that is done for this is to check the OVF flag in the INT0 ISR. That implies that the Overflow interrupt is pending. The code would look like:

   // Extract 16-bit Timer1 Value
   CurrentTime = TCNT1;
   
   if ( TIFR1 & (1 << TOV1 ) )
   {
       // Overflow interrupt pending - do it now
       Timer1_OVF_Counter++;

       // Clear the interrupt
       TIFR1 = (1 << TOV1 );
   }

   // Get OVF Count before enabling nested ISR's below
   Timer1_OVF_Counter_Flag = Timer1_OVF_Counter;
   
   // Reset Governor OVF Counter
   Timer1_OVF_Counter = 0;

Note that I clear the flag by writing a 1 to that bit. What I've done here is processed the overflow interrupt inside the INT0 interrupt without all that turning on and off of interrupts. Now you can rid of all the sei/cli crap.

Stu

Engineering seems to boil down to: Cheap. Fast. Good. Choose two. Sometimes choose only one.

Newbie? Be sure to read the thread Newbie? Start here!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Stu, thanks a lot! That's a great suggestion!

Sincerely,
James

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hey, could I just ask a "nested ISR" question here while I'm at it...?

If I have two ISR's, call them ISR1 & ISR2... and in each one I have an sei() statement halfway into them... if ISR1 gets called first, and ISR2 triggers immediately after... when ISR1 reaches its sei() statement halfway down & the code jumpts to start processing ISR2... what happens when it reaches the sei() statement within ISR2? Does it come back to ISR1 and finish it first? Or does it just stay in ISR2 & finish & then go back to ISR1? Or does the behavior here depend on the ISR priority?

I hope that was clear...

Thanks,
James

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi James

Firstly, I agree with Stu that you are lining up trouble for yourself by adopting a scheme which involves enabling interrupts within interrupts.

That said, to answer your specific question, what happens when you enable interrupts within the second ISR depends on (a) whether you have cleared the interrupt request bit for the first interrupt and (b) the relative priorities of the interrupts.

If interrupt 1 is a higher priority and you have not cleared the first interrupt request bit, then when you enable interrupts in the second interrupt, you will get a new interrupt 1. Almost certainly what you do not want.

What it will never do is "lose interest" in ISR 2 and go back to continue with ISR 1. It will have to complete ISR 2 before going back to continue with an old ISR 1. It has to do this or your stack would no longer make sense.

I hope that helps (and that you adopt a better scheme.) :)

A

If we are not supposed to eat animals, why are they made out of meat?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Andy, thanks for that explanation.... I can see now a serious flaw in my whole scheme. I had always assumed when my second ISR was given the sei() statement enabling global interrupts it would then go back & finish the first one... but I see what you're talking about... that if it did that, the stack pointer would end up lost. I'll re-write my code to avoid that whole situation... I had just seen times when my ISR's were slightly too long causing delayed Timer reads on other ISR's... so I'd put in the sei() to let them go get processed... but, I'll try to think of another way.

Thanks,
James

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
It will have to complete ISR 2 before going back to continue with an old ISR 1

And the real danger here is if another ISR1 trigger occurs while it's handling ISR2. Already on the stack is the return address to get back to main() from ISR1 followed by the ret address from ISR2 to get back to ISR1. Now another ISR1 interrupt occurs and because interrupts are enabled it interrupts ISR2, and pushes the address to get back to it on the stack and starts to handle ISR1 again. Not only are you eating through memory with nested interrupts but the first instance of ISR1 processing wasn't finished and yet now you are back into it.

Only in the very most carefully thought out circumstances can you ever consider sei() within an ISR - the key thing is that an interrupt of any type should not be expected to occur before the previous processing is guaranteed to have completed.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Cliff, thanks for that explanation also...

Here's one more quick question on the elapsed time calc itself... I currently have:

	static unsigned int CurrentTime=0, LastTime=0;
	
Gov.ETimeCounts = (unsigned long) CurrentTime - (unsigned long) LastTime + (unsigned long) Timer1_OVF_Counter_Flag * 0x10000;

but noticed after doing lots of *.s file comparisons that I can reduce the number of lines of assembly code by using:

	static unsigned int CurrentTime=0, LastTime=0;
	unsigned int tmp;

			tmp = CurrentTime - LastTime;
			
			Gov.ETimeCounts = (unsigned long)tmp + (unsigned long) Timer1_OVF_Counter_Flag * 0x10000;

primarily because of the "tmp" variable being only an "unsigned int" and leaving the "CurrentTime" and "LastTime" variables unsigned ints as well & then only in the second part of the calculation typecast as "unsigned long int"s since there might be several 16-bit timer rollovers involved...

Can you see anything wrong with doing it this way? i.e. If CurrentTime < LastTime and these variables are just unsigned ints will I have any problem?

Thanks,
James

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

jdowns wrote:
Can you see anything wrong with doing it this way? i.e. If CurrentTime < LastTime and these variables are just unsigned ints will I have any problem?
Not that I can see.

Stu

Engineering seems to boil down to: Cheap. Fast. Good. Choose two. Sometimes choose only one.

Newbie? Be sure to read the thread Newbie? Start here!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks Stu... it just kind of baffles me how you can have "unsigned" data types and have the math work out when you subtract a larger number from a smaller number... i.e. when you're producing a negative result... but the code seems to work. I used to have logic that explicitly checked for the Timer OVF condition and would add 65536 to the low number before subtracting... so that my unsigned ints never came up with a negative number... but then found a code excerpt in a book that showed just doing it like I have... not worrying that your "unsigned" datatypes are going to generate a negative number... and I just adopted it when it seemed to work ok. I was afraid to deviate from the formula at all, by changing from "unsigned long" to just "unsigned" since I was never sure why it was working to begin with.

Thanks,
James

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

stu_san wrote:
Do not enable interrupts inside an ISR -- you won't like the results. The trick that is done for this is to check the OVF flag in the INT0 ISR. That implies that the Overflow interrupt is pending. The code would look like:
   // Extract 16-bit Timer1 Value
   CurrentTime = TCNT1;
   
   if ( TIFR1 & (1 << TOV1 ) )
   {
       // Overflow interrupt pending - do it now
       Timer1_OVF_Counter++;

       // Clear the interrupt
       TIFR1 = (1 << TOV1 );
   }

   // Get OVF Count before enabling nested ISR's below
   Timer1_OVF_Counter_Flag = Timer1_OVF_Counter;
   
   // Reset Governor OVF Counter
   Timer1_OVF_Counter = 0;

Note that I clear the flag by writing a 1 to that bit. What I've done here is processed the overflow interrupt inside the INT0 interrupt without all that turning on and off of interrupts. Now you can rid of all the sei/cli crap.

Stu

In order to cover all cases I would change the one line above to something like:

       // Overflow interrupt pending - do it now
       if(CurrentTime < 128)
          Timer1_OVF_Counter++;
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

... but if I'm testing the TOV1 flag and it is set, than wouldn't that imply TCNT1 ~has~ to be small? It seems like that would just be a redundant test... or am I missing something?

Thanks,
James

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Oh wait... I think I understand... is that to cover the case in which a Timer1 rollover occurs immediately between the TCNT1 read and the TOV1 test?

James

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I was thinking you were grabbing ICP instead of TCNT1 directly. Although there is a chance of getting caught between your rollover and your flag check as well.