Critique my code

I’ve written a sketch (first one) that measures movement with an accelerometer and will turn a phone on and off based on that movement. I’m looking for a constructive critique of the code. Maybe places where it can be cleaned up or made a little more efficient.

/* -Accelerometer Based Motion Detection System-
This system measures motion with a 2-axis accelerometer and
if motion is detected within a certain threshold, a digital signal 
will be sent to switch a phone on or off.  Analog values at rest are 
typically 511. If there is a fluctuation of +or- 3 then it is safe
to assume there is movement. 

-Code by: SJS Enterprises -
-Version 0.01beta-
*/


int switchPin = 4; //pin that turns phone on and off
int xVal = 0; //first x-axis reading in for loop
int yVal = 0; //first y-axis reading in for loop
int xVal2 = 0; //second x-axis reading in for loop
int yVal2 = 0; //second y-axis reading in for loop
int x = 4; //analog input pin
int y = 5; //analog input pin
int xValHigh1 = 0;
int yValHigh1 = 0;
int xValLow1 = 0;
int yValLow1 = 0;
int xValHigh;
int xValLow;
int yValHigh;
int yValLow;
boolean phoneOn = FALSE;//is phone on
boolean movement = FALSE;

//turn on the power to phone when system is initially energized
void power(){
digitalWrite(switchPin, HIGH);
delay(3000);
digitalWrite(switchPin, LOW);
xVal = analogRead(x);
yVal = analogRead(y);
phoneOn = TRUE;
delay(3000);
}

void setup(){
pinMode(4,OUTPUT);
power();
}

void loop(){
//set initial high and low values
xValLow = 1023;
xValHigh = 0;
yValLow = 1023;
yValHigh = 0;


/*this 'for' loop grabs accelerometer readings every second for 
five minutes and stores the highest and lowest values that were 
received during that time
*/
for (int i=0; i<300; i++){
xVal = analogRead(x);//first x-axis reading
yVal = analogRead(y);//fist y-axis reading
delay(1000);//pause for 1 second before taking another axis reading
xVal2 = analogRead(x);//second x-axis reading
yVal2 = analogRead(y);//second y-axis reading
xValLow1 = min(xVal,xVal2);//set minimum value of two analog readings
xValHigh1 = max(xVal,xVal2);//set maximum value of two analog readings
yValLow1 = min(yVal,yVal2);//set minimum value of two analog readings
yValHigh1 = max(yVal,yVal2);//set maximum value of two analog readings
xValLow = min(xValLow,xValLow1);//lowest x-axis value during loop
xValHigh = max(xValHigh,xValHigh1);//highest x-axis value during loop
yValLow = min(yValLow,yValLow1);//lowest y-axis value during loop
yValHigh = max(yValHigh,yValHigh1);//highest y-axis value during loop
}

//is there movement within threshold on either x or y axis
if ((xValHigh-xValLow)>5) || ((yValHigh-yValLow)>5){
movement = TRUE;
}
else {
movement = FALSE;
}

//if there is movement and the phone is off then turn it on
if (movement == TRUE && phoneOn == FALSE){
digitalWrite(switchPin,HIGH);
delay(3000);
digitalWrite(switchPin,LOW);
phoneOn=TRUE;
}

//if there is no movement and phone is on then turn it off
if (movement == FALSE && phoneOn == TRUE){
digitalWrite(switchPin,HIGH);
delay(3000);
digitalWrite(switchPin,LOW);
phoneOn=FALSE;
}

A very good first sketch. Its clearly laid out and well commented. The lack of indenting is something your should fix, easy to do if you use the format tool in the IDE. But it was still easy to see what you wanted to achieve and your approach is well thought out. I am not sure it compiles as it is, I think there may be a misplaced brackets or two.

You can simplify the logic by not storing so many values. There is no reason not to just take an ADC reading when you need one.

I have attached a variation of your sketch which may prompt some ideas for you to consider.

I hope that helps.

int switchPin = 4; //pin that turns phone on and off
boolean phoneOn  = FALSE;  //is phone on
boolean movement  = FALSE;

//turn on the power to phone when system is initially energized
void TurnPhoneOn(){
  digitalWrite(switchPin, HIGH);
  delay(3000);
  digitalWrite(switchPin, LOW);
  phoneOn = TRUE;
  delay(3000);
}

void TurnPhoneOff() {
  // your code here to turn the phone off  
  
}

void setup(){
  pinMode(4,OUTPUT);
  TurnPhoneOn();
}


void loop(){
int x = 4; //analog input pin
int y = 5; //analog input pin
int xValHigh;
int xValLow;
int yValHigh;
int yValLow;

  //set initial high and low values
  xValLow = 1023;
  xValHigh = 0;
  yValLow = 1023;
  yValHigh = 0;


  /*this 'for' loop grabs accelerometer readings every second for 
   five minutes and stores the highest and lowest values that were 
   received during that time
   */
   
  
  for (int i=0; i<300; i++){     
    xValLow = min(xValLow, analogRead(x));
    yValLow = min( yValLow, analogRead(y));
    xValHigh = max(xValHigh,analogRead(x) );//set maximum value of two analog readings
    yValHigh = max(yValHigh, analogRead(y));//set minimum value of two analog readings
    delay(1000);//pause for 1 second before taking another axis reading
 
  }

  //is there movement within threshold on either x or y axis
  movement =  ((xValHigh-xValLow)>5) || ((yValHigh-yValLow)>5);

  //if there is movement and the phone is off then turn it on
  if (movement && ! phoneOn ){
    TurnPhoneOn(); 
  }
  //if there is no movement and phone is on then turn it off
  else if ( !movement  && phoneOn ){
    TurnPhoneOff();
  }
}

Thank you very much for taking the time to look at the code. You've opened my eyes and I see exactly how your changes cleaned it up. I spent several hours last night coming up with this and I knew that I was overthinking it, but I couldn't quite pare it down myself. Thanks again!!

You can save a couple of bytes by using #defines for the pin values. If the value never changes then there isn't really a reason for it to be a variable. #defines are filled in by the compiler and don't take up any RAM.

If you don't like that, another thing is that since the pin values are always less than 255 (for the foreseeable future) you can declare them as bytes, which are half the size of ints.

Something I neglected to mention before is that because you are toggling the phone on and off, this could result in the phone power getting out of sequence. If you could wire the phone so that it was on when the switchPin was high and off when the pin was low then it would always be in sync. An example of the code change necessary is included in a comment below.

I have also taken the liberty of a little more condensing of the code. Consider these as style suggestions rather then recommendations and you should always feel free to ignore constructs that you don’t feel comfortable with.

#define switchPin 4 //pin that turns phone on and off
#define xpin      4 //analog input pin
#define ypin      5 //analog input pin
#define THRESHOLD 5 // any sensed movment greater than this will turn the phone on 

boolean phoneOn;   // this is set in calls SetPhoneState

// turn the phone on if State is true, off if false
void SetPhoneState(boolean State ){
  digitalWrite(switchPin, HIGH);
  delay(3000);
  digitalWrite(switchPin, LOW);
  phoneOn = State;
  delay(3000);
}

/* here is a version that does not toggle
// turn the phone on if State is true, off if false
void SetPhoneState(boolean State ){
  digitalWrite(switchPin, State);  // phone wired so its on if pin is high, off if pin is low
//  phoneOn = State;  // no phone state variable is needed, this can be removed
}
*/

void setup(){
  pinMode(4,OUTPUT);
  SetPhoneState(true); //turn on the power to phone when system is initially energized
}

void loop(){
int xValHigh;
int xValLow;
int yValHigh;
int yValLow;

  //set initial high and low values
  xValLow =  yValLow = 1023;
  xValHigh = yValHigh = 0;

  /*this 'for' loop grabs accelerometer readings every second for 
   five minutes and stores the highest and lowest values that were 
   received during that time
   */   
  
  for (int i=0; i<300; i++){     
    xValLow = min(xValLow, analogRead(xpin));
    yValLow = min( yValLow, analogRead(ypin));
    xValHigh = max(xValHigh,analogRead(xpin));//set maximum value of two analog readings
    yValHigh = max(yValHigh, analogRead(ypin));//set minimum value of two analog readings
    delay(1000);//pause for 1 second before taking another axis reading 
  }

  //is there movement within threshold on either x or y axis
  boolean movement = ((xValHigh-xValLow)> THRESHOLD) || ((yValHigh-yValLow)> THRESHOLD);
  SetPhoneState(movement); // phone should be on if movement, off if not
}

Thanks again for the help.

1 This new set of code is going to take several read throughs on my part to understand it. It's not as easy as your other example.

2 is that the phone can only be wired one way. I had to actually take it apart and solder wires to the metal surfaces of the pushbutton and then run those wires to a 4066 quad bilateral switch which is operated by the i/o pin on the arduino. When you peel the front of the phone away you'll see the the on / off button is actually a metal ring going around a metal dot. The button is a concave round disc that makes contact with the ring and dot when pressed and closes that circuit. Essentially the arduino is like your finger, pushing and holding the on/off button of the phone for 3 seconds. Do it once and that turns the phone on and if you do it again, it turns the phone back off. If you do it quickly, that hangs up a call. If you hold it for 2.5-3 seconds, that operates the power.

This weekend I'm going to load your first suggestion onto my lilypad and give it a test (I'll probably load my first try too, just to see if I could have done it). After that, I have a Boarduino on order and am going to try and get it running at 3.7V (run it at 8Mhz) and then try the code on that because it has a better footprint than the lilypad.

Thanks again. You've been more than helpfull.

You should use whichever version you feel most comfortable with :)

If you stick with a version that toggles, and I can understand why you would, then be aware that the arduino can get out of sync if the phone is switched off external from your sketch or if the arduino resets while the phone is on.

If you need to, you can prevent this by connecting another wire to into somewhere on the phone that has a voltage only when the phone is on. If this is connected to an input pin then you could monitor that as the 'phoneOn' state and it will always tell you if the phone is actually on or not and you should never get out of step.

That's a great idea. I see what your saying about being out of sync. I think I'm going to poke around today and look for V when power is on and see what I can find. The phone isnt going to be used by anyone, so it shouldnt be shut off independently of the arduino (it'll be running a Java 2ME program and maybe acting as a server), but I can see if the arduino somehow resets and they'll be out of sync.

BTW..Boarduino arrived yesterday..one step closer to having something I can touch and see working.

Thanks again to all who have helped. It is up and running great. I used Mem's first revision of my code. The only change I had to make was to declare int FALSE = 0; and int TRUE = 1;. For some reason it wouldn't just take boolean. I never did find another way to check power, so I am going to run some extensive field tests to make sure the possibility of an out-of-sync issue is null. So far, for the past week, I've been ok.

The standard C boolean keywords are true and false all in lower case. If you use those then you don't need to define your own.

Good to hear you have it going.

Thanks to mem and others who helped me through my first project. Here is a link http://www.youtube.com/watch?v=z68zy86qIww to a video that shows it working in a prototype setup. I am 99% done developing the PCB for this, which will clean it up and shrink it drastically.. Right now it's pretty rude looking but the PCB will make it look nice.

It's a motion activated cell phone that will turn on and keep running as long as there is motion. You can load java2me apps on the phone and they will load upon power up and start running too.

I’ve written a sketch (first one) that measures movement with an accelerometer and will turn a phone on and off based on that movement. I’m looking for a constructive critique of the code. Maybe places where it can be cleaned up or made a little more efficient.

/* -Accelerometer Based Motion Detection System-

This system measures motion with a 2-axis accelerometer and
if motion is detected within a certain threshold, a digital signal
will be sent to switch a phone on or off.  Analog values at rest are
typically 511. If there is a fluctuation of +or- 3 then it is safe
to assume there is movement.

-Code by: SJS Enterprises -
-Version 0.01beta-
*/

int switchPin = 4; //pin that turns phone on and off
int xVal = 0; //first x-axis reading in for loop
int yVal = 0; //first y-axis reading in for loop
int xVal2 = 0; //second x-axis reading in for loop
int yVal2 = 0; //second y-axis reading in for loop
int x = 4; //analog input pin
int y = 5; //analog input pin
int xValHigh1 = 0;
int yValHigh1 = 0;
int xValLow1 = 0;
int yValLow1 = 0;
int xValHigh;
int xValLow;
int yValHigh;
int yValLow;
boolean phoneOn = FALSE;//is phone on
boolean movement = FALSE;

//turn on the power to phone when system is initially energized
void power(){
digitalWrite(switchPin, HIGH);
delay(3000);
digitalWrite(switchPin, LOW);
xVal = analogRead(x);
yVal = analogRead(y);
phoneOn = TRUE;
delay(3000);
}

void setup(){
pinMode(4,OUTPUT);
power();
}

void loop(){
//set initial high and low values
xValLow = 1023;
xValHigh = 0;
yValLow = 1023;
yValHigh = 0;

/*this ‘for’ loop grabs accelerometer readings every second for
five minutes and stores the highest and lowest values that were
received during that time
*/
for (int i=0; i<300; i++){
xVal = analogRead(x);//first x-axis reading
yVal = analogRead(y);//fist y-axis reading
delay(1000);//pause for 1 second before taking another axis reading
xVal2 = analogRead(x);//second x-axis reading
yVal2 = analogRead(y);//second y-axis reading
xValLow1 = min(xVal,xVal2);//set minimum value of two analog readings
xValHigh1 = max(xVal,xVal2);//set maximum value of two analog readings
yValLow1 = min(yVal,yVal2);//set minimum value of two analog readings
yValHigh1 = max(yVal,yVal2);//set maximum value of two analog readings
xValLow = min(xValLow,xValLow1);//lowest x-axis value during loop
xValHigh = max(xValHigh,xValHigh1);//highest x-axis value during loop
yValLow = min(yValLow,yValLow1);//lowest y-axis value during loop
yValHigh = max(yValHigh,yValHigh1);//highest y-axis value during loop
}

//is there movement within threshold on either x or y axis
if ((xValHigh-xValLow)>5) || ((yValHigh-yValLow)>5){
movement = TRUE;
}
else {
movement = FALSE;
}

//if there is movement and the phone is off then turn it on
if (movement == TRUE && phoneOn == FALSE){
digitalWrite(switchPin,HIGH);
delay(3000);
digitalWrite(switchPin,LOW);
phoneOn=TRUE;
}

//if there is no movement and phone is on then turn it off
if (movement == FALSE && phoneOn == TRUE){
digitalWrite(switchPin,HIGH);
delay(3000);
digitalWrite(switchPin,LOW);
phoneOn=FALSE;
}

Overall this is a good effort - lots of comments, meaningful names, logical flow, little code duplication.

There is, however, a tendency when documenting code to document the “what” rather than the “why” - this can lead to comments and code getting out of sync (e.g. the logic is changed, but not the corresponding comment). Also, such comments can be superfluous. In the last block of your code, you essentially state ‘if no movement and phone is on’ twice, once in a comment and once in code. If you’ve chosen meaningful variable and constant names (and you have), there is no need to duplicate the code in a comment. If something is so confusing as to need a comment, consider simplifying it.

Documenting the why, not the what, avoids these problems. In the example, something like ‘phone should be turned off when no movement, to save power’ (or whatever motivation) means the developer knows why the code is shutting off the phone. Code should be simple enough to be understood by a reasonably skilled practitioner of the language & concepts (unless it is intended as purely a teaching aid, of course). You can therefore also remove the cluttering ‘set X to min of Y and Z’-style comments.

I’m not a fan of ‘x = foo; x2 = foo2’ variable names. If you have a need for more than one of the same thing, consider an array, or more meaningful variable names. Either of these is fine:

int x_readings[2];
int first_x_reading, second_x_reading;

While the difference between the latter and ‘int reading, reading2’ is subtle, the more verbose form is less likely to lead to confusion since it avoids ‘throwaway variable name-itis’, as well as the thought in the mind of a reader of “why X, X2 and not X1, X2?” or “is X2 twice as big as X?”.

I would also create functions for turning on/ off the phone. This avoids code duplication (if there is ever a need to alter the phone’s state from another place in the code, you already have a way to do it), as well as making it more readable. Additionally, you can now easily change the mechanism by which the phone gets turned on or off in a single place, without the calling code needing to be aware of how this is achieved - a principle called abstraction.

A good effort though, and remember my comments are just opinion. I hope you continue to expand your coding horizons!

Thanks for the input simond. This started out as a challenge and has turned into a hobbie. Your points are well taken and I thank you for them. I am in the middle of writing my second sketch right now and I plan on implementing your suggestions. While my sketches are not that complicated, I hope that others will read mem's and your suggestions to me and take a little away from it. After all, helping each other is what the message board is all about.

Thanks again.

Sorry for resurrecting an old thread but I would like to get ahold of your final code on this project if you don't mind.