[SOLVED] Simple if statement problem!

Using Arduino Programming Questions
#1

Hi
I have a problem that an if statement is executed even if the statement is not true. Bellow is the part of code that is concerned.

void VB_Program(){
  
 Serial.print("Option :");
 Serial.print(Option);
 Serial.print(", Servo No :");
 Serial.print(ServoNo);
 Serial.print(", Option Val :");
 Serial.print(Option_Val);
 Serial.print(", Write :");
 Serial.print(Write);
 Serial.print(", High/Low :");
 Serial.print(High_Low_End_Point);
 Serial.print(", Option 2 :");
 Serial.println(Option2);
  //Serial.println("VB");
  
  DetachAllServos();                //Detach all servos to allow Callibration servo to be attaced where needed
  ParseSerialString();              //Read and parse serial string into Option, ServoNo, OptionValue, Write, High_Low_End_Point, Option2
  Set_VB_Cal_Vals();                //Set values used in callibrating with the  VB program

  //ValC = EEPROM.read(ValE);         //Read EEPROM for current callibration area
  if(First_Run == true){            //Only allow this set of steps to happen once at start
    if(Option == 1 || Option == 2 || Option == 3 || Option == 5 || Option == 6){
      ValC = EEPROM.read(ValE);     //Read EEPROM for current callibration area
      Serial.println(ValC);         //Send ValC - Current EEPROM value to VB program 
    }
    else if(Option == 4 ||Option == 7 || Option == 8){
      Val_a = EEPROM.read(ValE_A);  //8 bit hvalue
      Val_b = EEPROM.read(ValE_B);  //8 bit lvalue
        ValC = word(Val_a, Val_b);  //Set 16 bit value from 2 8 bit values
        Serial.println(ValC);       //Send ValC - Current EEPROM value to VB program 
    }
    CallibrationServo.detach();              //Detach old Callibration Servo
    CallibrationServo.attach(Call_Servo);    //Attach to new callibration Servo
    
    if(Option == 1 || Option == 2){ //End Limits selected
      CallibrationServo.write(ValC);                      //Move servo to current position set in EEPROM
    }    
    First_Run = false;                                    //Prevent from happening again
  }
//---------------------------------------------------------------------------------------------------------------  
  
//Enables, Servo/Air, Reversing
  if(Option == 3 || Option == 5 || Option == 6){
    
    TrimVal = Option_Val;          //Set new TrimsVal from VB program
    
    //Same value as in EEPROM
    //             Foward                      Reverse
    if((Option_Val == 0 && ValC == 0) || (Option_Val == 1 && ValC == 1)){
      Value_Send = "Already " + Value_Send;   //Set string to send to VB program
      Serial.println(Value_Send);              //Send string already ..... to VB program
    }
    //Differant Value from EEPROM
    else if((Option_Val == 0 and ValC == 1) || (Option_Val == 1 and ValC == 0)){       
      Value_Send1 = "Now " + Value_Send;
      Value_Send = "Change to " + Value_Send; //Set string to send to VB program
      Serial.println(Value_Send);              //Send string Change to ..... to VB program
    }
//**********Problem here*************************************  
 //Save button pressed in VB program
    if(Write == 1){
      EEPROM.write(ValE, TrimVal);             //Write new value to EEPROM
      ValC = EEPROM.read(ValE);                //Read EEPROM again
      if((ValC == 1 && Option_Val == 1) || (ValC == 0 && Option_Val == 0)){  //Check to see new value was writen to EEPROM sucesfully
        Serial.println(Value_Send1);           //Send confirmation of value changed
      Write = 0;  //reset Write (may need to be a timer.)
      }
      else Serial.println ("Error writing new value");
    }
//*********************************************************
  }
//---------------------------------------------------------------------------------------------------------------    
  //End Limits
  else if(Option == 1 || Option == 2){
    TrimVal = Option_Val;                           //Set new TrimsVal from VB program
    CallibrationServo.write(TrimVal);                          //Move servo to current position set in VB program
    Serial.println(TrimVal);                                   //Send value back to vb program
    
    //Save button pressed in VB program
    if(Write == 1){
      EEPROM.write(ValE, TrimVal);                             //Write new value to EEPROM  
      ValC = EEPROM.read(ValE);                                 //Read EEPROM again
      if(ValC == Option_Val) Serial.println(Value);  //Send confirmation of value changed
      else Serial.println ("Error writing new value");
    }  
  }
//---------------------------------------------------------------------------------------------------------------    
  //Speed type Cals
  else if(Option == 4 || Option == 7 || Option == 8){
    Speed_Val = Option_Val; //Set new Speed type value from VB program 
    Serial.println(Speed_Val);         //Send new value back to vb program
    
    //Save button pressed in VB program
    if(Write == 1){
      Val1_a = highByte(Speed_Val);    //8-bit hvalue
      Val1_b = lowByte(Speed_Val);     //8-bit lvalue    
        EEPROM.write(ValE_A, Val1_a);  //Write new value to EEPROM
        EEPROM.write(ValE_B, Val1_b);  //Write new value to EEPROM
        
      ValC_a = EEPROM.read(ValE_A);    //Read EEPROM again
      ValC_b = EEPROM.read(ValE_B);    //Read EEPROM again
        ValC = word(ValC_a, ValC_b);   //Set ValC from 2 EEPROM locations
        
      if(ValC == Option_Val) Serial.println(Val);  //Send confirmation of value changed
      else Serial.println ("Error writing new value");  
    }
    
  }
//---------------------------------------------------------------------------------------------------------------  
  
}

The problem lies in the line “if(Write == 1){”
if I enter <3,2,0,0,0,0> (Option = 3, ServoNo = 2, Option_Val = 0, Write = 0) in the serial monitor everything works as wanted and " if(Write == 1){" is not executed however if I enter <3,2,1,0,0,0> (Option = 3, ServoNo = 2, Option_Val = 1, Write = 0) “if(Write == 1){” is executed as “Serial.println(Value_Send1);” is executed and printed over the serial monitor even though Write = 0 (Write is the 4th number in the string of 6 entered) as can be seen in the following code which is used to parse the serial commands. I have also confirmed that Write = 0 with the serial.prints at the top of the code shown. How is it possible that this statement is executed???

//=============================================================================================
  //Read Serial String
  // With help from http://jhaskellsblog.blogspot.com.au/2011/05/serial-comm-fundamentals-on-arduino.html#comment-form

boolean getSerialString(){
  static byte dataBufferIndex = 0;
  while(Serial.available()>0){
    char incomingbyte = Serial.read();
      
    if(storeString){
      //Let's check our index here, and abort if we're outside our buffer size
      //We use our define here so our buffer size can be easily modified
      if(dataBufferIndex==DATABUFFERSIZE){
        //Oops, our index is pointing to an array element outside our buffer.
        dataBufferIndex = 0;
        break;
      }
            
      if(incomingbyte==endChar){
        dataBuffer[dataBufferIndex] = 0; //null terminate the C string
        //Our data string is complete.  return true
        return true;
      }
      else{
        dataBuffer[dataBufferIndex++] = incomingbyte;
        dataBuffer[dataBufferIndex] = 0; //null terminate the C string
      }
    }
                
    if(incomingbyte==startChar){
      dataBufferIndex = 0;  //Initialize our dataBufferIndex variable
      storeString = true;
    }       
    else{
    }
  }//End of while loop
    
    //We've read in all the available Serial data, and don't have a valid string yet, so return false
    return false;
}//End of boolean getSerialString
//===================================================================================================================
  //Parse Serial String
void ParseSerialString(){
  
  if(getSerialString()){  //getSerialString == true.  String available for parsing
     
     valPosition = strtok (dataBuffer, delimiters);          
    
     for(int i = 0; i < 6; i++){
      Serial_Data[i] = atoi(valPosition);        
      valPosition = strtok(NULL, delimiters);
     } 
    
     Option = Serial_Data[0];
     ServoNo = Serial_Data[1];
     Option_Val = Serial_Data[2];
     Write = Serial_Data[3];
     High_Low_End_Point = Serial_Data[4];
     Option2 = Serial_Data[5];
  }
}//End of void SerialString
//================================================================================================

Any help or ideas???

#2

How do you know you are seeing Serial.println(Value_Send1); rather than say Serial.println(TrimVal); ? It may be worth prefixing the serial prints with the name of the parameter.

#3

As
Serial.println(Value_Send1); would = “Now” + Value_Send eg “Now Foward”
Value send is set in void Set_VB_Cal_Vals() as either “Foward” or “Reverse”

while
Serial.println(TrimVal); would = Option_Val which is an integer sent from the serial monitor in this position <0,0,Option_Val as an int,0,0,0> eg “1”

It may be worth prefixing the serial prints with the name of the parameter.

I thought I did with the following

Serial.print("Option :");
 Serial.print(Option);
 Serial.print(", Servo No :");
 Serial.print(ServoNo);
 Serial.print(", Option Val :");
 Serial.print(Option_Val);
 Serial.print(", Write :");
 Serial.print(Write);
 Serial.print(", High/Low :");
 Serial.print(High_Low_End_Point);
 Serial.print(", Option 2 :");
 Serial.println(Option2);
#4

I thought I did with the following

Indeed. But I suspect you'll find it a lot easier to debug if you put identifying strings in before the other prints too.

#5

Putting each { on a new line, and using Tools + Auto Format, would make it easier to read your code. Creating some additional functions, to deal with various aspects of the input would be useful, too. Debugging 20 lines of code is far easier than debugging 200.

#6

grantastley:
As
Serial.println(Value_Send1); would = “Now” + Value_Send eg “Now Foward”
Value send is set in void Set_VB_Cal_Vals() as either “Foward” or “Reverse”

while
Serial.println(TrimVal); would = Option_Val which is an integer sent from the serial monitor in this position <0,0,Option_Val as an int,0,0,0> eg “1”

It may be worth prefixing the serial prints with the name of the parameter.

I thought I did with the following

Serial.print("Option :");

Serial.print(Option);
Serial.print(", Servo No :");
Serial.print(ServoNo);
Serial.print(", Option Val :");
Serial.print(Option_Val);
Serial.print(", Write :");
Serial.print(Write);
Serial.print(", High/Low :");
Serial.print(High_Low_End_Point);
Serial.print(", Option 2 :");
Serial.println(Option2);

I meant in this area of code:

    if(Write == 1){
      EEPROM.write(ValE, TrimVal);             //Write new value to EEPROM
      ValC = EEPROM.read(ValE);                //Read EEPROM again
      if((ValC == 1 && Option_Val == 1) || (ValC == 0 && Option_Val == 0)){  //Check to see new value was writen to EEPROM sucesfully
        Serial.println(Value_Send1);           //Send confirmation of value changed
      Write = 0;  //reset Write (may need to be a timer.)
      }
      else Serial.println ("Error writing new value");
    }
//*********************************************************
  }
//---------------------------------------------------------------------------------------------------------------    
  //End Limits
  else if(Option == 1 || Option == 2){
    TrimVal = Option_Val;                           //Set new TrimsVal from VB program
    CallibrationServo.write(TrimVal);                          //Move servo to current position set in VB program
    Serial.println(TrimVal);                                   //Send value back to vb program

So that you definitely know that it is entering the if(Write == 1){ block. Lets face it if Write==0 it is not entering that block, so etiher the value of Write is changing or the output you see does not come from that block.

#7

So a new day and a bit of time to think about it all and I found the problem :slight_smile:

I meant in this area of code:

Sorry. I realised what you ment after I posted the coment and did so this morning as follows

 //Save button pressed in VB program
    Serial.print(", Write :");
    Serial.print(Write); 
    if(Write == 1){
      Serial.println("in write/wrong area");
      EEPROM.write(ValE, TrimVal);             //Write new value to EEPROM
      ValC = EEPROM.read(ValE);                //Read EEPROM again
      if(Write == 1 && ((ValC == 1 && Option_Val == 1) || (ValC == 0 && Option_Val == 0))){  //Check to see new value was writen to EEPROM sucesfully
        Serial.println(Value_Send1);           //Send confirmation of value changed
      Write = 0;  //reset Write (may need to be a timer.
      }
      else Serial.println ("Error writing new value");
    }

and found that Write was actually equaling 1 before the if(Write == 1) statement but was then returning to 0 with the Write = 0; line of code. I found the area that it was being set to 1 (in the Set_VB_Cal_Vals(); function) so fixed that and all works as intended :slight_smile:

Creating some additional functions, to deal with various aspects of the input would be useful, too. Debugging 20 lines of code is far easier than debugging 200.

Do you mean like making a seporate a function for each area I have seporated by //----------------------? I would still need to debug the whole lot anyway and I have already split the code up into seporate functions and multiple tabs. What has been displayed here is only a small fraction of the whole of the code.

PS a bit off topic but speaking of tabs. Is there a way to display all tabs at the top of the IDE like maybe going onto 2 rows instead of just the 1?

Putting each { on a new line, and using Tools + Auto Format

I thought other than having the { on a new line my code was formated correctly? I leave the { on the same line as that is how I prefere it. I find having it on a seporate line bulks up the code so to say. The } is at the same indent as the if, while, void ect so it can still be distiguished as the closing bracket for that area easily. That is only a personal preferance though and as such in the future when posting here I will format it as you described to try make it as easy to read as possible, as that seems to be the prefered way.

#8

As for next time you could just put Serial.println(Write) inside the block, that way you see more clearly what’s going on :wink:

#9

Glad you got it fixed

#10