Input Capture Pin Difficulties

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

I have never used the input capture pin before so I decided to make something with it. The project is a simple RPM meter for my drill press.

I have the routines to display my numbers on a 4 digit 7seg and it works well so I have not included that bit of code below. I have a heartbeat led on PA7 so I know when the photo transistor is getting a signal as it can be hard to hold in place.

I think I understand how the ICP works as right now it is setup for falling edge trigger. With my m164p at 8MHz I have Timer1 setup with a 64 prescale yielding a 125kHz timer. Timer1 is 16bit so this would have a resolution of 8us to ~290ms before an overflow. The fastest my drill goes is 30,000RPM (500/sec) so each opto pulse will have 2ms of timer counting.

My problem with my code is that its jittery, and sometimes non respondent, and sometimes it spits out a completely random speed. This was going to be a simple test project but turned into an ordeal. Can anyone help me find what I did wrong?

volatile uint16_t speed_raw, speed_hz, speed_rpm;

void timers_init(void)
{
	/* Set up for input capture on PD6 */
  // Normal mode, OVF @ TOP (0xFFFF)
  // F_CPU/64, noice cancler on ICP
	// Enable ICP interrupt
	TCCR1B = (1<<ICNC1) | (3<<CS10);
	TIMSK1 = (1<<ICIE1);

	/* Set up for 4ms overflow */
  // Normal mode, OVF @ TOP (0xFF)
  // F_CPU/128
	// Enable OVF interrupt
	TCCR2B = (5<<CS20);
	TIMSK2 = (1<<TOIE2);
}

int main(void)
{
	init_peripherals();

	for(;;)
	{
		// Display speed
		Print(speed_rpm);

		// Dont update so fast!
		_delay_ms(1000);
	}

	return 0;
}

// Falling edge on ICP (PD6) detected
ISR(TIMER1_CAPT_vect) {
	// Disable interrupts for accurate readings
	cli();
	
	// Heartbeat tester
	PORTA |= _BV(PA7);

	// Read ICR & format speed
	// 8MHz / 64 prescale = 125kHz
        // 118kHz as F_CPU = 7.55MHz actually...
	speed_hz = 118000UL / ICR1;
	speed_rpm = speed_hz * 60U;

	// Reset Timer 1
	TCNT1 = 0x0000;

	// Turn interrupts back on
	sei();
}
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Do not put cli()/sei() in ISRs. Global interrupts are automatically disabled/re-enabled by the AVR.

Do not reset the timer in the ISR. Doing so adds jitter to the value, particularly since you are doing a fair amount of math before you reset it. Keep track of what the previous value was and subtract it from the current value.

Regards,
Steve A.

The Board helps those that help themselves.

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

Also, you are doing the full calculations every "hit", and in a manner that loses precision. Do the multiplies first, and then the divides. Watch for overflow. Re-calculate based on totals or averages a few times per second.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

Well I got the project working great now. I took a look at "easy rpm" in the projects page but it was coded for CodeVision and after I changed the function registers to avr-gcc's convention the code only worked with optimization disabled. Their was also a bit of code that was not so useful.

I re-wrote and commented everything so take a look at let me know if its any good!

Thanks!

Attachment(s): 

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

Quote:
the code only worked with optimization disabled
My guess, without looking at the code, is that some variable(s) need to be defined as volatile.

Regards,
Steve A.

The Board helps those that help themselves.

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

Yea their is some differences in variable optimizations between compilers but I just used the un-optimized skeleton to re-work my project.

This is the first real project I documented fully and was just hoping to get some feedback on if its any good. I would love to contribute some projects but I cant stand when their horribly put together or undocumented.

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

You are still zeroing out the timer inside the ISR. As I said earlier, don't do that. Keep the previous value and subtract it from the current value.

Regards,
Steve A.

The Board helps those that help themselves.

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

I don't see what your getting at. if I keep the previous value and then subtract it would make no difference in my linked program as the only thing timer0 is doing is a timeout to turn off the display. If its only accurate to 1s with jitter I don't care at all.

On top of that if I was doing something else with it the capture trigger is completely random if what I am sensing is changing speed so subtracting the previous timer value would still be useless.

Can you post some pseudo code to show what your getting at as I'm obviously not getting it.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
ISR(TIMER1_CAPT_vect) { 
static unsigned int curr,prev,diff;    
   // Heartbeat tester 
   PORTA |= _BV(PA7); 

   // Read ICR & format speed 
   // 8MHz / 64 prescale = 125kHz 
        // 118kHz as F_CPU = 7.55MHz actually... 
   curr =ICR1;
   diff = curr  - prev;
   prev = curr;
   speed_hz = 118000UL / diff; 
   speed_rpm = speed_hz * 60U; 

} 

Actually I'd place the calculations outside of the ISR. You'll also find the value will jump around a bit so you probably want a bit of filtering on the value to make it readable. You can also re-arrange the math so that you get rid of the * 60 operation - work that into your constant.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
ISR(TIMER1_OVF_vect)
{
	// Remove one from timeout
	// In the capture vector I have 5 as the timeout
	// limit, Timer 1 overflows every 524ms so if no
	// new data clear the display in 2.6s
  if (INPUT_TIMEOUT > 0)
		INPUT_TIMEOUT--;
}

ISR(TIMER1_CAPT_vect)
{
	// Add one to timeout so we know
	// their is new data to display
	// Reset Timer1 for next capture
	if(INPUT_TIMEOUT < 5)
		INPUT_TIMEOUT++;
  TCNT1 = 0;

	// Remember POS between calls
	static uint8_t POS;
  MEASURED[ POS ] = ICR1;
  POS++;
  if (POS >= NumMeasures)
		POS = 0;

	// Blink LSB's dp to show photosensor is recieving
  if (ICP_ACTIVE) 
		SEG_BCD[0] &= ~(dp);
}

In the new code the calculations are done outside the interrupts. For filtering the reading is the average of the current two and their is also a display delay so you can read the number and the LSB does not 'flicker'

I should work the *60 into the constant, I can make it longer in the comments to show whats going on. (I wanted it to make as much since as possible)

with:

curr = ICR1; 
diff = curr  - prev; 
prev = curr; 

I do not think I need to even bother with clearing ICR1 as if I am in the capture ISR its because the ICR1 has new data. The timer is another matter if I where to use:

curr = TCNT1; 
...

I assume to reset the counter you say TCNT1 = diff; but what If your curr is smaller then prev then diff is negative, or if curr is large and prev is small then diff is large. I want my timer is be completely reset on input capture so that it counts 5 times (2.6s) before turning off the display.

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

You're really missing the point! You don't need to touch TCNT1 - the input capture hardware copies the current TCNT1 value to ICR1. You only need to calc the difference between two captures. Since the vars are unsigned, you can't get negative values. Sit down with a pencil and paper and pretend you're the AVR, you'll see the calculation works out. This has been discussed many times before.

For your input timeout, when you get a capture load the timeout variable with a value, 5 I think you've chosen. Then in the overflow isr, decrement this variable to 0, when it gets to 0, then you've timed out.

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

Quote:
if I keep the previous value and then subtract it would make no difference in my linked program as the only thing timer0 is doing is a timeout to turn off the display.
What does timer0 have to do with it? You want this:

ISR(TIMER1_CAPT_vect)
{
    static uint8_t POS;
    static uint16_t previous;
    uint16_t current = ICR1;
    MEASURED[ POS ] = current - previous;
    POS++;
    previous = current;
}

With your method, TCNT1 may have advance an unknown number of ticks. By zeroing out the timer, the value you end up measuring is the time between when you zeroed it out and the next edge, instead of measuring the time between edges. This is especially important since the compare interrupt could have been triggered at the start of you timer 0 interrupt which would delay the capture interrupt a considerable amount.

Regards,
Steve A.

The Board helps those that help themselves.

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

I know this is an old thread but can someone explain this to me? I'm working on a tachometer and the hole subtracting current from previous sounds like it would make sense, and be the way to go, but for the life of me i can not get the calculations on paper to come out right.
Any help? Thanks In advance.

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

Current: 300
Previous:100
Elapsed: 200

Current: 300
Previous:1000
Elapsed: -700.

But it is an unsigned value. -700 = 0xfd44 = 64836. Elapsed = 64836.

Let's do one more, that can be counted on the fingers:

Current: 1
Previous: 65535

So, it counted from 65535 to 0 to 1, so two ticks elapsed. 1 - 65535 = -65534 = 0x0002 (in 16 bits) = 2.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

Thanks for the prompt reply, admittedly i still don't entirely understand, the math makes sense, what i don't understand is when it's doing the rpm calculation how a value of 64836 will give the same output as 300? I must be missing something but i'm not too worried, sometimes i just have to learn more about other aspects for the larger picture to fall into place. All in all i'm pretty sure it works but here is the problem... after about 1-3 calculations, depending my displays no longer update. The ISR is still firing cause i have an LED toggled and it still blinks. But nothing updates after displaying only a value or 2. Any help would be greatly appreciated, my code is just a modified version of the above.

//DLM 2010- Tachometer, for ATMEGA32 just Uses 4MHZ crystal, peter fluery lcd library.

/*********************************************
              		Includes
*********************************************/
#include 
#include 
#include 
#ifndef F_CPU
#define F_CPU 4000000UL
#endif
#include 
#include "lcd.h"

/*********************************************
              		Macros
*********************************************/
#define nop()  __asm__ __volatile__("nop")
// Info on the Input Capture Pin (PD6)
#define ICP_ACTIVE			!(PIND & 0x40)
// How many measurements to average over?
#define  NumMeasures		2


/*********************************************
              Global Variables
*********************************************/
uint32_t MEASURED[NumMeasures];
uint8_t  INPUT_TIMEOUT;
uint8_t  SEG_BCD[4];
uint8_t  RAW_BCD[4];
uint16_t SPEED;
uint8_t  TMP;
char rpmChar[4];


/*********************************************
        Interrupt Service Routines
*********************************************/

ISR(TIMER1_OVF_vect)
{
	// Remove one from timeout
	// In the capture vector I have 5 as the timeout
	// limit, Timer 1 overflows every 524ms so if no
	// new data clear the display in 2.6s
	if (INPUT_TIMEOUT > 0)
		INPUT_TIMEOUT--;
}
// NEW ISR <----------Locks up, tried with and w/o TCNT1=0;
ISR(TIMER1_CAPT_vect)
{
	if(INPUT_TIMEOUT < 5)
		INPUT_TIMEOUT++;
	TCNT1 = 0;
    static uint8_t POS;
    static uint16_t previous;
    uint16_t current = ICR1;
    MEASURED[ POS ] = current - previous;
    POS++;
    previous = current;

	// Blink LSB's dp to show pulse is being recieved
	if (ICP_ACTIVE) 
		PORTD ^= (1<<5);
} 
/*	Old ISR <---------WORKS
ISR(TIMER1_CAPT_vect)
{
	// Add one to timeout so we know
	// their is new data to display
	// Reset Timer1 for next capture
	if(INPUT_TIMEOUT < 5)
		INPUT_TIMEOUT++;
		TCNT1 = 0;

	// Remember POS between calls
	static uint8_t POS;
	MEASURED[ POS ] = ICR1;
	POS++;
	if (POS >= NumMeasures)
		POS = 0;
	ICR1 = 0;

	// Blink LSB's dp to show pulse is being recieved
	if (ICP_ACTIVE) 
		PORTD ^= (1<<5);
}
*/
/*********************************************
              Helper Functions
*********************************************/
void mcu_init(void)
{
lcd_init(LCD_DISP_ON);//initialize display   lcd_clrscr();	// clear display and home cursor
lcd_puts(" ENDER355 RULES \n");
_delay_ms(2000);					//Display splash screen for 2sec
	lcd_clrscr();

	// Initilize Timer 1
	// Normal mode, F_CPU/64 = 62HZ sampling rate
	// Input capture noise cancler
	// ISR on input capture & overflow
	TCCR1B = (1<<ICNC1) | (3<<CS10);
	//TIMSK1 = (1<<TOIE1) | (1<<ICIE1); //Mega324p
	TIMSK = (1<<TOIE1) | (1<<TICIE1);	//Mega32

	// Global enable interrupts
	sei();
}

void calculate_speed(void)
{
	uint32_t SUM;

	// Sum of MEASURED[0..NumMeasures]
	SUM = 0; // Clear previous value
	for (TMP = 0; TMP < NumMeasures; TMP++)
		SUM += MEASURED[TMP]; 

  // Derive the speed in RPM 
	// (125kHz * 60sec) / AVG
  SPEED = (uint32_t)(62500UL * 60U * NumMeasures) / SUM;
}

void display_Led(void)
{
	if(SPEED >= 7000)
	{
		PORTC = 0xFF;
	}
	else if(SPEED >= 6000)
	{
		PORTC |= (1<<0);
		PORTC |= (1<<1);
		PORTC |= (1<<2);
		PORTC |= (1<<3);
		PORTC |= (1<<4);
		PORTC |= (1<<5);
		PORTC &= ~(1<<6);
	}	
	else if(SPEED >= 5000)
	{
		PORTC |= (1<<0);
		PORTC |= (1<<1);
		PORTC |= (1<<2);
		PORTC |= (1<<3);
		PORTC |= (1<<4);
		PORTC &= ~(1<<5);
		PORTC &= ~(1<<6);
	}
	else if(SPEED >= 4000)
	{
		PORTC |= (1<<0);
		PORTC |= (1<<1);
		PORTC |= (1<<2);
		PORTC |= (1<<3);
		PORTC &= ~(1<<4);
		PORTC &= ~(1<<5);
		PORTC &= ~(1<<6);
	}
	else if(SPEED >= 3000)
	{
		PORTC |= (1<<0);
		PORTC |= (1<<1);
		PORTC |= (1<<2);
		PORTC &= ~(1<<3);
		PORTC &= ~(1<<4);
		PORTC &= ~(1<<5);
		PORTC &= ~(1<<6);
	}
	else	if(SPEED >= 2000)
	{
		PORTC |= (1<<0);
		PORTC |= (1<<1);
		PORTC &= ~(1<<2);
		PORTC &= ~(1<<3);
		PORTC &= ~(1<<4);
		PORTC &= ~(1<<5);
		PORTC &= ~(1<<6);
	}
	else if(SPEED >= 1000)
	{
		PORTC |= (1<<0);
		PORTC &= ~(1<<1);
		PORTC &= ~(1<<2);
		PORTC &= ~(1<<3);
		PORTC &= ~(1<<4);
		PORTC &= ~(1<<5);
		PORTC &= ~(1<<6);
	}
	else if(SPEED < 1000)
	{
		PORTC = 0x00;
	}
	if (SPEED > 3200)
	{
		PORTC |= (1<<7);	//Shift Light
	}
	else
		PORTC &= ~(1<<7);


}
void display_Lcd(void)
{
	// INPUT_TIMEOUT is > 0 so their is new data avalible
  if (INPUT_TIMEOUT)
	{ 
  	// If SPEED is <= 9999 then display
    if (SPEED <= 9999)
		{
     	// Convert SPEED into a LCD compatible string
		if(SPEED > 0)
			itoa(SPEED, rpmChar, 10);

		lcd_gotoxy(8,0);
		lcd_puts("OK");

		lcd_gotoxy(5,1);
		if(SPEED >= 1000)
		{
			lcd_puts(rpmChar);
		}
		else if(SPEED >= 100)
		{
			lcd_putc('0');
			lcd_puts(rpmChar);
		}
		else if(SPEED >= 10)
		{
			lcd_puts("00");
			lcd_puts(rpmChar);
		}
		else if(SPEED < 10)
		{
			lcd_puts("000");
			lcd_puts(rpmChar);
		}
		display_Led();
    }
  } 

	// INPUT_TIMEOUT was zero so turn off display
	else
	{
		lcd_gotoxy(8,0);
		lcd_puts("NC");
		lcd_gotoxy(5,1);
		lcd_puts("0000");
	}

}

/*********************************************
             Main Program Entry
*********************************************/
int main(void)
{
	DDRD |= (1<<5);
	DDRC = 0xFF;
	PORTD &= ~(1<<5);
	mcu_init();	
	
	lcd_puts("Status:\nRPM:");

  	for (;;)
	{
 		calculate_speed();
   		display_Lcd();

		// Dont update the disp so fast!
		_delay_ms(100);
	}

	return 0;
}

Sorry for posting the whole thing but i figured it'd help anyone generous enough to take a peek for me. It displays the Status, OK or NC in the first line depending on weather or not it's getting a pulse and the RPM on the second line as well as an 8LED bar, 1000rpm per LED. Thanks in advance and thanks to theusch for the explanation.

Last Edited: Fri. Nov 12, 2010 - 06:35 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Also i am using AVR Studio 4 and have the default optimization of 0s selected.

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

No, do not reset TCNT1.

Your "input timeout" stuff is funky, and I suspect that at speed you are getting a number of increments every timer period. Simply get rid of it for now, and just guard against any divide-by-0.

As a sanity check, get rid of all the averaging. Merely display the latest time between hits in timer ticks, say every 100ms. Using a signal generator or the like, there will be some bobble but the high digits should stay steady.

Using a left-justified itoa() will give you weird results when going to numbers with fewer significant digits. Clear the buffer each time. And for pity's sake make your buffer big enough. It isn't now...

The program boils down to the dozen lines below, once you have your inits done:

unsigned char buffer[22];  //wider than the screen so a whole line can be built
volatile unsigned int period; // timer ticks
unsigned int frog;			// work area after atomic access
...

ISR(TIMER1_CAPT_vect)
{
static uint16_t previous;
uint16_t current = ICR1;

    period = current - previous;
    previous = current;
}
... 
while (1)
	{
	_delay_ms(100); 
	lcd_clrscr(); 
	lcd_gotoxy(0,0); // the clear may already do this
	cli();
	frog = period; // assure atomic access
	sei();
	itoa(frog, buffer, 10);
	// alternate, for right-justify  sprintf (buffer, % 5u, period);
	lcd_puts(buffer); 
	}
...

Yes, certainly you can add your LED blinker as a sanity check.

Once the above is in place, I'd suggest leaving it there during dev, so then you always have the sanity of the raw period that should be proper.

Low-speed check is simply a matter of counting overflows. If more than one between samples then you are stopped, or at a very low rate. Add one in the overflow ISR; clear in the hit ISR. nearly done, and nearly that simple. You could, of course, change ranges but when you do the arithmetic you find you have plenty of range and resolution with a 16-bit timer with an appropriately-selected prescaler.

Bar graph? An awful lot of lines. Not necessarily inefficient, but I'd lean more towards

unsigned char krpm;

krpm = rpm / 1000;
if (krpm > 7) krpm = 7;
krpm = 7 - krpm;
PORTC = 0xff << krpm;  // or maybe >>

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

Thanks again for the replies the tachometer was working great. Recently though i fried my atmega32 experimenting with it, so i was going to order another one but would rather use the attiny2313 or other micro that i have. Problem is: not enough pins. i have i think 27 LEDs, bar graph style and there are a couple other things i would like to hook up. I have a couple LM3914's and those can be cascaded but if memory serves they use voltage as the input.. i haven't looked at the datasheet for my micros lately but i don't think they have a DAC and if they did wouldn't it take to many clock cycles on a 4MHZ oscillator that it could miss the rpm interrupt? Any help or information on ICs that i could possibly use to lessen the pins required for running these LEDs would be great.

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

When you get an input capture, just reload the input timeout var with a number. Then if you no input captures for x overflows, the count will decrement to zero and you can zero the rpm value or whatever. You can sample the capture periods at a slow rate- theres only a human reading it and he's not too fast. I filter the values so you dont get jitters and only display in units of 100 rpm. An engine changes its rpm so dont expect to read down to 1rpm. For the output you can use lm3914s if you so desire, but it is easier just to use port pins. Use a 74hc595 if you want more outputs. For a dac you can use the pwm feature with a resistor and capacitor- one dac sir. Another gotcha if you read rpm from the coil or inductive pickup you get ringing. The way around that is to use a compare channel that gets loaded with a lockout time when you get a capture, disable the capture. When the compare channel times out, cear the capture i terript flag and reenable captures.