Using RFID tags to controll a servo

Hello fellow buiders and coders,

some of you might have seen me posing before with a lot of questions, but now i have quite a code, and I would realy like it if you guys could give me some feadback.
so the idea is this, we have a buoy with a RFID tag strapped onto it, and a hovercraft with an Arduino Uno and a RFID reader.
so the hovercraft makes it way towards the buoy, and at a range of approx 0.3m the tag is sensed, this data stream flows into arduino, and is used to give a specific order to the servo controlling a "valve", or better said, the servo releases golf balls into the water.
so here is my code version 1.1, and I dont think its that good yet, so could you guys give me some feedback?
im not to familiar with all the different commands, so any suggestions on re-phrasing certain code blocks are welcome as well :slight_smile:

#include <Servo.h>
Servo myservo;  //servo is in the system
int servoPin = 9;  //at pin 9
int RFIDResetPin = 13;  //RFID reader data stream on pin 13
int val = 0;
int pos = 0; //servo start position 0, schould be a set position once implemented in the disign

char tag1[13] = "1E009A4067A3";  //the ID codes are not from the tags i have in position, how can i find those tag numbers?
char tag2[13] = "010230F28243";

void setup(){
  pinMode(servoPin, OUTPUT);    //servo = output
  myservo.attach(9);
  Serial.begin(9600);  //I copied this serial.begin(9600), what does it stand for?
 
  pinMode(RFIDResetPin, OUTPUT);
  digitalWrite(RFIDResetPin, HIGH); //i cant help but to notice in my setup I have no INPUT, is this nesseserily?
}

void loop(){
  
  char tagString[13];
  int index = 0;
  boolean reading = false;

   while(Serial.available()){

    int readByte = Serial.read(); //read next available byte

    if(readByte == 2) reading = true; //begining of tag
    if(readByte == 3) reading = false; //end of tag
    
    if(reading && readByte != 2 && readByte != 10 && readByte != 13){
      //store the tag
      tagString[index] = readByte;
      index ++;
    }
  }
  checkTag(tagString); //Check if it is a match
  clearTag(tagString); //Clear the char of all value
  resetReader(); //eset the RFID reader
}
void checkTag(char tag[]){
  
  if(strlen(tag) == 0) return; //empty, no need to contunue

  if(compareTag(tag, tag1)){ // if matched tag1, do this: make the servo move, i have not yet updated the positions, this is a test.
    for(pos = 0; pos < 180; pos +=1) //from 0 to 180
  {
    myservo.write(pos);
    delay(15); //wait 15ms
  }
  for(pos = 180; pos>=1; pos-=1) //from 180 to 0
  {
    myservo.write(pos);
    delay(15);  //wait 15ms
  }
  }else if(compareTag(tag, tag2)){ //if matched tag2, do this
   for(pos = 0; pos < 180; pos +=1) //same story, i got 2 tags, so i can make different commands for each tag
  {
    myservo.write(pos);
    delay(15);
  }
  for(pos = 180; pos>=1; pos-=1)
  {
    myservo.write(pos);
    delay(15);
  }
    }else{
    Serial.println(tag); //read out any unknown tag, does it show these readings somewhere?
  }
  }
void resetReader(){
///////////////////////////////////
//Reset the RFID reader to read again.
///////////////////////////////////
  digitalWrite(RFIDResetPin, LOW);
  digitalWrite(RFIDResetPin, HIGH);
  delay(150);
}
void clearTag(char one[]){
///////////////////////////////////
//clear the char array by filling with null - ASCII 0
//Will think same tag has been read otherwise
///////////////////////////////////
  for(int i = 0; i < strlen(one); i++){
    one[i] = 0;
  }
}
boolean compareTag(char one[], char two[]){
///////////////////////////////////
//compare two value to see if same,
//strcmp not working 100% so we do this
///////////////////////////////////

  if(strlen(one) == 0) return false; //empty

  for(int i = 0; i < 12; i++){
    if(one[i] != two[i]) return false;
  }

  return true; //no mismatches
}

read it trough, and plz tell me what you think.
this topic is purely about the code btw, tomorrow I will start testing it with the hardware.

You can find the tag numbers I spose by doing a test sketch to output any scanned id to serial.

You mix conventions.

i++;
i += 1;
i = i +1;

//In most circumstances pre-increment/decrement is the best choice ( fastest )
++i;

These are pretty much the same thing and the compiler will most probably modify them to the best version any way, however consistency should be maintained. You are commenting lines of code that are pretty much self explanatory while others without comments could do with some clarification.

ie.

int readByte = Serial.read(); //read next available byte

if(reading && readByte != 2 && readByte != 10 && readByte != 13){
...

also again considering the above if, someone that is reading/editing/updating your code may not have memorised operator precedence. Brackets can make a long algorithm far simpler to read/understand. This is not a super example but huge mathematical algorithms are easy to mis-calculate if precedence isn't considered ( or maybe confusion due to complexity ).

If you know an array/pointer is valid, and you are checking its an empty string, don't use a function call to do it.

//This checks if a string is empty.
if(strlen(tag) == 0)

//This is the same.
if( *tag == 0 )

//Or this to keep your text code character based.
if( *tag == '\0' )

Each section of a for loop can contain fully formed statements.

for(int i = 0; i < strlen(one); i++){
    one[i] = 0;
  }

//Or
for(int i = 0 ; i < strlen(one) ; one[ i++ ] = 0 );

strlen(one) maybe should be calculated into a temporary as 'one' is modified in the loop and might not be optimised automatically due to the modifications, therefore called every loop even though the size never changes... maybe

also

//>= and -= may be preventing optimisations.
for(pos = 180; pos>=1; pos-=1)

//May not
for(pos = 180; pos ; --pos )

This could be considered nit-picking ( well it is... ) as compilers are pretty smart and some things may not be necessary with our modern optimising compilers ( you may one day run into a compiler that wont optimise for you and cause your code to be unusable if heavy optimisations are expected ). Depending on the surrounding code compilers may confuse your intentions and optimise incorrectly. Mind you it is very hard to confuse the compiler, but if you write unpredictable code, you can't expect the compiler to predict your intentions ( not that you have, but one day might ). One mild proof of this concept is the volatile keyword, used extensively in Arduino interrupt code and on OS based systems for thread and process safety.