faulty code or hardware clock/timing problem?

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

greets,

 

I'm making a tap tempo to MIDI clock converter that takes 1/4 note pulses on the input and outputs MIDI clock. There is however a strange timing problem. The Atmega8A seems to be running a bit faster than it should, (16MHz on an external crystal), the duration between the 1/4 note pulses is being falsely calculated/measured/incrimented. E.g. At 120BPM the duration between two 1/4 notes should be 500 000 microseconds or close. My circuit counts an average value of 495 050 microseconds, which is actually the duration at 121 BPM (Beats per Minute).

 

Here is a short comparison between correct and incorrect values (time between two 1/4 notes at specific BPM):

 

Correct (averaged values):

 

119 BPM - 504 200 uS

120 BPM - 500 000 uS

121 BPM - 495 500 uS

 

Values I'm getting:

 

119 BPM - 500 000

120 BPM - 495 500

121 BPM - 491 000

 

btw. The formula to calculate perfect 1/4 interval is - 60 seconds / BPM.

 

So as you can see from the values above it looks like the the Atmega8a is running a bit fast (1BPM too fast).

 

For testing purposes I have a drum machine sending an audio metronome click to a 555 timer setup in monostable mode, its output goes to pin PD3 (External Interrupt 1 - rising edge) on an Atmega8A. I've tried the 555 timer with 2mS and 100mS (duration) pulses, but that did not make any difference, the problem persisted.

 

I've looked at the output of the 555 timer on an oscilloscope (at 120BPM) and it's a perfect 500 millisecond interval, so I doubt the 555 setup could be problem. I've tried swapping a few 16MHz crystals, no change. Reduced the capacitors from 22pF to 1pf, and even removed them, there was a slight increase in accuracy (ca. 1mS) but that's negligible.

 

I've also tried a simple metronome instead of the drum machine, still the same problem. So I'm not sure what else I could check in the hardware, probably it is the code:

 

#define F_CPU 16000000UL
#include "my_macros.h"
#include "mySerial_atmega8.h"
#include <avr/io.h>
#include <avr/interrupt.h>

volatile uint8_t pulseOn = false; //toggled to true if pulse on pin PD3 (external INT1)
unsigned long qNoteTime = 0; //stores the time between two pulses, in microseconds 

int main(void)
{
	init_io();
		
	while (!pulseOn){}; //wait for first pulse-trigger
		
		TCNT2 = 0; //reset/clear Timer2
		pulseOn = false;			
			
		while (true) {		
				
			while(!pulseOn){ //loop while waiting for pulse, increment qNoteTime
				if(readbit(TIFR, OCF2)){ //OCF2: Output Compare Flag 2 (at 160 ticks/ 10uS)
					TIFR = (1 << OCF2); //Clear the compare flag (inverted latch, write LOGICAL 1!!!)
					qNoteTime += 10; //+10uS
				}			
			}
			
			qNoteTime = 0;	
			TCNT2 = 0; //reset/clear Timer2	
			pulseOn = false;

			mySerial_atmega8::print(qNoteTime);
			mySerial_atmega8::print('\n');
				
						
							
		} //END while true
		
}


void init_io(void){
	TCCR2 = 0;     // clear TCCR2 – Timer/Counter Control Register
	TCCR2 |= (1<<WGM21) | (1<<CS20); //Timer/Counter2 set WGM21(CTC mode) and pre-scaler to F_osc/1
	OCR2 = 160; //Output Compare Register2 to 160 ticks at 1/16MHz = 1uS micro second
	TIFR = (1 << OCF2); //Clear the compare flag (inverted latch, write LOGICAL 1!!!)	
	
	setbit(GICR, INT1); //External INT1 enable, pin PD3
	setbit(MCUCR, ISC11); setbit(MCUCR, ISC10);//INT1 on rising edge
	
	setbit(SREG, 7);// Global Interrupts Enable
	
	mySerial_atmega8::begin(31250, F_CPU);
}


ISR(INT1_vect){ //INT1-PD3 rising edge triggered
	pulseOn = true;
}

 

Thanks for any input

Last Edited: Mon. Feb 23, 2015 - 05:09 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Your 160 should be 159.

0..160 is 161 ticks.

Iluvatar is the better part of Valar.

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

And more generally CTC is always N-1 because 0 counts as a "tick".

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

Thanks for pointing out the mistake. I've changed the OCR2 value to 159, the results improved but they're still off by about 1500/1700uS, here are some values:

 

Timer (OCR2) 159 ticks, qNoteTime = :

498190
498120
498150
498170
498110
498150

 

 

changing to

 

158 ticks:

 

Timer 158 ticks:

501260
501280
501280
501260
501260
501280

501220
501320

 

So the values are either lower or higher than they should be. Not sure if I should continue looking for the bug or just keep the 159 ticks setting and add 1500 uS to qNoteTime.

Last Edited: Mon. Feb 23, 2015 - 04:41 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I'm not sure I follow your logic but aren't the ::print()s part of what you are measuring?

 

I would have thought what you'd want to do if you are already "part way in" to a pulse period when you get back round to the top of the while(true) is to let the current one run to completion then discard that result. Now you are in sync and can do a second, complete measurement. When that is complete then report the result (eating slightly into the next cycle which will be discarded and so on...).

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

clawson wrote:

I'm not sure I follow your logic but aren't the ::print()s part of what you are measuring?

 

You mean that

 

mySerial_atmega8::print(qNoteTime);
mySerial_atmega8::print('\n');

 

is what creates the unexpected values?

 

i.e.

 

			TCNT2 = 0; //reset/clear Timer2	

starts the timer running from 0.

 

but because

 

mySerial_atmega8::print(qNoteTime);
mySerial_atmega8::print('\n');

is "in the way", before

 

if(readbit(TIFR, OCF2))

 

the comparison can not take place while the serial functions are printing the qNoteTime, i.e. the serial print routine takes up the missing ca. 1500 uS?

 

here is the print function, perhaps it does take the ca. 1500 uS to execute:

 

void mySerial_atmega8::print(unsigned long ul){    
	char cl_array[11];
	ulong2ascii(ul, cl_array); //convert long to char array
	print(cl_array);
}
void mySerial_atmega8::ulong2ascii(unsigned long ul, char* cl_array){
	
	//generate digits in reverse order
	char c_temp[11]; //unsigned longs have 10 digits + '\0'
	byte i = 0;
	
	do {
		c_temp[i] = (ul % 10) + 48;   // get next digit
		i++;
		} while ((ul /= 10) > 0);   
		c_temp[i] = '\0';

		//reverse the digits
		byte p = 0;
		do {
			cl_array[p] = c_temp[i-1];
			p++;
			i--;
			
		} while (i);
		cl_array[p] = '\0';
	}
void mySerial_atmega8::print(const char* s_array){
	byte c = 0;
	while (s_array[c] != 0){
		print(s_array[c]);
		c++;
	}
}
void mySerial_atmega8::print(char c){
	while (! (UCSRA & (1<<UDRE)) ); //wait for empty transmit buffer
	UDR = c; //Put data into buffer, write.
}

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
			qNoteTime = 0;	
			TCNT2 = 0; //reset/clear Timer2	
			pulseOn = false;

			mySerial_atmega8::print(qNoteTime);
			mySerial_atmega8::print('\n');

With that code, how are you sending anything but 0?

 

Also, why are you resetting TCNT2 to 0 at all? Surely the time you just threw away was part of the next pulse. You should be incrementing a value in the output compare ISR, then moving that value into qNoteTime in the INT1 ISR. Then in main() you simply send that value to print() when it sees the pulseOn flag. That way no matter how long the print is, it won't interfere with the reading.

 

 

Regards,
Steve A.

The Board helps those that help themselves.

Last Edited: Mon. Feb 23, 2015 - 05:51 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

electronaut wrote:

Thanks for pointing out the mistake. I've changed the OCR2 value to 159, the results improved but they're still off by about 1500/1700uS, here are some values:

About a microsecond.

That might be the time from the hardware's clearing of TCNT2 to your clearing of TCNT2.

There seems to be no reason for you to clear TCNT2.

The hardware does it for you.

 

The printing should not be a problem.

It does not interfere with the timer.

The printing does need to be done fast enough to allow prompt detection of the new value of pulseOn.

The traditional 9600 baud is roughly 1 ms/byte.

Iluvatar is the better part of Valar.

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

Now, I suspect there is more to this app than meets the eye.  And while a microsecond out of 500 isn't usually a problem, I don't like puzzles either.

 

From my reading, you are timing using the pulseOn which is coming from a 555 circuit?

 

And indeed you said:

it's a perfect 500 millisecond interval, so I doubt the 555 setup could be problem.

...but IME I've yet to see a perfect 555 signal.  Whatever.

 

-- You realize that you will always get some jitter with your posted approach.  It takes some dozen or two cycles to service the external interrupt.  Then it takes some cycles to complete your polling loop.

 

-- As you are looking at every "reading", it is going to take some time to get back and service the timer wraps and update qNoteTime.  You are losing everything during the serial.

 

-- You aren't showing us the real code.  You've given us test run data, but the posted code will only print a value of 0 for qNoteTime.

 

-- I mentioned that there must be more going on than just this.  Otherwise, you would be using timer1 and ICP to get values accurate to within one AVR clock cycle (less prescaler), less fuss, and no jitter.  I'm always tempted to run the clock slowly enough so that I don't have to count overflows.  If you want about 120bpm slowest, then /64.  Let's look at /256 for a moment.  Indeed, 16us resolution per beat but it will catch up the next time.  A reach down to about 30bpm.  16us variance on any particular 500000us interval is what?  0.0032%

 

-- And if you do care to count overflows, there is plenty of time to service them and you can get ISP down to one AVR clock cycle.

 

 

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

With that code, how are you sending anything but 0?

oops, I corrected that mistake but forgot to update the code here in the forum post. I have now:

 

	
			TCNT2 = 0; //reset/clear Timer2	
			pulseOn = false;

			mySerial_atmega8::print(qNoteTime);
			mySerial_atmega8::print('\n');
                        qNoteTime = 0;

 

Also, why are you resetting TCNT2 to 0 at all?

Oh ye I forgot that in CTC mode the timer get cleared automatically.

 

You should be incrementing a value in the output compare ISR, then moving that value into qNoteTime in the INT1 ISR. Then in main() you simply send that value to print() when it sees the pulseOn flag. That way no matter how long the print is, it won't interfere with the reading.

 

I actually thought about trying that out but an output compare ISR happening every 10uS seemed  to me like it would slow things down.

 

btw. the serial print routine is temporary, I will be replacing it with a SPI send (much faster) routine that will display the value on a 3 or 4 digit 7segment LED display.

 

---------------

 

There seems to be no reason for you to clear TCNT2.
The hardware does it for you.

Yep, as stated above.

 

The printing should not be a problem.
It does not interfere with the timer.
The printing does need to be done fast enough to allow prompt detection of the new value of pulseOn.
The traditional 9600 baud is roughly 1 ms/byte.

I'm using 31250 baud rate. It's not the sending of serial data, rather the conversion of an unsigned long to a char array that takes time, I believe.

 

------

 

it's a perfect 500 millisecond interval, so I doubt the 555 setup could be problem.

...but IME I've yet to see a perfect 555 signal.  Whatever.

Nope, the timer is only used to generate a clean pulse, it's triggered by a drum machine, as I mentioned in a post before:

 

For testing purposes I have a drum machine sending an audio metronome click to a 555 timer setup in monostable mode, its output goes to

pin PD3 (External Interrupt 1 - rising edge) on an Atmega8A.

 

-- You aren't showing us the real code. You've given us test run data, but the posted code will only print a value of 0 for qNoteTime.

Ye, forgot to update the code here on the forum.

 

-- I mentioned that there must be more going on than just this. Otherwise, you would be using timer1 and ICP to get values accurate to within one AVR clock cycle (less prescaler), less fuss, and no jitter.

If you mean the 16 bit counter, I need it for sending the Pulses Per Quarter note, i.e. MIDI clock, which is equal to qNoteTime / 24.

anywho, thanks for your help guys, I'll modify the code and let you know how it goes.

 

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

I'm using 31250 baud rate. It's not the sending of serial data, rather the conversion of an unsigned long to a char array that takes time, I believe.

At 31250, it takes 32us per character, therefore a 6 digit number (plus one for \n) takes 224us. The point is, anything taking more than 10us (160 cpu clocks) is going to mean you miss one or more timer compares.

I actually thought about trying that out but an output compare ISR happening every 10uS seemed  to me like it would slow things down.

A minimal ISR will take around 20 CPU clocks to execute. This is about 1/8 of your time, but as the code is written, you spend most of your time polling for the compare flag, leaving only (at most) 160 clocks to do everything else.

Regards,
Steve A.

The Board helps those that help themselves.

Last Edited: Mon. Feb 23, 2015 - 11:47 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

At 31250, it takes 32us per character, therefore a 6 digit number (plus one for \n) takes 224us. The point is, anything taking more than 10us (160 cpu clocks) is going to mean you miss one or more timer compares.

Hmm, I thought it would be 320uS per char. MIDI protocol is - start bit - data (8bits) - stop bit, so 10 bits to send a character therefore 3125 chars a second, so we get 1sec/3125 = 320uS per char.

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

skeeve wrote:
The printing should not be a problem.

It does not interfere with the timer.

The printing does need to be done fast enough to allow prompt detection of the new value of pulseOn.

The traditional 9600 baud is roughly 1 ms/byte.

Oops.

When I wrote that, I was mentally comparing 1000 bytes/sec with 2 beats/second.

I should have been comparing 6 ms with 10 us.

Now I'm not sure why your code works at all.

 

A traditional way of dealing with slow serial output is with a ring buffer that is filled in the main thread and emptied in an ISR.

Iluvatar is the better part of Valar.

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

therefore 3125 chars a second, so we get 1sec/3125 = 320uS per char.

You're right, I counted zeros incorrectly. But that means that the magnitude of the problem is even greater.

A traditional way of dealing with slow serial output is with a ring buffer that is filled in the main thread and emptied in an ISR.

 But the problem here is not that there is not enough time for serial output, it is that there is not enough time for anything. The program sits for 1/2 a second waiting for a flag to be set, then allows only 10us to actually do work.

Regards,
Steve A.

The Board helps those that help themselves.

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

 

---

 

Anyway I've changed the code, added an ISR routine on Timer2 compare match, the missing time problem during Serial print has been eliminated. I get the following values now, e.g. for 120BPM, that's 500 000 microseconds duration of a quarter note, here are a few:

500010
499940
500000
499980
499960
499960
499980

 

pretty good, considering that the serial print function has to convert a long int into a char array.

 

#define F_CPU 16000000UL

#include "my_macros.h"
#include "mySerial_atmega8.h"
#include <avr/io.h>
#include <avr/interrupt.h>

volatile byte pulseOn = false;
volatile unsigned long qNoteTime = 0;
volatile unsigned long qNoteTime2 = 0;
volatile uint16_t timer2comp_isr_uS = 0;

int main(void)
{
	init_io();
		
	while (!pulseOn){}; //wait for first pulse-trigger		
		pulseOn = false;			
			
		while (true) {		
			
			TIMSK &= ~(1<<OCIE2); // Timer/Counter2 Output Compare Match Interrupt DISABLE	
			
			while(!pulseOn){ //loop while waiting for pulse, increment the wait time
				if(readbit(TIFR, OCF2)){ //OCF2: Output Compare Flag 2 (at 160 ticks/ 10uS)
					qNoteTime += 10; //+10uS
					TIFR = (1 << OCF2); //Clear the compare flag (inverted latch, write LOGICAL 1!!!)
				}			
			} //END while(!pulseOn)
			
			qNoteTime2 = qNoteTime + timer2comp_isr_uS;
			timer2comp_isr_uS = 0;

			TIMSK |= (1<<OCIE2); // Timer/Counter2 Output Compare Match Interrupt Enable
						
			qNoteTime = 0;			
			pulseOn = false;
			
			mySerial_atmega8::print(qNoteTime2);
			mySerial_atmega8::print('\n');	
				
		} //END while true
		
    }


ISR(INT1_vect){//external pulse on INT1
	TCNT2 = 0;
	pulseOn = true;
}

ISR(TIMER2_COMP_vect){ //executes if/when Serial print takes place 
	timer2comp_isr_uS += 10; 
}

 

 

Last Edited: Wed. Feb 25, 2015 - 05:08 PM