4
\$\begingroup\$

I've recently started going through the Head First C book. I'd like to get up to speed in terms of writing good, idiomatic, modern C. I have read the first few chapters of the Modern C book too but found Head First to be a better introduction to the language.

The code below is for the first, bigger, arduino assignment. It's a pretty straight forward assignment, an LED should be used to indicate the soil moisture of a plant. I've gone beyond the base assignment by using a 3-color LED where pin 11 should be high for green, 12 high for red and both high for orange.

This creates a bit of a challenge in creating a nice, consistent api which abstracts those details. I wanted to use pointers and pointer arithmentic in the solution just to get some exposure to them. I could probably re-write using arrays and avoid the need for 0 last elements although I would still need to deal with pointer decay.

Any pointers :-) on what could be changed to turn this into a good, idiomatic, modern solution? Regardless of pointer use or not.


int const RED[] = { 12, 0 };
int const GREEN[] = { 11, 0 };
int const ORANGE[] = { 11, 12, 0 };
int const DEFAULT_DURATION = 1000;
int const DEFAULT_COUNT = 1;

void blink(int color[]);
void blink(int color[], int duration, int count);
void blinkSequence(int* color[]);
void blinkSequence(int* color[], int duration, int count);
void on(int color[]);
void off(int color[]);

void setup() {
  // initialize LED Pins as output
  Serial.begin(9600);
  pinMode(11, OUTPUT);
  pinMode(12, OUTPUT);
  pinMode(13, OUTPUT);
  digitalWrite(11, LOW);
  digitalWrite(12, LOW);
  digitalWrite(13, LOW);
}

void loop() {
  int m = analogRead(0);
  int interval = 2000;
  Serial.print("Moisture is: ");
  Serial.println(m);
  if (m <= 580) {
    //Really Dry!: 580-
    int* seq[] = { RED, ORANGE, 0 };
    blinkSequence(seq, 150, 2);
    interval = 0;
  } else if (m <= 700) {
    //Needs water: 700 - 581
    blink(RED, 500);
    interval = 500;
  } else if (m <= 740) {
    //Could be wetter: 740 - 701
    blink(ORANGE, 4000);   
    interval = 50;
  } else if (m <= 870) {
    //Happy range: 870 - 741
    blink(GREEN, 5000);
    interval = 50;
  } else {
    //Overwatered: 870+
    int* seq[] = { GREEN, ORANGE, 0 };
    blinkSequence(seq, 200, 5);
    interval = 3000;
  }

  delay(interval);
}

void blinkSequence(int* colors[], int duration, int count) {
  for (int i = count; i > 0; i--) {
    int** inner = colors;
    for(;*inner;inner++) {
      blink(*inner, duration);
    }
  }
}

void blink(int color[], int duration) {
  on(color);
  delay(duration);
  off(color);
}

void blinkSequence(int* colors[]) {
  for(;*colors;colors++) {
    blink(*colors);
  }
}

void blink(int color[]) {
  on(color);
  delay(DEFAULT_DURATION);
  off(color);
}

void on(int color[]) {
  for(;*color;color++) {
    digitalWrite(*color, HIGH);
    //Serial.print("HIGH:");
    //Serial.println(*color);
  }
}

void off(int color[]) {
  for(;*color;color++) {
    digitalWrite(*color, LOW);
    //Serial.print("LOW:");
    //Serial.println(*color);
  }
}
\$\endgroup\$
  • \$\begingroup\$ Can you add the header files you are including? Where is the definition or declaration of Serial? \$\endgroup\$ – pacmaninbw Jul 18 at 16:40
  • \$\begingroup\$ @pacmaninbw, I expect that the arduino ide adds those in. Serial, digitalWrite, analogRead, pinMode etc. are standard arduino methods. \$\endgroup\$ – CamW Jul 18 at 18:43
2
\$\begingroup\$

You wrote several for loops without any spaces. Please don't do that, as it makes the code harder to read.

Instead of

  for(;*colors;colors++) {
    blink(*colors);
  }

you might prefer

  while (*colors) {
    blink(*colors++);
  }

Moving loop() to the bottom of the file would save you some forward declarations.

Your Serial.print() statements might benefit from the addition of a debug flag, so there's no need to comment them.

Things like counts and durations won't go negative. I can't see the signatures for delay() and digitalWrite(), but it seems like some of your ints might want to be uint.

\$\endgroup\$
  • \$\begingroup\$ Thanks, all good points. \$\endgroup\$ – CamW Jul 20 at 8:50
2
\$\begingroup\$

First this is probably C++ rather than C, the C programming language does not support function overloading (blink as an example).

Magic Numbers

In programming a Magic Number is a numeric constant in code that doesn't identify what is is. Magic numbers make the code harder to understand, debug and maintain. When magic numbers are used as a basis for arrays it means that the code may need to be edited in multiple places rather than just in one location.

Example:

int *colors[10];

for (c = 0; c < 10; c++)
{
    do_something_with_color(colors[c]);
}

This would be easier to modify and maintain if a symbolic constant was defined.

int const MAX_COLORS = 10;
int *colors[MAX_COLORS];

for (c = 0; c < MAX_COLORS; c++)
{
    do_something_with_color(colors[c]);
}

There are a large number of numeric constants in the program, in very few cases they have been defined using int const SYMBOLIC_NAME = value. It might be better if all the numeric constants were defined in this manner. Some examples of these numeric constants are 700, 581, 500, 2000, 4000, etc.

Are 500, 2000 and 4000 milliseconds?

Comments
The comments

    //Really Dry!: 580-
    //Needs water: 700 - 581
    //Could be wetter: 740 - 701

aren't very meaningful in this context. If you want to create a block comment that describes what the LED is being used for and then base the symbolic name of the constants on this block comment it might be more helpful.

\$\endgroup\$
  • \$\begingroup\$ Thanks, all good points. I see here that it seems like arduino is sort of a mix and customization of C and C++. arduino.stackexchange.com/a/817 \$\endgroup\$ – CamW Jul 20 at 8:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.