Could not call the sub-routine correctly Microcontroller PIC18F

Question

I have a project to program a microcontroller PIC18F, I have to connect a switching circuit to the microcontroller board, this switching circuit has an electric lock and a buzzer to be connected to it.

The lock is initially powered. It is supposed that when I send '1', the buzzer will be powered with a square wave and the lock will be powered off. When it receives '0', the buzzer will be switched off without powering the lock again. When it receives '2' the lock should be powered but if the buzzer was unpowered before, it should not be powered again.

My confusion is in the last part. When I send '2' via the hyperterminal, and I sent '0' before it, the buzzer is powered again.

Here is the code,

void buzzertest();
char uart_rd;
int buzzer;
void main() {
TRISB=0X00;
PORTB=0x00;
RB5_bit = 0xFF;                  //lock  open
UART1_Init(9600);               // Initialize UART module at 9600 bps
while (1) {                     // Endless loop
 if (UART1_Data_Ready())       // If data is received,
 {
    buzzer=1;
    uart_rd = UART1_Read();     // read the received data,
    if(uart_rd =='1') {
       RB5_bit = 0x00;  //lock closed
       buzzertest();
     }
     if(uart_rd =='0' ){   //disable buzzer
        RB1_bit = 0x00;   //buzzer
        buzzer=0;
       }//end if
      buzzer=0;

      if(uart_rd =='2'){   //disable lock
        RB5_bit=0xFF;
        if(buzzer!=1){
            buzzertest();
         }
       }//end if
     } //end outer if
    } //end while
}//end main
void buzzertest(){
 while(1){
  RB1_bit = 0xFF;  //buzzer
  Delay_ms(1000);
  RB1_bit = 0x00; //buzzer
  Delay_ms(1000);
  if (UART1_Data_Ready())
  break;
 }//end while loop
}

Can please anyone help me solving this?

You are unnecessarily testing uart_rd three times while its value cannot change - use a `switch( uart_rd )` instead. — Clifford, May 22 '12 at 18:03

score 2 · Accepted Answer · answered May 22 '12 at 10:50

2

You're setting buzzer to 0 outside the if(uart_rd='0') block. So when you come to the if(uart_rd='2') block, buzzer is always 0 and so the if(buzzer!=1) block is always called.

Have you tried stepping through this with a debugger? It would show up this kind of thing easily. You could also change those if blocks either to a switch statement or a series of if / else if statements to avoid these sorts of issues.

answered May 22 '12 at 10:50

Vicky

12,934
4
46
54

sorry if(buzzer!=1) it is a typo, it was if(buzzer==1) – Sahar Nadi May 22 '12 at 10:52
1

OK, if you want us to debug your code, you need to post the actual code you are using. Have you tried a debugger as I suggested? – Vicky May 22 '12 at 10:57
Vicky is correct. You need to clean up your logic. You have two outputs (buzzer and lock), one input (the UART), and one state variable (buzzer active). Think of it in these terms. – Adam Casey May 22 '12 at 14:47
1

@Sahar: If the code you posted is not identical (i.e. a copy & paste) to the code you are running, we are wasting time looking at it! Now the error has been pointed out why don't you correct it!? Either way the unconditional `buzzer=0` line will prevent it working by either always enabling the buzzer or never enabling it - depending on what code your are really running. – Clifford May 22 '12 at 17:56
Thanks all of your reply, it is working now. I'll post the running code in minutes – Sahar Nadi May 23 '12 at 09:13

score 0 · Answer 2 · answered May 23 '12 at 09:19

here is the running code:

void buzzertest();
char uart_rd;
int buzzer;
void main() {
TRISB=0X00;
PORTB=0x00;
RB5_bit = 0xFF;                  //lock  open
UART1_Init(9600);               // Initialize UART module at 9600 bps
while (1) {                     // Endless loop
if (UART1_Data_Ready())       // If data is received,
 {
  uart_rd = UART1_Read();     // read the received data,
  if(uart_rd =='1') {
    RB5_bit = 0x00;  //lock closed
    buzzertest();
    buzzer=1  ;
   }
  else if(uart_rd =='0' ){   //disable buzzer
    RB1_bit = 0x00;   //buzzer
    buzzer=0;
   }//end else if

   else if(uart_rd =='2'){   //disable lock
     RB5_bit=0xFF;
     if(buzzer==1){
       buzzertest();
      }
    }//end else if
   } //end outer if
 } //end while
}//end main
void buzzertest(){
 while(1){
  RB1_bit = 0xFF;  //buzzer
  Delay_ms(1000);
  RB1_bit = 0x00; //buzzer
  Delay_ms(1000);
  if (UART1_Data_Ready())
   break;
 }//end while loop
}

Are you posting this as the "solution" or is this still the code that does not work? Since there was some uncertainty from your comments about whether the code you posted in the question was "real", it is now unclear whether this is a response to that, in which case it should be an edit to the question, or if it is your final corrected code, in which case it is unnecessary. The `if..else if..else` construct is better, but a `switch` would be more appropriate - this requires 1 to 3 tests where a switch has only one. — Clifford, May 24 '12 at 01:32
@Clifford .. Actually this is the solution, the second posted code is the correct one, I'll rewrite it in a switch instead of if conditions, I think yes it will be more better, Thanks! :) — Sahar Nadi, May 26 '12 at 15:49

Could not call the sub-routine correctly Microcontroller PIC18F

2 Answers2