I am starting to learn C and I am stuck on multiple expressions in an if and if-else statement. I want a red led to be on and green led off when neither of 2 buttons are pressed but if the failsafe button is pressed then red is off and green is on. I have coded what I thought would work but the red light remains when I press it.

Can anyone see what I am missing?

#include <pic16f690.h>
#include <htc.h>

//Port initialize

#define Button_Input_Init TRISBbits.TRISB7
#define Fan_Red_Light_Output_Init TRISBbits.TRISB6
#define Fan_Green_Light_Output_Init TRISBbits.TRISB5
#define Failsafe_Input_Init TRISCbits.TRISC2

// Port bit names

#define Button_State PORTBbits.RB7
#define Red_Light_State PORTBbits.RB6
#define Green_Light_State PORTBbits.RB5
#define Failsafe_State PORTCbits.RC2

void main(void)
{
    // Initiate's I/O port bits

    Button_Input_Init = 1;
    Fan_Red_Light_Output_Init = 0;
    Fan_Green_Light_Output_Init = 0;
    Failsafe_Input_Init = 1;

   if(Button_State == 0 && Failsafe_State == 0)
    {
        
        Red_Light_State = 1;
        Green_Light_State = 0;

    }
   else if(Button_State == 0 && Failsafe_State == 1)
   {
       Red_Light_State = 0;
        Green_Light_State = 1;
   }
   
    while(1);

}

I am using MPLAB, PICKIT 2 and pic16f690

 

Since you may not be certain on the order of operations, always enclose your statements with brackets to make certain you know which operations are performed first, e.g.

if ( (Button_State == 0)  &&  (Failsafe_State == 0) )

You can also use DeMorgan's Equivalents to simplify the expression, e.g.

if ( Button_State ||  Failsafe_State )

Note the resulting code flow has to be reversed.

 

Thanks for the tip MrChips, Its still not working. Any ideas on why? I changed the code so if both buttons were 1 then the light would change to green. Does this mean that both buttons have to change to 1 at the exact same time? If not i'm not sure whats up.

 

Ok. Here is what it looks like to me.

Your code is executed sequentially. So you execute first if statement. Then you execute the else if statement. Then you enter while super loop and never exit it.

Of the top of my head it seems to me that your if and else if statements executed so fast that there is no time for user to change anything. So basically you are providing the definitions of what you want to detect. But you are not providing a detection method.

Normally the simplest way to do detection is to use polling. For the sake of simplicity at this point you can put your if statements inside the while super loop. This way the program will constantly check the states of the buttons.

Also. Your conditions for which light is On and which light is Off are separate, not related to each other. You should be using two if statements instead of if and else if. The two if statements should not be nested. Think of them as cases.
Case 1: if this and that happens, do X.
Case 2: if this other and that other happens, do Y.
See? The two cases are not related to each other, so you don't need a relationship of if-else.

 

I read that while(1) at the bottom of the code would be an infinite loop. I think I may of interpreted this wrong. So what you are saying is put the while(1) loop at the top before the if statements start?

 

I read that while(1) at the bottom of the code would be an infinite loop. I think I may of interpreted this wrong. So what you are saying is put the while(1) loop at the top before the if statements start?

while(1) is infinite loop, but it is also an empty loop.

What you want is this:

while(1)
{
  if(condition1 && condition2)
  {
     Do X;
  }

  if(condition3 && condition4)
  {
     Do Y;
  }
}

See? You have your conditions and commands inside the infinite loop. The program will constantly check the validity of the conditions (polling) and if one set of conditions is valid, the command attached to that condition will be executed.

 

while(1); <- see the semicolon!
Is the same as loop: goto loop
Your program ends in an infinite loop.

If you wan't to execute all statements in an infinite loop, put while(1) without semicolon at the top and the statements within curly brackets.

Edit: I was slow, shteii01 explained the same.

 

void main(void) {

BOOL exit = 0;

while (exit = 0) {
if (Button_State == 0) {
	if (Failsafe_State == 0) {
	Red_Light_State = 1;
	Green_Light_State = 0;
	} else { // Failsafe_State
	Red_Light_State = 0;
	Green_Light_State = 1;
	} // Failsafe_State
} // Button_State
} // while

} // main

 

I am starting to learn C and I am stuck on multiple expressions in an if and if-else statement. I want a red led to be on and green led off when neither of 2 buttons are pressed but if the failsafe button is pressed then red is off and green is on. I have coded what I thought would work but the red light remains when I press it.

Can anyone see what I am missing?
...

I think the main thing you are missing is telling us if the button/LED states are temporary or latching! We need to know that.