Button Sequence Script, what am I doing wrong?

I am trying to test a button sequence sketch that I found online.

However, when hooking up three buttons I can’t seem to get it to work. It recognizes the amount of presses I have, but even when I input the correct combination(1332 from the example) it still denies me.

I’ve had the serial window open to check that 1332 actually shows up there when reading on 9600, and it does.

Here is the code, adapted to my board. I also tried with longer delay to debounce the buttons (I am using heavy duty buttons that are originally for 220v, because I need them to be pretty durable later).

Here is the code in its entirety.

const int buttonPin = 2;
const int buttonPin2 = 3;
const int buttonPin3 = 4;
const int ledPin = 13;
const int ledPin2 = 12;

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

int buttonState1 = 0;
int buttonState2 = 0;
int buttonState3 = 0;

void setup() {
  // put your setup code here, to run once:
pinMode(ledPin, OUTPUT);
pinMode(buttonPin, INPUT);
pinMode(buttonPin2, INPUT);
pinMode(buttonPin3, INPUT);
pinMode(ledPin2, OUTPUT);
Serial.begin(9600);
}

void loop() {
  // put your main code here, to run repeatedly:
buttonState1 = digitalRead(buttonPin);
buttonState2 = digitalRead(buttonPin2);
buttonState3 = digitalRead(buttonPin3);

digitalWrite (ledPin, LOW);

if (buttonState1 == HIGH) {
    userInput[pwcount] = '1';
    pwcount++;
    delay(900);
    Serial.print('1');
}
if (buttonState2 == HIGH){
    userInput[pwcount] = '2';
    pwcount++;
    delay(900);
    Serial.print('2');
}
if (buttonState3 == HIGH){
    userInput[pwcount] = '3';
    pwcount++;
    delay(900);
    Serial.print('3');
}

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


if (userInput[pwcount] == combination[n] && pwcount >=4){
    digitalWrite(ledPin2, HIGH);
    Serial.println("unlocked");
    pwcount = 0;

}
else {
    if(userInput[n] != combination[n] && pwcount >=4){
        digitalWrite(ledPin, HIGH);
        delay (500);
        digitalWrite(ledPin, LOW);
        delay (500);
        digitalWrite(ledPin, HIGH);
        delay (500);
        digitalWrite(ledPin, LOW);
        delay (500);
        digitalWrite(ledPin, HIGH);
        delay (500);
        digitalWrite(ledPin, LOW);
        delay (500);
        Serial.println("Denied");
        pwcount = 0;
        n = 0;
      }
    }
  }
}

Now guys, what can be wrong with it?

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


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

You seriously need to reorganize this.

First, you should be doing nothing if pwcount is not 4 or more. So, that should be the outer if statement.

Second, you are only allowing for a 4 character combination, so iterating 5 times is wrong.

Third, you should be comparing userInput[n] to combination[n].

Fourth, you should assume a match of all positions, and set a flag to false if any position doesn’t match.

Fifth, the body of the if statement is not the place to unlock the door. That should happen only after the loop ends without having set the flag to false.

Thank you so much. I rewrote it a little, like this:

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


    if (pwcount ==4){

      if (userInput[n] == combination[n]) {
      digitalWrite(ledPin, HIGH);
      Serial.println("unlocked");
      delay(5000);
      digitalWrite(ledPin, LOW);
      pwcount = 0;
      } else {
        digitalWrite(ledPin2, HIGH);
        delay(1500);
        digitalWrite(ledPin2, LOW);
        Serial.println("Denied");
        pwcount = 0;
        n = 0;
        
        }
      
    }
    else {


      }
    }    }

However I now ran into another problem. It accepts codes such as 1223 and 1232. I don’t know why really.

How could I double-check the positions?

I do not understand the iterating 5 times?

I rewrote it a little, like this:

Why?

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


    if (pwcount ==4){

Loop 5 times checking if pwcount is 4. Put the if test FIRST.

Loop FOUR times!

      if (userInput[n] == combination[n]) {
      digitalWrite(ledPin, HIGH);
      Serial.println("unlocked");

If any ONE (or more) digits in the combination is right, open the door. Good idea. Gives anyone a 4 in 10 chance of getting in.

I do not understand the iterating 5 times?

So, lets see. n=0, n=1, n=2, n=3, n=4. That looks like 5 to me.

Jeesus. I am sorry I asked such an incredible easy question then!

I am very new to this so not all of it make sense to me. I do not understand all those codes, as I copy/pasted it from somewhere and am trying to make it work for my use.

I hope someone else can explain to me what the code SHOULD say, as I will not be able to figure out on my own how to check for positions.

I hope someone else can explain to me what the code SHOULD say

How about like this:

   if(pwcount == 4)
   {
       bool valid = true;

       for(byte n = 0; n <4; n++)
       {
          if(userInput[n] != combination[n])
          {
             valid = false;
          }
       }

       if(valid)
       {
          // open the door
       }
   }
  1. The if statement comes first
  2. The loop iterates four times
  3. The result of the loop is that valid is true if, and only if, all 4 values match
  4. The place to open the door is AFTER the checking and inside the if block.

Thank you. I read up on “for” loops, and I understand it now I think.
I could not make sense of it.

Your code worked, of course. But I found out I had to add a pwcount = 0; after adding the serialprintin with (“denied”).

One thing I am wondering about though, is why do I get somewhere between 1-5 “denied” messages after purposely inputting a wrong code?

I added another button and made the code 13421 instead. It works but I still get a lot of “denied” messages after inputting wrong code and I don’t know why.

I was also wondering if you knew the best way to “reset” the code when no button has been pressed for like 40 seconds.
Should I use a “do while” within the “for” statement maybe?
I can’t use delay, and I understand there is no “wait” function so I am unsure how to reset pwcount after 40 seconds inactivity…

One thing I am wondering about though, is why do I get somewhere between 1-5 "denied" messages after purposely inputting a wrong code?

Clearly you are not running the same code you posted last time. So, it's hard to say what is wrong with the code you didn't post.

I was also wondering if you knew the best way to "reset" the code when no button has been pressed for like 40 seconds.

loop() will iterate thousands of times per second while the user enters the combination. On each and every one of those iterations, you can test whether now (as returned by millis()) minus then (the last time a key was pressed, as recorded my millis()) exceeds some interval. If it does, reset the index and array.

The blink without delay examples shows how.

Yes, BlinkWithoutDelay is what I ended up doing and it works like a charm.

Now if pwcount > 0 && pwcount <5, it will reset itself after 10 seconds of ineffectivity. Thank you so much for your help. I think I understand more of it now.

const int ledPin = 13;
const int ledPin2 = 12;
const int b1 = 2;  // Button 1
const int b2 = 3;  // Button 2 pin
const int b3 = 4;  // Button 3 pin
const int b4 = 5;  // BUtton 4 pin

int pwcount;  // pwcount checks how many times buttons are pressed
byte combination[] = "13324";   // set your combination here
byte userInput[5];  // Change 5 to whatever number of keys/presses you need to get your code right (13324 is five presses, 1334 is four).

int buttonstate1 = 0; // These set buttonstates to 0.
int buttonstate2 = 0;
int buttonstate3 = 0;
int buttonstate4 = 0;

int ledState = LOW;     // ledState used to set the LED
int sec = 0; // making a integer for seconds

unsigned long previousMillis = 0;      // code pasted from the "BlinkWithoutDelay" guide.
const long interval = 1000;            // ^^ same.

// Necessary setup to work.
void setup() {
  pinMode(ledPin, OUTPUT);
  pinMode(ledPin2, OUTPUT);
  pinMode(b1, INPUT);
  pinMode(b2, INPUT);
  pinMode(b3, INPUT);
  pinMode(b4, INPUT);
  Serial.begin(9600);
}

void loop(){
  buttonstate1 = digitalRead(b1);   // Tells loop to check all four button states continuously.
  buttonstate2 = digitalRead(b2);
  buttonstate3 = digitalRead(b3);
  buttonstate4 = digitalRead(b4);
  unsigned long currentMillis = millis();   // Tells loop to also check how many seconds pass.

  if (buttonstate1 == HIGH){       // If button 1 is pressed
    userInput[pwcount] = '1';       // it counts as digit 1.
    pwcount++;                     // pwcount should increase by one, so we know that one key out of five has been pressed.
    sec = 0;                       // set seconds back to 0, to avoid "time out".
    delay(300);                    // delay 300ms for the button to settle to avoid multiple readings.
    Serial.print('1');             // print 1 in serial so that you may debug. 
  }
  if (buttonstate2 == HIGH){
    userInput[pwcount] = '2';
    pwcount++;
    sec = 0;
    delay(300);
    Serial.print('2');
  }
  if (buttonstate3 == HIGH){
    userInput[pwcount] = '3';
    pwcount++;
    sec = 0;
    delay(300);
    Serial.print('3');
  }
  if (buttonstate4 == HIGH){
   userInput[pwcount] = '4';
   pwcount++;
   sec = 0;
   delay(300);
   Serial.print('4');
 }


 if(pwcount == 5)     // when five buttons has been pressed (any buttons) 
   {
       bool valid = true;    //set flag to true

       for(byte n = 0; n <5; n++)    // run integers five times
       {
          if(userInput[n] != combination[n])   // if the buttons pressed do not match the combination,
          { 
             valid = false;                    // set flag to false
             pwcount = 0;                      // reset button count, to start over on new code
             Serial.println("denied");         // print denied to serial.       
          }
       }

       if(valid)                               // if code is valid! yay!, then,
       {
          digitalWrite(ledPin2, HIGH);         // turn on the LED or turn relay or do whatever you want.
          Serial.println();                    // new line in serial
          Serial.println("unlocked");          // print "unlocked"
          delay(5000);                         // unlocked for 5 seconds
          digitalWrite(ledPin2, LOW);          // led goes off and
          pwcount = 0;                         // button count is reset

       }
   } else if (pwcount > 0 && pwcount < 5) {                   // if pwcount is MORE than 0 and less than 5,
        if (currentMillis - previousMillis >= interval) {
    previousMillis = currentMillis;


    if (ledState == LOW) {        // not sure if this code is necessary really
      ledState = HIGH;
    } else {
      ledState = LOW;
    }


    sec++;      // if user has input 1 or more buttons, then increase sec by 1 for each second.
  }
if (sec >=10) {     // if 10 seconds pass without any button presses
   sec = 0;             // then reset sec to 0, and start over.
   pwcount = 0;         // reset button count
   Serial.println();          // print newline
   Serial.println("time out");    // print 'time out'
}
}


    }
  unsigned long currentMillis = millis();   // Tells loop to also check how many seconds pass.

I REALLY hate that name. It means nothing.

unsigned long now = millis();

Easier to type AND means something (to me, at least).

unsigned long previousMillis = 0;      // code pasted from the "BlinkWithoutDelay" guide

Another meaningless name. Wouldn't lastKeyPressTime be easier to understand?

    delay(300);                    // delay 300ms for the button to settle to avoid multiple readings.

That's at least an order of magnitude longer than any but the crappiest switches will bounce.

             pwcount = 0;                      // reset button count, to start over on new code
             Serial.println("denied");         // print denied to serial.

Doing that, and printing that, EVERY time there is a mismatch is a bad idea. You give away how many of the 5 values don't match. Stop doing that.

       if(valid)                               // if code is valid! yay!, then,
       {
          digitalWrite(ledPin2, HIGH);         // turn on the LED or turn relay or do whatever you want.
          Serial.println();                    // new line in serial
          Serial.println("unlocked");          // print "unlocked"

An else statement and block after this if block would be where to print "denied" and reset pwcount.

   } else if (pwcount > 0 && pwcount < 5) {                   // if pwcount is MORE than 0 and less than 5,

Since you immediately handled the case when pwcount is 5, the if part of this statement is unnecessary. Resetting pwcount to 0 if it is 0 won’t hurt anything. There is no way for pwcount to be negative, so dealing with, or ignoring, negative values isn’t necessary. And, there is no way for pwcount to exceed 5, so dealing with, or ignoring, such values isn’t necessary.

    if (ledState == LOW) {        // not sure if this code is necessary really
      ledState = HIGH;
    } else {
      ledState = LOW;
    }

In this case, no.

The reason I put the if in there was to avoid having the program do anything if no buttons are pressed and pwcount already is zero.

I tried it without this if, and the serial continued to print (time out) every 10 second nothing was pressed.

I may want to hook it up to a display that displays denied or approved etc, and "time out".