Number of pulses Zeroise when no pulse pick up

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

I am counting the pulses picked up from a coil and monitoring on Br@y Terminal. When there is no pick up of pulses for a few seconds, the pulse quantity just zeroises.  tacho_block just ensures there is only one addition when the pin on PortB0 goes High for a few milliseconds.   tacho_pulse is declared as integer. tacho_pulse doesnt show up anywhere else except in RS-232 string.

 

        if(((PINB)&(0x01))==1)

        {

             if(tacho_block ==0)

             {

                 tacho_block = 1;

                 tacho_pulse++;

              }

        }

        else

             tacho_block = 0;

 

How and why does the tacho_pulse zeroise  when there is no  pulse pick up for few seconds? (when motor stopped)  All other quantities remain unchanged .

Monitored Vcc on CRO, is steady, so chance of controller reseting

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

Use ext int or pcint to sense and count pulse.
.
MG

I don't know why I'm still doing this hobby

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

The same PORT pin is used as Input Capture also for Tacho speed measurement.

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

My guess is the reason is in the part of the code you haven't attached. My suggestion is: attach the complete code.

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

I think you're going to need to show us all your code.

 

One thought...if tacho_pulse is uint_8t there is nothing to stop it from rolling over from 255 to zero.

 

Or, and this is a favourite for odd variable behavior, you need 'volatile' somewhere.

'This forum helps those who help themselves.'

 

pragmatic  adjective dealing with things sensibly and realistically in a way that is based on practical rather than theoretical consideration.

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

 

How to properly post code: http://www.avrfreaks.net/comment...

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

And, in addition to the above, if there is some sort of signal averaging going on, (not yet shown), then obviously the average goes to zero when the pulse stream stops.

 

JC

 

Edit: Typo

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

tacho_pulse is declared as integer so no roll over at 255, I am just waving a magnet across the pick up coil. Neither am I averaging. This happens only when input capture pin is enabled. Input capture function is used to calculate the speed of transport belt.. When the motor stops and speed is Zero, Tacho_speed zeroises and Tacho_pulse also zeroises.  Tacho_pulse is no way connected to the speed calculation. It is a seperate entity.

 

If the Input capture is disabled, the Tach_pulse value holds.

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

K.Chandramohan wrote:

tacho_pulse is declared as integer so no roll over at 255...

 

OK, so it'll roll over at 65,535.

 

This is perfectly valid C code...

 

int my_variable;

void main(void)
{
	while (1)	{
		my_variable++;
	}
}

 

...and will run and run and run until the end of time.

'This forum helps those who help themselves.'

 

pragmatic  adjective dealing with things sensibly and realistically in a way that is based on practical rather than theoretical consideration.

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

Oh, and I repeat...

 

SHOW US ALL YOUR CODE.

'This forum helps those who help themselves.'

 

pragmatic  adjective dealing with things sensibly and realistically in a way that is based on practical rather than theoretical consideration.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
/* TACHO (ATMega8 at 8 MHz) 
  todo- 1.set dia 2.i2c-belt speed & pulse count to main board by I2C	*/

#include <inttypes.h>
#include <avr/io.h>
#include <avr/interrupt.h>
#include <avr/eeprom.h>
#include <string.h>
#include <stdio.h>
#include <stdlib.h>
//#include <twi_slave.h>
//#include <util/atomic.h>


#define		GRN_on 	 	PORTD &= ~(1<<3);
#define 	GRN_off		PORTD |= 1<<3;

#define		RED_on 	 	PORTD &= ~(1<<4);
#define		RED_off		PORTD |= 1<<4;

	
 

volatile unsigned char  update,tick,flash,tacho_block,seconds;
volatile unsigned char  *tx_char,Tx_Buffer[80],Rx_Buffer[20],*Rx_char,Rx_Wr_Index,serial_transmit;

volatile signed int	tacho_pulse,circumference,Timer_val,period,rev,tacho_speed,diameter;

	



void update_delays(void);
void tacho_input(void);
void belt_speed(void); 

void uart_txn(void);
void uart_data(void);
void string_transfer(void);
void USART_Init(void);


SIGNAL(SIG_OVERFLOW2)							//timer 2 overflow  isr
{
	cli();
	TCNT2 = 0x00;						//7D;							
	tick++;
	update = 1;	
	sei();
}
	
ISR(TIMER1_CAPT_vect)
{
	cli();
	Timer_val = ICR1L;
	Timer_val|=(ICR1H<<8);	
	TCNT1 = 0;
	
	//tacho_pulse++;
	sei();
	
}	


int main(void)
{	
	DDRB = 0xFC;				//FE
	PORTB = 0xFE;				//FE
	
	DDRC = 0x00;													
	PORTC = 0x00;										
	
	DDRD = 0xFF;			//0xC2;  0x82B				// PortD,2to 5 input keys of display,D6 &D7 as as Input enable 
	PORTD = 0xFF;	
	
	OSCCAL = eeprom_read_word(0x0000);		//set OSCCAL to Freq CAL value at 8 MHz
	
	//ADCSRA = 0x07;							// ADC  prescalar--8MHz/64 = 125KHz
	//ADCSRA = (ADCSRA |0x98);					// ADC  Set ADEN,ADIF & ADIE	
	
	TCCR2 =  (1<< CS20)|(1<< CS21)|(1<< CS22);				//Timer 2 settings;	prescaler: f = 8Mhz, fixed to 1/1024	= 7812.5
	TIMSK = (1<<TOIE2);					//sbi(TIMSK, TOIE2); enable timer 2 interrupt
	
	TCCR1A = 0;TCNT1=0;							//// Input Capture Pin Setup; 16 bit counter to  zero	
	TCCR1B |= (1<<ICES1)|(1<<CS10)|(1<<CS12);	// get rising edge first; prescale to 1024;			last was 256	
	TIMSK |= (1<<TICIE1)|(1<<TOIE1);	// enable ICP interrupt; enable timer 1

	
	sei();
	//i2c_init();
	//TWI_slave_init();
	USART_Init();
	diameter = 300;
		
		
	
	
	__main:
	update_delays();
	uart_txn();
	tacho_input();
	belt_speed();
	
	goto __main;	
}



void update_delays(void)   
{
	if(update == 1)
	{
		if(tick > 30)   						//112				 		
		{
			tick = 0;
			flash = 1-flash;
			if(flash ==1)
			{
				RED_on		
			}
			else
			{
				RED_off
			}	
			
			serial_transmit = 1;
				
			/*seconds++;
			if(seconds > 5)
			{
				seconds = 0;				
				serial_transmit = 1;
			}	*/				
		}		
		update = 0;				
	}
}	

void tacho_input(void)
{
	if(((PINB)&(0x01))==1)						//if(((PINB) & (0x01))!=0)
	{
		if(tacho_block == 0)
		{
			tacho_block = 1;
			tacho_pulse++;		
			
		}
		GRN_on				
	}
	else
	{
		tacho_block =0;
		GRN_off
	}

	
}	
	
void belt_speed(void)
{
	circumference = 31.429*diameter;				// circumference x 10 for accuracy calc
	period = Timer_val*1.28;						// freq/1024 = 7812.5 Hz; 1/7812.5 is period of one pulse on Timer = 0.128mSec
	rev = (100000/period);						//rev x 10 for accuracy calc
	tacho_speed = (circumference/1000)*rev;		// speed x 100 	for accuracy calc
	if(tacho_speed < 1)							// to avoid speed in  7 digit figure when stationary
	{
		tacho_speed = 0; 
	}
	/*		
	

	tacho_speed_lo = tacho_speed & 0xFF;		// low
	tacho_speed_hi = (tacho_speed >>8);
	
	write_data[0] = tacho_speed_lo;
	write_data[1] = tacho_speed_hi;	
	
	tacho_pulse_lo = tacho_pulse & 0xFF;		// low
	tacho_pulse_hi = (tacho_pulse >>8);
			
	write_data[2] = tacho_pulse_lo;
	write_data[3] = tacho_pulse_hi;			*/

}		



/*-------------------------------UART------------------------------------------------------------------*/
void uart_data(void)
{
	sprintf(Tx_Buffer,"Dia:%d Pulse:%d circum:%d period:%d rev:%d Speed:%d",diameter,tacho_pulse,circumference,period,rev, tacho_speed);	
	tx_char = Tx_Buffer;	
}


void uart_txn(void)	
{
	if(serial_transmit == 1)
	{
		uart_data();	
		string_transfer();

		serial_transmit = 0;	
	}
}	

void string_transfer(void)
{
	int i;	
	for (i=0; tx_char[i]!='\0'; i++)		// transfer the string
	{ 
		while(!(UCSRA & (1<<UDRE)));	
		UDR = tx_char[i];
		
	}
	while(!(UCSRA & (1<<UDRE)));	
	UDR = 0x0D;								// carriage return 	(to home position)
	while(!(UCSRA & (1<<UDRE)));	
	UDR = 0x0A;								// line feed		(next line)
}	

/*SIGNAL(SIG_UART_RECV)
{
	Rx_char = UDR;
	Rx_Buffer[Rx_Wr_Index] = Rx_char;
	Rx_Wr_Index++;	
	//Rx_cntr++;	
}	*/
	
void USART_Init(void)
{	
	UBRRH = 0;					//Set baud rate       UBRR = (fOSC/16*BAUD) - 1;  then change to hex code
	UBRRL = 0x03;				//set for 115200 baud
	
	UCSRA = 0;	
	//UCSRB = (1<<TXEN)|(1<<RXEN)|(1<<RXCIE);
	UCSRB = (1<<TXEN);
	UCSRC = (1<<URSEL)|(1<<UCSZ1)|(1<<UCSZ0); 			//8bit data,  Zero parity , 1 stop bit
}	
	

 

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

Is the tacho input pin connected with timer 1 capture pin? Your code enables timer 1 overflow interrupt, but you have no handler for this interrupt. So, if there is no input capture interrupt for 8 seconds, timer 1 overflow occurs and, since there is no overflow interrupt chandler, the microcontroller is reset and the code restarts and tacho_pulse is set to its initial value 0.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
ISR(TIMER1_CAPT_vect)
{
uint16_t curr;
static uint16_t prev;

curr = ICR1;
Timer_val = curr - prev;
prev = curr;

	
	//tacho_pulse++;

}

You don't need the cli() and sei() as the AVR does this already for you. If using input capture,

don't touch TCNT, otherwise the benefits of using the input capture are negated.

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

Kartman wrote:

You don't need the cli() and sei() as the AVR does this already for you.

It's not only not needed, it is just wrong.

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

There's also a myriad of bad and dangerous practices. There's a mix of integer and floating point calcs that's bound to end in tears, no atomic protection of multi-byte variables shared with isrs. it goes on....

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
	__main:
	update_delays();
	uart_txn();
	tacho_input();
	belt_speed();
	
	goto __main;	

Wow that's "old school". Most people these days have let "goto" go and would opt for:

    while(1) {
	update_delays();
	uart_txn();
	tacho_input();
	belt_speed();
    }

Also why have you made every one of your global variables "volatile". The only ones that needed it are those shared between threads of execution.

if(((PINB)&(0x01))==1)	

I'm all for the use of parentheses to show the order of evaluation but this has too many and obfuscates the code. try:

if((PINB & 0x01) == 1)	

Also while this works for bit 0 if it were bit 5 (say) would you really write?:

if((PINB & 0x40) == 0x40)	

Is it not easier simply to use the fact that in C anything non0 is true:

if (PINB & 0x40)

which achieves the same effect.

/*SIGNAL(SIG_UART_RECV)
{
	Rx_char = UDR;
	Rx_Buffer[Rx_Wr_Index] = Rx_char;
	Rx_Wr_Index++;	
	//Rx_cntr++;	
}	*/

I know that is commented but it is dangerous code - there is no check for the index exceeding the length of the buffer.

void string_transfer(void)
{
	int i;	
	for (i=0; tx_char[i]!='\0'; i++)		// transfer the string
	{ 
		while(!(UCSRA & (1<<UDRE)));	
		UDR = tx_char[i];
		
	}
	while(!(UCSRA & (1<<UDRE)));	
	UDR = 0x0D;								// carriage return 	(to home position)
	while(!(UCSRA & (1<<UDRE)));	
	UDR = 0x0A;								// line feed		(next line)
}	

A few things about that. To save you writing the same code every time you want to send a character doesn't it make more sense to:

void uart_sendchar(char c) {
    while(!(UCSRA & (1<<UDRE)));	
    UDR = c;
}

void string_transfer(void)
{
    int i;	
    for (i=0; tx_char[i]!='\0'; i++)		// transfer the string
    { 
        uart_sendchar(tx_char[i]);
        
    }
    uart_sendchar(0x0D);								// carriage return 	(to home position)
    uart_sendchar(0x0A);								// line feed		(next line)
}	

For 0x0D is more usual to use '\r' and for 0x0A one usually uses '\n'

 

Also if you added the \r\n at the end of the sprintf() you wouldn't need to add them here (because sometimes you may have a string for which you don't want to immediately send \r\n every time):

sprintf(Tx_Buffer,"Dia:%d Pulse:%d circum:%d period:%d rev:%d Speed:%d\r\n",diameter,tacho_pulse,circumference,period,rev, tacho_speed);

In the line:

	tx_char = Tx_Buffer;	

the names here are misleading. I think most programmers would expect TX_Buffer to be the name of an array or at least a pointer to char but "tx_char" is a curious name for what is actually a pointer!

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

And let's not even mention the lack of comments.

'This forum helps those who help themselves.'

 

pragmatic  adjective dealing with things sensibly and realistically in a way that is based on practical rather than theoretical consideration.

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

I'm just quoting this so that when the OP goes and edit's their post we'll have a record of what's changed.

 

 

K.Chandramohan wrote:

/* TACHO (ATMega8 at 8 MHz) 
  todo- 1.set dia 2.i2c-belt speed & pulse count to main board by I2C	*/

#include <inttypes.h>
#include <avr/io.h>
#include <avr/interrupt.h>
#include <avr/eeprom.h>
#include <string.h>
#include <stdio.h>
#include <stdlib.h>
//#include <twi_slave.h>
//#include <util/atomic.h>


#define		GRN_on 	 	PORTD &= ~(1<<3);
#define 	GRN_off		PORTD |= 1<<3;

#define		RED_on 	 	PORTD &= ~(1<<4);
#define		RED_off		PORTD |= 1<<4;

	
 

volatile unsigned char  update,tick,flash,tacho_block,seconds;
volatile unsigned char  *tx_char,Tx_Buffer[80],Rx_Buffer[20],*Rx_char,Rx_Wr_Index,serial_transmit;

volatile signed int	tacho_pulse,circumference,Timer_val,period,rev,tacho_speed,diameter;

	



void update_delays(void);
void tacho_input(void);
void belt_speed(void); 

void uart_txn(void);
void uart_data(void);
void string_transfer(void);
void USART_Init(void);


SIGNAL(SIG_OVERFLOW2)							//timer 2 overflow  isr
{
	cli();
	TCNT2 = 0x00;						//7D;							
	tick++;
	update = 1;	
	sei();
}
	
ISR(TIMER1_CAPT_vect)
{
	cli();
	Timer_val = ICR1L;
	Timer_val|=(ICR1H<<8);	
	TCNT1 = 0;
	
	//tacho_pulse++;
	sei();
	
}	


int main(void)
{	
	DDRB = 0xFC;				//FE
	PORTB = 0xFE;				//FE
	
	DDRC = 0x00;													
	PORTC = 0x00;										
	
	DDRD = 0xFF;			//0xC2;  0x82B				// PortD,2to 5 input keys of display,D6 &D7 as as Input enable 
	PORTD = 0xFF;	
	
	OSCCAL = eeprom_read_word(0x0000);		//set OSCCAL to Freq CAL value at 8 MHz
	
	//ADCSRA = 0x07;							// ADC  prescalar--8MHz/64 = 125KHz
	//ADCSRA = (ADCSRA |0x98);					// ADC  Set ADEN,ADIF & ADIE	
	
	TCCR2 =  (1<< CS20)|(1<< CS21)|(1<< CS22);				//Timer 2 settings;	prescaler: f = 8Mhz, fixed to 1/1024	= 7812.5
	TIMSK = (1<<TOIE2);					//sbi(TIMSK, TOIE2); enable timer 2 interrupt
	
	TCCR1A = 0;TCNT1=0;							//// Input Capture Pin Setup; 16 bit counter to  zero	
	TCCR1B |= (1<<ICES1)|(1<<CS10)|(1<<CS12);	// get rising edge first; prescale to 1024;			last was 256	
	TIMSK |= (1<<TICIE1)|(1<<TOIE1);	// enable ICP interrupt; enable timer 1

	
	sei();
	//i2c_init();
	//TWI_slave_init();
	USART_Init();
	diameter = 300;
		
		
	
	
	__main:
	update_delays();
	uart_txn();
	tacho_input();
	belt_speed();
	
	goto __main;	
}



void update_delays(void)   
{
	if(update == 1)
	{
		if(tick > 30)   						//112				 		
		{
			tick = 0;
			flash = 1-flash;
			if(flash ==1)
			{
				RED_on		
			}
			else
			{
				RED_off
			}	
			
			serial_transmit = 1;
				
			/*seconds++;
			if(seconds > 5)
			{
				seconds = 0;				
				serial_transmit = 1;
			}	*/				
		}		
		update = 0;				
	}
}	

void tacho_input(void)
{
	if(((PINB)&(0x01))==1)						//if(((PINB) & (0x01))!=0)
	{
		if(tacho_block == 0)
		{
			tacho_block = 1;
			tacho_pulse++;		
			
		}
		GRN_on				
	}
	else
	{
		tacho_block =0;
		GRN_off
	}

	
}	
	
void belt_speed(void)
{
	circumference = 31.429*diameter;				// circumference x 10 for accuracy calc
	period = Timer_val*1.28;						// freq/1024 = 7812.5 Hz; 1/7812.5 is period of one pulse on Timer = 0.128mSec
	rev = (100000/period);						//rev x 10 for accuracy calc
	tacho_speed = (circumference/1000)*rev;		// speed x 100 	for accuracy calc
	if(tacho_speed < 1)							// to avoid speed in  7 digit figure when stationary
	{
		tacho_speed = 0; 
	}
	/*		
	

	tacho_speed_lo = tacho_speed & 0xFF;		// low
	tacho_speed_hi = (tacho_speed >>8);
	
	write_data[0] = tacho_speed_lo;
	write_data[1] = tacho_speed_hi;	
	
	tacho_pulse_lo = tacho_pulse & 0xFF;		// low
	tacho_pulse_hi = (tacho_pulse >>8);
			
	write_data[2] = tacho_pulse_lo;
	write_data[3] = tacho_pulse_hi;			*/

}		



/*-------------------------------UART------------------------------------------------------------------*/
void uart_data(void)
{
	sprintf(Tx_Buffer,"Dia:%d Pulse:%d circum:%d period:%d rev:%d Speed:%d",diameter,tacho_pulse,circumference,period,rev, tacho_speed);	
	tx_char = Tx_Buffer;	
}


void uart_txn(void)	
{
	if(serial_transmit == 1)
	{
		uart_data();	
		string_transfer();

		serial_transmit = 0;	
	}
}	

void string_transfer(void)
{
	int i;	
	for (i=0; tx_char[i]!='\0'; i++)		// transfer the string
	{ 
		while(!(UCSRA & (1<<UDRE)));	
		UDR = tx_char[i];
		
	}
	while(!(UCSRA & (1<<UDRE)));	
	UDR = 0x0D;								// carriage return 	(to home position)
	while(!(UCSRA & (1<<UDRE)));	
	UDR = 0x0A;								// line feed		(next line)
}	

/*SIGNAL(SIG_UART_RECV)
{
	Rx_char = UDR;
	Rx_Buffer[Rx_Wr_Index] = Rx_char;
	Rx_Wr_Index++;	
	//Rx_cntr++;	
}	*/
	
void USART_Init(void)
{	
	UBRRH = 0;					//Set baud rate       UBRR = (fOSC/16*BAUD) - 1;  then change to hex code
	UBRRL = 0x03;				//set for 115200 baud
	
	UCSRA = 0;	
	//UCSRB = (1<<TXEN)|(1<<RXEN)|(1<<RXCIE);
	UCSRB = (1<<TXEN);
	UCSRC = (1<<URSEL)|(1<<UCSZ1)|(1<<UCSZ0); 			//8bit data,  Zero parity , 1 stop bit
}	
	

 

'This forum helps those who help themselves.'

 

pragmatic  adjective dealing with things sensibly and realistically in a way that is based on practical rather than theoretical consideration.

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

I wouldn't be using i2c in an industrial setting unless the two chips are on the one pcb.

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

Thanks gentlemen for your feed back. this was a enlighting session for me.

Yes,the tacho input pin  and the timer 1 capture pin are the same.

After handling the Timer1 OV, the problem is solved. Thank you MarekJ71.

 

All feedbacks have been implemented by me barring a few.

 

1. Kartman mentioned---If using input capture, don't touch TCNT, otherwise the benefits of using the input capture are negated.

    I dropped TCNT out and the readings were erratic, so I included it back. Where am I wrong?

 

2. Clawson wrote---- there is no check for the index exceeding the length of the buffer. How do I implement it?

   

      

SIGNAL(SIG_UART_RECV)
{
	Rx_char = UDR;
	Rx_Buffer[Rx_Wr_Index] = Rx_char;
	Rx_Wr_Index++;	
	//Rx_cntr++;	
}	

3.  Kartman again---There's also a myriad of bad and dangerous practices. There's a mix of integer and floating point calcs that's bound to    end in tears, no atomic protection of multi-byte variables shared with isrs. it goes on....   

     Any study material to look at this aspect ?

 

4. Brian Fairchild---I thoroughly enjoyed it.

     

int my_variable;

void main(void)
{
	while (1)	{
		my_variable++;
	}
}

 

Thanks again

 

 

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

K.Chandramohan wrote:
2. Clawson wrote---- there is no check for the index exceeding the length of the buffer. How do I implement it?

Implementing the check is trivial - it's a simple 'if' 

 

Deciding what to do when it happens is a design decision - you have basically 2 choices:

 

  1. Ignore further incoming characters until space becomes available
    (ie, keep the oldest data)
     
  2. Discard some existing characters to make space for the incoming characters
    (ie, keep the newest data)

 

Only you can decide which is the most appropriate for your particular system. 

 

EDIT

 

typo

Last Edited: Wed. Oct 11, 2017 - 12:51 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

During development, I usually add code that shows in the clear way that there is MCU reset, for example two LED-s blinking three times or something like this. It really helps to find out what happens if the device doesn't work not as expected.

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

K.Chandramohan wrote:
How do I implement it?
Well you know from here:

Rx_Buffer[20]

that the buffer is 20 bytes long. So in:

	Rx_char = UDR;
	Rx_Buffer[Rx_Wr_Index] = Rx_char;
	Rx_Wr_Index++;	

you need something like:

	Rx_char = UDR;
	Rx_Buffer[Rx_Wr_Index] = Rx_char;
	Rx_Wr_Index++;
        if (Rx_Wr_Index > 19) {
           Rx_Wr_Index = 0;
        }

which will cause the buffer to "wrap" back round to the start if it reaches the end. Because of this buffers like this are often called "ring" or "circular" buffers. Obviously a problem does exist of you reach Rx_Buffer[19] and wrap back to Rx_Buffer[0] before the contents of  [0] have been used yet. You need to make sure bytes are taken from the buffer fast enough so you don't "catch up" with the tail end of the data. Another approach is to make the buffer larger which gives you more time to get old data processed. Or ultimately you can add a test to see if the writing point has caught up with the reading point and make a decision to simply drop the most recently arrived characters. Or you might prefer to over-write the oldest data on the basis that it is "out of date".

 

To do a lot of this stuff most ring/circular buffers (also known as "FIFOs") have both a write pointer and a read pointer. So you might maintain Rx_Wr_Index as the pointer/index for where the next write will occur and Rx_Rd_Index as a poiter/index for where the next piece of data will be read from. You can then test for the situation where the read and write pointers had collided.

 

Yet another approach (often considered the "best") is that as well as keeping Rx / Wr indices you also keep a count of useable bytes in the buffer. Each time one arrives and is added you increment the count (as well as adjusting the Wr index) and each time you take a byte from the buffer to use you reduce the count (as well as moving the Rd index). An empty buffer then has a count of 0 and a full one will (in this case) have a count of 20.

 

BTW there's tons of existing (good) implementations of ring buffers out on the internet to save you reinventing the wheel each time. For example one is here:

 

http://www.fourwalledcubicle.com...

 

All the code of that is in a single .h file:

 

http://www.fourwalledcubicle.com...

Last Edited: Wed. Oct 11, 2017 - 01:07 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

MarekJ71 wrote:
During development, I usually add code that shows in the clear way that there is MCU reset, for example two LED-s blinking

Yes.

 

It is very important that you consider how you will debug your system as part of the design.

 

I would include:

  • Connections for the debugger.
  • A serial port for diagnostics

 

A wise manager at a client once said,

If you don't have enough resources to dedicate 10% to debug/diagnostics, then you don't have enough resources at all!

 

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

 

K.Chandramohan wrote:
1. Kartman mentioned---If using input capture, don't touch TCNT, otherwise the benefits of using the input capture are negated.     I dropped TCNT out and the readings were erratic, so I included it back. Where am I wrong?  

How do I know what you've attempted? Your code is riddled with defects, so by fixing one, you probably highlight another. Did you understand the implication of what your code was doing vs what mine did? Were there other things to consider?

circumference = 31.429*diameter;

what do you expect this line of code to do? circumference and diameter are declared as a signed int, but you're trying to do floating point. The compiler is going to truncate 31.249 to 31 and do an integer calculation. Is this what you wanted?

 

You've thrown the code together and it sort of works, but what testing have you done? Should you be surprised that there are problems?

There's plenty of online tutorials,books and references on the C language. The classic reference is the K&R book. Google that one.

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

Do re-design your code. It did confusing.
.
MG

I don't know why I'm still doing this hobby

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

Kartman wrote---circumference and diameter are declared as a signed int, but you're trying to do floating point. The compiler is going to truncate 31.249 to 31 and do an integer calculation. 

 

You are very much right.

              

circumference = 31.429*diameter;

diameter= 300; and I have shifted decimal one digit right to the value of Pi. Result on RS232 output gives 9428 ( should have been 9428.7)

circumference = 300mm*Pi = 942.87 is the right value. I get 9428 which meets my requirement.

Tried with different diameter values.

 

Can any one throw light on this floating point Calc in AVR where one has declared quantities as signed Integer. So we know our limitations.

 

Any way thanks for enlightening me as I am self taught and hence prone to errors (probably because I presume)

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

K.Chandramohan wrote:

	circumference = 31.429*diameter;				// circumference x 10 for accuracy calc

Apart from the 'C' issues already pointed out, your value for pi is wrong!!

 

Note that GCC provides a built-in definition of pi for you - so you don't have to:

 

https://www.gnu.org/software/libc/manual/html_node/Mathematical-Constants.html

 

 

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

Yes I2C communication in Industrial environment but within the same metal tube housing. May be CAN would help but IC count increases in the housing.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
circumference = 31.429*diameter;

There are following steps done by the code above:

1. diameter is implicitly converted to float

2. multiplication is done

3. the result is converted to signed int, rounding down occurs

So the result will be almost correct assuming that the precision you get is OK for you. But it could be done better.

circumference = (signed int)(31.415 * diameter + 0.5);

The changes are:

1. correct value of pi

2. 0.5 is added to the result of multiplication, so when the result is converted to int, you get correct rounding to the nearest integer, not always down

3. explicit conversion from float to int

About 3 - You should never use implicit conversion when there can be data loss, for example when converting from long to int (overflow can occur) or from float to int (you lose the factional part and also overflow can occur). The compiler should warn you when there is such implicit conversion and if you use explicit conversion, it means that you probably know what are you doing.

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

Your problem has nothing to do with the AVR - it’s standard C. An integer has no decimal point - you maybe want a single or float type - but be aware of the execution time as floating point takes more compute cycles.

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

K.Chandramohan wrote:
Can any one throw light on this floating point Calc in AVR

It has nothing to with the AVR;  it is standard 'C' behaviour - so the same for any compiler.

 

So see your 'C' textbook for details.

 

Here are some 'C' learning & reference materials for you: http://blog.antronics.co.uk/2011... - including a free online textbook.

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

Kartman wrote:
circumference and diameter are declared as a signed int,

And that's a deliberate, explicit "signed" - not just be default:

K.Chandramohan wrote:

volatile signed int	tacho_pulse,circumference,Timer_val,period,rev,tacho_speed,diameter;

Why on earth would you define them as signed?

 

What meaning can a negative circumference or diameter have??

 

BTW: don't be afraid of whitespace - code is so much easier to read when the separate elements are, well - separated:

volatile signed int	tacho_pulse, circumference, Timer_val, period, rev, tacho_speed, diameter;

 

Personally, I would never have multiple declarations like that;  always give each one its own statement - makes maintenance much easier.

 

Also, with things like circumference or diameter - add a comment to state your units of measure.

 

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

Thanks a lot for the inputs and direction to proceed.