Using push buttons...

[programmer36]

Hi all,

My 1st post asking help/guidance. Thanks for the opportunity.

Can anyone identify any mistakes in this coding? Its for dig clock with 16f877a.

interrupt isr(void)	
			
{
	if(INTF)
	{
		while(!RB0)
		{
			while(RB0)
			{
				if (!RB7)
				{
					while(!RB7);
					if(Hour<12)
						Hour++;
					else
						Hour=1;
					let[5]= (Hour/10) +0x30;
					let[6]= (Hour%10) +0x30;
			        PORTD=0XC0;    
			        lcd_write();        
			        write_let();
				}
				if (!RB6)
				{
					while(!RB6);
					if(Hour>1)
						Hour--;
					else
						Hour=12;                                                                                                         `12;
					let[5]= (Hour/10) +0x30;
					let[6]= (Hour%10) +0x30;
			        PORTD=0XC0;
			        lcd_write();
			        write_let();
				}
				if (!RB5)
				{
					while(!RB5);
					if(Minute<59)
						Minute++;
					else
						Minute=0;
					let[8]= (Minute/10) +0x30;
					let[9]= (Minute%10) +0x30;
			        PORTD=0XC0;
			        lcd_write();
			        write_let();
				}
				if (!RB4)
				{
					while(!RB4);
					if(Minute>0)
						Minute--;
					else
						Minute=59;
					let[8]= (Minute/10) +0x30;
					let[9]= (Minute%10) +0x30;
			        PORTD=0XC0;
			        lcd_write();
			        write_let();
				}
			}
		}		
		
		INTF=0;
	}
}

 

[Tahmid]

One thing I notice is that you posted the ISR only - and it's too big. The ISR should be kept short with most of the work done in the main code. I've noticed that you even wrote to the LCD in the ISR. You should be aiming to keep the ISR short - as short as possible.

 

[andre_luis]

while(!RB0)
		{
			while(RB0)
			{

This implementation could be replaced by an edge interrupt handling.
And more, If you did not performed a circuit based debounce, routine will not work properly.

Also, I agree totally with Tahmid : You cannot call routines from inside interrupt service.


+++

 

[programmer36]

One thing I notice is that you posted the ISR only - and it's too big. The ISR should be kept short with most of the work done in the main code. I've noticed that you even wrote to the LCD in the ISR. You should be aiming to keep the ISR short - as short as possible.

Thanks for the reply tahmid.

As I know, aftr interrupt is invoked, the ISR will be executed.
So the ISR is the programme that we want to run after halting the current operation.
Then, why do we want to set it short?

Pls correct me if im wrong. Thanks.

 

[andre_luis]

Because all other interrupts will be locked, once you are running the program under ISR vector adress.
If there are not more interrupts to handle, and if there is not a problem to be blinded to newest incoming event, probably your program will work.

However, take in mind that it is a dammed programming practice.
The only task ISR routine should do is to set a flag status.


+++

 

[programmer36]

Because all other interrupts will be locked, once you are running the program under ISR vector adress.
If there are not more interrupts to handle, and if there is not a problem to be blinded to newest incoming event, probably your program will work.

However, take in mind that it is a dammed programming practice.
The only task ISR routine should do is to set a flag status.


+++

Hi andre, thanks for the reply.

Im using one ISR only for the push buttons.
Cn u show me an example of setting flag status using ISR?

 

[Tahmid]

In the program interrupt is generated by external interrupt. So, if external interrupt occurs every second, for example, and your ISR takes more than 1 second to execute, then you just messed things up.

What you should be aiming to do is something like this:

interrupt isr (void){
   if (INTF){
      someflag = 1;
      INTF = 0;
   }
}

void main (void){
....... //Rest of code - initalization, etc
.......
while (1){
...... //Rest of code
......//Rest of code

if (someflag){
   .......//Now do what you have to here, eg, sending data to LCD, etc
}
}
}

Hope this helps.
Tahmid.

 

[andre_luis]

Nice explanation Tahmid.
Just a litle remark :


if (someflag){
.......//Now do what you have to here, eg, sending data to LCD, etc
someflag = 0 ; /// That´s important to avoid running this code repeatly.



+++

 

[Tahmid]

Good point. Slipped me. Good that you brought it up.

 

[programmer36]

In the program interrupt is generated by external interrupt. So, if external interrupt occurs every second, for example, and your ISR takes more than 1 second to execute, then you just messed things up.

What you should be aiming to do is something like this:

interrupt isr (void){
   if (INTF){
      someflag = 1;
      INTF = 0;
   }
}

void main (void){
....... //Rest of code - initalization, etc
.......
while (1){
...... //Rest of code
......//Rest of code

if (someflag){
   .......//Now do what you have to here, eg, sending data to LCD, etc
}
}
}

Hope this helps.
Tahmid.

Hi tahmid,

I did it this way and tested. Yes it worked. Thanks alot.

interrupt isr(void)	
			
{
	while(1)
{
	while(!TMR2IF)
	TMR2IF=0;
	count++;
	if(count==125)
{
	count=0;				
}		
}
}

Now able to use the push buttons. However 1 thing noticeable is the time is keep on running while I press the push button to adjust.
Is it because any errors in the code? Nd your advice. Thanks.