Arduino project to indicate soil moisture using LEDs

Question

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);
  }
}

Answer 1

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.

Answer 2

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.