Combine codes for RFID

Hi Guys,
I have here a Parallax RFID Read/Write Module: http://www.parallax.com/Store/Accessories/CommunicationRF/tabid/161/ProductID/688/List/0/Default.aspx?SortField=ProductName,ProductName
and a EM4x50 tag: http://www.parallax.com/Store/Accessories/CommunicationRF/tabid/161/ProductID/689/List/0/Default.aspx?SortField=ProductName,ProductName

And I found these 2 codes on how to read and write data in the tag:

Read:

#include <SoftwareSerial.h>
#define RFID_READ 0x01
#define txPin 6
#define rxPin 8
#define whichSpace 9
SoftwareSerial mySerial(rxPin, txPin);
int val;
int runs = 0;
int points = 0;
void setup()
{
  Serial.begin(9600);
  Serial.println("RFID Read/Write Test");
  mySerial.begin(9600);
  pinMode(txPin, OUTPUT);    
  pinMode(rxPin, INPUT);      
}

void suppressAll()                                //suppresses the "null result" from being printed if no RFID tag is present
{
    if(mySerial.available() > 0)
    { mySerial.read();
      suppressAll();
    }
}

void loop()
{
 int val;
  mySerial.print("!RW");
  mySerial.write(byte(RFID_READ));
  mySerial.write(byte(whichSpace));

  if(mySerial.available() > 0)
  {      
    val = mySerial.read();                        //The mySerial.read() procedure is called, but the result is not printed because I don't want the "error message: 1" cluttering up the serial monitor
      if (val != 1)                                   //If the error code is anything other than 1, then the RFID tag was not read correctly and any data collected is meaningless. In this case since we don't care about the resultant values they can be suppressed
       {suppressAll();}                              
  }      


 if(mySerial.available() > 0) {      
    val = mySerial.read();
    Serial.print("1st:");
    Serial.println(val, DEC);
   points = val; 
 }

if(mySerial.available() > 0) {        
    val = mySerial.read();
    Serial.print("2nd:");
    Serial.println(val, DEC);
    }

if(mySerial.available() > 0) {      
    val = mySerial.read();
    Serial.print("3rd:");
    Serial.println(val, DEC);
    }

if(mySerial.available() > 0) {          
    val = mySerial.read();
    Serial.print("4th:");
    Serial.println(val, DEC);
    Serial.println("-----------------");
   }  
 delay(750);
}

Write:

#include <SoftwareSerial.h>
 #define RFID_WRITE 0x02
 #define txPin 6
 #define rxPin 8

 #define whichSpace 9

 #define first 1                 // first, second, thrid, and fourth are four arbitrary values which will be written to the RFID tag at address whichSpace
 #define second 26
 #define third 3
 #define fourth 27

SoftwareSerial mySerial(rxPin, txPin);

void setup()
{
  Serial.begin(9600);
  Serial.println("RFID Write Test");
  mySerial.begin(9600);
  pinMode(txPin, OUTPUT);     
  pinMode(rxPin, INPUT);      
}


void suppressAll()                                      //Keeps error code & the "write confirmation" codes from being printed in the serial monitor       
{
    if(mySerial.available() > 0)
    { mySerial.read();
      suppressAll();
    }
} 

 void loop()
{
  int val;

  mySerial.print("!RW");
  mySerial.write(byte(RFID_WRITE));
  mySerial.write(byte(whichSpace));
  mySerial.write(byte(first));
  mySerial.write(byte(second));
  mySerial.write(byte(third));
  mySerial.write(byte(fourth));

if(mySerial.available() > 0) {        
    val = mySerial.read();
    if (val == 1)                                        //If data was written successfully
      { Serial.println("Data written succesfully!");
        suppressAll();
      }
    else suppressAll();                                  //If an error occured during writing, discard all data recieved from the RFID writer
    }
delay(250);
}

these 2 codes are from: Arduino Playground - ParallaxRFIDreadwritemodule

My problem here is that I do not know how to combine these 2 into a single code, here's how I wanted the code to execute:

  1. Read data from tag
  2. Save "first" as integer "points"
  3. When button is pressed, "points" will add by 1 then prompt user to tab card again
  4. when card is present, write "points" into card and display "Data written successfully"
    note: data cannot be read when button is pressed.

I tried combining the two codes, but it will write a random number, most of the time it will write first:33, second:82, third:87, fourth:2

This is my failed code:

#include <SoftwareSerial.h>
 #define RFID_READ 0x01
 #define RFID_WRITE 0x02
 #define txPin 6
 #define rxPin 8

 #define whichSpace 9

 #define first points                 // first, second, thrid, and fourth are four arbitrary values which will be written to the RFID tag at address whichSpace
 #define second 0
 #define third 0
 #define fourth 0

SoftwareSerial mySerial(rxPin, txPin);
int val;
int runs = 0;
const int buttonPin = 13;
int buttonState = 0;
int buttonCount = 0;
int points = 0;

void setup()
{
  Serial.begin(9600);
  Serial.println("RFID Write Test");
  mySerial.begin(9600);
  pinMode(txPin, OUTPUT);     
  pinMode(rxPin, INPUT);      
}


void suppressAll()                                      //Keeps error code & the "write confirmation" codes from being printed in the serial monitor       
{
    if(mySerial.available() > 0)
    { mySerial.read();
      suppressAll();
    }
} 

 void loop()
{int val; mySerial.print("!RW");
  mySerial.write(byte(RFID_READ));
  mySerial.write(byte(whichSpace));

  if(mySerial.available() > 0)
  {      
    val = mySerial.read();                        //The mySerial.read() procedure is called, but the result is not printed because I don't want the "error message: 1" cluttering up the serial monitor
      if (val != 1)                                   //If the error code is anything other than 1, then the RFID tag was not read correctly and any data collected is meaningless. In this case since we don't care about the resultant values they can be suppressed
       {suppressAll();}                              
  }      


 if(mySerial.available() > 0) {      
    val = mySerial.read();
    Serial.print("1st:");
    Serial.println(val, DEC);
   points = val; //value read will be saved as points
 }

if(mySerial.available() > 0) {        
    val = mySerial.read();
    Serial.print("2nd:");
    Serial.println(val, DEC);
    }//read second

if(mySerial.available() > 0) {      
    val = mySerial.read();
    Serial.print("3rd:");
    Serial.println(val, DEC);
    }// read third

if(mySerial.available() > 0) {          
    val = mySerial.read();
    Serial.print("4th:");
    Serial.println(val, DEC);
    Serial.println("-----------------");
   }  //read fourth
  Serial.println(points);
 delay(750);
 // Up to this point is the reading code
 
while(digitalRead(buttonPin) == HIGH){
  buttonCount = buttonCount + 1;
 points = points + 1;}
 
 while(buttonCount != 0){
   //write starts here
   Serial.println("Tab Card Again");
  mySerial.print("!RW");
  mySerial.write(byte(RFID_WRITE));
  mySerial.write(byte(whichSpace));
  mySerial.write(byte(points));
  mySerial.write(byte(second));
  mySerial.write(byte(third));
  mySerial.write(byte(fourth));

if(mySerial.available() > 0) {        
    val = mySerial.read();
    if (val == 1)                                        //If data was written successfully
      { Serial.println("Data written succesfully!");
       buttonCount = 0; //buttoncount will reset when write is successful
       suppressAll();
      }
    else suppressAll();                                  //If an error occured during writing, discard all data recieved from the RFID writer
    }
    
delay(250);
}
}

Attached is my hardware connection
Please help me out. Thanks

RFID connection.jpg

I've not had a chance to go through your code, as I'm on an iPhone, but my initial thought would be debounce or multiple sample issues.

If you don't allow for it then pressing a button once can be read as an 'on' state many times, depending on how long the press is. This, and/or contact bounce, could account for your 'random' increments. Your code could be working fine, just incrementimg multiple times. Remember your MCU is running at 16MHz so is checking that pin state very often.

Look into debouncing and also checking for the change of state, rather than the state itself, I.e when the button goes from open to closed and vice versa. That way you will only register a single press each time.

EDIT: Just had a quick look and the issue above seems to be one as you increment the 'points' variable as long as the button is pressed. The longer you hold this the higher that value will be as it is in incrementing on every loop, while the button is pressed.

You also use #define for first, second, third, fourth when this isn't how you should use #define.

#define is like an alias, it's a name you give to a value to make it easier to use. So, when you have something like #define REDPIN 4 or just means that instead of remembering you have a red LED on pin 4 when you want to turn it on you can use REDPIN in place of the 4.

You should just declare your first, second, third, fourth as variables and lose the #defines for them. You've declared first as int first = 0 to start, which is fine. You should do the same for the others. If they will never be negative then you can use 'unsigned int'. If you need them to exceed 65535 then you'll need it to be a long. At the moment an int allows a range of -32767 to +32767

Another thing to bear in mind is that an int on the Arduino is actually 16 bits, where you seem to be writing a 'byte' to the card, which is 8 bits. That may cause issues with datatype mismatching.

Wow thanks for the detailed reply, tack.
Just to clear things about the #define part, I do think that what you mentioned is true, however, to be on the safe side, I'd like to follow the original program and #define the first, second, third, fourth. Hope you will understand.

I have also solved the problem by removing the "else suppressAll(); " from the write code. Although it will take a while for the data to be written into the card, it worked properly and wrote the correct data into the tag.

Sorry for troubling all of you. Thanks for helping out. I really appreciate it.

In the example code the #define was used to set an alias to each value. You now want to use those as proper variables, so using #define isn't the way to do it.

#define allows a constant value to be given a name and then this is processed before the sketch is compiled. So, if you do '#define points 0', then later on do 'int points = 10;' then that second statement gets 'points' replaced with '0' before compiling, hence the second statement becomes 'int 0 = 10'. You see how that can lead to confusion and errors?

The example write code basically said 'where I write "first" replace it with "1" before compiling', where I write "second" replace it with "26" before compiling, etc

The read sketch never used #define for the 'first' variable, it declared it as an 'int', since you would be assigning whatever value you read from the card. When you combine the two sketches you need to lose the #defines as the values are going to now be proper variables.

Your code also has such a confusion at the beginning where you do '#define first points'. This isn't assigning the value of a 'points' variable to 'first'. This actually says 'wherever I say "first" I want it replacing with "points" before compiling'. Think of it like a search and replace before the code is compiled.

You should declare and set your variables based on the type and extent of then. I.e, if they will really be positive or negative, between -37675 and +37675 then declare as int (16 bits). If it'll be in the range 0 to 65535 then unsigned int. Accordingly if larger number then long or unsigned long.

You need to look at some basic programming before modifying the existing code, so that you understand the datatypes and ranges. The example was using static values and hadn't declared that they were 16 bit values, compared to what appears to be you entering 8 bit values to the RFID tag.

If it's definitely a value of 0-255 you need then I'd be inclined to declare as:-

byte first = 0, second = 0, third = 0, fourth = 0;

To ensure you have 8 bit datatypes you are sending to the RFID tag in your write code.

As tack points out, you need to declare a set of variables to hold the values read from the card, if you want to store them and write them back. You do use a variable for points so this is probably enough to read/write the first value on the card, but you won't be storing the subsequent values.

The function suppressAll() is recursive (it calls itself) which is dangerous (can result in stack overflow) and is not needed here. Judging by the comment "//Keeps error code & the "write confirmation" codes from being printed in the serial monitor" I guess you are just trying to flush all the data received on mySerial. You could do that without recursion using:

while(mySerial.available() > 0)
{
    (void) mySerial.read();  // returned character is discarded
}

In loop() you send an RFID_READ command over mySerial (presumably to the RFID reader) and then check for a response on mySerial four times. Before the response is available your command has to be transmitted bit by bit over the slow serial link, the reader has to process the command and generate a response, and the response has to be dribbled back over the slow serial link to the Arduino. This will take far, far longer than the time needed to execute a couple of instructions in your Arduino, so your code will probably be through here before the response arrives. Most likely, the response would then sit in the serial buffer until you try the next command, which would then process the response from this one. I am not familiar with that RFID unit but it looks as if you expect it to send a four-byte response to the command. In that case a far more reliable way to process the response would be to wait for each byte to be received, and then process it:

...
while(mySerial.available() <= 0)
{
    // do nothing
}
// first byte now available
val = mySerial.read();
if (val != 1)
{
    // RFID command failed
    Serial.print("RFID_READ failed with code ");
    Serial.println(val);
    suppressAll();
    return; // if we can't read the tag, there is no point trying subsequent processing
}
... etc

The code that reads buttonPin() will increment buttonCount and points each time digitalRead returns HIGH. This means the values will increment very rapidly while the button is down, and these variables are probably overflowing multiple times in the short time you hold the button down. You don't say how the button is connected to the Arduino, so I can't tell whether the code will successfully detect button presses. You should test this by putting print statements in the code so you can be sure it's detecting them correctly. I guess that what you want to achieve here is to stop and wait for a button press, and then increment points and continue. You could do that like this:

Serial.println("Waiting for button press");
while(digitalRead(buttonPin) == LOW) // I assume that the input reads LOW when the button is not pressed
{
    // do nothing
}
Serial.println("Button press detected");
points++;

Your code to write to the RFID card will suffer from the same timing problems as the code to read from it. I see that this is essentially code that you have just taken from a working example, but it seems pretty unlikely to me that it will work at all, let alone reliably, and the bizarre code in suppressAll() does not give me any confidence that the original author knew what he was doing, so I suggest it needs fixing anyway.