Inaccurate timing using ATmega8A @ 3.6864MHz

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

Hi there,

I'd like to turn ON a LED for 10 hours then turn OFF for 14 hours. So i made my code of which does not work as expected.

My problem is that i do not get accurate timing.

E.g: i get 8 hours timing instead of 10 hours

 

My fuse settings are Low: 0xff High:0xd9. Using 3.6864MHz xtal.

#define F_CPU 3686400UL

#include <avr/io.h>
#include <util/delay.h>
#include <stdio.h>
#include <string.h>

#define TIME_TO_TURN_ON	10//hours
#define WHOLE_DAY 24//hours
#define TIME_TO_TURN_OFF (WHOLE_DAY-TIME_TO_TURN_ON)

int main(void)
{
	unsigned int k,i;

	DDRC=0xff;
	PORTC=0x00;//turn off LED

	for(i=0;i<4;i++)
	{
		PORTC^=(1<<PC5); //blinking LED at startup
		_delay_ms(1000);
	}

    while (1)
    {
		//---------Turn on----------------
		PORTC=0xff;//LED turned ON
		k=360*TIME_TO_TURN_ON;//3600

		for (i=0; i<k; i++)
		{
			_delay_ms(10000);
	    }

		//----------Turn off--------------
		PORTC=0x00;//relay turned OFF
		k=360*TIME_TO_TURN_OFF;//5040

		for (i=0; i<k; i++)//blinking LED while relay is turned OFF
		{
			_delay_ms(5000);
			PORTC=0xff;//LED turned ON
			_delay_ms(5000);
			PORTC=0x00;//LED turned OFF
	    }
	}
	return 0;
}

Have you got any ideas why my timing is inaccurate?

This topic has a solution.

Last Edited: Sat. Feb 9, 2019 - 04:12 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

_delay_ms() and its siblings are NOT precision timers. For that, you REALLY ought to use built-in hardware counter/timers. 

 

Jim

 

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

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

But 8 instead of 10 sounds pretty bad. What if you change the code to set only one loop (one execution of delay_ms); do you get accurate timing? That should me much easier to measure.

/Jakob Selbing

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

Max23243 wrote:
E.g: i get 8 hours timing instead of 10 hours
Does that mean that you get 11 hours and 12 minutes instead of 14 hours?

 

I note that 3.6864 MHz / 0.8 is another 'magic' crystal frequency:  4.608 MHz.  Are you sure your crystal is 3.6864 MHz and not 4.608 MHz?

 

ka7ehk wrote:
_delay_ms() and its siblings are NOT precision timers. 
Sort of.  Modern AVR Libc's _delay_ms() is in fact cycle-accurate.   But with 36,864,000 cycles for each invocation of _delay_ms(), even an overhead of 100 cycles would only be about 4 PPM, and should result in longer, not shorter times.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

HF crystals are very accurate. Even Ebay crystals. Check the frequency printed on the case.
.
I would expect your _delay_ms() to be accurate within a few 10s of seconds over 10 hours.
If you used a hardware timer, I would expect to be accurate to a few seconds over 10 hours.
.
You can do your tests with _delay_ms(10) to check your crystal. 36 seconds is easier than waiting for 36000 seconds
.
David.

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

Why wouldn't you simply use a timer and slave your 10/14 hour periods off that? No on in real life uses _delay_ms for this kind of thing.

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

Put a watch crystal on Timer 2, running asynchronously, and set it to generate a 1s interrupt. Do your timekeeping in the ISR. Job done(properly).

#1 This forum helps those that help themselves

#2 All grounds are not created equal

#3 How have you proved that your chip is running at xxMHz?

#4 "If you think you need floating point to solve the problem then you don't understand the problem. If you really do need floating point then you have a problem you do not understand." - Heater's ex-boss

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

It seems perfectly reasonable for a beginner to start with delay() on an unknown hardware target.
I woud certainly do the same for ARM or ESP or Renesas.
.
Having got a simple project working with delays, I would then investigate the hardware timers e.g. set a 10 second interrupt and count 3600 interrupts.
.
I suspect like Joey that the crystal has a different value.
The fuses indicate that it IS using the crystal. So it is not a RC clock problem.
.
David.

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

Although rare, there have been cases of mislabelled crystals.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

Max23243 wrote:

Have you got any ideas why my timing is inaccurate?

 

What are you checking your timings against?

#1 This forum helps those that help themselves

#2 All grounds are not created equal

#3 How have you proved that your chip is running at xxMHz?

#4 "If you think you need floating point to solve the problem then you don't understand the problem. If you really do need floating point then you have a problem you do not understand." - Heater's ex-boss

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

Set up a one second blink and time how long it takes to blink 100 times. That will quickly give you an idea of how inaccurate things are.

 

 

Also, what size capacitors do you have on your crystal?

 

Can you show a photo of your hardware.

#1 This forum helps those that help themselves

#2 All grounds are not created equal

#3 How have you proved that your chip is running at xxMHz?

#4 "If you think you need floating point to solve the problem then you don't understand the problem. If you really do need floating point then you have a problem you do not understand." - Heater's ex-boss

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

Go on.   You can pull a crystal by a few ppm.   There is no way to go that far wrong.

 

A crystal will either oscillate or be stone dead e.g. with 22nF instead of 22pF burden capacitors.

 

Let's wait for the OP to reply.

 

David.

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

As said before me, in the near future start using the timer hardware and periodic interrupts.

 

But as a first test.

Toggle a led every _delay( 10s) Is that also 20% off?

 

Doing magic with a USD 7 Logic Analyser: https://www.avrfreaks.net/comment/2421756#comment-2421756

Bunch of old projects with AVR's: http://www.hoevendesign.com

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

Max23243 wrote:

I'd like to turn ON a LED for 10 hours then turn OFF for 14 hours. So i made my code of which does not work as expected.

My problem is that i do not get accurate timing.

E.g: i get 8 hours timing instead of 10 hours

 

What test equipment do you have ?

Is that 8.00 hours instead of 10 hours ?  - if you measure carefully, you can extract the equivalent clock the AVR is using.

 

Are you sure all of your files recompiled with a F_CPU change ? 

 

First, code a simple, slow loop, within your instruments range - a 10Hz pin toggle for example, would use  _delay_ms(50); calls, and you can confirm your delay calibrate there.

 

You can use PC Sound cards as ocilloscopes, if you do not have a Multimeter with a Hz scale.

 

I also find this,

https://www.avrfreaks.net/commen...

seems _delay_ms, was a little rough.... which version do you have, and what assembler code is created for the inner-most loop ?

 

Google finds this from 2017, showing some generated assembler - looks to use 16b innermost loop for  _delay_us(100), and a 24b loop for _delay_ms(100)

It does seem to pass the scaled number into the innermost loop, which is nice to see.

 

#define F_CPU 20000000
#include <util/delay.h>

int main(void)
{
    while (1) 
    {
        _delay_us(100);
    }
}

And the resulting section of disassembled code:  (100us)

    __builtin_avr_delay_cycles(__ticks_dc);
  f6:   83 ef           ldi r24, 0xF3   ; 243
  f8:   91 e0           ldi r25, 0x01   ; 1
  fa:   01 97           sbiw    r24, 0x01   ; 1
  fc:   f1 f7           brne    .-4         ; 0xfa <main+0x4>
  fe:   00 c0           rjmp    .+0         ; 0x100 <main+0xa>
 100:   00 00           nop
 102:   f9 cf           rjmp    .-14        ; 0xf6 <main>

Here I was only delaying 100 microseconds but if I change it to 100 milliseconds I still don't code as lengthy as yours:

    __builtin_avr_delay_cycles(__ticks_dc);
  f6:   2f e7           ldi r18, 0x7F   ; 127
  f8:   8a e1           ldi r24, 0x1A   ; 26
  fa:   96 e0           ldi r25, 0x06   ; 6
  fc:   21 50           subi    r18, 0x01   ; 1
  fe:   80 40           sbci    r24, 0x00   ; 0
 100:   90 40           sbci    r25, 0x00   ; 0
 102:   e1 f7           brne    .-8         ; 0xfc <main+0x6>
 104:   00 c0           rjmp    .+0         ; 0x106 <main+0x10>
 106:   00 00           nop
 108:   f6 cf           rjmp    .-20        ; 0xf6 <main>

 

The innermost 24bit counting loop is 5 sysclks, so a quick check gives  0x61a7f*5/20M = 0.09999975

but there is also code to load registers, and a call/RET overhead, so overall delay is slightly longer  (0x61a7f*5+5)/20M = 100.000ms  (or, for 16b loop :  (0x1f3*4+4)/20M = 100.000us

 

The timing limit of a 24b delay loop, looks to be

 At 20MHz   (0xffffff*5+5)/20M = 4.194304s

 At 3.6864MHz  (0xffffff*5+5)/3.6864M = 22.7555's
 

 

Last Edited: Sun. Feb 3, 2019 - 09:51 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi there,

Thanks to you all for your help.
It is a LED grow light project with my self-developed hardware.
My second code got stuck somewhere or some noise may have affected MCU that LED and relay kept turning on.

My latest code is here. (it seems to work) The difference from my second code is that i use 'volatile' . Tomorrow will be the truth of moment to see if it works. It should work.

Kind regards,

Balázs
 


//Atmega8A @ 3.6864MHz.
//Fuse: Low: 0xff High:0xd9
// 2019.02.03.

#define F_CPU 3686400UL


#include <avr/io.h>
#include <util/delay.h>
#include <stdio.h>
#include <avr/interrupt.h>

#define TURNED_ON_STATE	10//hours
//-----------------------------------------------------------------------------------------------------
#define TURNED_ON_STATE_IN_SECONDS	3600*(volatile unsigned int)TURNED_ON_STATE
#define A_WHOLE_DAY 24//hours
#define TURNED_OFF_STATE_IN_SECONDS 3600*((volatile unsigned int)A_WHOLE_DAY-TURNED_ON_STATE)
//-----------------------------------------------------------------------------------------------------
#define ONE_SECOND 14400
#define TIMING ONE_SECOND
volatile unsigned int i=0;
volatile unsigned char k=0;

void timer1_init()
{
	TCCR1B  |= (1 << WGM12); //  Configure  timer 1 for CTC  mode
	TIMSK  |= (1 << OCIE1A); //  Enable  CTC  interrupt

	OCR1A  = TIMING;  // Set CTC  compare  value to 1Hz at xMHz  AVR clock , with a prescaler  of 256
	TCCR1B |= (1<<CS12); //256-os osztó: F_CPU / 256 = 3.6864MHz / 256 = 14400
}

int main(void)
{	
	DDRB=0xff;
	PORTB=0xff;//Turning ON relay

	DDRC=0xff;
	PORTC=0xff;//Turning ON LED
    
	timer1_init();
	sei();

    while(1)
	{
	}
	return 0;
}


ISR(TIMER1_COMPA_vect)
{
	i++;

	if((i==TURNED_ON_STATE_IN_SECONDS)&&(k==0))
	{
		PORTC = 0x00;//LED OFF
		PORTB=0x00;//Relay OFF
		k=1;
		i=0;
	}

	if(k==1)
	{
		if(i==TURNED_OFF_STATE_IN_SECONDS)
		{
			PORTC = 0xff;//LED ON
			PORTB=0xff;//Relay ON
			k=0;
			i=0;
		}
	}
}

 

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

volatile is only needed for variables that are modified/read in two threads of execution. Your variables aren't they are only within the ISR, the other thread of execution is the while(1) loop in main() but nothing is done there.

 

But just a few points:

 

1) globals called things like "i" and "k" are a terrible idea. Why not give them meaningful names like "seconds"

 

2) Also do not use C's base types ((unsigned) short, int, long, etc) use types from <stdint.h> that will survive portability and are more self documenting about the counting range they hold. There is no doubt that "uint8_t" will have a counting range of 0..255 on any architecture and equally no doubt that int16_t holds values from -32768 to +32767 and so on, so I'd make "i" a uint16_t that has a counting range of 0..65535 which should cater for its maximum 50,400 and make "k" a "uint8_t"

 

3) However, if "k" is a "state variable" then consider using C's facilities for "states" which are called "enum"s so:

typedef enum {
    RELAY_OFF,
    RELAY_ON
} relay_e;

 

4) they shouldn't be globals anyway, as I say they are only used in the ISR() so they should be "statics" wihtin it:

typedef enum {
    RELAY_OFF,
    RELAY_ON
} relay_e;

ISR(TIMER1_COMPA_vect)
{
    static uint16_t seconds = 0;
    static relay_e on_off = RELAY_OFF;

    seconds++;

    if((seconds == TURNED_ON_STATE_IN_SECONDS) && (on_off == RELAY_OFF))
    {
        PORTC = 0x00;//LED OFF
        PORTB = 0x00;//Relay OFF
        on_ff = RELAY_ON;
        seconds = 0;
    }

    if(on_off == RELAY_ON)
    {
        if (seconds == TURNED_OFF_STATE_IN_SECONDS)
        {
            PORTC = 0xff;//LED ON
            PORTB = 0xff;//Relay ON
            on_off = RELAY_OFF;
            seconds = 0;
        }
    }
}

But having tried to make that more "self documenting" it seems a little confused. Consider when seconds reaches 50,400 then it will execute the first conditional block and on_off will be set to RELAY_ON so it will then start to execute the second conditional and will only be prevented from executing the "ON" code because the Off count is 36,000 not 50,400. But consider if you set it to 12hrs and 12hrs. Surely the logic in the ISR should be:

if (RELAY_ON) {
    some stuff
}
else {
    some other stuff
}

then do your seconds range checking in each block so:

if (RELAY_ON) {
    if (seconds = END_OF_ON_TIME) {
        some stuff
    }
}
else {
    if (seconds = END_OF_OFF_TIME) {
        some other stuff
    }
}

To be honest, this is how you should be writing your code anyway - don't just wade straight into the C but plan out the design/strategy first and when you have it clear in your mind what you want to do then the actual coding in C becomes trivial.

 

Oh and you default "k" which, as I say, seems to be the relay/LED on/off state to be:

volatile unsigned char k=0;

and yet in your startup code you do:

	DDRB=0xff;
	PORTB=0xff;//Turning ON relay

	DDRC=0xff;
	PORTC=0xff;//Turning ON LED

So the state variable seems to be saying "it's off" but the reality is that it starts "on" ??

 

Or is it really the case that "k" is upside-down and 0 = ON and 1 = OFF ?

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

Hi,

It is okay, thanks for your substantial reply. I'll give it a try but my code works properly so far.

Kind regards,
Balázs

This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Code that works by accident is not code that works by design!