detecting pushbutton

[laugh]

hi.. i need help with this program. this is a program to turn 'ON' and 'OFF' 1 LED using 2 push buttons.
the underline statement suppose to wait for button press. but it is alway false even i have press the button. is there any one who can tell me what's wrong with the statement.. thank alot..

#include <p18f4620.h>

#pragma config OSC = HS                              
#pragma config WDT = OFF
#pragma config LVP = OFF

#define btn_on     PORTBbits.RB4
#define btn_off     PORTBbits.RB5
#define led           PORTAbits.RA0

void on(void);
void off(void);
void Delay(void);

void main(void)
{
TRISA = 0;                          //set Port A(LED) as output
PORTAbits.RA0 = 0;             //reset LED
Rpt:
   [U] if (btn_on || btn_off)[/U]        //wait for btn press
    {   
              if (btn_on == 1)     //btn on pressed 
              {
                      Delay();     //This is the real trick, Debounce the input!!
                      if (!btn_on)      //btn on still pressed?
                          goto Rpt;     //No
                      on();
              }
               if (btn_off == 1)      //btn off pressed
              {
                      Delay();      //This is the real trick, Debounce the input!!
                      if (!btn_off)      //btn off still pressed?
                          goto Rpt;     //No
                      off();
              }
}
goto Rpt;
} //end main

    void on(void)
    {
Rpt_on:
       while(btn_on);             //wait for btn(RB4) released 
        Delay();                     //debounce
        if (btn_on)                  //btn on still released?
            goto Rpt_on;            //No
       PORTAbits.RA0 = 0x0F;        //on LED
    } //end on

    void off(void)
    {
Rpt_off:
       while(btn_off);           //wait for btn(RB5) released 
        Delay();                   //debounce
        if (btn_off)                //btn off still released?
            goto Rpt_off;         //No
       PORTAbits.RA0 = 0x00;      //off LED
    } //end off

void Delay(void)
{
int i;
for(i=0; i<2048; i++);
}

[thekashyap]

1. Don't use goto, language provides so many other better/safer/understandable ways.
2. Seems like PORTAbits.RAXX is an int/short instead of a boolean. I would suggest:
A) Trace out their values.
B) See how to check such flags. I remember there was some thread where I described how bitwise operations are done for exactly this purpose.

[Salem]

Consider writing a version which runs on your PC first, to get all of the logic sorted out.

With practice, you can do this well enough that the amount of work you need to do to get it to also do the same thing on your target is pretty minimal. If the host works, and the target doesn't, and the number of changes are localised, it really narrows down the stuff you have to look at.

Eg.

#if defined(HOST)

#include <stdio.h>
#include <stdlib.h>

#else  /* HOST */

#include <p18f4620.h>

#pragma config OSC = HS                              
#pragma config WDT = OFF
#pragma config LVP = OFF

#define btn_on     PORTBbits.RB4
#define btn_off    PORTBbits.RB5
#define led        PORTAbits.RA0

#endif  /* HOST */

void led( int state );
int  pressed( int button, int state );

int main ( ) {
    int btn1 = 0, btn2 = 0;

#if !defined(HOST)
    TRISA = 0;  // set Port A(LED) as output
    led = 0;    // reset LED
#endif

    for ( ; ; ) {
        btn1 = pressed( 1, btn1 );
        btn2 = pressed( 2, btn2 );
        if ( btn1 && !btn2 ) {
            led( 1 );
        } else
        if ( btn2 && !btn1 ) {
            led( 0 );
        }
    }
}

void led ( int state ) {
#if defined(HOST)
    printf("The LED state is %d\n", state );
#else   /* HOST */
    if ( state == 1 ) {
        led = 0x0F;
    } else {
        led = 0x00;
    }
#endif  /* HOST */
}

int pressed ( int button, int state ) {
#if defined(HOST)
    char buff[100];
    printf( "Button %d is currently in state %d; 1=Press, 0=Release > ", 
            button, state );
    fflush(stdout);
    fgets( buff, sizeof buff, stdin );
    sscanf( buff, "%d", &state );
    return state;
#else   /* HOST */
    if ( button == 1 ) {
        return btn_on;
    } else {
        return btn_off;
    }
#endif  /* HOST */
}

Compiling on the host for example,

$ gcc -DHOST foo.c
$ ./a.exe
Button 1 is currently in state 0; 1=Press, 0=Release > 0
Button 2 is currently in state 0; 1=Press, 0=Release > 0
Button 1 is currently in state 0; 1=Press, 0=Release > 1
Button 2 is currently in state 0; 1=Press, 0=Release > 0
The LED state is 1
Button 1 is currently in state 1; 1=Press, 0=Release > 1
Button 2 is currently in state 0; 1=Press, 0=Release > 0
The LED state is 1
Button 1 is currently in state 1; 1=Press, 0=Release > 0
Button 2 is currently in state 0; 1=Press, 0=Release > 0
Button 1 is currently in state 0; 1=Press, 0=Release > 0
Button 2 is currently in state 0; 1=Press, 0=Release > 1
The LED state is 0
Button 1 is currently in state 0; 1=Press, 0=Release > 0
Button 2 is currently in state 1; 1=Press, 0=Release > 0
Button 1 is currently in state 0; 1=Press, 0=Release >

When you compile the same code for the target, it won't have "-DHOST" so you'll get all the code which reads switches directly and which writes to the LED directly. Assuming you're happy with the logic of how it worked on the host, then it should do the same on the target.