Help with cleaning up programming. I think it's over complicated.

Hi all, I know there's an easier way to do this but I'm just not sure how. I come from a ladder logic/PLC background so using functions makes the most sense to me.

Here's a video of everything in action. Arduino stepper positions set using keypad passwords and a momentary button. - YouTube
The video description lays everything out.

The thing I've had the most difficulty with so far is having the stepper wait for a button press to confirm it's ok to move. My fix was to make each function loop till the button is pressed. But I think this will create problems in the future if I want a timeout function or something.

Anyway here's the code tell me what you think? I'm linking on my phone so excuse any formatting errors.

/*Project Title: Multiple Keypad Passwords for Multiple Stepper Positions 

   Keypad Connection
 * ROW0 to digital pin 22
 * ROW1 to digital pin 28
 * ROW2 to digital pin 26
 * ROW3 to digital pin 24
 * COL0 to digital pin 25
 * COL1 to digital pin 23
 * COL2 to digital pin 27

 Operation:
 '#' = OK -> checks if the password is correct or not
 '*' = CLEAR -> clears the password typed
*/
#include <AccelStepper.h>
#include <Password.h>
#include <Keypad.h>

Password redPass = Password( "789" ); //This is red password
Password bluPass = Password( "456" ); //This is blue password
Password grnPass = Password( "123" ); //This is green password

// Define a stepper and the pins it will use
AccelStepper stepper(1,2,3); //1 = EasyDriver, 2 = step, 3 = direction

//LED pins
#define grnLED 42
#define redLED 41
#define bluLED 40
#define waitLED 52
#define goLED 51

int buttonPin = 53;    // the pin that the pushbutton is attached to



// Variables will change:

const byte ROWS = 4; // 4 rows
const byte COLS = 3; // 3 columns
char keys[ROWS][COLS] = { // Define the Keymap
  {'1','2','3'},
  {'4','5','6'},
  {'7','8','9'},
  {'*','0','#'}
};

byte rowPins[ROWS] = {22, 28, 26, 24};// Connect keypad ROW0, ROW1, ROW2 and ROW3 to these Arduino pins.
byte colPins[COLS] = {25, 23, 27};// Connect keypad COL0, COL1 and COL2 to these Arduino pins.

// Create the Keypad
Keypad keypad = Keypad( makeKeymap(keys), rowPins, colPins, ROWS, COLS );

void setup()
{
  pinMode(grnLED, OUTPUT);
  pinMode(redLED, OUTPUT);
  pinMode(bluLED, OUTPUT);
  pinMode(waitLED, OUTPUT);
  pinMode(goLED, OUTPUT);
  
  // initialize the button pin as a input:
  pinMode(buttonPin, INPUT);
  digitalWrite(buttonPin, LOW);

  Serial.begin(9600);
  Serial.print("Enter Password: ");
  keypad.addEventListener(keypadEvent); //add an event listener for this keypad
  stepper.setMaxSpeed(5000.0);
  stepper.setAcceleration(2000.0);
}

void loop()
{
  keypad.getKey();  
}

//check the keypad events
void keypadEvent(KeypadEvent keyPress)
{
  switch (keypad.getState())
  {
    case PRESSED:
    Serial.print(keyPress); //print the keypress
    switch (keyPress)
    {
	  case '#':
            checkPassword();
          break;

          case '*':  
            
            grnPass.reset();
            digitalWrite(goLED, LOW);
            Pos_Home();
            digitalWrite(grnLED, LOW);
            Serial.print("                ");
            
            redPass.reset();
            digitalWrite(goLED, LOW);
            Pos_Home();
            digitalWrite(redLED, LOW);
            Serial.print("                ");
            
            bluPass.reset();
            digitalWrite(goLED, LOW);
            Pos_Home();
            digitalWrite(bluLED, LOW);
            Serial.print("                ");
            
          break;

          default:
          grnPass.append(keyPress);
          redPass.append(keyPress);
          bluPass.append(keyPress);
     }
  }
}

//check the entered password
void checkPassword(){
  
  if (grnPass.evaluate()) {//if green password is correct
    Serial.println(" Correct Password"); //print a message on serial monitor
    digitalWrite(grnLED, HIGH); //turn ON green LED
    Pos_1(); //add function for stepper position 1
    grnPass.reset(); //reset password
    }
  
  else if (bluPass.evaluate()) { //if blue password is correct
    Serial.println(" Correct Password"); //print a message on serial monitor
    digitalWrite(redLED, HIGH); //turn ON red LED
    Pos_2(); //add function for stepper position 2
    bluPass.reset(); //reset password
  }
  else if (redPass.evaluate()) { //if red password is correct
    Serial.println(" Correct Password"); //print a message on serial monitor
    digitalWrite(bluLED, HIGH); //turn ON yellow LED
    Pos_3(); //add function for stepper position 3
    redPass.reset();//reset password
  }  
  else //if wrong password
  {
    Serial.println(" Wrong Password"); //if password is wrong
    digitalWrite(grnLED, HIGH); //cascade LEDs
    delay(100);
    digitalWrite(redLED, HIGH);
    delay(100);
    digitalWrite(bluLED, HIGH);
    delay(500);
    digitalWrite(grnLED, LOW);
    digitalWrite(redLED, LOW);
    digitalWrite(bluLED, LOW);
    grnPass.reset(); //reset passwords
    redPass.reset();
    bluPass.reset();
  }
}

void Pos_Home() { //stepper home position
  digitalWrite(waitLED, HIGH);
  stepper.runToNewPosition(0);
  digitalWrite(waitLED, LOW);
  
}
void Pos_1() { //stepper position 1
  digitalWrite(waitLED, HIGH);
  if(digitalRead(buttonPin) == HIGH) { //if button is presed
  
  /////would like to add flashing redLED while stepper is moving/////
  
    stepper.runToNewPosition(200);
    digitalWrite(waitLED, LOW);
    digitalWrite(goLED, HIGH);
  }
  else { //loop until button pressed
    Pos_1();
  }  
}
 
void Pos_2() { //stepper position 2
  digitalWrite(waitLED, HIGH); 
  
  if(digitalRead(buttonPin) == HIGH) { //if button is presed
  
    /////would like to add flashing redLED while stepper is moving/////
  
    stepper.runToNewPosition(400);
    digitalWrite(waitLED, LOW);
    digitalWrite(goLED, HIGH);
  }
  else { //loop until button pressed
    Pos_2();
  }
}

void Pos_3() { //stepper position 3
  digitalWrite(waitLED, HIGH);
  if(digitalRead(buttonPin) == HIGH) { //if button is presed
  
    /////would like to add flashing redLED while stepper is moving/////

    stepper.runToNewPosition(800);
    digitalWrite(waitLED, LOW);
    digitalWrite(goLED, HIGH);
  }
  else { //loop until button pressed
    Pos_3();
  }
}
  keypad.addEventListener(keypadEvent); //add an event listener for this keypad

Do you know what this is doing, and why you might need to do this?

  keypad.getKey();

Is there any good reason for asking the keypad instance what key is pressed, and then throwing the result away?

            Pos_Home();
            Pos_Home();
            Pos_Home();

Are you sure you don't need to call this a few more times if the * key is pressed? Maybe three times isn't enough.

            digitalWrite(goLED, LOW);
            digitalWrite(goLED, LOW);
            digitalWrite(goLED, LOW);

Are you sure you don't need to call this a few more times if the * key is pressed? Maybe three times isn't enough.

  else { //loop until button pressed
    Pos_1();
  }

Calling this function recursively is NOT a good idea. A while statement is.

I guess @PaulS has said it more eloquently as usual.

I don't understand your overal program logic.

It looks as is you have surrendered complete control of your program to the function keypadEvent(). That's not necessarily a bad thing (but probably is). I will ,make the same comment about it that I make when people shove all their code into loop(). Don't put all the code into the one function. Use that function to capture the keypad input and save it somewhere. Then the other functions can use that data to decide what they need to do.

The reason I thing it is a bad idea to surrender everything to the keypadEvent() function is that it looks as if nothing will happen if you don't press a key.

I suggest you take back control of your program by calling the various activities from within loop() and probably not bother to use keypadEvent() at all. Just read the keypad once per iteration of loop(). Loop() might look like this ...

void loop() {
   readKeypad();
   moveMotors();
  whateverElseIsNeeded();
}

l

...R

PaulS:

  keypad.addEventListener(keypadEvent); //add an event listener for this keypad

Do you know what this is doing, and why you might need to do this?

  keypad.getKey();

Is there any good reason for asking the keypad instance what key is pressed, and then throwing the result away?

            Pos_Home();

Pos_Home();
            Pos_Home();



Are you sure you don't need to call this a few more times if the * key is pressed? Maybe three times isn't enough.



digitalWrite(goLED, LOW);
            digitalWrite(goLED, LOW);
            digitalWrite(goLED, LOW);



Are you sure you don't need to call this a few more times if the * key is pressed? Maybe three times isn't enough.



else { //loop until button pressed
    Pos_1();
  }



Calling this function recursively is NOT a good idea. A while statement is.

Honestly for the keypad parts I started with some example code from the keypad and password library's and just added some things from there. It's my fault for not understanding exactly what they do.

I see now that when "*" is preset I can reset all passwords as well as go to Pos_home and set LEDs low.

As far as the while loop I tried doing it that way (but probably incorrectly) and either nothing would happen or the stepper would only move if I was holding the button when I pressed #.

I can't recall exactly what I wrote but it's on my computer at home.

Robin2:
I guess @PaulS has said it more eloquently as usual.

I don't understand your overal program logic.

It looks as is you have surrendered complete control of your program to the function keypadEvent(). That's not necessarily a bad thing (but probably is). I will ,make the same comment about it that I make when people shove all their code into loop(). Don't put all the code into the one function. Use that function to capture the keypad input and save it somewhere. Then the other functions can use that data to decide what they need to do.

The reason I thing it is a bad idea to surrender everything to the keypadEvent() function is that it looks as if nothing will happen if you don't press a key.

I suggest you take back control of your program by calling the various activities from within loop() and probably not bother to use keypadEvent() at all. Just read the keypad once per iteration of loop(). Loop() might look like this ...

void loop() {

readKeypad();
   moveMotors();
  whateverElseIsNeeded();
}




l

...R

I understand what you're saying and it definitely makes sense. This was suggested to me elsewhere.
"The way to think about this is that you have several states. This doesn't have everything you described, but it should give you an idea:
Device locked
Device unlocked, stepper hold at home
Stepper cleared to move
Stepper has been moved
You transition from 1 to 2 by entering a correct password, from 2 to 3 by pressing the button, the program is responsible for 3 to 4 and you fall from 2 or 4 back to 1 by pressing '*'.
Then your loop can look like this:

void loop () {
  if (deviceIsLocked) {  
    // State 1
    keypad.getKey(); // enter password and unlock
  } else if (stepperHold) { 
    // State 2
    checkButton(); // clear the hold here
    keypad.getKey(); // check for '*'
  } else if (stepperIsAtHome) {
    // State 3
    moveStepper(); 
  } else { // Stepper has been moved already
    // State 4
    keypad.getKey(); // check for '*'
  }
}

P.S. this:

} else if (stepperIsAtHome) {
  moveStepper(); 
}

can probably be handled by the checkButton function, but I left it in the main loop so you can see the progression."

Is this similar to what you're saying?

I don't like the code too, but have seen something like this before. Why? Because people how don't know how to program, peak one example that is "mostly" the same thing that they need to do and they change it a little to get what they want. (I think that until this point is all normal)
What I don't think is normal is that the only example of keypad+password is one that uses Events and an Event Listener. The persons how don't have many practice with this will think that is the only way to do it!

The example is this:

/*
||  Simple Password Entry Using Matrix Keypad
||  4/5/2012 Updates Nathan Sobieck: Nathan@Sobisource.com
||
*/


//* is to validate password   
//# is to reset password attempt

/////////////////////////////////////////////////////////////////

#include <Password.h> //http://www.arduino.cc/playground/uploads/Code/Password.zip
#include <Keypad.h> //http://www.arduino.cc/playground/uploads/Code/Keypad.zip

Password password = Password( "1234" );

const byte ROWS = 4; // Four rows
const byte COLS = 4; //  columns
// Define the Keymap
char keys[ROWS][COLS] = {
  {'1','2','3','A'},
  {'4','5','6','B'},
  {'7','8','9','C'},
  {'*','0','#','D'}
};

byte rowPins[ROWS] = { 9,8,7,6 };// Connect keypad ROW0, ROW1, ROW2 and ROW3 to these Arduino pins.
byte colPins[COLS] = { 5,4,3,2, };// Connect keypad COL0, COL1 and COL2 to these Arduino pins.


// Create the Keypad
Keypad keypad = Keypad( makeKeymap(keys), rowPins, colPins, ROWS, COLS );

void setup(){

  Serial.begin(9600);
  keypad.addEventListener(keypadEvent); //add an event listener for this keypad
}

void loop(){
  keypad.getKey();
}

//take care of some special events
void keypadEvent(KeypadEvent eKey){
  switch (keypad.getState()){
    case PRESSED:
	Serial.print("Pressed: ");
	Serial.println(eKey);
	switch (eKey){
	  case '*': checkPassword(); break;
	  case '#': password.reset(); break;
	  default: password.append(eKey);
     }
  }
}

void checkPassword(){
  if (password.evaluate()){
    Serial.println("Success");
    //Add code to run if it works
  }else{
    Serial.println("Wrong");
    //add code to run if it did not work
  }
}

it comes with the "Password" library and confuses people.

So, don't be hard with the OP, the problem is not only, his code, but his source.

EDIT: When I begin to write this reply I only see the 2 first replies from PaulS and Robin2.
I've seen this code (this example) before.

crane_ep:
P.S. this:

} else if (stepperIsAtHome) {

moveStepper();
}



can probably be handled by the checkButton function, but I left it in the main loop so you can see the progression."

I emphatically would not have it in the checkButton function because it has absolutely ZERO to do with checking buttons. I wouldn't have it in loop() either - I would give it its own stepperMove() function.

And please don't quote entire posts in your reply - just enough to let people see what or who you are commenting on.

...R

The example without Events:

/*
||  Simple Password Entry Using Matrix Keypad
||  4/5/2012 Updates Nathan Sobieck: Nathan@Sobisource.com
||
*/


//* is to validate password   
//# is to reset password attempt

/////////////////////////////////////////////////////////////////

#include <Password.h> //http://www.arduino.cc/playground/uploads/Code/Password.zip
#include <Keypad.h> //http://www.arduino.cc/playground/uploads/Code/Keypad.zip

Password password = Password( "1234" );

const byte ROWS = 4; // Four rows
const byte COLS = 4; //  columns
// Define the Keymap
char keys[ROWS][COLS] = {
  {'1','2','3','A'},
  {'4','5','6','B'},
  {'7','8','9','C'},
  {'*','0','#','D'}
};

byte rowPins[ROWS] = { 9,8,7,6 };// Connect keypad ROW0, ROW1, ROW2 and ROW3 to these Arduino pins.
byte colPins[COLS] = { 5,4,3,2, };// Connect keypad COL0, COL1 and COL2 to these Arduino pins.


// Create the Keypad
Keypad keypad = Keypad( makeKeymap(keys), rowPins, colPins, ROWS, COLS );

void setup(){

  Serial.begin(9600);
}

void loop(){
  if (keypad.getState() == PRESSED) {
     Serial.print("Pressed: ");
      char key = keypad.getKey();
      Serial.println(key);
      switch (key){
         case '*': checkPassword(); break;
         case '#': password.reset(); break;
         default: password.append(key);
     }
  }
}

void checkPassword(){
  if (password.evaluate()){
    Serial.println("Success");
    //Add code to run if it works
  }else{
    Serial.println("Wrong");
    //add code to run if it did not work
  }
}

Can you please tell me it it work? I don't have any keypad.

Ok, looks like a rewrite is in order. Starting with calling my functions from the main loop. I can handle that.

Quick other question, I'd like the waitLED to flash while the stepper is moving. Sort of an alert to watch for movement, I'm familiar with the blink without delay program, will this work while the stepper is moving?

I believe the stepper.runToNewPosition() call of the accelStepper library is a blocking call so I assume nothing else can happen simultaneously. Thoughts?

I believe the stepper.runToNewPosition() call of the accelStepper library is a blocking call so I assume nothing else can happen simultaneously. Thoughts?

Use one of the non-blocking methods.

crane_ep:
Ok, looks like a rewrite is in order. Starting with calling my functions from the main loop. I can handle that.
(...)

I read in the subject "Help with cleaning up programming." The problem is that your program is (and I'm sorry to say this) a piece of garbage. If you read the reply #5, you will see why.

This code, answers the first question that you had in this thread:

PaulS:

  keypad.addEventListener(keypadEvent); //add an event listener for this keypad

Do you know what this is doing, and why you might need to do this?
(...)

I know it's garbage, that's why I'm here :smiley: