Entry Code Issue - Always access granted

Hi Guys,

Could you help me out why I am always getting ‘unlocked’ output with the following code no matter what code i enter?

const int greenled = 8;
const int redled = 7;
const int b1 = 4;
const int b2 = 5;
const int b3 = 6;
const int unlockpin = 3;

int pwcount;
byte combination[] = "1332";
byte userInput[4];

int buttonstate1 = 0;
int buttonstate2 = 0;
int buttonstate3 = 0;

void setup() {
 pinMode(greenled, OUTPUT);
 pinMode(redled, OUTPUT);
 pinMode(b1, INPUT_PULLUP);
 pinMode(b2, INPUT_PULLUP);
 pinMode(b3, INPUT_PULLUP);
 Serial.begin(9600);
 pinMode(unlockpin, OUTPUT);
}

void loop(){
 buttonstate1 = digitalRead(b1);
 buttonstate2 = digitalRead(b2);
 buttonstate3 = digitalRead(b3);

 if (buttonstate1 == LOW){
   userInput[pwcount] = '1';
   pwcount++;
   delay(300);
   Serial.print('1');
 }
 if (buttonstate2 == LOW){
   userInput[pwcount] = '2';
   pwcount++;
   delay(300);
   Serial.print('2');
 }
 if (buttonstate3 == LOW){
   userInput[pwcount] = '3';
   pwcount++;
   delay(300);
   Serial.print('3');
 }
 for(byte n = 0; n <=4; n++){


   if (userInput[n] == combination[n] && pwcount >=4){
     digitalWrite(redled, LOW);
     digitalWrite(greenled, HIGH);
     delay(500);
     digitalWrite(greenled, LOW);
     delay(500);
     Serial.println("unlocked");
     pwcount = 0;
     digitalWrite(unlockpin, HIGH);
         }
   else {
     if(userInput[n] != combination[n] && pwcount >=4){
       digitalWrite(greenled, LOW);
       digitalWrite(redled, HIGH);
       delay(500);
       digitalWrite(redled, LOW);
       delay(500);
       digitalWrite(redled, HIGH);
       delay(500);
       digitalWrite(redled, LOW);
       delay(500);
       digitalWrite(redled, HIGH);
       delay(500);
       digitalWrite(redled, LOW);
       delay(500);
       digitalWrite(redled, HIGH);
       delay(500);
       digitalWrite(redled, LOW);
       delay(500);
       Serial.println("Denied");
       pwcount = 0;
       n = 0;
     }
   }
 }  }
  for(byte n = 0; n <=4; n++){


    if (userInput[n] == combination[n] && pwcount >=4){
      digitalWrite(redled, LOW);
      digitalWrite(greenled, HIGH);
      delay(500);
      digitalWrite(greenled, LOW);
      delay(500);
      Serial.println("unlocked");
      pwcount = 0;
      digitalWrite(unlockpin, HIGH);
          }

You’re only asking for one number to be right. If this matches once then you print unlocked and unlock the door. You need to keep a tally to make sure all 4 of the numbers are right.

You should probably also look into the State Change Example in the IDE and only add to your password on press and release. That way if you hold the button for longer than 300ms it doesn’t loop back around and re-read the same button again.

for(byte n = 0; n <=4; n++){

Five iterations for a four digit code?

Please remember to use code tags when posting code.

It may simplify things for you to use memcmp() since your data is already in arrays. This way you don't need a counter for the compare.

C++ reference memcmp()

AVR memcmp()

Not familiar with memcmp(). I can’t figure this out, I changed the [n] into [pwcount] but now it is always denied. What am I missing?

const int greenled = 8;
const int redled = 7;
const int b1 = 4;
const int b2 = 5;
const int b3 = 6;
const int unlockpin = 3;

int pwcount;
byte combination[] = "1332";
byte userInput[4];

int buttonstate1 = 0;
int buttonstate2 = 0;
int buttonstate3 = 0;

void setup() {
 pinMode(greenled, OUTPUT);
 pinMode(redled, OUTPUT);
 pinMode(b1, INPUT_PULLUP);
 pinMode(b2, INPUT_PULLUP);
 pinMode(b3, INPUT_PULLUP);
 Serial.begin(9600);
 pinMode(unlockpin, OUTPUT);
}

void loop(){
 buttonstate1 = digitalRead(b1);
 buttonstate2 = digitalRead(b2);
 buttonstate3 = digitalRead(b3);

 if (buttonstate1 == LOW){
   userInput[pwcount] = '1';
   pwcount++;
   delay(300);
   Serial.print('1');
 }
 if (buttonstate2 == LOW){
   userInput[pwcount] = '2';
   pwcount++;
   delay(300);
   Serial.print('2');
 }
 if (buttonstate3 == LOW){
   userInput[pwcount] = '3';
   pwcount++;
   delay(300);
   Serial.print('3');
 }
 for(byte n = 0; n <=4; n++){


   if (userInput[pwcount] == combination[n] && pwcount >=4){   //CHANGED N INTO pwcount on userinput here
     digitalWrite(redled, LOW);
     digitalWrite(greenled, HIGH);
     delay(500);
     digitalWrite(greenled, LOW);
     delay(500);
     Serial.println("unlocked");
     pwcount = 0;
     digitalWrite(unlockpin, HIGH);
         }
   else {
     if(userInput[n] != combination[n] && pwcount >=4){
       digitalWrite(greenled, LOW);
       digitalWrite(redled, HIGH);
       delay(500);
       digitalWrite(redled, LOW);
       delay(500);
       digitalWrite(redled, HIGH);
       delay(500);
       digitalWrite(redled, LOW);
       delay(500);
       digitalWrite(redled, HIGH);
       delay(500);
       digitalWrite(redled, LOW);
       delay(500);
       digitalWrite(redled, HIGH);
       delay(500);
       digitalWrite(redled, LOW);
       delay(500);
       Serial.println("Denied");
       pwcount = 0;
       n = 0;
     }
   }
 }  }

for(byte n = 0; n <=4; n++) Deja vu

Just changing to pwcount helps how?

IF you're going to try to compare a four digit long password, then you need to compare all four digits. So you'll have to have something that is 0 and then 1 and then 2 and then 3 to go one by one and compare them. It isn't going to happen in one line. You'll probably need a for loop here. Check the first digit, if it is ok then check the second digit and if it is ok then the third etc etc. If you find one that is wrong then kick it out to the denied part.

Or you need to google memcmp and learn that function. Either way.

zorion:
Not familiar with memcmp(). I can’t figure this out, I changed the [n] into [pwcount] but now it is always denied. What am I missing?

Run this.

void setup() {
  Serial.begin(9600);
 for (byte n = 0; n <= 4; n++) {
    Serial.print(n);
    Serial.print(" ");
  }
}

void loop() {
}

[/quote]

hmm, when I run your code I can see that I am looking at 5 digits but my serial is outputting 4 and then it says denied or unlocked. I think i will have a look at the memcpc function instead...

zorion:
hmm, when I run your code I can see that I am looking at 5 digits but my serial is outputting 4 and then it says denied or unlocked. I think i will have a look at the memcpc function instead...

see replies 2 and 5

AWOL:
see replies 2 and 5

hm when I use for(byte n = 0; n <=3; n++) it only looks for 3 digits and then decides if access is denied or granted

I am having a real issue with this code. How can I get it to count the next value of the array and not only look at the first value? I have looked at examples on google but a lot of them come up with the same FOR loop.

Can you work out the logic without the code? Can you explain in english what you would do if I gave you a stack of four pieces of paper and each one had one letter on it and asked you if they matched with a secret code?

Always start there. If you can’t explain how it would happen in plain English then you will never be able to write code for it.

So you get the stack of papers, what do you do?

Post your code as it looks now and your observations of how it behaves.

const int greenled = 8;
const int redled = 7;
const int b1 = 4;
const int b2 = 5;
const int b3 = 6;
const int unlockpin = 3;


byte combination[] = "2331";
byte userInput[4];
int pwcount;

int buttonstate1 = 0;
int buttonstate2 = 0;
int buttonstate3 = 0;

void setup() {
pinMode(greenled, OUTPUT);
pinMode(redled, OUTPUT);
pinMode(b1, INPUT_PULLUP);
pinMode(b2, INPUT_PULLUP);
pinMode(b3, INPUT_PULLUP);
Serial.begin(9600);
pinMode(unlockpin, OUTPUT);
}

void loop(){
buttonstate1 = digitalRead(b1);
buttonstate2 = digitalRead(b2);
buttonstate3 = digitalRead(b3);

if (buttonstate1 == LOW){
 userInput[pwcount] = '1';
 pwcount++;
 delay(300);
 Serial.print('1');
}
if (buttonstate2 == LOW){
 userInput[pwcount] = '2';
 pwcount++;
 delay(300);
 Serial.print('2');
}
if (buttonstate3 == LOW){
 userInput[pwcount] = '3';
 pwcount++;
 delay(300);
 Serial.print('3');
}
for(byte n = 0; n < 4; n++)

{


 if (userInput[n] != combination[n] && pwcount ==4){ 
  
   Serial.println("denied");
   pwcount = 0;
   n = 0;
       }
 else {
   if(userInput[n] == combination[n] && pwcount ==4){ // I understand that when pwcount matches 4 it will only check the first element of the array as n starts out as 0 in this FOR loop. As soon as the first element of 'userInput' matches the first element of 'combination' it will continue to access granted. What do I need here to check all 4 elements of the arrays? Is this even possible with a FOR loop?
    
     Serial.println("granted");
     pwcount = 0;
     n = 0;
   }
 }
}  }

Looking at the pieces of paper analogy: What I have are 2 arrays of 4 papers (indexed 0-3). One array is empty and the other has 1 number on each paper. I first need to put a number on each of the empty papers and when all papers have a number I need to compare the 2 complete arrays against each other.

Compare the complete arrays is memcmp.

I was looking more for an algorithm to loop through the papers. Like look at the first one if it matches then move on and look at the second one etc til you get to the last one. If you find any that don't match then call it a fail and stop right there.

What you're doing right now is the opposite. You're comparing the first and if it is not a match then you move on to the next. And if you find any one that is a match then you call it success and unlock the door.

Try this: Create a boolean and give it a nice name, like codeMatched. Right before entering the loop set codeMatched true. Now, as you iterate through the loop if any pair does *not * match set codeMatched to false. Once you exit the loop the status of codeMatched will tell you if a good code was entered.

Delta_G:
Compare the complete arrays is memcmp.

I was looking more for an algorithm to loop through the papers. Like look at the first one if it matches then move on and look at the second one etc til you get to the last one. If you find any that don't match then call it a fail and stop right there.

What you're doing right now is the opposite. You're comparing the first and if it is not a match then you move on to the next. And if you find any one that is a match then you call it success and unlock the door.

If I run and test the code I have now, it only compares the first one. I can enter anything I want on 2-4 and it will accept the code as long as the first is a 2 (Code is 2331). How do I get it to go through all the values? Is this possible in a loop?

Yes it is possible. Read #16 it describes how.

Delta_G:
Yes it is possible. Read #16 it describes how.

I kind of get that but I still can’t get it to check more then 1 value. This is what I have now. Access is still granted when the first entry matches. This is what I have now:

const int greenled = 8;
const int redled = 7;
const int b1 = 4;
const int b2 = 5;
const int b3 = 6;
const int unlockpin = 3;

int pwcount;
byte combination[4] = "2323";
byte userInput[4];

int buttonstate1 = 0;
int buttonstate2 = 0;
int buttonstate3 = 0;

bool CodeMatched;


void setup() {
 pinMode(greenled, OUTPUT);
 pinMode(redled, OUTPUT);
 pinMode(b1, INPUT_PULLUP);
 pinMode(b2, INPUT_PULLUP);
 pinMode(b3, INPUT_PULLUP);
 Serial.begin(9600);
 pinMode(unlockpin, OUTPUT);
}

void loop() {
 buttonstate1 = digitalRead(b1);
 buttonstate2 = digitalRead(b2);
 buttonstate3 = digitalRead(b3);

 if (buttonstate1 == LOW) {
   userInput[pwcount] = '1';
   pwcount++;
   delay(300);
   Serial.print('1');
 }
 if (buttonstate2 == LOW) {
   userInput[pwcount] = '2';
   pwcount++;
   delay(300);
   Serial.print('2');
 }
 if (buttonstate3 == LOW) {
   userInput[pwcount] = '3';
   pwcount++;
   delay(300);
   Serial.print('3');
 }
 CodeMatched = true;
 { for (byte n = 0; n <= 4; n++)

     if (userInput[n] != combination[n] && pwcount == 4)

     { Serial.println("Denied");
       Serial.println(CodeMatched);
       CodeMatched = false;
       Serial.println(CodeMatched);
       pwcount = 0;
       

     } else




       if (CodeMatched == true && pwcount == 4)
       {
         Serial.println("granted");
         digitalWrite(unlockpin, HIGH);
         pwcount = 0;
         n = 0;
         delay(1000);

       }
 }
}