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);
}
}
const byte button1Pin = 2; // Assignment of the push button pins
That's not a typical newbie way of declaring a pin
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
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:
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?
