Code Review for a Newbie

Using Arduino Programming Questions
1

I was hoping someone can review my code for a project I've been working on. I've only been at this a few weeks and I'm sure I've implemented this in the most roundabout way possible.

Either way - it works for my purpose, but would appreciate any feedback so I know what I could do better on next time (or improve on this code!).

Thank you very much.

/*
 *  The goal is to have X (currently 2) toggle switches which, when used, turn an LED on or off and performs a single keystroke.
 */

int switch1Pin = 4;              // Indicates Toggle1 is connected to Pin 4.
int led1Pin = 7;                 // Indicates LED1 is connected to Pin 7.
int val;                         // Indicates variable for reading Toggle1 status.
int val2;                        // Indicates variable for reading Toggle1 delayed/debounced status.
int button1State;                // Indicates the state of Toggle1.
int light1Mode = 0;              // Indicates the status of the LED1 (On/Off).

int switch2Pin = 9;              // Indicates Toggle2 is connected to Pin 9.
int led2Pin = 12;                // Indicates LED2 is connected to Pin 7.
int val3;                        // Indicates variable for reading Toggle2 status.
int val4;                        // Indicates variable for reading Toggle2 delayed/debounced status.
int button2State;                // Indicates the state of Toggle2.
int light2Mode = 0;              // Indicates the status of LED2 (On/Off).

void setup() {
  pinMode(switch1Pin, INPUT);    // Indicates Toggle1 is the input.
  pinMode(led1Pin, OUTPUT);      // Indicates LED1 is the output. 

  pinMode(switch2Pin, INPUT);    // Indicates Toggle2 is the input.
  pinMode(led2Pin, OUTPUT);      // Indicates LED2 is the output.
  
  Serial.begin(9600);                         // Sets up serial communication with PC at 9600bps.
  button1State = digitalRead(switch1Pin);     // Indicates the intial state of Toggle1.
  button2State = digitalRead(switch2Pin);     // Indicates the intial state of Toggle2.
}

void loop(){
  val = digitalRead(switch1Pin);              // Indicates the value of Toggle1 and stores in it a variable.
  delay(10);                                  // Indicates a 10 milliseconds delay.
  val2 = digitalRead(switch1Pin);             // Indicates the value of Toggle1 and stores it in a variable.
  if (val == val2) {                          // Confirmation of 2 consistent readings of Toggle1.
    if (val != button1State) {                // If values are not consistent and,
      if (val == LOW) {                       // If Toggle1 was used and,
        if (light1Mode == 0) {                // If LED1 is off,
          light1Mode = 1;                     // Then turn on LED1 and,
          digitalWrite(led1Pin, HIGH);        // Indicates the voltage of LED1 is changed to high.
          Keyboard.write('a');                // Type the letter indicated and,
          delay(50);                          // Indicates a 50 milliseconds delay.
          Keyboard.release('a');              // Release the letter indicated.
        } else {                              // else
          light1Mode = 0;                     // Then turn off LED2 and,
          digitalWrite(led1Pin, LOW);         // Indicates the voltage of LED1 is changed to low.
          Keyboard.write('a');                // Type the letter indicated and,
          delay(50);                          // Indicates a 50 milliseconds delay.
          Keyboard.release('a');              // Release the letter indicated.
        }
      }
    }
    button1State = val;                       // Indicates the changed state has been stored as a variable.
  }

  val3 = digitalRead(switch2Pin);             // Indicates the value of Toggle2 and stores in it a variable.
  delay(10);                                  // Indicates a 10 milliseconds delay.
  val4 = digitalRead(switch2Pin);             // Indicates the value of Toggle2 and stores it in a variable.
  if (val3 == val4) {                         // Confirmation of 2 consistent readings of Toggle1.
    if (val3 != button2State) {               // If values are not consistent and,
      if (val3 == LOW) {                      // If Toggle2 was used and,
        if (light2Mode == 0) {                // If LED2 is off,
          light2Mode = 1;                     // Then turn on LED2 and,
          digitalWrite(led2Pin, HIGH);        // Indicates the voltage of LED2 is changed to high.
          Keyboard.write('b');                // Type the letter indicated and,
          delay(50);                          // Indicates a 50 milliseconds delay
          Keyboard.release('b');              // Release the letter indicated.
        } else {                              // else
          light2Mode = 0;                     // Then turn off LED2 and,
          digitalWrite(led2Pin, LOW);         // Indicates the voltage of LED2 is changed to low.
          Keyboard.write('b');                // Type the letter indicated and,
          delay(50);                          // Indicates a 50 milliseconds delay.
          Keyboard.release('b');              // Release the letter indicated.
        }
      }
    }
    button2State = val3;                      // Indicates the changed state has been stored as a variable.
  }
  }
2

asabassa1:
I was hoping someone can review my code for a project I've been working on. I've only been at this a few weeks and I'm sure I've implemented this in the most roundabout way possible.

Either way - it works for my purpose, but would appreciate any feedback so I know what I could do better on next time (or improve on this code!).

Thank you very much.

/*

*  The goal is to have X (currently 2) toggle switches which, when used, turn an LED on or off and performs a single keystroke.
*/

int switch1Pin = 4;              // Indicates Toggle1 is connected to Pin 4.
int led1Pin = 7;                // Indicates LED1 is connected to Pin 7.
int val;                        // Indicates variable for reading Toggle1 status.
int val2;                        // Indicates variable for reading Toggle1 delayed/debounced status.
int button1State;                // Indicates the state of Toggle1.
int light1Mode = 0;              // Indicates the status of the LED1 (On/Off).

int switch2Pin = 9;              // Indicates Toggle2 is connected to Pin 9.
int led2Pin = 12;                // Indicates LED2 is connected to Pin 7.
int val3;                        // Indicates variable for reading Toggle2 status.
int val4;                        // Indicates variable for reading Toggle2 delayed/debounced status.
int button2State;                // Indicates the state of Toggle2.
int light2Mode = 0;              // Indicates the status of LED2 (On/Off).

void setup() {
  pinMode(switch1Pin, INPUT);    // Indicates Toggle1 is the input.
  pinMode(led1Pin, OUTPUT);      // Indicates LED1 is the output.

pinMode(switch2Pin, INPUT);    // Indicates Toggle2 is the input.
  pinMode(led2Pin, OUTPUT);      // Indicates LED2 is the output.
 
  Serial.begin(9600);                        // Sets up serial communication with PC at 9600bps.
  button1State = digitalRead(switch1Pin);    // Indicates the intial state of Toggle1.
  button2State = digitalRead(switch2Pin);    // Indicates the intial state of Toggle2.
}

void loop(){
  val = digitalRead(switch1Pin);              // Indicates the value of Toggle1 and stores in it a variable.
  delay(10);                                  // Indicates a 10 milliseconds delay.
  val2 = digitalRead(switch1Pin);            // Indicates the value of Toggle1 and stores it in a variable.
  if (val == val2) {                          // Confirmation of 2 consistent readings of Toggle1.
    if (val != button1State) {                // If values are not consistent and,
      if (val == LOW) {                      // If Toggle1 was used and,
        if (light1Mode == 0) {                // If LED1 is off,
          light1Mode = 1;                    // Then turn on LED1 and,
          digitalWrite(led1Pin, HIGH);        // Indicates the voltage of LED1 is changed to high.
          Keyboard.write('a');                // Type the letter indicated and,
          delay(50);                          // Indicates a 50 milliseconds delay.
          Keyboard.release('a');              // Release the letter indicated.
        } else {                              // else
          light1Mode = 0;                    // Then turn off LED2 and,
          digitalWrite(led1Pin, LOW);        // Indicates the voltage of LED1 is changed to low.
          Keyboard.write('a');                // Type the letter indicated and,
          delay(50);                          // Indicates a 50 milliseconds delay.
          Keyboard.release('a');              // Release the letter indicated.
        }
      }
    }
    button1State = val;                      // Indicates the changed state has been stored as a variable.
  }

val3 = digitalRead(switch2Pin);            // Indicates the value of Toggle2 and stores in it a variable.
  delay(10);                                  // Indicates a 10 milliseconds delay.
  val4 = digitalRead(switch2Pin);            // Indicates the value of Toggle2 and stores it in a variable.
  if (val3 == val4) {                        // Confirmation of 2 consistent readings of Toggle1.
    if (val3 != button2State) {              // If values are not consistent and,
      if (val3 == LOW) {                      // If Toggle2 was used and,
        if (light2Mode == 0) {                // If LED2 is off,
          light2Mode = 1;                    // Then turn on LED2 and,
          digitalWrite(led2Pin, HIGH);        // Indicates the voltage of LED2 is changed to high.
          Keyboard.write('b');                // Type the letter indicated and,
          delay(50);                          // Indicates a 50 milliseconds delay
          Keyboard.release('b');              // Release the letter indicated.
        } else {                              // else
          light2Mode = 0;                    // Then turn off LED2 and,
          digitalWrite(led2Pin, LOW);        // Indicates the voltage of LED2 is changed to low.
          Keyboard.write('b');                // Type the letter indicated and,
          delay(50);                          // Indicates a 50 milliseconds delay.
          Keyboard.release('b');              // Release the letter indicated.
        }
      }
    }
    button2State = val3;                      // Indicates the changed state has been stored as a variable.
  }
  }

Good show, applaud on your commenting your code.

Small improvements - suggestions.

Comment should describe what the code is doing and it is not necessary to just replicate the actual code.
For example
delay(50); // Indicates a 50 milliseconds delay.

would be more descriptive as
delay(50); // wait for next keystroke

( Of course this is an example - keyboard doe not need rest in this case )

Another example

light2Mode = 1; // Then turn on LED2 and,
digitalWrite(led2Pin, HIGH); // Indicates the voltage of LED2 is changed to high.

more descriptive
light2Mode = 1; // set LED2 state to on
digitalWrite(led2Pin, HIGH); // turn LED2 on

Other that that , you are writing the code / comments for yourself ( or maybe for your boss ) not for the forum , so if you prefer your style it is OK.

Good work.
Cheers

3

Just a few quick observations:

I'd change the name of the val, val1, val2 and val3 and val4 to something more descriptive - something that tells me what the value is. It also looks as though these can be moved to be declared inside the function instead of being global variables.

change the following vars to be const: switch1pin, led1pin, switch2pin, led2pin. If the values are not going to change, you should make them constants by adding 'const' to the start of their declaration.

4

Save some memory by using byte sized variables instead of ints wherever possible. This does not matter in this program but could be important in the future.

You do not say how your inputs are wired. Do you have any pullup or pulldown resistors to keep the inputs at a known state rather than floating at an uncertain voltage ? Consider activating the internal pullup resistors by using

pinMode(switch1Pin, INPUT_PULLUP);

if (light1Mode == 0) {                // If LED1 is off,
          light1Mode = 1;

A neater way to do this is to use

light1Mode = ! lightMode1;

Then you could remove the if/else and have (not tested)

if (val == LOW) 
{                       // If Toggle1 was used and,
  light1Mode = !light1Mode;                     // Then turn on LED1 and,
  digitalWrite(led1Pin, light1Mode);        // Indicates the voltage of LED1 is changed to high.
  Keyboard.write('a');                // Type the letter indicated and,
  delay(50);                          // Indicates a 50 milliseconds delay.
  Keyboard.release('a');              // Release the letter indicated.
}

Note that I prefer to put each { and } on its own line to make the code block stand out, but that is up to you

5

Have a look at planning and implementing a program - especially how the code is organized into small functions that can each be tested separately.

In your code everything is mixed up like muesli.

Also see how millis() is used to manage timing. In general, don't use the delay() function as the Arduino cannot do anything else during a delay().

...R

6 
int switch1Pin = 4;

Unless you're planning on changing the pin assignment whilst the sketch is running,
const byte switch1Pin = 4; (etc) would be better.

7

Rarely is it necessary to comment every line of code in a program. For example:

          Keyboard.write('b');                // Type the letter indicated and,
          delay(50);                          // Indicates a 50 milliseconds delay.
          Keyboard.release('b');              // Release the letter indicated.

would be less cluttered as:

          Keyboard.write('b');                // Write and release key
          delay(50);                          
          Keyboard.release('b');

The same is true for if-else statement blocks. Reduce the comments and factor out code that is common to either clause:

        if (light2Mode == 0) {             // If LED2 is off, turn it on...
          light2Mode = 1;                   
          digitalWrite(led2Pin, HIGH);      
           
        } else {                           // ...otherwise turn it off...
          light2Mode = 0;                     
          digitalWrite(led2Pin, LOW);         
        }
        Keyboard.write('b');               // ...and then type the letter indicated.
        delay(50);                         
        Keyboard.release('b');
8

Robin2:
Have a look at planning and implementing a program - especially how the code is organized into small functions that can each be tested separately.

In your code everything is mixed up like muesli.

Also see how millis() is used to manage timing. In general, don't use the delay() function as the Arduino cannot do anything else during a delay().

...R

OT
Yes, just do not read the OP and make a blanket statement like this one.
I am surprised you did not add "use blink without delay".

To the OP
most old hands use "keep it simple stupid" - KISS - method. Using millis() when a simple delay is needed is not KISS.

As far as "mixed up" - it is easier to start with performing a task and reusing it ( copy and paste ) and when the code is working THAN you can start replacing common code with functions etc.
The code will be shorter , but not necessarily more humanly readable.

On real note - when you evaluate condition for true (if( condition)) it helps to add "else" just to catch possible errors in your code.

IMHO adding feedback to even most simplest code is important- as you did using the LED's.

.

9

Vaclav:
I am surprised you did not add "use blink without delay".

Actually I did, but not using those exact words.

...R

10

What is this "Keyboard" of which you speak?

... but seriously, since it would leap out at anyone as new, you really should have some documentation in there to explain it. Is is an external library? What does it mean to write to a keyboard???? You should explain it in the comments.

Edit: Aha, it's for the Zero or Due... anyway it needs some inline documentation.

11

Vaclav:
I am surprised you did not add "use blink without delay".

We're astonished you managed to get quote tags correct - well done!

12

This piece caught my eye in Reply #6

if (light2Mode == 0) {             // If LED2 is off, turn it on...
          light2Mode = 1;                   
          digitalWrite(led2Pin, HIGH);     
           
        } else {                           // ...otherwise turn it off...
          light2Mode = 0;                     
          digitalWrite(led2Pin, LOW);         
        }

I think this single line would have the same effect

digitalWrite(led2Pin, light2Mode); // If LED2 is off, turn it on, otherwise off

However I think I would be tempted to have a function such as this (I'm assuming there are two LEDs)

void updateLEDs() {
    digitalWrite(led1Pin, light1Mode); 
    digitalWrite(led2Pin, light2Mode); 
}

which would be called from loop()

...R

13

@Robin2: I think all of us would make major mods to the code if it were ours. My post was simply trying to deal with all of the unnecessary comments and factor out the obvious.

14

I think this single line would have the same effect

No. The original code toggled the value of light2Mode. Your single line replacement does not.

15

econjack:
@Robin2: I think all of us would make major mods to the code if it were ours. My post was simply trying to deal with all of the unnecessary comments and factor out the obvious.

Sorry, @econjack - I did not intend my comment to refer to your post - only to the piece of the OP's code that you had picked out.

PaulS:
No. The original code toggled the value of light2Mode. Your single line replacement does not.

You are quite right - I had not spotted that. In my haste I had assumed the variable light2Mode was changed elsewhere.

But then, perhaps all that is needed is

digitalWrite(led2Pin, ! digitalRead(led2Pin));

...R

16

But then, perhaps all that is needed is

No. That's sets the state of the pin to the opposite of what it is, but gets the pin out of sync with the state variable. Maybe the state variable is not needed. But, if it is, you need two statements.

light2Mode = !light2Mode;
digitalWrite(led2Pin, light2Mode);