Arduino controller to control the backlighting of a gear selector

Question

Background

Gear selector

The gear selector on a car allows the driver to select which state the transmission should be in.

In the example picture, one of the five letters (P, R, N, D & S) lights up to indicate which state the transmission is presently in.

The position of the rotary dial is sufficient to determine which state the gear selector should be in (and subsequently which letter should light up). With this analog input (coming from a pair of Hall sensors), an Arduino board should be able to figure out which LED should be lit.

Code at present

This is my first attempt at writing C code, so pardon any Perl dialect.

"U" represents an undefined state

#define NOFIELD 505L // Calibration constant
#define TOMILLIGAUSS 1953L // Unit conversion factor
#define P_led 13
#define R_led 12
#define N_led 11
#define D_led 10
#define S_led 9
#define U_led 8
#define numLED 5
struct interval
{
 int lower;
 int upper;
};
struct state
{
 char name;
 int led;
 interval gauss1;
 interval gauss2;
} ;
state shift_condition[] = {
 { 'P', P_led, { 74, 95 }, { -86, -40 } },
 { 'R', R_led, { 61, 92 }, { -20, 10 } },
 { 'N', N_led, { -30, 6 }, { 36, 90 } },
 { 'D', D_led, { -80, -53 }, { 2, 50 } },
 { 'S', S_led, { -25, 15 }, { -80, -25 } },
};
state prev_state;
long prev_gauss1;
long prev_gauss2;
bool gear_defined;
bool inRange( long value, struct interval range )
{
 return range.lower <= value && value <= range.upper;
}
void selectGear(int raw1, int raw2)
{
 long gauss1 = ( raw1 - NOFIELD ) * TOMILLIGAUSS / 1000;
 long gauss2 = ( raw2 - NOFIELD ) * TOMILLIGAUSS / 1000;
 // if no signal, no need to switch gear, bail out
 if ( prev_gauss1 == gauss1 && prev_gauss2 == gauss2 ) {
 return;
 }
 // update values for next evaluation
 prev_gauss1 = gauss1;
 prev_gauss2 = gauss2;
 // if new readings in existing state range,
 // no need to switch gear, bail out
 if ( gear_defined && inRange(gauss1, prev_state.gauss1) && inRange(gauss2,prev_state.gauss2) ) {
 return;
 }
 for( int i=0; i<numLED; i++) { // determine new state
 state curr_state = shift_condition[i];
 if ( inRange(gauss1, curr_state.gauss1) && inRange(gauss2, curr_state.gauss2) ) {
 Serial.print("Switching from ");
 Serial.print(prev_state.name);
 Serial.print(" to ");
 Serial.println(curr_state.name);
 digitalWrite(prev_state.led, LOW);
 digitalWrite(curr_state.led, HIGH);
 prev_state = curr_state;
 gear_defined = true;
 return;
 }
 }
 // no states matched, default to "undefined" state
 gear_defined = false;
 digitalWrite(prev_state.led, LOW);
 digitalWrite(U_led, HIGH);
}
void setup()
{
 Serial.begin(9600);
 pinMode(P_led, OUTPUT);
 pinMode(R_led, OUTPUT);
 pinMode(N_led, OUTPUT);
 pinMode(D_led, OUTPUT);
 pinMode(S_led, OUTPUT);
 pinMode(U_led, OUTPUT);
}
void loop()
{ 
 selectGear(analogRead(1),analogRead(2));
}

Questions/Concerns

• As this is my first attempt at writing C code, what constructs/conventions could I leverage to write more robust code? The "undefined" state in particular bothers me, as does the fact that there doesn't seem to be a built-in way to count the length of an array.
• Are there any gotchas pertaining to Arduino and typedefs? I tried to set a typedef on a struct and the code didn't compile.
• I used char name to act as a pseudo-hash key. Is there a smarter way to go about defining dictionaries or hashes with (or without?) structs in C?
• In the context of Arduino programming, how good is this implementation of a finite state machine? Are there any issues that may not be obvious in the course of this limited testing?
• How would the use of pointers benefit memory usage here?

The main purpose of posting this question is to learn better C (& Arduino) by doing.

• Grajdeanu Alex: Welcome to codereview! This is a nice first question and I hope you'll get the answer you're looking for
• Grajdeanu Alex: it'd also be a good idea to add the includes you've used in your code.
• Zaid: @Dex'ter there are no other explicit includes. If there are includes, they will be implicit to the Arduino Uno.

Answer 1

I see some things that may help you improve your code.

The Arduino language isn't C

The language used for the Arduino isn't quite C and isn't quite C++, so if your goal is "to learn C," you might want to be careful about the differences. Specifically, the Arduino's use of setup and loop is unique to it. Also, all of the "built-in" things such as digitalWrite and Serial are non-standard. You can still learn useful things by learning to program the Arduino, but it's important to remain aware of the differences.

Know the differences between C++ and C

As mentioned above, the Arduino language isn't quite C or C++, but in most ways, it's closer to C++ than C. One such difference that affects this code is the use of the struct interval. In C++ the name of a struct automatically defines a type. In C, it does not. So the state in plain C would need to be written like this:

struct state
{
 char name;
 int led;
 struct interval gauss1;
 struct interval gauss2;
};

Alternatively, one could use a typedef in the declaration of the interval structure like this:

typedef struct interval_s
{
 int lower;
 int upper;
} interval;

Eliminate global variables where practical

Having routines dependent on global variables makes it that much more difficult to understand the logic and introduces many opportunities for error. Eliminating global variables where practical is always a good idea, whether programming for desktop machines or for embedded systems. For global variables such as prev_gauss1 and prev_gauss2, consider wrapping them in objects to make it easier to see that they're related. Here again, this is a difference in the Arduino language where it is common to have global variables and plain C where it is rare.

Understand the fundamental types

The analogRead() call returns an int (which is 2 bytes on the Arduino), but the converted value of gauss1 and gauss2 is a long which is 4 bytes long on the Arduino. However, all of the numbers in the table are small enough to fit into an int which suggests that you probably don't really need a long there.

Minimize runtime conversions

It's not critical here, because normal drivers don't expect microsecond shifting, but in general, it's good to minimize the work that an embedded system must do at runtime. This tends to make the program more efficient and more responsive, as well as usually resulting in smaller code. In this case, the raw readings are taken from the sensors, converted using a simple linear equation and then compared to fixed values in the table. Instead, the table could store the corresponding raw values and the comparison could be made without conversion.

Make use of defined constants

The numLED constant is defined, correctly, but their connection to the array of states is not explicit. I'd suggest changing the declaration from this:

state shift_condition[] = {

To this:

state shift_condition[numLED] = {

Reconsider naming

The names gauss1 and gauss2 are not terrible, but it might be worth considering what the values really mean. Because they're actually sensor data, it might be better to name them for what they represent rather than the units.

Set states explicitly

The only time the U_led is written, it's set HIGH if no states are matched. If it ever gets to that point, there's no recovery even if it gets in range of one of the gears. Further, it's often good in embedded systems to set states explicitly. So when you set one LED, it's often good to explicitly unset the LEDs that are not supposed to be lit. The reason for this is that glitches can happen in the real world. For example, an energetic gamma particle can flip a bit in RAM and it's useful to have a recovery for such events.