Whats wrong with my nfc code?

Hey, I’ve got this code using the nfc module N532. Its supposed to work like this, If the module gets “good card” ID then the green led lights up but if it gets the ID of any other tag then the red led lights up.

And it works… sort of. Problem is it works the other way around, when it gets “good card” ID the red led lights up and and other tags light up the green led. Now before you ask, I did check whether I misplaced the pins multiple times. But the problem persists. Another problem I’m having is that the green led keeps blinking as if it just got an ID tag and is responding to it.

Thanks

#include <Wire.h>
#include <PN532_I2C.h>
#include <PN532.h>
PN532_I2C pn532i2c(Wire);
PN532 nfc(pn532i2c);

int greenLED=2;
int redLED=13;
byte goodCard[] ={0x64,0x40,0xB1,0x76};

void setup(){
  nfc.begin();
  pinMode(greenLED,OUTPUT);
  pinMode(redLED,OUTPUT);
}
void loop(){
  boolean success;
  uint8_t uid[] = {0,0,0,0,0,0,0};
  uint8_t uidLength;
  success = nfc.readPassiveTargetID(PN532_MIFARE_ISO14443A, &uid[0], &uidLength);
  if(memcmp(uid, goodCard, sizeof(goodCard))){
    digitalWrite(greenLED,HIGH);
    delay(1000);
    digitalWrite(greenLED,LOW);
  }
  else{
    digitalWrite(redLED,HIGH);
    delay(1000);
    digitalWrite(redLED,LOW);
  }
}
  else
  {
    digitalWrite(redLED,HIGH);
    delay(1000);
    digitalWrite(redLED,LOW);
  }

This why the LED flashes continuously. This block of code will run every time through loop() regardless of whether a card has been read or not. It is irrelevant whether a tag has been read and matches or not.

As to your main problem, how are the LEDs wired ?

If a good card is 4 bytes, why is the scanned card array 7 bytes?

  if(memcmp(uid, goodCard, sizeof(goodCard))){

The memcmp() function returns one of 3 values. You really ought to look up what they mean. 0 means that they match. Your code interprets 0 as false.

This why the LED flashes continuously. This block of code will run every time through loop() regardless of whether a card has been read or not. It is irrelevant whether a tag has been read and matches or not.

As to your main problem, how are the LEDs wired ?

Why Did I not see that?
I tried attaching a photo. But basically,
Arduino pin 2 goes to green led, then to ground.
Arduino pin 13 goes to red led, then to ground. There’s also the built in led of pin 13.
Then NFC pn532 connections are:
pn532 Ground to ground
pn532 VCC to 5v on the Arduino
pn532 SDA to Arduino pin A4
pn532 SCL to Arduino pin A5

The memcmp() function returns one of 3 values. You really ought to look up what they mean. 0 means that they match. Your code interprets 0 as false.

Yeah I tried Google it a bunch of times but it just keeps giving me memcpy() instead.

Yeah I tried Google it a bunch of times but it just keeps giving me memcpy() instead.

http://www.cplusplus.com/reference/cstring/memcmp/

Excellent! I got it to work using the code below. But I’m still having the blinking problem, although this time its the red led.

#include <Wire.h>
#include <PN532_I2C.h>
#include <PN532.h>
PN532_I2C pn532i2c(Wire);
PN532 nfc(pn532i2c);

int greenLED=2;
int redLED=13;
byte goodCard[] ={0x64,0x40,0xB1,0x76};

void setup(){
  nfc.begin();
  pinMode(greenLED,OUTPUT);
  pinMode(redLED,OUTPUT);
}
void loop(){
  boolean success;
  uint8_t uid[] = {0,0,0,0,0,0,0};
  uint8_t uidLength;
  success = nfc.readPassiveTargetID(PN532_MIFARE_ISO14443A, &uid[0], &uidLength);
  int ID;
  ID=memcmp(uid, goodCard, sizeof(goodCard));
  if(ID==0){
    digitalWrite(greenLED,HIGH);
    delay(1000);
    digitalWrite(greenLED,LOW);
  }
  if(ID>0||ID<0){
    digitalWrite(redLED,HIGH);
    delay(1000);
    digitalWrite(redLED,LOW);
  }
}
1 Like

Read UKHeliBob's reply, #1.

  if(ID==0){
  }
  if(ID>0||ID<0){

If it isn’t 0, doesn’t it HAVE to be less than 0 or greater than 0?

But I’m still having the blinking problem, although this time its the red led.

Perhaps it’s because you only want to call memcmp() and test the result when you have actually read a tag.

I tried adding

else{
    digitalWrite(greenLED,LOW);
    digitalWrite(redLED,LOW);
}

at the end of the code thinking if any of the “if” statements don’t pass then it will do this “else” statement instead. But it still won’t work.

If it isn’t 0, doesn’t it HAVE to be less than 0 or greater than 0?

I actually don’t understand your question. Must be because I only understood the “0” part of the return value in the site you mentioned and not the “”<0 or “>0”.

I think I got it!

void loop(){
  boolean success;
  uint8_t uid[] = {0,0,0,0,0,0,0};
  uint8_t uidLength;
  success = nfc.readPassiveTargetID(PN532_MIFARE_ISO14443A, &uid[0], &uidLength);
  int ID;
  ID=memcmp(uid, goodCard, sizeof(goodCard));
  if(success){
    if(ID==0){
      digitalWrite(greenLED,HIGH);
      delay(1000);
      digitalWrite(greenLED,LOW);
    }
    if(ID>0||ID<0){
      digitalWrite(redLED,HIGH);
      delay(1000);
      digitalWrite(redLED,LOW);
    }
  }
  else{
    digitalWrite(greenLED,LOW);
    digitalWrite(redLED,LOW);
  }
}

I saw I had a boolean “success” which is connected to the code that makes the pn532 module scan for an ID. Then I made so the Arduino can only turn an led on if it has an ID. Otherwise it will keep the leds off.

Thanks you guys!!

I actually don’t understand your question. Must be because I only understood the “0” part of the return value in the site you mentioned and not the “”<0 or “>0”.

The return value is either 0 or it isn’t, right? Non-zero means that it is greater than 0 or less than 0. So:
if(ID==0){
digitalWrite(greenLED,HIGH);
delay(1000);
digitalWrite(greenLED,LOW);
}
else
{
digitalWrite(redLED,HIGH);
delay(1000);
digitalWrite(redLED,LOW);
}

By the way, the memcmp() call should be in the if(success) block.

Yup gotta practice efficient coding, thanks.

By the way, the memcmp() call should be in the if(success) block.

Do you mean to replace the (success) block with the memcmp() call? Or somthing like this?

if(success){
if(memcmp(uid, goodCard, sizeof(goodCard))){
digitalWrite(greenLED,HIGH);
delay(1000);
digitalWrite(greenLED,LOW);
}

Either one didn’t work for me…

Do you mean to replace the (success) block with the memcmp() call? Or somthing like this?

No and no. Can’t you see that you are back to NOT properly checking the return code from memcmp?

Get in the habit NOW of putting every { on a new line.

if(success)
{
if(memcmp(uid, goodCard, sizeof(goodCard)) == 0)
{
digitalWrite(greenLED,HIGH);
delay(1000);
digitalWrite(greenLED,LOW);
}

Yeah when I first got into Arduino and opened the IDE, it always had void setup and loop pre loaded for me with those curly brackets in that setup. So I just got used doing it that way. Although I did notice others doing it your way, might as well do so. :slight_smile:

Anyway, I never thought of put in the code in that way. I had it in a way that the return code from memcmp would fill the int 'ID' then later compare its content to the 'if' statements.

Sorry for the late reply, Thanks again!!

By the way there are no resistors in line with those LEDs so you will be damaging both LED and arduino by passing too much current.