RTC Problems

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

Hi all,

I've written this code to display time on an lcd. No problem, the code works but its losing seconds rapidly, about ten seconds in 3 minutes. obviously this needs to be reduced drastically!!

I'm using a mega32, with an 8mhz resonator and a 32.768Khz watch xtal on the TOSC pins. I'm using Timer0 interrupt at 2hz to update the display, and timer2 interrupt to get 1hz count out of the watch xtal in asynchronous mode. Both are using CTC mode.

Heres the code:

#include 
#include 
#include 
#include "lcd.h"


#define bit_get(p,m) ((p) & (m))
#define bit_set(p,m) ((p) |= (m))
#define bit_clear(p,m) ((p) &= ~(m))
#define bit_flip(p,m) ((p) ^= (m))
#define bit_write(c,p,m) (c ? bit_set(p,m) : bit_clear(p,m))
#define BIT(x) (0x01 << (x))
#define LONGBIT(x) ((unsigned long)0x00000001 << (x))

volatile int sec = 0;
volatile int min = 0;
volatile int hrs = 0;
volatile int tmpint = 0;
volatile char tmpstr[3];
volatile char tempsec;
volatile char tempmin;
volatile char temphr;
volatile int i=0;

/////////////////////////////
// Timing routine
/////////////////////////////
ISR(TIMER0_COMP_vect) // LCD Update Routine
{	
	//get 10xhrs
	if (sec < 10) {
			sprintf(tmpstr, "%d", sec);
			lcd_gotoxy(6,0);
			lcd_puts("0");
			lcd_gotoxy(7,0);
			lcd_puts(tmpstr);
			}
		else 
			{
			sprintf(tmpstr, "%d", sec);
			lcd_gotoxy(6,0);
			lcd_puts(tmpstr);
			}
		
		if (min < 10) {
			sprintf(tmpstr, "%d", min);
			lcd_gotoxy(3,0);
			lcd_puts("0");
			lcd_gotoxy(4,0);
			lcd_puts(tmpstr);
			}
		else 
			{
			sprintf(tmpstr, "%d", min);
			lcd_gotoxy(3,0);
			lcd_puts(tmpstr);
			}
		
		if (hrs < 10) {
			sprintf(tmpstr, "%d", hrs);
			lcd_gotoxy(0,0);
			lcd_puts("0");
			lcd_gotoxy(1,0);
			lcd_puts(tmpstr);
			}
		else 
			{
			sprintf(tmpstr, "%d", hrs);
			lcd_gotoxy(0,0);
			lcd_puts(tmpstr);
			}
}

ISR(TIMER2_COMP_vect) // Counting routine
{
   PORTB ^= (1 << 0); // Toggle the LED
	sec++;

	if (sec==60) {
		min++;
		sec = 0;

		if (min==60) {
			hrs++;
			min = 0;

			if (hrs==24) {
				sec = 0;
				min = 0;
				hrs = 0;
			}
		}
	}
}

ISR(INT0_vect) { 

	sec = 0;

	if (++hrs >= 24) {
		hrs = 0;
	}

}

ISR(INT1_vect) { 

	sec = 0;
	
	if (++min >= 60) {
		min = 0;
	}

}

////////////////////////////
// MAIN
////////////////////////////
void main() {

	init();
	
	lcd_puts("Clock V2");
	_delay_ms(1000);

	lcd_clrscr();
	lcd_puts("00:00:00");

}

/////////////////////////
// INIT function
/////////////////////////
void init() {
	lcd_init(LCD_DISP_ON);
	lcd_clrscr();
	DDRB |= (1 << 0); // Set LED as output
	MCUCR |=(1<<ISC11) | (1<<ISC01); // Set INT1 and INT0 to Falling Edge Triggered 
	GICR |= (1<<INT0) | (1<<INT1); //turn on INT0 and INT1
	
	TCCR0 |= (1<<WGM01) | (1<<CS02); //  Set Timer 0  as ctc with 256 prescaler
	OCR0 = 15625; // set CTC to 32 for 1Hz operation
	TIMSK |= (1 << OCIE0); //Enable CTC interrupt
	

	TCCR2 |= (1<<WGM21) | (1<<CS22) | (1<<CS21) | (1<<CS20); // Set Timer2 as 1024 prescale, in CTC Mode
	OCR2 = 32; // set CTC to 32 for 1Hz operation
	ASSR |= (1<<AS2); // Set Asynchronous mode to use TOSC with 32.768khz watch xtal
	TIMSK |= (1 << OCIE2); // Enable CTC interrupt
	sei(); // enable global interrupts
	


}

Any ideas?!? Thanks for your help!

Dave

Dave Harrod

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

Doing all then LCD work in the ISR is a "Bad Idea"(tm). You are probably losing interrupts. Why not just have the ISR update a flag to say that an LCD refresh is needed then do the work in a main() loop that is polling that flag?

Also, as it stands you are dropping out of the bottom of main() if you are lucky this week your compiler may be providing a "catch all" infinite loop there but a return from main() on an embedded system is never a great idea.

Actually your includes suggest GCC - the latest version DOES have such a catch-all but there was one of the issues a while back where this behaviour was changed and it broke a lot of apps like yours as the code when swanning off into no-man's land.

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

Are you sure that you don't need 31 instead of 32 in OCR2? (And what Cliff said!)

Four legs good, two legs bad, three legs stable.

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

So your saying that i need to have another int called something like update, when the ISR that is currently handling the LCD stuff is called it sets update to 1. the main() then does its init(), then goes into a for(;;) which looks at update. if its 1 it does the lcd stuff, then clears update. same again. so on and so on.

I'll check my calcs, and try 31!!!!

Thanks guys

Dave Harrod

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

Yup I'm suggest you lift everything that currently lives in ISR(TIMER0_COMP_vect) and put it into a "normal" function. Then the body of that ISR simply becomes:

ISR(TIMER0_COMP_vect) {
 lcd_refresh_needed = 1;
}

Then back over in main() after you've done all the initialisation stuff you drop into a:

while (1) {
  if (lcd_refresh_needed) {
    call_previous_body_of_the_ISR_that_does_LCD();
    lcd_refresh_needed = 0;
  }
}

In fact if you aren't going to do anything else in main() why bother with the ISR and the flag at all - just repeatedly repaint the LCD:

while (1) {
    call_previous_body_of_the_ISR_that_does_LCD();
}

Cliff

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

PS There may be an issue though - say you are half way through updating the hh:mm:ss and the time is 11:59:59 - you've output the 11: but then the second increment ISR happens. The time now is 12:00:00 and because you've already output 11: you go on to actually draw 11:00:00 (which is wrong). To avoid this, before you start the LCD update (outside of interrupt protection) you should probably have a short:

cli();
copy_secs = secs;
copy_mins = mins;
copy_hrs = hrs;
sei();
//display LCD using copy_ variables

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

Ok thanks. I've got it working now and it looks like its a lot more accurate. i'll implement the copy variables. Thanks alot for your help! this learning curves a steep one!!!!

Dave Harrod

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

Ok, so it now looks like this. it seems to be working pretty well, but i'll have to leave it running for testing.

#include 
#include 
#include 
#include "lcd.h"


#define bit_get(p,m) ((p) & (m))
#define bit_set(p,m) ((p) |= (m))
#define bit_clear(p,m) ((p) &= ~(m))
#define bit_flip(p,m) ((p) ^= (m))
#define bit_write(c,p,m) (c ? bit_set(p,m) : bit_clear(p,m))
#define BIT(x) (0x01 << (x))
#define LONGBIT(x) ((unsigned long)0x00000001 << (x))

volatile int sec = 0;
volatile int min = 0;
volatile int hrs = 0;
volatile int tmpint = 0;
volatile char tmpstr[3];
volatile int tmpsec;
volatile int tmpmin;
volatile int tmphrs;
volatile int i=0;
volatile int update=0;
/////////////////////////////
// Timing routine
/////////////////////////////
ISR(TIMER0_COMP_vect) // LCD Update Routine
{	
 update = 1;	
}

ISR(TIMER2_COMP_vect) // Counting routine
{
   PORTB ^= (1 << 0); // Toggle the LED
	sec++;

	if (sec==60) {
		min++;
		sec = 0;

		if (min==60) {
			hrs++;
			min = 0;

			if (hrs==24) {
				sec = 0;
				min = 0;
				hrs = 0;
			}
		}
	}
}

//////////////////////////
// ISR to handle HR setting
//////////////////////////
ISR(INT0_vect) { 

	sec = 0;

	if (++hrs >= 24) {
		hrs = 0;
	}

}

////////////////////////////
// ISR to handle MIN setting
////////////////////////////

ISR(INT1_vect) { 

	sec = 0;
	
	if (++min >= 60) {
		min = 0;
	}

}

////////////////////////////
// MAIN
////////////////////////////
void main() {

	init();
	
	lcd_puts("LCD Clock V2.1");
	_delay_ms(1000);

	lcd_clrscr();
	lcd_puts("00:00:00");

	for (;;) {
		if (update) {
			//get 10xhrs
			put_time();
		}

	}

}


/////////////////////////
// INIT function
/////////////////////////
void init() {
	lcd_init(LCD_DISP_ON);
	lcd_clrscr();
	DDRB |= (1 << 0); // Set LED as output
	MCUCR |=(1<<ISC11) | (1<<ISC01); // Set INT1 and INT0 to Falling Edge Triggered 
	GICR |= (1<<INT0) | (1<<INT1); //turn on INT0 and INT1
	
	TCCR0 |= (1<<WGM01) | (1<<CS02); //  Set Timer 0  as ctc with 256 prescaler
	OCR0 = 15625; // set CTC to 15625 for 2Hz operation
	TIMSK |= (1 << OCIE0); //Enable CTC interrupt
	

	TCCR2 |= (1<<WGM21) | (1<<CS22) | (1<<CS21) | (1<<CS20); // Set Timer2 as 1024 prescale, in CTC Mode
	OCR2 = 31; // set CTC to 32 for 1Hz operation
	ASSR |= (1<<AS2); // Set Asynchronous mode to use TOSC with 32.768khz watch xtal
	TIMSK |= (1 << OCIE2); // Enable CTC interrupt
	sei(); // enable global interrupts
	


}

//////////////////
// Function to print time on LCD
//////////////////

void put_time() {

tmpsec = sec; // Make a copy of all values
tmpmin = min;
tmphrs = hrs;

if (sec < 10) {
			sprintf(tmpstr, "%d", tmpsec);
			lcd_gotoxy(6,0);
			lcd_puts("0");
			lcd_gotoxy(7,0);
			lcd_puts(tmpstr);
			}
		else 
			{
			sprintf(tmpstr, "%d", tmpsec);
			lcd_gotoxy(6,0);
			lcd_puts(tmpstr);
			}
		
		if (min < 10) {
			sprintf(tmpstr, "%d", tmpmin);
			lcd_gotoxy(3,0);
			lcd_puts("0");
			lcd_gotoxy(4,0);
			lcd_puts(tmpstr);
			}
		else 
			{
			sprintf(tmpstr, "%d", tmpmin);
			lcd_gotoxy(3,0);
			lcd_puts(tmpstr);
			}
		
		if (hrs < 10) {
			sprintf(tmpstr, "%d", tmphrs);
			lcd_gotoxy(0,0);
			lcd_puts("0");
			lcd_gotoxy(1,0);
			lcd_puts(tmpstr);
			}
		else 
			{
			sprintf(tmpstr, "%d", tmphrs);
			lcd_gotoxy(0,0);
			lcd_puts(tmpstr);
			}
			
			lcd_gotoxy(2,0);
			lcd_puts(":");
			lcd_gotoxy(5,0);
			lcd_puts(":");
			update = 0;

		}


Dave Harrod

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

Quote:

volatile int sec = 0;
volatile int min = 0;
volatile int hrs = 0;
volatile int tmpint = 0;
volatile char tmpstr[3];
volatile int tmpsec;
volatile int tmpmin;
volatile int tmphrs;
volatile int i=0;
volatile int update=0;

Make them all (or at least all that are not used in a signed fashion) unsigned. And I'd use char vs. int whenever the values are known to be small enough. It may make no difference in a small app on a "big" AVR, but get into the habit of being niggardly with SRAM usage.

Quote:

tmpsec = sec; // Make a copy of all values
tmpmin = min;
tmphrs = hrs;

Turn off interrupts as suggested to get a "clean" copy.

Quote:

if (sec==60)

Just in case you or sunspots or whatever mess things up, I would always write this as "if (sec >= 60)" when it costs you nothing. Many have gotten into the habit [I never have; too many years] of writing comparisons as

if (60 == sec)

so that the compiler can help you in case you make the common, frustrating, and hard to find typo "if (sec = 60)".

if (sec < 10) {
         sprintf(tmpstr, "%d", tmpsec);
         lcd_gotoxy(6,0);
         lcd_puts("0");
         lcd_gotoxy(7,0);
         lcd_puts(tmpstr);
         }
      else
         {
         sprintf(tmpstr, "%d", tmpsec);
         lcd_gotoxy(6,0);
         lcd_puts(tmpstr);
         }
      
      if (min < 10) {
         sprintf(tmpstr, "%d", tmpmin);
         lcd_gotoxy(3,0);
         lcd_puts("0");
         lcd_gotoxy(4,0);
         lcd_puts(tmpstr);
         }
      else
         {
         sprintf(tmpstr, "%d", tmpmin);
         lcd_gotoxy(3,0);
         lcd_puts(tmpstr);
         }
      
      if (hrs < 10) {
         sprintf(tmpstr, "%d", tmphrs);
         lcd_gotoxy(0,0);
         lcd_puts("0");
         lcd_gotoxy(1,0);
         lcd_puts(tmpstr);
         }
      else
         {
         sprintf(tmpstr, "%d", tmphrs);
         lcd_gotoxy(0,0);
         lcd_puts(tmpstr);
         }
         
         lcd_gotoxy(2,0);
         lcd_puts(":");
         lcd_gotoxy(5,0);
         lcd_puts(":");
         update = 0;

After making tmpxxx unsigned, I'd replace the above with

sprintf (tmpstr, "%02u:%02u:%02u", tmphrs, tmpmins, tmpsecs);
lcd_gotoxy(0,0);
lcd_puts(tmpstr); 
update = 0;

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

theres always more!! Thats exactly what i need, i guess it always helps to get into good habits.

Dave Harrod

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

If i turn of all the interrupts to get a clean copy of the sec, min and hrs variables wont i run the risk of missing a count on timer2, therefore missing a second or two?

Dave Harrod

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

No, the interrupt flag will still be set when you re-enable interrupts.

Four legs good, two legs bad, three legs stable.

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

You are turning off interrupts for like a microsecond. It helps to ensure that when you are at a rollover point you get a valid time vs an invalid one. if seconds are rolling over into minutes--the most common--you might be at 43:59 just about ready to roll to 44:00. If the interrupt hits, you could display 43:59, then 43:00, then (when you "catch up" the next time) 44:00 or 44:01. It's called "atomic access"--you want all the pieces to be indivisible like an atom.

Lee

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

ok, so is it just a case of adding gei() and sei() like this?

//////////////////
// Function to print time on LCD
//////////////////

void put_time() {
gei();
tmpsec = sec; // Make a copy of all values
tmpmin = min;
tmphrs = hrs;
sei();

sprintf (tmpstr, "%02u:%02u:%02u", tmphrs, tmpmin, tmpsec);
lcd_gotoxy(0,0);
lcd_puts(tmpstr);
update = 0;

}

Dave Harrod

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

I think you'll find "gei()" is spelt "cli()" in fact.