Feedback on LED "lock" game

Hey guys!

So, I am total rookie when it comes to programming and complete newbie when it comes to hardware. I decided one of the best ways to teach myself everything was to learn some basics and use those basics to create a mini-game. Essentially, my game randomizes a lock combination of LEDs. You have to get them in the right corner to "win." Basically, turn off/on the right red LEDs and the green one lights up.

I am looking for feedback and/or critique of my code. Everything works right now just fine, but I know being a newbie I may have missed something or there may be a better way to improve my work.

My plan for the next version is to include another button, where you press it to verify and if its not correct, then it resets all the LEDs.

I appreciate all the help!

const byte button1Pin = 2;    // Assignment of the push button pins
const byte button2Pin = 3;
const byte button3Pin = 4;

const byte led1Pin = 8;         // Assignment of the LED pins
const byte led2Pin = 9;
const byte led3Pin = 10;
const byte winledPin = 11;

byte led1State = LOW;          // Current state of LEDs
byte led2State = LOW;
byte led3State = LOW;

byte button1State = 0;         // Set the current state of the push buttons
byte button2State = 0;
byte button3State = 0;

long time = 0;         // monitor time of when the push button was toggled
long debounce = 200;   // the debounce time

byte randNumber1;     // random numbers for the lock order
byte randNumber2;
byte randNumber3;


void setup() {

  pinMode(led1Pin, OUTPUT);  // the status LEDs for the "lock" you must arrange in the proper order
  pinMode(led2Pin, OUTPUT);
  pinMode(led3Pin, OUTPUT);

  pinMode(winledPin, OUTPUT);  // the status light on whether you have "won"

  pinMode(button1Pin, INPUT);  // buttons for changing status of each led or "lock" arrangement
  pinMode(button2Pin, INPUT);
  pinMode(button3Pin, INPUT);

  // Generate random numbers for lock order
  randomSeed(analogRead(0));
  randNumber1 = random(2);
  randNumber2 = random(2);
  randNumber3 = random(2);
  
  // Activate the serial monitor for quality control
  Serial.begin(9600);
  
  // Show the random numbers in serial monitor to verify correct order
  Serial.println(randNumber1);
  Serial.println(randNumber2);
  Serial.println(randNumber3);
}

void loop() {

// Read the states of the button's input
  button1State = digitalRead(button1Pin);
  button2State = digitalRead(button2Pin);
  button3State = digitalRead(button3Pin);

// Set on or off LED 1
  if (button1State == HIGH && millis() - time > debounce) {
    if (led1State == HIGH)
      led1State = LOW;
    else
      led1State = HIGH;
  digitalWrite(led1Pin, led1State);
     time = millis();    
  }

// Set on or off LED 2
  if (button2State == HIGH && millis() - time > debounce) {
    if (led2State == HIGH)
      led2State = LOW;
    else
      led2State = HIGH;
  digitalWrite(led2Pin, led2State);
     time = millis();    
  }

// Set on or off LED 3
  if (button3State == HIGH && millis() - time > debounce) {
    if (led3State == HIGH)
      led3State = LOW;
    else
      led3State = HIGH;
  digitalWrite(led3Pin, led3State);
     time = millis();    
  }


// Determines if the correct code (LED arrangement) has been entered. ON/ON/OFF
// If correct order has been picked, then green LED will light up
  if (led1State == randNumber1 && led2State == randNumber2 && led3State == randNumber3) {
    digitalWrite (winledPin, HIGH);
  }
    else {
      digitalWrite (winledPin, LOW);
    }
}

asookah:

const byte button1Pin = 2;    // Assignment of the push button pins

That's not a typical newbie way of declaring a pin :slight_smile:

This is a (minor) error, though, cuasing the code to misbehave after some 24 days:

asookah:

long time = 0;         // monitor time of when the push button was toggled

Make that an unsigned long.
Otherwise after a good 24 days the counter reaches the point where it changes sign, and starts counting up from the negative limit, and your time calculations don't work properly any more.

That's not a typical newbie way of declaring a pin :slight_smile:

Is that because I used byte instead of int? I figured I have a limited amount of pins, so why burn up valuable space by using int when byte uses fewer bits.

Make that an unsigned long.

Roger that! I suppose that is for applications you expect to have running for a while?

asookah:
Roger that! I suppose that is for applications you expect to have running for a while?

As in almost all situations, there is a right and a not right way to do things.

Doing it right means that something you were not thinking about at the time will not jump up and bite you when you least expected or desired it. :grinning:

Many parts of the code are repeated 3 times. Learn to use arrays and for-loops to remove these and shorten your code significantly.

For example

const byte led1Pin = 8;         // Assignment of the LED pins
const byte led2Pin = 9;
const byte led3Pin = 10;

becomes

const byte ledPin[3] = {8, 9, 10};         // Assignment of the LED pins

and

  pinMode(led1Pin, OUTPUT);  // the status LEDs for the "lock" you must arrange in the proper order
  pinMode(led2Pin, OUTPUT);
  pinMode(led3Pin, OUTPUT);

becomes

 for (byte i=0; i<3; i++) pinMode(ledPin[i], OUTPUT);  // the status LEDs for the "lock" you must arrange in the proper order

That said, for just three LEDpins, it is quite likely that the loop code is not only slower, but uses more actual code space. :roll_eyes:

Macros may be neater.

asookah:
Is that because I used byte instead of int?

That - and the const part. Not often I see that used in first time programs. It's a good habit but most people won't think of it until they start hitting limits.

Roger that! I suppose that is for applications you expect to have running for a while?

Yes - of course it only becomes an issue after those 24-25 days. But better make it a habit, as @Paul__B said it can come back and bite you in the backside. I have had this happen to me in similar situations, where I forgot to make something unsigned (default is signed) and started to get some really strange results...

Basically in C++ you have to think about every variable declaration: what values do you expect, so what's the most appropriate type for this. I prefer to be even more explicit and use int16_t or uint16_t for an int, as on some platforms (AVR) an int is 16-bit and others (ESP) it's 32-bit...

The loop is I think a good thing, too. For just three LEDs it may not make the code shorter, but if you also declare the number of LEDs as variable at the top it becomes really easy to change the number of LEDs in your project. That's another reason to go with loops.

PaulRB:
Many parts of the code are repeated 3 times. Learn to use arrays and for-loops to remove these and shorten your code significantly.

Thank you so much! This is the kind of thing I was looking for! I really appreciate that. It means a lot!

wvmarle:
The loop is I think a good thing, too. For just three LEDs it may not make the code shorter, but if you also declare the number of LEDs as variable at the top it becomes really easy to change the number of LEDs in your project. That's another reason to go with loops.

This is fantastic. I do plan on working my way into some larger LED projects. Thanks a ton!

This has been hugely helpful. I really want to make sure I do things the right way. Good habits, you know?

    if (led2State == HIGH)
      led2State = LOW;
    else
      led2State = HIGH;

can be shortened to

    led2State = !led2State;

(This is a little more contraversial, I wonder if anyone will object...)