Simplifying Code - 8x8 LED Gear Shift Indicator

EDIT: Thanks to everyone here I've successfully re-factored the code to be far more flexible and friendly! See the latest version here if you are interested:

Hi there!

Long time lurker, first time poster here - thanks for all the resources, they've been incredibly helpful already.

I have some code (below) that along with some Hall Effect Sensors, a magnet, and an Arduino Uno (rev3?) serves as a Gear Shift Indicator for my dad's car - which is displayed on an 8x8 dot matrix LED.

Thanks to everyone here it works great as-is, but I'm looking to make the code more efficient and readable before I add new features - I have an absolute mess of IF/ELSE statements that was a nightmare to wrap my head around and I'd like to fix that before moving the section to its own function.

I'm aware of arrays, and that multiple variables ending in numbers often indicates that an array combined with a loop could likely make it simpler but I'm having trouble conceptualising it and translating what I currently have.
I was hoping someone might be able to assist or point me towards a relevant resource (I have searched, but I'm unable to find someone doing what I'm trying to achieve).

Any help would be greatly appreciated!

Code is below, and attached is a simple wiring diagram minus the LED.

#include <MD_Parola.h>
#include <MD_MAX72xx.h>
#include <SPI.h>

#define MAX_DEVICES 1
#define CLK_PIN 13
#define DATA_PIN 11
#define CS_PIN 10

// constants won't change. They're used here to set pin numbers:
const int hallOne = A0;     // the number of the hall effect sensor pin
const int hallTwo = A1;
const int hallThree = A2;
const int hallFour = A3;
const int hallFive = A4;
const int hallSix = A5;
// variables will change:
int hallOneState = 0;          // variables for reading the hall sensors status
int hallTwoState = 0;
int hallThreeState = 0;
int hallFourState = 0;
int hallFiveState = 0;
int hallSixState = 0;
char previousDisplay = "";

MD_Parola P = MD_Parola(CS_PIN, MAX_DEVICES);

void setup() {
  // initialize the hall effect sensor pins as an input:
  pinMode(hallOne, INPUT);
  pinMode(hallTwo, INPUT);
  pinMode(hallThree, INPUT);
  pinMode(hallFour, INPUT);
  pinMode(hallFive, INPUT);
  pinMode(hallSix, INPUT);
  // start LCD display
  P.begin();
  P.setIntensity(4);
}

void loop() {
  // read the state of the hall effect sensors:
  hallOneState = digitalRead(hallOne);
  hallTwoState = digitalRead(hallTwo);
  hallThreeState = digitalRead(hallThree);
  hallFourState = digitalRead(hallFour);
  hallFiveState = digitalRead(hallFive);
  hallSixState = digitalRead(hallSix);


  if (hallOneState == LOW) {
    if (previousDisplay == 'P') {
      previousDisplay = 'R';
      P.displayText("R", PA_CENTER, 50, 1, PA_SCROLL_UP, PA_NO_EFFECT);
      while (!P.displayAnimate())
         ;
      } else if (previousDisplay == 'R') {
        P.displayText("R", PA_CENTER, 0, 0, PA_PRINT, PA_NO_EFFECT);
        P.displayAnimate();
      }
      else if (previousDisplay == 'N') {
        previousDisplay = 'R';
        P.displayText("R", PA_CENTER, 50, 1, PA_SCROLL_DOWN, PA_NO_EFFECT);
        while (!P.displayAnimate())
         ;
      } else {
        P.displayText("R", PA_CENTER, 0, 0, PA_PRINT, PA_NO_EFFECT);
        P.displayAnimate();
        previousDisplay = 'R';
      }
  }
  else if (hallTwoState == LOW) {
    if (previousDisplay == 'R') {
      previousDisplay = 'N';
      P.displayText("N", PA_CENTER, 50, 1, PA_SCROLL_UP, PA_NO_EFFECT);
      while (!P.displayAnimate())
         ;
      } else if (previousDisplay == 'N') {
        P.displayText("N", PA_CENTER, 0, 0, PA_PRINT, PA_NO_EFFECT);
        P.displayAnimate();
      }
      else if (previousDisplay == 'D') {
        previousDisplay = 'N';
        P.displayText("N", PA_CENTER, 50, 1, PA_SCROLL_DOWN, PA_NO_EFFECT);
        while (!P.displayAnimate())
         ;
      }
      else {
        P.displayText("N", PA_CENTER, 0, 0, PA_PRINT, PA_NO_EFFECT);
        P.displayAnimate();
        previousDisplay = 'N';
      }
  }
  else if (hallThreeState == LOW) {
    if (previousDisplay == 'N') {
      previousDisplay = 'D';
      P.displayText("D", PA_CENTER, 50, 1, PA_SCROLL_UP, PA_NO_EFFECT);
      while (!P.displayAnimate())
         ;
      } else if (previousDisplay == 'D') {
        P.displayText("D", PA_CENTER, 0, 0, PA_PRINT, PA_NO_EFFECT);
        P.displayAnimate();
      }
      else if (previousDisplay == '3') {
        previousDisplay = 'D';
        P.displayText("D", PA_CENTER, 50, 1, PA_SCROLL_DOWN, PA_NO_EFFECT);
        while (!P.displayAnimate())
         ;
      }
      else {
        P.displayText("D", PA_CENTER, 0, 0, PA_PRINT, PA_NO_EFFECT);
        P.displayAnimate();
        previousDisplay = 'D';
      }
  }
  else if (hallFourState == LOW) {
    if (previousDisplay == 'D') {
      previousDisplay = '3';
      P.displayText("3", PA_CENTER, 50, 1, PA_SCROLL_UP, PA_NO_EFFECT);
      while (!P.displayAnimate())
         ;
      } else if (previousDisplay == '3') {
        P.displayText("3", PA_CENTER, 0, 0, PA_PRINT, PA_NO_EFFECT);
        P.displayAnimate();
      }
      else if (previousDisplay == '2') {
        previousDisplay = '3';
        P.displayText("3", PA_CENTER, 50, 1, PA_SCROLL_DOWN, PA_NO_EFFECT);
        while (!P.displayAnimate())
         ;
      }
      else {
        P.displayText("3", PA_CENTER, 0, 0, PA_PRINT, PA_NO_EFFECT);
        P.displayAnimate();
        previousDisplay = '3';
      }
  }
  else if (hallFiveState == LOW) {
    if (previousDisplay == '3') {
      previousDisplay = '2';
      P.displayText("2", PA_CENTER, 50, 1, PA_SCROLL_UP, PA_NO_EFFECT);
      while (!P.displayAnimate())
         ;
      } else if (previousDisplay == '2') {
        P.displayText("2", PA_CENTER, 0, 0, PA_PRINT, PA_NO_EFFECT);
        P.displayAnimate();
      }
      else if (previousDisplay == '1') {
        previousDisplay = '2';
        P.displayText("2", PA_CENTER, 50, 1, PA_SCROLL_DOWN, PA_NO_EFFECT);
        while (!P.displayAnimate())
         ;
      }
      else {
        P.displayText("2", PA_CENTER, 0, 0, PA_PRINT, PA_NO_EFFECT);
        P.displayAnimate();
        previousDisplay = '2';
      }
  }
  else if (hallSixState == LOW) {
    if (previousDisplay == '2') {
      previousDisplay = '1';
      P.displayText("1", PA_CENTER, 50, 1, PA_SCROLL_UP, PA_NO_EFFECT);
      while (!P.displayAnimate())
         ;
      } else if (previousDisplay == '1') {
        P.displayText("1", PA_CENTER, 0, 0, PA_PRINT, PA_NO_EFFECT);
        P.displayAnimate();
      }
      else {
        P.displayText("1", PA_CENTER, 0, 0, PA_PRINT, PA_NO_EFFECT);
        P.displayAnimate();
        previousDisplay = '1';
      }
  }
  else {
    if (previousDisplay == 'R') {
      previousDisplay = 'P';
      P.displayText("P", PA_CENTER, 50, 1, PA_SCROLL_DOWN, PA_NO_EFFECT);
      while (!P.displayAnimate())
         ;
      } else {
        previousDisplay = 'P';
        P.displayText("P", PA_CENTER, 0, 0, PA_PRINT, PA_NO_EFFECT);
        P.displayAnimate();
        previousDisplay = 'P';
      }
  }
}

I suggest that you make your code data driven. You can use an array of structs. Each struct will specify a hall sensor pin, a prior state and a new state.

All your code has to do then is loop through the array. For each element, if the prior state (gear) there matches the gear you're in, read the hall sensor indicated. If it's low, set the gear display to the new state and break out of the loop.

“ I'm aware of arrays, and that multiple variables ending in numbers often indicates that an array combined with a loop could likely make it simpler ”

Bingo. Take some time off from your project, google some tutorial or video that matches your level of knowledge and learning style and get the basics of arrays into you toolkit.

If you do that, array-izing this code will be a nice test of what you know. When you have trouble with that, we here.

a7

oh, also, I haven’t googled an 8x8 LCD display, do you have a part number or link?

a7

const int hall[6] = {A0, A1, A2, A3, A4, A5};
int hallState[6];
char displayGear[6] = {'R', 'N', 'D', '3', '2', '1'};  // got this sequence from your if statements assigning 
                                                                 // assigning them to previousDisplay (which is not in the
                                                                // right spot btw.

in setup()

for (uint8_t i = 0; i <6; i++) {
  pinMode(hall[i], INPUT);
}

and in loop

for (uint8_t i = 0; i <6; i++) {
  hallstate[i] = digitalRead(hall[i]);
}

This is the general idea. I think you can apply the principle to most of your if statements. You can compare previousDisplay to displayGear[current - 1] or displayGear[current + 1], but be sure to prevent reading beyond the size of the array.
If you need more explanation on array you should look at the IDE examples or read a C++ book about it.

alto777:
oh, also, I haven’t googled an 8x8 LCD display, do you have a part number or link?

a7

Apologies but there was a grammar error - it's LED not LCD, like this one here:

https://www.jaycar.com.au/arduino-compatible-8-x-8-led-dot-matrix-module/p/XC4499

wildbill:
I suggest that you make your code data driven. You can use an array of structs. Each struct will specify a hall sensor pin, a prior state and a new state.

All your code has to do then is loop through the array. For each element, if the prior state (gear) there matches the gear you're in, read the hall sensor indicated. If it's low, set the gear display to the new state and break out of the loop.

I have some difficulty with the specifics of what you've written, but I understand the concept you're putting forward and once I do some more studying of arrays I'm sure I can work it out.
Thanks for the idea! I was struggling in my head with how to reformat it but you explained it well.

alto777:
“ I'm aware of arrays, and that multiple variables ending in numbers often indicates that an array combined with a loop could likely make it simpler ”

Bingo. Take some time off from your project, google some tutorial or video that matches your level of knowledge and learning style and get the basics of arrays into you toolkit.

If you do that, array-izing this code will be a nice test of what you know. When you have trouble with that, we here.

a7

I appreciate what you're trying to do, but I've already spent hours and hours doing that and posted here specifically so someone could give me even just a basic, relevant example that I can deconstruct and study.
I understand the basics of arrays but despite all the videos and tutorials I was obviously struggling to make the mental/logical leap myself from the basic examples in them to what I'm trying to achieve.
Now that I have the examples above I'm pretty confident I can work the rest out now.

“ Apologies but there was a grammar error - it’s LED not LCD, like this one here:”

Damn. I was so hoping there was such an item! And just skimming your code looked kinda like you really were talking to an LCD, as I had first assumed you must have meant LED…

Libraries, can’t live with them, can’t live without them.

a7

“ Now that I have the examples above I'm pretty confident I can work the rest out now.”

Nice. Everything is hard until it is easy! I understand that one good example that is more or less in the area of your project can be very helpful.

I learn quite a bit from taking things apart also, helps to have something to. Take apart.

My older brother will never forgive me nor let me forget how much I learned by thoroughly disassembling his toy train set… putting things back together was something I got better at eventually.

a7

Deva_Rishi:

const int hall[6] = {A0, A1, A2, A3, A4, A5};

int hallState[6];
char displayGear[6] = {‘R’, ‘N’, ‘D’, ‘3’, ‘2’, ‘1’};  // got this sequence from your if statements assigning
                                                                // assigning them to previousDisplay (which is not in the
                                                               // right spot btw.



in setup()


for (uint8_t i = 0; i <6; i++) {
 pinMode(hall[i], INPUT);
}



and in loop


for (uint8_t i = 0; i <6; i++) {
 hallstate[i] = digitalRead(hall[i]);
}



This is the general idea. I think you can apply the principle to most of your if statements. You can compare previousDisplay to displayGear[current - 1] or displayGear[current + 1], but be sure to prevent reading beyond the size of the array.
If you need more explanation on array you should look at the IDE examples or read a C++ book about it.

This is exactly what I needed!
Could you please explain a bit more about:

// assigning them to previousDisplay (which is not in the
                                                                // right spot btw.

I’m not sure what you mean, thanks for your post!

alto777:
“ Now that I have the examples above I’m pretty confident I can work the rest out now.”

Nice. Everything is hard until it is easy! I understand that one good example that is more or less in the area of your project can be very helpful.

I learn quite a bit from taking things apart also, helps to have something to. Take apart.

My older brother will never forgive me nor let me forget how much I learned by thoroughly disassembling his toy train set… putting things back together was something I got better at eventually.

a7

Hahaha that’s exactly it, just needed something to deconstruct so I can have that “eureka” moment!

if (hallOneState == LOW) {
    if (previousDisplay == 'P') {
      previousDisplay = 'R';  // <-- this statement is now here
      P.displayText("R", PA_CENTER, 50, 1, PA_SCROLL_UP, PA_NO_EFFECT);
      while (!P.displayAnimate())
         ;
      } else if (previousDisplay == 'R') {
        P.displayText("R", PA_CENTER, 0, 0, PA_PRINT, PA_NO_EFFECT);
        P.displayAnimate();
      }
      else if (previousDisplay == 'N') {
        previousDisplay = 'R';   // <-- and here
        P.displayText("R", PA_CENTER, 50, 1, PA_SCROLL_DOWN, PA_NO_EFFECT);
        while (!P.displayAnimate())
         ;
      } else {
        P.displayText("R", PA_CENTER, 0, 0, PA_PRINT, PA_NO_EFFECT);
        P.displayAnimate();
        previousDisplay = 'R';   // <--- and here
      }
     // --> previousDisplay = 'R';  // if you put it here, the other 3 statements can be removed
  }

Oh i had not noticed that there are actually 7 possible positions (i’d overlooked ‘P’) , so that would actually make the sequencechar displayGear[7] = {'P' , 'R', 'N', 'D', '3', '2', '1'}; Which is a little tricky because now the indices of the arrays don’t line up perfectly.
Would you should probably do instead of assigning values to an array is check the digital pin and assign a value (0 - 6) to a single variable like this

uint8_t gear = 6;
while ((gear >0) && (digitalRead(hall[gear - 1]))) { // note the gear - 1
                        // if the first part of an if statement is false anything beyond the '&&' is not compared 
                       //or executed, you could even write digitalRead(hall[--gear])
  gear--;
}

at the end of the little loop ‘gear’ should line up with the displayGear array.

I see what you mean about the unnecessary previousDisplay now, thanks for that catch!

Deva_Rishi:
Oh i had not noticed that there are actually 7 possible positions (i'd overlooked 'P') , so that would actually make the sequence

char displayGear[7] = {'P' , 'R', 'N', 'D', '3', '2', '1'};

Which is a little tricky because now the indices of the arrays don't line up perfectly.
Would you should probably do instead of assigning values to an array is check the digital pin and assign a value (0 - 6) to a single variable like this

uint8_t gear = 6;

while ((gear >0) && (digitalRead(hall[gear - 1]))) { // note the gear - 1
                       // if the first part of an if statement is false anything beyond the '&&' is not compared
                      //or executed, you could even write digitalRead(hall[--gear])
 gear--;
}



at the end of the little loop 'gear' should line up with the displayGear[] array.

I think I can avoid that entire issue by having the default char be 'P' and display the same any time all states are low?

Previously I would do something along the lines of:

if (hallOneState == LOW && hallTwoState == LOW && hallThreeState == LOW && hallFourState == LOW && hallFiveState == LOW && hallSixState == LOW){
    P.displayText("P", PA_CENTER, 0, 0, PA_PRINT, PA_NO_EFFECT);
        P.displayAnimate();
  }

at the start of the loop()?

I think I can avoid that entire issue by having the default char be 'P' and display the same any time all states are low?

Maybe... But if there are actually 7 possible states (or gears) then your reading of the pins should have 7 possible values as an outcome. Then you can start processing the result.
If the 'gear' value ranges from 0 to 6, you can use the result, not only to display the correct character, but also to control the animation (or not)
if instead of

char previousDisplay = "";  // this is an incorrect statement btw a char is in between ' '

you use an integer

int8_t previousGear = 0;

You can calculate the difference between the last and the current state, and animate according to the result.

int8_t change = previousGear - gear;
if (change == -1) P.displayText(displayGear[gear], PA_CENTER, 50, 1, PA_SCROLL_UP, PA_NO_EFFECT);
else if (change == 1) P.displayText(displayGear[gear], PA_CENTER, 50, 1, PA_SCROLL_DOWN, PA_NO_EFFECT); 
else P.displayText(displayGear[gear], PA_CENTER, 50, 1, PA_PRINT, PA_NO_EFFECT);

Unfortunately Deva_Rishi, I didn’t see your reply until just now! I probably could’ve saved myself some trouble if I’d checked sooner!

I went away and rewrote my code using the information from the previous posts (thank you all!) and think (hope?) I have a better version now.
It seems to have followed the same idea you responded with - albeit with a different implementation that I hope makes sense.

This is what I have now, could anyone confirm it will behave the way I intend it to? I’m unable to physically test with the hardware just now.
I get the feeling I can still clean up the IF statements in displayGear(), as some of the conditions are unnecessary…

#include <MD_Parola.h>
#include <MD_MAX72xx.h>
#include <SPI.h>
#include <CircularBuffer.h>

#define MAX_DEVICES 1
#define CLK_PIN   13
#define DATA_PIN  11
#define CS_PIN    10

// constants won't change:
const int numHall = 6; // set this to match the number of hall sensors
const int hall[] = {3, 4, 5, 6, 7, 8}; // set to match wiring of sensors to their pins
const char gearChars[8] = {'P', 'R', 'N', 'D', '3', '2', '1'};
MD_Parola P = MD_Parola(CS_PIN, MAX_DEVICES);

// variables will change:
int hallState[7];
char currentGear;
int hallTotal = 0;
int prevGear;
int curGear;

CircularBuffer<char, 4> previousGear; // use a buffer to store last 4 gear positions
//buffer will be used to detect 'sequence' such as 1-2-1-2, which can then be acted upon

// initialize the hall effect sensor pins as inputs:
void hallSetup() {
  for (int i = 0; i < numHall; i++) {
    pinMode(hall[i], INPUT);
  }
}

// setup LED display
void displaySetup() {
  P.begin(); // initialise display
  P.setIntensity(4); //set display intensity/brightness (0-15)
  P.displayClear();
  P.displayText("Startup Text", PA_CENTER, 100, 0, PA_SCROLL_LEFT, PA_SCROLL_LEFT); // display message on startup
  P.displayAnimate();
  if (P.displayAnimate()) { // If finished displaying message, reset and clear display
    P.displayReset();
    P.displayClear();
  }
}

// loop through all sensors once and record to array - includes 'fake' sensor as first value to represent 'P'
void hallRead() {
  hallState[0] = 0;
  for (int i = 1; i < numHall + 1; i++) {
    hallState[i] = digitalRead(hall[i - 1]);
    if (hallState[i] == HIGH ) {
      hallTotal = hallTotal + 1;
    }
  }
  if (hallTotal > 0) {
    hallState[0] = 1;
  } 
}

// translates reading to relevant character by checking for non-zero value
char getGear() {
  hallRead();
  for (int i = 0; i < numHall + 1; i++) {
    if (hallState[i] == HIGH) {

      return gearChars[i];
    }
  }
}

// displays current gear on LED, checks if animations should be used depending on previous (if any) gear value
void displayGear(char x) {
  if (previousGear.isEmpty() || x == previousGear.last()) { // if no previous value or same as previous, simply print
    P.displayText(x, PA_CENTER, 0, 0, PA_PRINT, PA_NO_EFFECT);
    P.displayAnimate();
  }
  else if (previousGear.size() > 0 && x != previousGear.last()) { // if prev gear value exists AND is not the same as previous value
    for (int i = 0; i < 7; i++) { // loop through possible characters
      if (x == gearChars[i]) {
        curGear = i; // assign numeric value to current gear based on location in array
      }
      if (previousGear.last() == gearChars[i]) {
        prevGear = i; // assign numeric value to previous gear based on location in array
      }
    }
    // if the previous gear is situated to the left of current gear (in char array) then scroll up
    if (prevGear < curGear) {
      P.displayText(x, PA_CENTER, 50, 1, PA_SCROLL_UP, PA_NO_EFFECT);
      while (!P.displayAnimate())
        ;
      // if the previous gear is situated to the right of current gear (in char array) then scroll down
    } else if (prevGear > curGear) {
      P.displayText(x, PA_CENTER, 50, 1, PA_SCROLL_DOWN, PA_NO_EFFECT);
      while (!P.displayAnimate())
        ;
    }
  }
  previousGear.push(x); // stores current gear in buffer at the end, overwriting old data if necessary
}

void setup() {
  hallSetup();
  displaySetup();
}

void loop() {
  currentGear = getGear();
  displayGear(currentGear);
}

That last suggestion is good, but remember that displayGear needs to be an array of c-strings, not single characters, to work with Parola animations. I would also recommend that you define a constant for the number of gears and use that consistently everywhere instead of the magic number 6 or 7. This is good programming practice and will make it clear what the loops are doing. Changing to add or delete and gears in future will also be a lot simpler.

const uint8_t NUM_GEARS = 7;

char displayGear[NUM_GEARS][2] = {"P" , "R", "N", "D", "3", "2", "1"};

marco_c:
That last suggestion is good, but remember that displayGear needs to be an array of c-strings, not single characters, to work with Parola animations. I would also recommend that you define a constant for the number of gears and use that consistently everywhere instead of the magic number 6 or 7. This is good programming practice and will make it clear what the loops are doing. Changing to add or delete and gears in furture will also be a lot simpler.

const uint8_t NUM_GEARS = 7;

char displayGear[NUM_GEARS][2] = {"P" , "R", "N", "D", "3", "2", "1"};

I see what you mean about the constant, thanks!
I also understand the need for the characters to be c-strings if the Parola requires it (I've heard strings can cause issues with dynamic memory allocation on 'smaller' devices which is why I used chars - do c-strings suffer from the same?).
With chars alone, the documentation advises using an extra char during initialisation for the terminating character - is this now unnecessary if we're using c-strings?

Finally, why is [2] used during initialisation?

char displayGear[NUM_GEARS][2] = {"P" , "R", "N", "D", "3", "2", "1"};

It simply reserves 2 bytes for the string (the character and the terminating '\0'). the memory would be laid out as P, \0, R, \0, N, \0, etc. It declares and array of NUM_GEARS of 2 byte character arrays.

An equivalent would be

char *displayGear[NUM_GEARS] = { "P", "R", ...

In this case each element is displayGear is a pointer to the string but you can use it in the same way as the previous declaration. The memory is implicitly allocated and the pointers stored in the array. I don't think, in this case, you are guaranteed that the strings are stored one after the other, but that is not relevant for this application.

Changing to add or delete and gears in furture will also be a lot simpler.

On the display yes, adding gears to the car is not going to get any easier though.

for (int i = 0; i < 7; i++) { // loop through possible characters

This part of the code is the bit that can be simplified of course. If you refer to the gears by number instead of by character and only refer to them by character hen you need it.

remember that displayGear needs to be an array of c-strings, not single characters, to work with Parola animations

I didn’t know that of course, but why not simply create a local 2 byte c-string

char forParole[2];
forParole[0] = displayGear[gear];

assign the gear we want to that, and then pass that on to Parole animation. Either way is fine with me of course.