[SOLVED] PIC16F877 RTC C Compiler opinion required

[WStevens_sa]

Hi all

I have 877 programmed as RTC using TMR0. With individual timers for each day of the week. Each day has a start/stop timer (Day,Hour,Min) variable to compare to current (day, hour, min). I am using arrays for the variables. For example

timer_status is either 0= disabled, 1= enabled. Each day has its own timer. the "group" timer will default the whole week to one timer

int timer_status[8] = {0,0,0,0,0,0,0,0}; // mon,tue,wed,thu,fri,sat,sun,group

timer_on will store the (day,hour,min) variable for when the timer must turn on. This is where I have run into a snag. I am not sure of what datatype I should store it as, as I need to compare it to the current (Day,hour,min) which has day as char and hours, min as int. These are the options I have thought of

Option 1. Convert and store both to char and do a text compare to turn timer on or off
Option 2. Convert the timer on / off values as hex. Then convert current day,hour to hex to compare.
Option 3. Shoot myself in the head
Option 4. See what everybody else thinks.

int timer_on[8] = {0,0,0,0,0,0,0,0}; // store on day hour min eg Mon 13:45

timer_of will store the day,hour and min the timer must switch off. Same problem as above

int timer_off[8] = {0,0,0,0,0,0,0,0}; // store on day hour min eg Mon 13:45

Below is hte partial code of the RTC

void interrupt(void) {

if(T0IF_bit == 1){ // Check for interrupt flag

T0IF_bit=0; //Reset flag
counter++ ;
if(counter > 15625){ // 1 sec 16 Mhz crystal = 16000000 / 4 / 256
counter = 0 ;
seconds++ ;
if(seconds == 60){
seconds = 0 ;
minutes++ ;
if(minutes == 60){
minutes = 0 ;
hours++ ;
if(hours == 24){
hours = 0 ;
days++;
if (days == 7){
days = 0;
}
}
}
}
}
}
}
void main(void){
while(1){
//SECONDS
Lcd_Chr(1,12,48+(seconds % 10)); // seconds counter LSB
Lcd_Chr(1,11,48+((seconds /10) % 10)); //Seconds counter MSB

//MINUTES
Lcd_Chr(1,9,48+(minutes % 10)); // minutes counter LSB
Lcd_Chr(1,8,48+((minutes /10) % 10)); //minutes counter MSB

//HOURS
Lcd_Chr(1,6,48+(hours % 10)); // hours counter LSB
Lcd_Chr(1,5,48+((hours /10) % 10)); // hours counter MSB

//DAYS
Lcd_out(1,1,weekdays[days]); // days of the week

}
}

 

[WStevens_sa]

I would like to know what everybody else thinks

 

[arthur0]

Hi.
If I were you, I’d let the “RTC” increment a single, big, minute counter that could be stored in an int (it’ll only need to count up to 10080, which is 7 days, i.e. cnt = ++cnt % 10080).
Checking the current time against your stored daily-timers (which are also expressed in minutes, of course) becomes thus trivial.
I’d do all the conversions only if and when displaying is needed. (You could still keep a separate seconds counter for displaying purposes.)

Another tip: make sure you declare the current-time counter volatile and protect its access outside the RTC ISR (disable interrupts; read counter’s value; enable interrupts).

Arthur

 

[WStevens_sa]

Okay I like the idea never thought of it. Brilliant. The seconds will not be displayed. I am only showing it for now for testing so I can get the timing right as it looses a few seconds over a few hours.

Only problem with making current time volitile is will this not stop me from setting the day, hours and minutes as I am using below

//PUSHBUTTON INPUT HOURS RC1_BIT
if (Button(&PORTC, 1, 1, 1)) {
hours++;
Delay_ms(200); // Adjust counting time when button depressed
if (hours ==24) {hours=0;}
}

Another tip: make sure you declare the current-time counter volatile and protect its access outside the RTC ISR (disable interrupts; read counter’s value; enable interrupts).

 

[123jack]

First I'd follow arthur's advice then as a general rule for future use:

What is the highest value you need to store for each variable?
That tells you what to use. If its less than 256 use shorts and so on.
Always choose the smallest size that fits (being aware of any math you do of course)

Also it can depend on how efficient you need to be.
For ease of programming I'd normally use shorts for flags. Then if I have low resources I'd go through and change things to bit fields where I can etc.
If resources are high I dont bother (the micro always transfers bytes anyway)

As a rule I try to stick with the convention of using the label "shorts" for numeric data and "chars" for 8 bit text (assuming a compiler with 8 bit shorts of course) . Just a personal preference to help document things.

jack

 

[WStevens_sa]

Thanks Jack I will keep that in mind as it makes sense to reduce the amount of limited resources.

 

[arthur0]

WStevens,
I don't really understand what you mean. I can’t see any problems with setting the current time by making the counter volatile. The keyword volatile will only prevent the compiler from attempting any optimizations regarding the accessing of the variable (just to make sure it’s always reading from the memory location where it is stored instead of using a value “cached” by some register, since the actual value might have changed by the ISR in the meantime).
Also, when I said “protect its access outside the RTC ISR”, I didn’t mean to make the variable read-only, but to make sure you do a synchronized access (I should have said “synchronized” instead of “protect”) on it.
In more detail, imagine that the main loop is accessing the variable to read its content. But since it is 2-bytes long and it can only access 1-byte at a time, the ISR might interrupt in the middle of the access and do some work on the same variable. So, when the control returns to the main loop, the second byte’s value might be changed and the whole access can be considered corrupt.
That’s why you need to ensure that the whole access is atomic (i.e. can’t be split/interrupted). The easiest way to do that is to disable interrupts while you do it. (A nicer way would be to use a semaphore, or a byte-flag that can be accessed with one instruction.)

[COLOR="green"]/* main loop */[/COLOR]
int minute_counter_copy;

[COLOR="green"]/* read-access on the volatile int minute_counter */[/COLOR]
disable_ints();
   minute_counter_copy = minute_counter;
enable_ints();

[COLOR="green"]/* do work on the counter’s copy here, such as set the time */

/* write-access on the volatile int minute_counter */[/COLOR]
disable_ints();
   minute_counter = minute_counter_copy;
enable_ints();

I hope I understood and clarified your dilemma…
Arthur