Shorten code

Hi all,
Kind of new to this forum, so excuse me if I’m in the wrong place!

I wrote a well-working program for an Arduino Motion-alarm. I think it’s a fairly simple program, but too long (in code). Not a very big problem, but I think I will get in trouble writing complexer programs.
So my question is: how can I significantly shorten my code? Which commands can I use to improve my sketch? It’s just a general question, but here’s my sketch for an example.
What can I improve? Can I improve something? And anyone got some tips for me in general?
This is my first program, so I like to hear some good advice!

// This alarm will detect motion using a PIR sensor. Thi sensor gives a HIGH output when motion is detected.
// Atter the motion is detected, the alarm waits to trigger and give the (legal) user some time to deactivate it.
// If the alarm isn't deactivated after an amount of time, the red LED goes HIGH and the alarm will beep.

// U S E D  M A T E RI A L S

// 3 LED's ; green, yellow, red
// 1 small speaker, piezo or something
// 1 button, for deactivating the alarm.
// 1 PIR sensor
// 1 resistor
// Jumper wires

// Green led from dpin 8 to ground
// Yellow led from dpin 9 to ground
// Red led from dpin 10 to ground
// PIR from 5V to ground and dpin 2
// button from 5V to dpin 6, other end to ground using a resistor
// Speaker from dpin 4 to ground

// Variables

int pirPin = 2;                // Locate the used pins
int greenLed = 8;            
int yellowLed = 9;             
int redLed = 10;               
int buttonPin = 6;           
int speakerPin = 4;            

int startingTime = 2;          // Times green led blinks to show alarm is activated
int i = 0;
int deactivationTime = 8;      // Time in seconds to deactivate a triggered alarm
int x = 0;
int deactivation;
int y = 0;
int alarmTime = 3;             // Times the alarm beeps
int safeTime = 10;             // Seconds alarm will be deactivated

//Setup

void setup() {                                                // Setup; only runs once
  Serial.begin(9600);
  Serial.println("Starting program ; setup");
  Serial.println("Starting program ; setting variables");
    pinMode(pirPin,INPUT);                                    // Setting Pins as in-/ outputs
    pinMode(greenLed,OUTPUT);
    pinMode(yellowLed,OUTPUT);
    pinMode(redLed,OUTPUT);
    pinMode(buttonPin, INPUT);
    pinMode(speakerPin, OUTPUT);
   Serial.println("Starting program ; variables set");
      
      while(i < startingTime) {                               // Attention for turned on device
       delay(100);
       digitalWrite(greenLed, LOW);
       delay(100);
       digitalWrite(greenLed, HIGH);
       i = i + 1;
      tone(speakerPin, 1000, 10);
       }
       
deactivationTime = deactivationTime * 5;                     // Scale deactivationTime
}                                                            // End setup

//Actual program

void loop() {
  Serial.println("Alarm has been activated!");
  int Motion = digitalRead(pirPin);                            // Read PIRsensor and store it in 'Motion'

    if(Motion == HIGH) {                                       //If there's motion, trigger alarm
     Serial.println("Alarm has been triggered!");
     Serial.print("You have ");
     Serial.print(deactivationTime / 5);
     Serial.println(" seconds to deactivate.");     
     digitalWrite(greenLed, LOW);
     digitalWrite(yellowLed, HIGH);                          // Turns on warning led and sound
     tone(speakerPin, 800,50);
     y = 0;
          
     while(x < deactivationTime) {                           // Gives time to deactivate the alarm
       if(digitalRead(buttonPin) == LOW) {                   // If button won't be pushed within deactivationTime, variable will stay '0'
         deactivation = 0;
       }
       else {                                                // If button is pushed, variable will  to '1'
         deactivation = 1;
         x = deactivationTime - 1;                           // Skip deactivationTime
       }
       
       x = x + 1;
       delay(200);
       Serial.println(x);
     }
     
     digitalWrite(yellowLed, LOW);                           // Turn of warning led
     
     if(deactivation == 0) {                                 // Retrieves saved variable; when deactivation = '0', so no deactivation code > ALARM
     Serial.print("BUSTED! The alarm triggered at ");
     digitalWrite(redLed, HIGH);
     while(y < alarmTime) {
       delay(500);
       tone(speakerPin, 1500, 400);
       y = y +1;
    x = 30;                                                  // Gives less deactivatonTime in next loop, so alarm will be beeping untill deactivated
   }
   }
   
     else {                                                  // Deactivates the alarm
       Serial.print("Alarm has been deactivated for ");
       Serial.print(safeTime);
       Serial.println("seconds.");
       digitalWrite(redLed, LOW);
       digitalWrite(greenLed, HIGH);
       delay(safeTime * 1000);                               // Safetime that alarm is deactivated
   x = 0;  
   }
     
}
}

Nothing immediately springs to mind it looks fine. You could learn to use arrays so that you do not have to specifically use a line for every pinMode statement and do it in a for loop.

Learn to layout your code for readability! Use the auto format option in the IDE.

Mark

Remove all the comments that you don't need because they are obvious or not needed otherwise. e.g. //variables

The other comments keep them small // Green - pin 8 is sufficient as all leds need a GND.

If you use descriptive variables names the code comments itself

In short: only use comments to explain the WHY (the what/how should be obvious from code structure/naming )


2) do not use delay() Look at the blink without delay example how you can program timing without delay. It might seem overdone sometimes but it really makes your programs more responsive.

robtillaart: ... The other comments keep them small // Green - pin 8 is sufficient as all leds need a GND. ...

I have to disagree with Rob here your comments about the wiring of the LED helps me to say: "did you include a series resistor with the LED?" it's not smart to drive a LED without one also the LED could easily have been wired from pin to Vcc so the comments are valid as written :)

you can also do away with the resistor for the switch look at using the internal pullup resistor

finally (for now :) ) I would worry about connecting a speaker directly to the Arduino as well far too easy to exceed the output current

Code is fine as-is, nice job. But since you asked for feedback on efficiency,

  1. If the value never changes -- like a pin number, for example -- use a const:
const int pirPin = 2;                // Locate the used pins

This will save on precious RAM and also keep you from accidentally changing the variable to an unintended value later.

  1. If the variable does change, but will never go beyond a values between 0 and 255, use a byte instead of an int:
byte safeTime = 2;

Not only do bytes use up half as much RAM as ints, the CPU can execute instructions involving bytes (about) twice as fast as ints. Similarly, if you have a variable that never takes on a value beyond -128 and 127, use a 'char'. It sounds odd, but yes, you can store numbers in a 'char'.

int pirPin = 2;

In my opinion there are three possibilities:

const int pirPin = 2;
static const int pirPin = 2;
#define pirPin 2

but there is a big debate which is better: Const vs #define

Use port manipulation, it rocks. http://www.arduino.cc/en/Reference/PortManipulation You could replace all those pinModes with a sinlge line like

DDRD  |= B11111100;

or what ever bit mask your application requires. It is also more efficient when doing multiple digital reads or writes as you can Get/Set multiple pins in a single instruction allowing you to set multiple pins simultaneously, something digitalRead / Write can not.