4
\$\begingroup\$

I wired up a Gear Shift Indicator for my dad's car to display what gear it is currently in, and showing changes with animations (sliding up and down as appropriate, with a few bonus ones for fun at car shows).

Security is not really a concern due to its localised nature, but I'd like to release the code for others to use in their own projects and so I'd like the code to be bullet-proof before doing so. I have little to no experience with best coding practices etc, other than what I've picked up along the way and would love some input from others who know better than I do.

My main areas of concern (in order of priority) are as follows:

  1. Reliability

    • The application is not critical to the vehicle's function; at worst the gear indicator doesn't show, but I'd like to make sure any weird edge cases won't break the code.
      • For example, I learned that defaulting to a known state (in this case, the 'Park' position) upon boot/start is a good practice and could assist in troubleshooting, but I haven't really used any exception catching and I'm not even sure if it would be necessary.
      • I added a debugging function which prints raw inputs from the hall effect sensors, as well as buffer contents, to the serial output to check that everything is working correctly. Is the code I've written for this purpose appropriate? I made it dependant on a global variable rather than any kind of input as the Arduino has limited processing capacity, which leads into the next topic nicely...
  2. Performance

    • Memory usage and the code's running speed are very important, as the Arduino Uno I used is very limited in these aspects.
      • Some code is commented out (for example, code allowing LED brightness and animation speeds to be controlled by potentiometers) as it is not being used for my particular use-case, though this will be explained in the documentation. Am I correct in thinking that this helps free up memory and keeps the code performant as it is not running unnecessary checks (i.e. as opposed to checking a variable like in the debug function)? Or am I better off having multiple versions of the code with the different functions included or excluded as necessary?
      • Is my code as efficient as it could be? In places I used nested IF statements rather than things like SWITCH as I read online that they are often more efficient than the alternatives. Is this true?
  3. Best Coding Practices

    • As I mentioned previously, I have no experience with best practices in general, though some I have encountered were not applicable due to the nature of the Arduino platform where a priority is often given to efficiency and performance. Is there anything I can do to better adhere to general guidelines, or to make the code more readable?
      • Possible exceptions to these may be the style of commenting and line lengths - most of the commenting was formatted this way so that I could generate documentation using Doxygen, or to make the code super readable for people who may be unfamiliar with code in general. Is there anything I could have done better, or any practices I have overlooked that would assist with this end-goal?

I'd appreciate any input you may have on my code, anything I can do to make it better will no doubt help someone down the line who may want to use this project.

Code is as follows:

/**
 * @file
 * @author Ryan Jolliffe ([email protected])
 * @brief Use hall effect sensors to determine and display vehicle gear selection
 * on an 8x8 LED display.
 * @version v0.7
 * @copyright Copyright (c) 2021
 */

/** @mainpage Introduction and Installation
 *
 * @section intro_sec Introduction
 *
 * This is the documentation for GearShift6_8x8.ino - named as such due
 * to it being code that handles Gear Shifting using 6 (by default) hall
 * effect sensors which determines the current gear position then displays
 * the result on an 8x8 LED display matrix with relevant (and not so
 * relevant) animations. 
 *
 * @subsection note_sub Please Note
 * This code was originally developed for my Dad's scratch-built Locost 7.
 * As such, whilst this code was designed with flexibility and customisation
 * in mind, it of course comes with a few caveats. For example:
 * - Whilst the gear/sensor numbers and values can be adjusted,
 * it is still assumed that there are only 2 directions for gear changes - Up and Down.
 *     + Other configurations will still work, but the animations rely on
 *     this structure and as such will still slide up or down only.
 *     This is on my TODO list for future changes!
 * - To enable potentiometer-driven values for scrolling speed and display brightness,
 * uncomment the appropriate lines of code and set relevant pins in the settings section.
 *     + These values are set only once at startup to reduce the number of analogReads,
 *     which would otherwise slow down the speed with which the program updates.
 * 
 *
 * @section install_sec Software Installation
 *
 * @subsection step1 Step 1: Install Arduino IDE and relevant libraries.
 * 3 additional libraries are used in this code and need to be installed
 * via Tools > Manage Libraries in the Arduino IDE. Searching for their
 * names should allow you to find them with ease.
 * Library          | Used For:
 * -------------    | -------------
 * MD_MAX72xx.h     | Interacting with display(s) 
 * MD_Parola.h      | Text and Sprite Animation 
 * CircularBuffer.h | Tracking gear change history 
 *
 * @subsection step2 Step 2: Adjust variables.
 * Most variables are able to be changed to suit your particular setup.
 * Take care to read the documentation for each, as some must meet
 * particular conditions to work as expected (in particular; pin numbers).
 * If using potentiometers for animation speed and/or display brightness
 * make sure to uncomment the appropriate lines as explained in their
 * comments.
 *
 * @subsection step3 Step 3: Verify changes.
 * Use the Verify button (looks like a check/tick underneath the "File" menu heading)
 * to verify that any changes have been implemented correctly. The IDE will
 * warn of any errors it encounters when compiling the code.
 * For additional information, ensure the "Show verbose output" checkboxes
 * are marked in the IDE Preferences (File > Preferences, Settings Tab).
 * - This step is obviously not necessary if no changes are made to the code.
 *
 * @subsection step4 Step 4: Connect Arduino and upload.
 * If connecting via USB, the appropriate port and board should be automatically
 * selected by the IDE, but can be confirmed or manually adjusted under
 * the Tools menu heading (see "Board" and "Port" subsections).
 *
 * @subsection step5 Step 5: Wire up Arduino and test!
 * Currently, no instructions are available from us but this will be changed
 * in the future. Ensure the pins used match what is written in the code's
 * variables.
 *
 * @section anim_sec Animations
 *
 * As the [MD_Parola library](https://github.com/MajicDesigns/MD_Parola) is used,
 * animations using the [relevant sprites](https://arduinoplusplus.wordpress.com/2018/04/19/parola-a-to-z-sprite-text-effects/)
 * can be used if desired and a few select ones are already implemented.
 */


/* Necessary libraries, ensure they are
  installed in Arduino IDE before uploading */
#include <SPI.h>                                                                // should be included in default Arduino IDE
#include <MD_Parola.h>
#include <MD_MAX72xx.h>
#include <CircularBuffer.h>

/** Change if using PAROLA_HW or other LED hardware,
* incorrect setting can cause orientation issues */
#define HARDWARE_TYPE MD_MAX72XX::GENERIC_HW

/* Settings: */

// GEAR SETTINGS
/** How many gears are used - must match the number of gear characters in GearChars array */
const byte    NUM_GEARS                        = 7;
/** Used for loop counting etc when starting with 0 */
const int8_t  NUM_LOOPS                        = NUM_GEARS - 1;
/** Set number of stored previous gear states - 4 used here used to detect 'sequence' such as 1-2-1-2, which can then be acted upon */
const uint8_t BUFFER_SIZE                      = 4;
/** Layout here from left to right should match gear order on vehicle from top to bottom/start to finish */
const char    GearChars[NUM_GEARS]             = {'P', 'R', 'N', 'D', '3', '2', '1'};
// DISPLAY AND SENSOR SETTINGS
/** Hall Effect sensor pins in the same order as GearChars array - 'Park' position is assumed to not have a sensor and so the first pin represents "R" */
const uint8_t Hall[NUM_LOOPS]                  = {      3,   4,   5,   6,   7};   
/** Array for storing relevant LED pins; (DATA_PIN, CLK_PIN, CS_PIN, MAX_DEVICES) */
const uint8_t LED[]                            = { 11,  13,  10,  1};
/** Remove comment ('//') to enable potentiometer-driven scroll speed */
//const uint8_t POT_PIN_SPEED                    = 8;
/** Remove comment ('//') to enable potentiometer-driven LED display brightness */
//const uint8_t POT_PIN_SPEED                    = 9;

// CUSTOMISATION
/** Text scrolled upon boot */
const char    StartupText[]                    = {"Startup Text Goes Here"};
/** Sequence of gears necessary to display and loop animations (i.e. D-N-D-N), must be same length as BUFFER_SIZE */
const char    ANIM_SEQUENCE[BUFFER_SIZE]       = {'D', 'N', 'D', 'N'};
/** Sequence of gears necessary to display and loop the startup text (i.e. R-N-R-N), must be same length as BUFFER_SIZE */
const char    SCROLLTEXT_SEQUENCE[BUFFER_SIZE] = {'R', 'N', 'R', 'N'};
/** Speed that StartupText and animations are scrolled, number is milliseconds between frame updates */
const uint8_t SCROLL_SPEED                     = 75;
/** Set brightness of LEDs (using range 0-15) - comment out ('//') when using potentiometer-derived value */
const byte    BRIGHTNESS                       = 4;

// DEBUGGING
/** Set to 1 to enable debugging via Serial (baud) */
const byte    DEBUG_MODE                       = 0;
/** Number of readings in debug mode, recommended to match buffer size or larger */
const uint8_t DEBUG_READS                      = BUFFER_SIZE;
/** Delay between debug readings in milliseconds (3 seconds by default) */
const int     DEBUG_DELAY                      = 3000;
/** Set baud rate here for Serial communication */
const int     BAUD_SPEED                       = 9600;

// HARDWARE SPI
/** Creates display instance using given settings (HARDWARE_TYPE, CS_PIN, MAX_DEVICES) */
MD_Parola Parola = MD_Parola(HARDWARE_TYPE, LED[2], LED[3]);
// SOFTWARE SPI
//MD_Parola P = MD_Parola(HARDWARE_TYPE, DATA_PIN, CLK_PIN, CS_PIN, MAX_DEVICES);
    

/* Sprite definitions - stored in RAM/PROGMEM to save memory */

const uint8_t F_PMAN = 6;
const uint8_t W_PMAN = 8;
const uint8_t PROGMEM pacman[F_PMAN * W_PMAN] = {
  0x00, 0x81, 0xc3, 0xe7, 0xff, 0x7e, 0x7e, 0x3c,
  0x00, 0x42, 0xe7, 0xe7, 0xff, 0xff, 0x7e, 0x3c,
  0x24, 0x66, 0xe7, 0xff, 0xff, 0xff, 0x7e, 0x3c,
  0x3c, 0x7e, 0xff, 0xff, 0xff, 0xff, 0x7e, 0x3c,
  0x24, 0x66, 0xe7, 0xff, 0xff, 0xff, 0x7e, 0x3c,
  0x00, 0x42, 0xe7, 0xe7, 0xff, 0xff, 0x7e, 0x3c,
};
/**< Sprite definition for gobbling pacman animation */

const uint8_t F_PMANGHOST = 6;
const uint8_t W_PMANGHOST = 18;
static const uint8_t PROGMEM pacmanghost[F_PMANGHOST * W_PMANGHOST] = {
  0x00, 0x81, 0xc3, 0xe7, 0xff, 0x7e, 0x7e, 0x3c, 0x00, 0x00, 0x00, 0xfe, 0x7b, 0xf3, 0x7f, 0xfb, 0x73, 0xfe,
  0x00, 0x42, 0xe7, 0xe7, 0xff, 0xff, 0x7e, 0x3c, 0x00, 0x00, 0x00, 0xfe, 0x7b, 0xf3, 0x7f, 0xfb, 0x73, 0xfe,
  0x24, 0x66, 0xe7, 0xff, 0xff, 0xff, 0x7e, 0x3c, 0x00, 0x00, 0x00, 0xfe, 0x7b, 0xf3, 0x7f, 0xfb, 0x73, 0xfe,
  0x3c, 0x7e, 0xff, 0xff, 0xff, 0xff, 0x7e, 0x3c, 0x00, 0x00, 0x00, 0xfe, 0x73, 0xfb, 0x7f, 0xf3, 0x7b, 0xfe,
  0x24, 0x66, 0xe7, 0xff, 0xff, 0xff, 0x7e, 0x3c, 0x00, 0x00, 0x00, 0xfe, 0x73, 0xfb, 0x7f, 0xf3, 0x7b, 0xfe,
  0x00, 0x42, 0xe7, 0xe7, 0xff, 0xff, 0x7e, 0x3c, 0x00, 0x00, 0x00, 0xfe, 0x73, 0xfb, 0x7f, 0xf3, 0x7b, 0xfe,
};
/**< Sprite definition for ghost pursued by pacman */

const uint8_t F_SAILBOAT = 1;
const uint8_t W_SAILBOAT = 11;
const uint8_t PROGMEM sailboat[F_SAILBOAT * W_SAILBOAT] = {
  0x10, 0x30, 0x58, 0x94, 0x92, 0x9f, 0x92, 0x94, 0x98, 0x50, 0x30,
};
/**< Sprite definition for sail boat */

const uint8_t F_STEAMBOAT = 2;
const uint8_t W_STEAMBOAT = 11;
const uint8_t PROGMEM steamboat[F_STEAMBOAT * W_STEAMBOAT] = {
  0x10, 0x30, 0x50, 0x9c, 0x9e, 0x90, 0x91, 0x9c, 0x9d, 0x90, 0x71,
  0x10, 0x30, 0x50, 0x9c, 0x9c, 0x91, 0x90, 0x9d, 0x9e, 0x91, 0x70,
};
/**< Sprite definition for steam boat */

const uint8_t F_HEART = 5;
const uint8_t W_HEART = 9;
const uint8_t PROGMEM beatingheart[F_HEART * W_HEART] = {
  0x0e, 0x11, 0x21, 0x42, 0x84, 0x42, 0x21, 0x11, 0x0e,
  0x0e, 0x1f, 0x33, 0x66, 0xcc, 0x66, 0x33, 0x1f, 0x0e,
  0x0e, 0x1f, 0x3f, 0x7e, 0xfc, 0x7e, 0x3f, 0x1f, 0x0e,
  0x0e, 0x1f, 0x33, 0x66, 0xcc, 0x66, 0x33, 0x1f, 0x0e,
  0x0e, 0x11, 0x21, 0x42, 0x84, 0x42, 0x21, 0x11, 0x0e,
};
/**< Sprite definition for beating heart */

const uint8_t F_INVADER = 2;
const uint8_t W_INVADER = 10;
const uint8_t PROGMEM spaceinvader[F_INVADER * W_INVADER] = {
  0x0e, 0x98, 0x7d, 0x36, 0x3c, 0x3c, 0x36, 0x7d, 0x98, 0x0e,
  0x70, 0x18, 0x7d, 0xb6, 0x3c, 0x3c, 0xb6, 0x7d, 0x18, 0x70,
};
/**< Sprite definition for space invader */

const uint8_t F_FIRE = 2;
const uint8_t W_FIRE = 11;
const uint8_t PROGMEM fire[F_FIRE * W_FIRE] = {
  0x7e, 0xab, 0x54, 0x28, 0x52, 0x24, 0x40, 0x18, 0x04, 0x10, 0x08,
  0x7e, 0xd5, 0x2a, 0x14, 0x24, 0x0a, 0x30, 0x04, 0x28, 0x08, 0x10,
};
/**< Sprite definition for fire */

const uint8_t F_WALKER = 5;
const uint8_t W_WALKER = 7;
const uint8_t PROGMEM walker[F_WALKER * W_WALKER] = {
  0x00, 0x48, 0x77, 0x1f, 0x1c, 0x94, 0x68,
  0x00, 0x90, 0xee, 0x3e, 0x38, 0x28, 0xd0,
  0x00, 0x00, 0xae, 0xfe, 0x38, 0x28, 0x40,
  0x00, 0x00, 0x2e, 0xbe, 0xf8, 0x00, 0x00,
  0x00, 0x10, 0x6e, 0x3e, 0xb8, 0xe8, 0x00,
};
/**< Sprite definition for walking stick figure */

/* Struct used for storing/retrieving sprite settings. */
struct
{
  const uint8_t *data;
  uint8_t width;
  uint8_t frames;
}
sprite[] =
{
  { fire, W_FIRE, F_FIRE },
  { pacman, W_PMAN, F_PMAN },
  { walker, W_WALKER, F_WALKER },
  { beatingheart, W_HEART, F_HEART },
  { sailboat, W_SAILBOAT, F_SAILBOAT },
  { spaceinvader, W_INVADER, F_INVADER },
  { steamboat, W_STEAMBOAT, F_STEAMBOAT },
  { pacmanghost, W_PMANGHOST, F_PMANGHOST }
};

/* Variables that will change during runtime: */

/** An integer representing the current gear position
* (as given in the GearChars array). */
uint8_t currentGear;
/** A Circular Buffer (array of length BUFFER_SIZE)
* used to store the previous gear positions. */
CircularBuffer<byte, BUFFER_SIZE> previousGears;
/** Remove comment ('//') to enable potentiometer-driven scroll speed */
uint16_t scrollSpeed = SCROLL_SPEED;
uint8_t displayBrightness = BRIGHTNESS;

/**
 * @brief Initialises sensors and LED display.
 * 
 * Function that runs *once* when Arduino is first
 * booted; initialises sensors and LED display  
 * (brightness and scroll speed set only once during
 * initialization for efficiency).
 * Loads known state (Park position) then checks if 
 * DEBUG_MODE is enabled.
 * Also adds randomness to random() calls via
 * an analogue 'Pin 0' read.
 *
 */
void setup() {
  hallSetup();                                                                  // initialise sensors
  displaySetup();                                                               // initialise display
  currentGear = 0;                                                              // set current gear to 'Parked' position until first sensor read to establish known state
  previousGears.push(currentGear);                                              // push 'Park' {"P"} position to buffer also, which is translated to *char via GearChars[0]
  if (DEBUG_MODE == 1) {                                                        // check if DEBUG_MODE is enabled, and runs debugFunction() if TRUE
    Serial.begin(BAUD_SPEED);
    debugFunction();
  }
  randomSeed(analogRead(0));                                                    // take 'noisy' reading (i.e. hopefully random) as the seed for our random() calls; adds randomness
}

/**
 * @brief Main loop.
 * 
 * The main loop that runs the core components
 * repeatedly until power-off.
 *
 */
void loop() {
  currentGear = getGear();                                                      // read hall effect sensors and calculate current gear position
  displayGear(currentGear);                                                     // display the current gear, with appropriate animation if different from previous gear
  checkHistory();                                                               // checks gear history for defined sequences and calls relevant functions
}

/** @brief Initialize the hall effect sensor pins as inputs. */
void hallSetup() {
  for (int8_t i = 0; i < NUM_LOOPS; i++) {
    pinMode(Hall[i], INPUT);
  }
}

/** @brief Setup LED display*/
void displaySetup() {
  Parola.begin();                                                               // initialise display
  Parola.setIntensity(displayBrightness);                                       // set display intensity/brightness
  Parola.displayClear();
  Parola.displayScroll(StartupText, PA_LEFT, PA_SCROLL_LEFT, scrollSpeed);      // display message on startup
  while (!Parola.displayAnimate())                                              // play animation once until complete
    ;
  Parola.displayReset();
  Parola.displayClear();
  //Parola.setIntensity(static_cast<int>(analogRead(POT_PIN_BRIGHTNESS) / 68.2)); // update display brightness to match potentiometer reading - uncomment to enable
  //scrollSpeed = analogRead(POT_PIN_SPEED);                                      // update scrollSpeed to match potentiometer reading - uncomment to enable
}

/**
 * @brief Loop through sensors until LOW reading detected
 * 
 * @return gear
 * A numeric value representing the current gear,
 * matching the gear's position in the GearChars array.
 */
int8_t getGear() {
  int8_t gear = NUM_LOOPS;
  while ((gear) && (digitalRead(Hall[gear - 1]))) {
    gear--;
  }
  return gear;
}

/**
 * @brief Displays current gear on LED, and checks if animations
 * should be used depending on previous gear value.
 *
 * @param gearValue A numeric value representing the current gear,
 * matching the gear's position in the GearChars array.
 */
void displayGear(int8_t gearValue) {
  char curGearChar[2] = {GearChars[gearValue]};                                 // convert gearValue to c-string character for display purposes by pulling from null terminated array using pointers
  if (gearValue == previousGears.last()) {                                      // if current gear is same as previous, simply print
    Parola.displayText(curGearChar, PA_CENTER, 0, 0, PA_PRINT, PA_NO_EFFECT);   // set display settings
    Parola.displayAnimate();                                                    // display appropriate character
  }
  else if ((previousGears.last() < gearValue)) {                                // if the previous gear is situated to the left of current gear (in char array) then scroll down
    Parola.displayText(
      curGearChar, PA_CENTER, scrollSpeed, 1, PA_SCROLL_DOWN, PA_NO_EFFECT      // set scrolling text settings
    );
    while (!Parola.displayAnimate())                                            // play once animation until complete
      ;
    previousGears.push(gearValue);                                              // push current gear to buffer as it is different
  } else {                                                                      // if the previous gear is not situated left (i.e. is to the right of current gear in char array) then scroll up
    Parola.displayText(
      curGearChar, PA_CENTER, scrollSpeed, 1, PA_SCROLL_UP, PA_NO_EFFECT
    );
    while (!Parola.displayAnimate())
      ;
    previousGears.push(gearValue);                                              // push current gear to buffer as it is different
  }
}

/** @brief Checks for given sequence of gear changes using
* buffer functionality and calls other functions as required. */
void checkHistory() {
  if (previousGears.isFull()) {
    char gearHistory[BUFFER_SIZE];                                              // create new char array from history for comparison
    for (int8_t i = 0; i < BUFFER_SIZE; i++) {                                  // loop to populate array with char equivalents
      gearHistory[i] = GearChars[previousGears[i]];
    }
    if (checkArrays(gearHistory, ANIM_SEQUENCE, BUFFER_SIZE)) {                 // compares the two arrays; if buffer history matches ANIM_SEQUENCE, then display animation
      displayAnimation(random(ARRAY_SIZE(sprite) - 1));                         // selects and displays random animation from struct array
    }
    else if (checkArrays(gearHistory, SCROLLTEXT_SEQUENCE, BUFFER_SIZE)) {
      scrollSpeed = analogRead(POT_PIN_SPEED);                                  // update scrollSpeed to match potentiometer reading
      Parola.displayClear();
      Parola.displayScroll(StartupText, PA_LEFT, PA_SCROLL_LEFT, scrollSpeed);  // scroll StartupText
      while (!Parola.displayAnimate())                                          // play animation once until complete
        ;
      Parola.displayReset();
      Parola.displayClear();
    }
  }
}

/** @brief Compares 2 char arrays and returns boolean result. */
boolean checkArrays(char arrayA[], char arrayB[], long numItems) {
  boolean matchCheck = true;
  long i = 0;
  while (i < numItems && matchCheck) {
    matchCheck = (arrayA[i] == arrayB[i]);
    i++;
  }
  return matchCheck;
}

/** @brief Displays an animation based on the previously
* selected sprite definition from checkHistory function. */
void displayAnimation(byte selection) {
  char curGearChar[2] = {GearChars[previousGears.last()]};
  Parola.displayReset();
  Parola.displayClear();
  Parola.setSpriteData(
    sprite[selection].data, sprite[selection].width, sprite[selection].frames,  // entry sprite
    sprite[selection].data, sprite[selection].width, sprite[selection].frames   // exit sprite
  );
  Parola.displayText(
    curGearChar, PA_CENTER, scrollSpeed, 1, PA_SPRITE, PA_SPRITE                // set animation settings
  );
  while (!Parola.displayAnimate())                                              // play animation once until complete
    ;
}

/** @brief Functions useful for debugging.
*
* Writes DEBUG_READS lots of readings (default:4) from
* all hall sensors to Serial - with a delay to allow changing
* gear - then fills & prints buffer; for debugging purposes. */
void debugFunction() {
  String buf = "";
  String bufChars = "";
  for (int8_t i = 0; i < DEBUG_READS; i++) {
    delay(DEBUG_DELAY);                                                         // wait to allow gear changing/hall sensor/magnet position changes
    for (int8_t x = 0; x < NUM_LOOPS; x++) {                                    // loop through all sensors and print values to Serial
      Serial.println(
        String(x) + ") Digital Reading: " + String(digitalRead(Hall[x])) +
        " | Analogue Reading: " + String(analogRead(Hall[x]))
      );
    }
    previousGears.push(random(NUM_LOOPS));                                      // push pseudorandom GearChar values to buffer
    buf = buf + previousGears.last();                                           // add current gear in numeric form to a string for printing to Serial
    bufChars = bufChars + GearChars[previousGears.last()];                      // add current gear in char form to a string for printing to Serial
  }
  Serial.println("Buffer contents: " + buf + bufChars);                         // ...print buffer contents, to Serial...
  while (true);                                                                 // puts arduino into an infinite loop, reboot to start again
}
\$\endgroup\$
4
  • \$\begingroup\$ Does the compiler support constexpr? Generally, I would suggest changing the global const values to constexpr. \$\endgroup\$
    – JDługosz
    Commented Oct 4, 2021 at 16:11
  • 1
    \$\begingroup\$ const uint8_t Hall is missing }. \$\endgroup\$
    – vnp
    Commented Oct 4, 2021 at 16:41
  • \$\begingroup\$ @JDługosz I believe so; I trust that you know better than I and will update the code accordingly but it looks like I have some more research to do on why! Thanks :-) \$\endgroup\$ Commented Oct 16, 2021 at 10:22
  • \$\begingroup\$ @Phrontistes re why: "constexpr is the new static const". \$\endgroup\$
    – JDługosz
    Commented Oct 18, 2021 at 13:58

1 Answer 1

2
\$\begingroup\$

About your concerns

The application is not critical to the vehicle's function

Indeed, the vehicle will function fine without it, but consider that a person is driving in the vehicle, and making decisions based on what your gear shift indicator shows. What if your indicator shows it's in reverse, but the gear actually is in first or drive?

learned that defaulting to a known state (in this case, the 'Park' position) upon boot/start is a good practice and could assist in troubleshooting

There's two different things here. Making sure all variables are initialized to something before they are used is good practice. However, if that means your indicator might show "Park" at startup, even if the actual gear is in another state, then that is bad. I would reserve some value to represent the state "Unknown", and initialize the current gear to that at startup, and then avoid displaying anything while it is in that state.

I added a debugging function which prints raw inputs from the hall effect sensors, as well as buffer contents, to the serial output to check that everything is working correctly. Is the code I've written for this purpose appropriate?

Personally, I would avoid pretty-printing the state, just print the values you are interested. Also, your debug function is called in setup() and then just goes into an infinite loop. So you are not debugging the system in action. I would rather make it print the state every so often during normal operation.

Some code is commented out [...] Am I correct in thinking that this helps free up memory and keeps the code performant as it is not running unnecessary checks?

Usually you have much more flash memory than RAM. If the whole program with nothing commented out fits in flash, then I would leave everything uncommented. Also consider not having to enable functionality by changing a constant in the source code, but read the state of some pin (either just cone in setup() or often in loop()) in a variable instead. Then physically, you can add a microswitch or jumper to your hardware and enable debug functionality for example without needing to reflash the device.

Is my code as efficient as it could be? In places I used nested if statements rather than things like switch as I read online that they are often more efficient than the alternatives. Is this true?

The compiler will do a good job optimizing the code for you, and should produce the same assembly whether you use if or switch to do the same thing.

Is there anything I can do to better adhere to general guidelines, or to make the code more readable?

If you are already aware of the general guidelines and are trying to adhere to them, that's great. I think you only get better at this by doing more programming projects, and by trying to keep up with the best practices (like by reading other code review questions and answers on this site, watching C and C++ conference talks online, and so on). I'll just comment on specific things I see in your code that could be done better.

Naming things

Try to ensure names of variables and functions are clear and concise. While most naming choices are fine, there are some things that can be improved:

  • NUM_LOOPS -> NUM_SENSORS: while it is used in loops, it represents the number of Hall sensors you have.
  • Hall -> HallPins: since it is an array, use the plural to indicate it holds multiple values. Each value is the pin number of Hall sensor.
  • LED -> LEDPins
  • sprite -> sprites
  • debugFunction -> logSensorReadings: you use this function for debugging, but the name of the function should represent what it actually does.

Avoid fluff

It looks cool to print a scrolling startup message, but is it useful? The longer this takes, the longer the user has to wait until they can see the actual gear state. Also, it looks like the startup message or an animation sequence can show up every time a certain pattern of gear shifts is detected. It would be very annoying to me to suddenly see this when I just want it to show the current gear.

Keep it simple and to the point. It might be boring, but that usually is a good thing for a device you want to rely on.

Getting the current gear

Sensors are not perfect. Even if you get a noise-free signal out of all six Hall sensors when the gear is in a well-defined position, there will almost certainly be times when you are shifting the gear that it is in an in-between position where either none of the Hall sensor signals are high, or when at least two of them are high. Furthermore, while the gear is near a position where a Hall sensors would switch from low to high, then due to noise and vibrations (remember that you are probably in a driving car) the signal might temporarily rapidly toggle between states. Ideally, your device handles those situations. For example, you could:

  • Read the sensors multiple times until you have at least N consecutive results with the same value (the higher the value of N, the less sensitive to noise, but of course you don't want it too large).
  • Don't stop the loop when you encounter the first sensor which is high, read all of them and only consider the result valid if at most one sensor is high.

Note that by not having a Hall sensors for the Park position, it's hard to distinguish between the gear being in Park or being in an inbetween position where none of the Hall signals are high. So I would only consider the gear to be in Park when none of the Hall sensors are high for a time longer than you'd reasonably take to shift the gear from one position to another.

\$\endgroup\$
4
  • \$\begingroup\$ Thank you for your detailed input! They're all great points that I'll definitely be correcting. I understand your concerns about "Fluff"; for me the animations are essential as they were requested (for car shows etc), but the reasons you mention are totally valid so I think I'll adjust their implementation and triggers. I do have one question: "Read the sensors multiple times... but of course you don't want it too large." I understand this would change depending on the hardware, but are there any rules/guides that would advise how many loops are inefficient? Just trial and error? \$\endgroup\$ Commented Oct 16, 2021 at 10:13
  • 2
    \$\begingroup\$ Ah, if it's for car shows, then of course go all out :) If it's used for both normal driving and car shows, perhaps add a switch so you can select whether to show animations or not. As for the sensor reading: if you can hook up an oscilloscope and look at the signal of each Hall sensor when shifting gears, you can get a feel of how much noise there is. Or just write some code for the Arduino to sample the signals frequently and log any state changes. If I'd had to just guess, I'd sample every 1 ms and require the signal to be stable for 10 ms (so 10 samples). \$\endgroup\$
    – G. Sliepen
    Commented Oct 16, 2021 at 12:54
  • 1
    \$\begingroup\$ Yeah I had considered having some way to switch between the modes early on, but was constrained by my specific use case, in that 1) the Arduino needs to be sealed away from the elements (open top car) and so it isn't easily accessible, and 2) adding physical switches just for that purpose would ruin the aesthetic of the dash, and so I was trying to keep it within the current hardware set-up and provide a software-only solution. Hence the animation trigger sequences. Thanks for the advice on sensor reading, no oscilloscope so I'll have to experiment and see! \$\endgroup\$ Commented Oct 16, 2021 at 15:03
  • 1
    \$\begingroup\$ Perhaps instead of a switch, you can arrange for the startup animation to be interrupted (stopped) when a shift event happens? That way, you get the cool part, but it is overridden when practicality matters. \$\endgroup\$ Commented Nov 26, 2021 at 9:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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