Simon Says using Arduino UNO

Question

I built a Simon Says game using an Arduino UNO board. This was my first "real" arduino project. I would like to know how to make it better, game mechanics and code efficiency.

#define RedTone 165
#define GreenTone 330
#define YelloTone 659
#define BlueTone 1319
#define FailTone 100

#define BuzzerPin 8


/* The led and button pins are set this way because I use their position as a hexadecimal.
 *  LED PINS:
      red:0
      green:1
      yello:2
      blue:3
    Button Pins:
      red:4
      green:5
      yello:6
      blue:7
*/


byte mem[20];
int sound[16] = {0, RedTone, GreenTone, 0, YelloTone, 0, 0, 0, BlueTone, 0, 0, 0, 0, 0, 0, FailTone};  //the tones are placed in the arrey in the hex spot for turning on the leds.


void setup() {
  DDRD = 0x0F;
  randomSeed(analogRead(0));
  pinMode(BuzzerPin, 1);
}


void blink(int i) { //Blinking and making sound
  PORTD = i;
  tone(BuzzerPin, sound[i]);
  delay(500);
  PORTD = 0x00;
  noTone(BuzzerPin);
  delay(500);
}


void NT(int i) { //Just blinking
  PORTD = i;
  delay(500);
  PORTD = 0x00;
  delay(500);
}


void set_mem() {  //sets the random order of the leds and transforms into the led's pin in hexadecimal
  int b;
  for (int i = 0; i < 20; i++) {
    b = random(4);
    if (b == 3)
      mem[i] = 0x08;
    else if (b == 2)
      mem[i] = 0x04;
    else if (b == 1)
      mem[i] = 0x02;
    else
      mem[i] = 0x01;
  }
}


bool check(int i) { //checks if the pressed button is correct
  byte look;
  for (int x = 0; x <= i; x++) {
    look = 0;
    do
    {
      look = PIND >> 4;  //makes the button press pin to the pin of led with the same color
    }
    while (look == 0);  //waits until a button is pressed
    blink(look); //lights up the button's led
    if (look != mem[x])
      return 0;
  }
  return 1;
}


void loop() {
  set_mem();
  for (int i = 0; i < 20; i++) {
    for (int x = 0; x <= i; x++)
      blink(mem[x]);
    NT(0x09); //signals when it finnished showing the order
    if (check(i) == 0) {
      blink(0x0F); //shows if the player failed
      break; //couses a restart
    }
    NT(0x06); //signals when the player finished pressing the leds
  }
}

How it looks and works:

(I'm having problems making the snippet work, so here is the link to the site itself and here is a video of it working (sorry for the low quality).)

<iframe frameborder='0' height='448' marginheight='0' marginwidth='0' scrolling='no' src='https://circuits.io/circuits/4145099-simon-says-leds-and-buttons/embed#breadboard' width='650'></iframe>

Answer 1

Here are a few suggestions, in my opinion, to improve your code:

1. Using Port Registers:
   You use PORTD, PIND, and DDRD in the code. These can be replaced bydigitalWrite(),digitalRead() and pinMode(). Here is the Arduino reference page for Port Registers. Although they will quicken the speed of execution, in your case, that high speed is not required, especially in the setup. The suggested alternatives will fulfill the task equally well.
2. Electronic Advice:
   Avoid using the pin 0 and pin 1. These are used for Serial debugging, and even though you are not using serial communication, it is good practice to not use them for I/O. Here is a reddit post discussing that, here is the official board layout reference.
3. Specific Code reductions:
   in set_mem(), instead of the logical conditions, you can predefine an array map = {0x01, 0x02, 0x04, 0x08} and then set the mem[i] = map[random(4)].
4. Gameplay improvements
   As it stands, it is very confusing to see what is going on, and how to play the game. Assuming that you are limited by these components and the difficulty level of the code is not to be scaled, you can consider the following:
   1. Indicating correct pressing. Currently, it is difficult to make out which two LEDs signal the completion of showing the required sequence, and which two signal you having pressed the buttons correctly. You can create another function which does not light up any LED but just signals a correct press by means of a musical sound, the relative pleasantness indicates to any onlooker that the answer was correct. Consider a simple increasing and decreasing tone, or even better, look-up the frequencies somewhere like here of musical notes and create a pleasant tune! Just replace the NT(0X06)by the new function.
   2. Indicating failure. You can include the Failtone for a larger duration with all the 4 LEDs to indicate failure. You might have to make a new function to replace NT(0X09).
   3. I am not sure whether having to repeat the sequence with one new added press each time is a part of the design. But if it is not, you can create a new sequence each time, by keeping set_mem() in the loop, so that every time they complete a sequence, an entirely new sequence of presses (longer by 1) comes up next.
5. Provisions for faulty game play. Currently, the person can press the buttons while the sequence is being displayed. Although it does not affect the gameplay in any way, it is generally good practice to anticipate faulty play. adding, for example, the following in the code below blink(mem[x]) in the nested for loop, may achieve that, where failurefunction is what you use to indicate failure.

Code:

  look = PIND >> 4;
  if (look != 0){
  failurefunction();
  break;
  }

I hope this was helpful.