Code Cleanliness And Organization

Projects Programming
1

Long time reader, first time poster.

I bought my Arduino not too long ago and aside from a few leds and motors my quadrotor is my first project.
My issue is not in interfacing with the hardware. iI already got the code set up enough to get the serial readout for each of my sensors, which are an Ultrasonic Distance Sensor (HC-SR04), 3 Axis Gyro (L3G4200D) and a 3 Axis Accelerometer(MMA7361).

The code is made up of 3 existing codes for each module and my own written serial display code.

My questions are simple:

Does my code look proper? Structured correctly?
Is there any unnecessary elements aside from the comment tags?
How can i Graph this data?

And the code:

#define trigPin 12
#define echoPin 13

int sleepPin = 4;
int xpin = A0;
int ypin = A1;
int zpin = A2;


#include <Wire.h>

#define CTRL_REG1 0x20
#define CTRL_REG2 0x21
#define CTRL_REG3 0x22
#define CTRL_REG4 0x23
#define CTRL_REG5 0x24

int L3G4200D_Address = 105;

int x;
int y;
int z;

void setup()
{
  Wire.begin();
  Serial.begin(9600);

  Serial.println("Starting Up Sensors");  
  setupL3G4200D(250); // 150, 500, 2000
  
  pinMode(trigPin, OUTPUT);
  pinMode(echoPin, INPUT);
  
  pinMode(sleepPin,OUTPUT);
  digitalWrite(sleepPin, HIGH);
  
  pinMode(xpin, INPUT);
  digitalWrite(xpin, HIGH);
  
  pinMode(ypin, INPUT);
  digitalWrite(ypin, HIGH);
  
  pinMode(zpin, INPUT);
  digitalWrite(zpin, HIGH);
  
  delay(1000); 
 }

void loop(){  
  
  int duration, distance, xmath, xdisp, ymath, ydisp, zmath, zdisp;
  
  digitalWrite(trigPin, HIGH);
  delayMicroseconds(100);
  digitalWrite(trigPin, LOW);
  duration = pulseIn(echoPin, HIGH);
  distance = (duration/2)/29.10;
  
  xmath = analogRead(xpin);
  xdisp = (xmath-658);
  ymath = analogRead(ypin);
  ydisp = (ymath-664);
  zmath = analogRead(zpin);
  zdisp = (zmath-729);
  
  getGyroValues();
  
  delay(500); //Delay for readability
Serial.print("A-X : ");
Serial.print(xdisp); 
Serial.print(" | A-Y : ");
Serial.print(ydisp);
Serial.print(" | A-Z : ");
Serial.print(zdisp);
  Serial.print(" || G-X:");
  Serial.print(x);
  Serial.print(" | G-Y:");
  Serial.print(y);
  Serial.print(" | G-Z:");
  Serial.print(z);
Serial.print(" | Altitude : ");

if (distance <= 5.08){Serial.println("TOO LOW!");}
  else {
    Serial.print(distance/2.54);
    Serial.println(" in");}
}

 void getGyroValues(){

  byte xMSB = readRegister(L3G4200D_Address, 0x29);
  byte xLSB = readRegister(L3G4200D_Address, 0x28);
  x = ((xMSB << 8) | xLSB);

  byte yMSB = readRegister(L3G4200D_Address, 0x2B);
  byte yLSB = readRegister(L3G4200D_Address, 0x2A);
  y = ((yMSB << 8) | yLSB);

  byte zMSB = readRegister(L3G4200D_Address, 0x2D);
  byte zLSB = readRegister(L3G4200D_Address, 0x2C);
  z = ((zMSB << 8) | zLSB);
}

int setupL3G4200D(int scale){

  writeRegister(L3G4200D_Address, CTRL_REG1, 0b00001111);

  writeRegister(L3G4200D_Address, CTRL_REG2, 0b00000000);

  writeRegister(L3G4200D_Address, CTRL_REG3, 0b00001000);

  if(scale == 250){
    writeRegister(L3G4200D_Address, CTRL_REG4, 0b00000000);
  }else if(scale == 500){
    writeRegister(L3G4200D_Address, CTRL_REG4, 0b00010000);
  }else{
    writeRegister(L3G4200D_Address, CTRL_REG4, 0b00110000);
  }

  writeRegister(L3G4200D_Address, CTRL_REG5, 0b00000000);
}

void writeRegister(int deviceAddress, byte address, byte val) {
    Wire.beginTransmission(deviceAddress); 
    Wire.write(address);
    Wire.write(val);
    Wire.endTransmission();
}

int readRegister(int deviceAddress, byte address){

    int v;
    Wire.beginTransmission(deviceAddress);
    Wire.write(address); // register to read
    Wire.endTransmission();

    Wire.requestFrom(deviceAddress, 1);

    while(!Wire.available()) {
    }

    v = Wire.read();
    return v;
}

[code/]

Thanks in advance to any help that is given.

Des
2

oh it looks fine to me, its structured and blocked where you can easily identfy each section and its tidy

I dont dig the

  }else if(yah){
    blah
  }else{

style but thats a personal choice, and as long as its easy to follow and read I doubt anyone is going to have issue with what you presented. I only ever get strict on style when working with groups of people, but I will do ever so slightly different things so I know if I wrote it vs someone else.

maybe one of the high C preist around here will nitpick on the extra tabs in your block of serial prints, otherwise you could slap all that text into tables and do a loop, but its already written and working, ship it.

3

I dont dig the "code ref" style

What would be another option

4

desolation:

I dont dig the "code ref" style

What would be another option

if (blah) {
  blah
}
else if (blah){
  blah
}
else {
  blah
}

it really doesn't matter; it doesn't look terrible and people use it.

You could replace the Serial.prints with this: http://arduino.cc/forum/index.php/topic,120440.0.html

5

or

if(blah)
{
  stuff
}

its totally personal opinion, I call that "noobie expanded" but its dead easy to tell where brackets go when you start battle axe hunting a bug

for example:

int counter = 0;
void setup(){};
void loop()
{
  if(counter < 256)
  {
    for(int i = 0; i < 12; ++i)
    {
      if(i % 2)
      {
        Serial.print("even");
      }
    }
  }
}

find and remove the for loop while leaving the second if, much harder if its in the
}if{blah
}else{

style (IMO)

6 
  xdisp = (xmath-658);

The brackets aren't necessary here and make it look like you left out a function call by mistake.

#define trigPin 12
#define echoPin 13

int sleepPin = 4;
int xpin = A0;
int ypin = A1;
int zpin = A2;

xpin and ypin etc. don't change, right? So make them const. And make up your mind if you are going to use defines or ints.

eg. better would be:

const byte trigPin = 12;
const byte echoPin = 13;

const byte sleepPin = 4;
const byte xpin = A0;
const byte ypin = A1;
const byte zpin = A2;

  Wire.requestFrom(deviceAddress, 1);

    while(!Wire.available()) {
    }

    v = Wire.read();

Wire.requestFrom doesn't return until the requested data is there, or something is wrong. So the call to Wire.available() is redundant. In fact, it will make it hang if the device isn't there. Better would be:

   if (Wire.requestFrom(deviceAddress, 1) != 1)
      {
      // handle error
      return -1;
      }

   // we must have data now
    v = Wire.read();
7

Osgeld:
or

if(blah)

{
 stuff
}




its totally personal opinion, I call that "noobie expanded" but its dead easy to tell where brackets go when you start battle axe hunting a bug

Mainly, it's about how easy it is to see and validate the control structures. This type of thing is completely opaque and requires you to count braces and parentheses to work out what's going on:

if (distance <= 5.08){Serial.println("TOO LOW!");}
  else {
    Serial.print(distance/2.54);
    Serial.println(" in");}
}

My advice is to put every { and } on a separate line with matching pairs indented by the same amount and the enclosed section indented one extra level, as Osgeld shows.

There are other ways to format code, including putting the opening { on the preceding line, which was very popular when we were programming with teletype terminals and space was at a premium. These days that is not an issue and code clarity and correctness is vastly more important.

8

My main concern is performance. I want my quadrotor to perform optimally, so iI guess I need to rephrase.

What, if anything do I need to do to make sure my code isn't unnecessarily large?

The serial print is just for the prototyping to make sure everything is working properly, in the end i will end up using this data for the autopilot and other autonomous features. The reason why i am concerned about the code, intensity, for the lack of a better term, is that i plan on using several more sensors and i don't want the ATMega to be strained and cause sensor lag. Am i worrying for nothing?

9

Ah, now the real requirement surfaces. Largeness isn't your concern it is efficiency, right?

I see three * analogRead in your loop plus some delays. I would be worried about those myself. Each analogRead takes 104 uS so you are up to 312 uS straight away. And then you throw in a 100 uS delay. If you want responsiveness you need to rewrite without delay, and do the analogRead asynchronously.

10

This type of thing is completely opaque and requires you to count braces and parentheses to work out what's going on

Which,ofcourse,ishardtodobecausetherearen'tnearenoughdamnedspacesinthatcode.

11

There are other ways to format code, including putting the opening { on the preceding line, which was very popular when we were programming with teletype terminals and space was at a premium. These days that is not an issue and code clarity and correctness is vastly more important.

I would believe this if I would not see so much CamelCase nowadays. If space is not at a premium and readability is important, so why is CamelCase so popular? INCaseIDidNotMentionItIHateCamelCaseBecauseItMakesVariableNamesSoMuchHarderToRead. Compare_this_with_the_underscore_style_which_appears_to_be_much_more_readable_but_takes_more_space.

12

Those are some rather extreme examples. Worst case I ever saw in an actual work environment was a function called:

AsicMonsterOverrideBurstSizeAndInterval(int, int);

I've personally never had any trouble reading CamelCase (even your unrealistic example), and the majority of variables/functions don't need to be more than 2-3 words long to be adequately described anyways.

13

I'm of the opposite opinion in that I find the underscore between words harder to read and adds "useless" horizontal space.

14

Worst case I ever saw in an actual work environment was a function called:

AsicMonsterOverrideBurstSizeAndInterval(int, int);

The worst I have seen so far was >200 characters (216 if I remember it right). The median of the lengths over the whole repository was (if I remember it right) ~60 characters. Yes, I complained about this but some things are to messed up to be fixed.

I'm of the opposite opinion in that I find the underscore between words harder to read and adds "useless" horizontal space.

IDon'tBuyThis.WhyIsStandardTextThenClutteredWithUselessSpaces?AreAllTheBookAuthorsWrong?OrDoesCodeHaveSomeMagicComponentThatMakesItEasierToReadIfSpacesAreOmmited?WhyDoesYourForumPostContainSpaces?IDoUnderstandThatCamelCaseIsMuchEasierToType.AndOfCourseShortIdentifiersAreAlwaysEasyToRead.ButForLongerIdentifiersIDoNotBelieveThisTheory.

I confess sometimes I use CamelCase as well. Especially if I code in contexts where all others do it that way. But as I said I do not buy it. In general I try to avoid it.

15

desolation:
My main concern is performance. I want my quadrotor to perform optimally, so iI guess I need to rephrase.

To be perfectly honest, I don't think long data names will make his quad rotor perform un-optimally. Nor will camel case or the lack of it.

16

Correct. In compiled languages name length is usually irrelevant for performance. And in interpreted language it does not matter to much either.

17

In addition to the other replies, there's a couple of points I want to add:

  1. You need to comment your code - for example, what the heck are each of those #defines for? I don't know; I suspect you'll forget, too, when you come back to the code in a few years (trust me, you'll come back).

  2. I also see waaaay too many "magic numbers"! What are "magic numbers"? Magic numbers are numbers scattered throughout code without explanation; it kinda relates to my first item, too. These numbers should be declared as #define or constants of some sort, and should have well thought out variable names, and/or should be commented as to what (and why) the numbers are for. Again - when you come back to the code in a few years, you'll be grateful you did so.

Trust me on both of these: For almost any code you write, you'll be supporting it until the end of time. If you aren't supporting it, /someone else will be/! That "someone else" might very well be a "future you" who has forgotten what the code does and why. If you don't keep any documentation on it (especially in the code!), you'll sit there wondering what the hell was going on when you wrote it, and likely will end up scrapping it, and re-doing it (even if the code was perfectly fine, logically, to begin with). Thereby, wasting not only your (or someone else's) time "now", but your (or their) time "then" as well.

Do all of us software developers a favor. Comment your code. Eliminate "magic numbers". We'll all be grateful.

/if I had a nickle for every time I've seen both of these abuses in my career...

18

Nick, you read my mind. Thank you for un-hijacking my thread. No offense to other posters, always nice to hear some internet "my belief is truth" wars, as long as there is a wee bit of education in there.

Now back on topic. I know the 304us is delay does not matter while reading the serial output at 500ms intervals but would that really make a difference for my automated functions on the quad?