-1

I'm unable to break out of an if statement after a button has been depressed. The button activates a relay and the depression turns the relay off.

Here is my full code: I'm new to C so all comments and suggestions are much appreciated.

 #include "mcc_generated_files/mcc.h"
 #define FCY  8000000UL
 #include <libpic30.h>
//#define baudrate 19200

 int main(void)
 {
     // Initialise the device
      SYSTEM_Initialize();
      while(1)
      {
          LED4_SetLow();                    // Turn on 3v3 LED
          LED12v_SetHigh(); 
          LED3_SetHigh(); 
          LED1_SetHigh(); 

          int button;
          button = UART5_Read();
          if (button == 0x08)
          {
              RLY1_SetHigh();
              RLY3_SetHigh(); 
              if (button == 0x00)
                  goto finished;
          }

          finished:
          RLY1_SetLow();
          RLY3_SetLow();
      }   
      return (0);
}

I have now edited the code as follows. The relays are able to be set high but the condition 0x00 is never reached. Can someone please explain?

#include "mcc_generated_files/mcc.h"
#define FCY  8000000UL
#include <libpic30.h>
//#define baudrate 19200

int main(void)
{
    // Initialise the device
    SYSTEM_Initialize();
    while(1)
    {
        LED4_SetLow();                  // Turn on 3v3 LED
        LED12v_SetHigh(); 
        LED3_SetHigh(); 
        LED1_SetHigh(); 

        int button;
        int release;
        button = UART5_Read();
        release = UART5_Read();
        if (button == 0x08)
        {
            RLY1_SetHigh();
            RLY3_SetHigh(); 
        }
        if (release == 0x00)
        {
            RLY1_SetLow();
            RLY3_SetLow();
        }
        __delay_ms(1000);   
    }    
    return (0);
}
Mike
  • 4,041
  • 6
  • 20
  • 37
Finners
  • 37
  • 7
  • So did you debug step by step ? What's going on ? until what point in the code do you go ? Anyway you don't need a `goto` for that. `goto`is usually considered as a dangerous instruction and frequently forbidden by common coding rules. – Guillaume Petitjean Oct 18 '19 at 11:53
  • 1
    The code fragment you've shown is not sufficient to answer your question. Please edit your question to make it contain a [mcve]. – rustyx Oct 18 '19 at 11:53
  • The code you have posted doesn't make much sense. The `if (button == 0x00) goto finished;` jumps just past the end of the outer `if` block, but it is _already_ just before the end of the outer `if` block, so the whole `if (button == 0x00) goto finished;` statement doesn't achieve anything useful. – Ian Abbott Oct 18 '19 at 11:53
  • Besides Ian's comment, the button is `0x08` so will never be `0x00` (unless volatile change by function calls in if-block). – Paul Ogilvie Oct 18 '19 at 12:17
  • My earlier comment also applies to the full code you posted. Also, the value of the `button` variable doesn't change inside the outer `if (button == 0x08)` block, so the `if (button == 0x00)` condition will never be satisfied. – Ian Abbott Oct 18 '19 at 12:26
  • Why do you send binary 8 over UART? What does the protocol look like? – Lundin Oct 18 '19 at 12:59
  • Where do you initialize the UART? – Lundin Oct 18 '19 at 12:59
  • Lundin, 0x08 is the Up command in Pelco D protocol. The UART is initialised in SYSTEM_Initialise. The command 0x00 is the command that the GUI sends once the button has been de-pressed. – Finners Oct 18 '19 at 13:03

1 Answers1

0

I haven't used this MCU nor its libs, but I believe correct code should look something along the lines of this:

#include "mcc_generated_files/mcc.h"
#include <libpic30.h>
#include <stdint.h>

#define BUTTON_CODE 0x8u   // use named constants instead of magic numbers

void main (void)
{
  // Initialise the device
  SYSTEM_Initialize();

  for(;;)
  {
    uint8_t data;

    LED4_SetLow();                    // Turn on 3v3 LED
    LED12v_SetHigh(); 
    LED3_SetHigh(); 
    LED1_SetHigh(); 

    if(UART5_Data_Ready())
    {
      data = UART5_Read();
      if(data == BUTTON_CODE)
      {
        RLY1_SetHigh();
        RLY3_SetHigh(); 
      }
      else
      {
        RLY1_SetLow();
        RLY3_SetLow();
      }
    }
  }
}
Lundin
  • 195,001
  • 40
  • 254
  • 396