How does my code look? Can it be improved/optimised

Projects Programming
1

Hi guys,

My first post.
I have basic experience with a few programming languages writing pc programs but this is my first arduino project.
I've been been writing this code for a few weeks.
The board I'm using is a SparkFun Pro Micro 5V/16MHz.

I have used UnoArduSim heavily in the writing and testing of the code and it seems to give the same results as the Pro Micro on the breadboard.

My project is basically a fancy controller for an exhaust fan. 2 pull up momentary push buttons (pins 2 & 3) each with an indicator led (pins 4 & 5) and a relay to run the fan (pin 6).

Pressing the on button will turn on the on led and fan relay and will stay on indefinitely until the button is pressed again.
Pressing the timer button will blink the timer led and turn on the fan relay for a preset number of minutes then turn off.
You can freely switch between on and timer modes at any time.
Holding both buttons for at least 10 seconds will enter programming mode where the fan timer duration can be changed, holding buttons again will accept changes.
Programming mode will time out and cancel changes if no buttons are pressed for 60 seconds.

I intend to have it powered and running long term, years if possible.
Currently the code works well but I'm interested in getting your opinions on where it can be improved or optimised.

#include <EEPROM.h>
#define onbutton 2
#define timerbutton 3
#define onled 4
#define timerled 5
#define fanrelay 6
int onbuttonstate = HIGH;
int onbuttonpreviousstate = HIGH;
int timerbuttonstate = HIGH;
int timerbuttonpreviousstate = HIGH;
int onledstate = LOW;
int timerledstate = LOW;
int fanrelaystate = LOW;
unsigned long buttonholdstart = 0;
unsigned long buttonholdtime = 0;
unsigned long timertime = 0;
unsigned long timerprevioustime = 0;
unsigned long starttime = 0;
int timertempminutes = 1;
int timersetminutes = 1;
int flashcount = -5;
int mode = 0;     // 0=off 1=on 2=timer 3=programming

void setup()
{
  if ((EEPROM.read(0) == 0) || (EEPROM.read(0) > 30)) {
    EEPROM.write(0, 1);
  }
  timertempminutes = EEPROM.read(0);
  timersetminutes = EEPROM.read(0);
  pinMode(onbutton, INPUT_PULLUP);
  pinMode(timerbutton, INPUT_PULLUP);
  pinMode(onled, OUTPUT);
  pinMode(timerled, OUTPUT);
  pinMode(fanrelay, OUTPUT);
}
void loop()
{
  onbuttonstate = digitalRead(onbutton);
  timerbuttonstate = digitalRead(timerbutton);

  //hold both buttons detection
  if ((timerbuttonstate != timerbuttonpreviousstate && timerbuttonstate == LOW) || (onbuttonstate != onbuttonpreviousstate && onbuttonstate == LOW)) {
    buttonholdstart = millis();
  }
  if (timerbuttonstate == LOW && onbuttonstate == LOW) {
    buttonholdtime = millis() - buttonholdstart;
    if (buttonholdtime > 9995) {
      while(digitalRead(onbutton) == LOW || digitalRead(timerbutton) == LOW) {
        delay(10);
        digitalWrite(onled, HIGH);
        digitalWrite(timerled, HIGH);
      }
      if (mode == 3) {
        mode = 0;
        timersetminutes = timertempminutes;
        EEPROM.update(0, timertempminutes);
      } else {
        mode = 3;
        flashcount = -5;
        starttime = millis();
        timertempminutes = timersetminutes;
      }
      onbuttonstate = digitalRead(onbutton);
      timerbuttonstate = digitalRead(timerbutton);
      onbuttonpreviousstate = onbuttonstate;
      timerbuttonpreviousstate = timerbuttonstate;
    }
  }

  if (mode == 3) {

    //in programming mode
    fanrelaystate = LOW;
    timertime = millis() - starttime;
    if (onbuttonstate != onbuttonpreviousstate && onbuttonstate == HIGH) {
      if (timertempminutes < 30) {
        ++timertempminutes;
      }
      starttime = millis();
      flashcount = -5;
    } else if (timerbuttonstate != timerbuttonpreviousstate && timerbuttonstate == HIGH) {  
      if (timertempminutes > 1) {
        --timertempminutes;
      }
      starttime = millis();
      flashcount = -5;
    }
    if (flashcount < (timertempminutes+1)) {
      if (timertime - timerprevioustime >= 350) {
        if (timerledstate == LOW && flashcount > 0) {
          onledstate = HIGH;
          timerledstate = HIGH;
        } else {
          onledstate = LOW;
          timerledstate = LOW;
          ++flashcount;
        }
        timerprevioustime = timertime;
      }
    } else {
      flashcount = -5;
    }
    // auto exit programming mode
    if (timertime > 59995) {
      mode = 0;
      timertempminutes = timersetminutes;
    }

  } else {

    // in normal mode
    if (onbuttonstate != onbuttonpreviousstate && onbuttonstate == HIGH) {
      if (mode == 1) {
        mode = 0;
      } else {
        mode = 1;
      }
    } else if (timerbuttonstate != timerbuttonpreviousstate && timerbuttonstate == HIGH) {  
      if (mode == 2) {
        mode = 0;
      } else {
        mode = 2;
        starttime = millis();
        timerledstate = HIGH;
      }
    }
    if (mode == 0) {
      onledstate = LOW;
      timerledstate = LOW;
      fanrelaystate = LOW;
      timertime = 0;
      timerprevioustime = 0;
    } else if (mode == 1) {
      onledstate = HIGH;
      timerledstate = LOW;
      fanrelaystate = HIGH;
      timertime = 0;
      timerledstate = LOW;
      timerprevioustime = 0;
    } else if (mode == 2) {
      timertime = millis() - starttime;
      onledstate = LOW;
      fanrelaystate = HIGH;
      if (timertime - timerprevioustime >= 1000) {
        if (timerledstate == LOW) {
          timerledstate = HIGH;
        } else {
          timerledstate = LOW;
        }
        timerprevioustime = timertime;
      }
      if ((timertime) >= (timersetminutes * 59995)) {
        mode = 0;
      }
    }     
  }

  onbuttonpreviousstate = onbuttonstate;
  timerbuttonpreviousstate = timerbuttonstate;
  digitalWrite(onled, onledstate);
  digitalWrite(timerled, timerledstate);
  digitalWrite(fanrelay, fanrelaystate);
  delay(20);
}
2 
int onbuttonstate = HIGH;

Do you really need 16 bits to hold a 1 bit value? Arduino data types. Generally, use the smallest data type that will do the job to save precious SRAM.

3 
int mode = 0;     // 0=off 1=on 2=timer 3=programming
...
      if (mode == 2) {

You can improve readability and also have type safety, with an enum:

 typedef enum { OFF,
               ON,
               TIMER,
               PROGRAMMING
             } modeStates;
  // current line input state
  modeStates modeState = OFF;
...
      if (mode == TIMER) {

I guess I'm a purist, but do you really need this:

 delay(20);

?

What is this "magic number"? You should either document it, or if it is a calculated number, calculate it in code. Constant calculations don't result in extra memory use.

    if (timertime > 59995) {
4

Tips:

We prefer a 'const' int or 'const' byte instead of #define
The pin for the fan might use a relay, but that is not very interesting. Therefor "fanPin" is enough information.

const int onLedPin = 4;
const int timerLedPin = 5;
const int fanPin = 6;

Some Arduino users get nervous when 8 whole bits are wasted, but I use 'int' because it is the compiler-default and it is okay. See also the example code of digitalRead().

More comments please.
A header with the date, the used Arduino board, the version of the Arduino IDE, and if you include all sorts of libraries, then a list with links of all those library. In the code more explanation what it is.
Does this look okay, or can't you see the code anymore because of the comments ?

  // ----------------------------------------
  // Gather all the information
  // ----------------------------------------
  onbuttonstate = digitalRead(onbutton);
  timerbuttonstate = digitalRead(timerbutton);

  // ----------------------------------------
  // Process the information
  // ----------------------------------------
  //hold both buttons detection
  if ((timerbuttonstate != timerbuttonpreviousstate && timerbuttons ...

I think the comment blocks are nice. However, when I make an example sketch for others, then the first thing some do is immediate strip all those comment blocks away, because they want to see the code :confused:

More local variables.
The variables above setup() are global variables. When a variable is used locally, then it is better to make that variable local.
You must be very sure that you remove the global variables when you make them local. The compiler will not give an error message when you have two variables with the same name.

  int onbuttonstate = digitalRead(onbutton);
  int timerbuttonstate = digitalRead(timerbutton);

Your 'mode' is a variable that determines the state that the sketch is in. There is a common and nice way to use a 'state'. It is called a Finite State Machine. That is a fancy word for something simple: The Finite State Machine | Majenko Technologies.
You can see the difference with a normal sketch and the same thing with a Finite State Machine. The normal sketch is easier to read, so a Finite State Machine is for specific situations.
When your code looks like spaghetti and there is a 'state', then a Finite State Machine will untangle the spaghetti.
I'm sorry to say, but your code is too much entangled. To know what is going on, I have to keep track of the variables and read the code over and over again.
Good code has the different functionality in different blocks of code.

I have to talk about balloons.
Visualize your project in front of you and each section of the sketch is a balloon. There is a balloon with a timer with millis(), and a balloon for the switch and a balloon for the EEPROM. In the middle is a balloon that processes the information. Between the balloons are ropes and information travels along those ropes. If you can do that, then you can pull the functionality apart from each other and the code does not get entangled.
Programming is not typing code, programming is leaning back and think about what is going on.

Now I get serious :wink: Let's not talk about ropes but about data streams.
Normally a sketch has code that does things. There is an other way. See everything as data streams. A sensor creates a stream of data, but also a button creates a stream of data (pressed or released). There is also a datastream going to the fan (turn it on or off). When you read a button at the beginning of the loop(), then that is were the data stream injects its data into the sketch. Therefor it does not make sense when you read the buttons two times more further down the sketch.

5

Ok, lots to think about.

I have changed #define to const byte,
changed IO states from int to bool,
changed all int that do not exceed 0-255 to byte
and removed the delay at the end.

The Finite State Machine or Enum sounds interesting. Will have to look into it.

My original intention for the delay at the end was to force the loop to run less often thinking it would improve long term board life but what I know now I'm not sure it would help.

The 59995 magic number is for 60000 milliseconds. The simulator I'm using seems to over shoot by about 5 milliseconds. The timing isn't critical so using a nice round number probably makes more sense.

I hadn't considered an in code header. I will add one.
I do like comments. I wasn't sure whether they were helpful for you or not.

I intentionally made all the variables global because in the simulator any local variables flicker on the screen.
Is this because they are constantly getting created and destroyed?
Which variables do you recommend I make local?

I was having a problem where button presses were carrying over after entering or exiting programming mode. Adding another button read seemed to solve this. Using the Finite State Machine may fix this aswell.

I think I understand what you mean about the balloons. Do you mean like some of the alternative arduino interfaces which are very visual based? I agree that the code does look a bit messy I think as I find different ways to achieve the same result I can clean it up.

I was wondering about timing and millis() rollover but after reading http://www.gammon.com.au/millis it seems I am already using the subtraction method so I shouldn't have any timing issues, should I?

6

See if you can move some of the code in loop into separate functions, maybe two to start with - normal mode and programming mode.

As far as getting rid of globals, try and restrict every variable you use to the smallest scope possible. It makes debugging easier. Don't forget that you can make local variables static if you need to persist their values between function calls.

Finite state machines are powerful and useful. They're also curiously hard to comprehend to start with.

Enum on the other hand is easy to understand and will make your code more readable - you can do that right away.

The Arduino CPU runs at full speed all the time (unless you put it to sleep). There is no longevity benefit to be had by running loop less frequently.

7

The Finite State Machine or Enum sounds interesting. Will have to look into it.

You are already using a Finite State Machine, so you have no logic changes to make. The 'enum' just makes it easier to read, and as I mentioned, it can catch some errors.

The 59995 magic number is for 60000 milliseconds. The simulator I'm using seems to over shoot by about 5 milliseconds. The timing isn't critical so using a nice round number probably makes more sense.

Here is how I would do that:

const unsigned long MS_PER_SEC = 1000;
const unsigned long TIMER_MAX = 60 * MS_PER_SEC - 5; // 60 seconds minus 5ms
...
    // auto exit programming mode
    if (timertime > TIMER_MAX) {

This makes it more foolproof and you can adjust all your parameters at the top of the file, instead of finding them buried somewhere in the code.

8

nickthenack9:
I intentionally made all the variables global because in the simulator any local variables flicker on the screen.
Is this because they are constantly getting created and destroyed?
Which variables do you recommend I make local?

I prefer to make variables as local as possible. The compiler has more possibilities to optimize local variables.
If a variable is only used in a for-loop, then create it in the for-loop.
If a variable is only used in the Arduino loop() function, then create it in the loop() function.
I have been testing the UnoArduSim, that's not bad. The interface is from the 1990's but it works. It seems to run in linux with wine.
If you want to learn what the variables do and keep them global, that's fair enough !

nickthenack9:
I was having a problem where button presses were carrying over after entering or exiting programming mode.

The State Change Detection example sketch is to detect the moment that a button was pressed.
A Finite State Machine can do the same thing without that example. Sometimes it is easier for the code to have both.

nickthenack9:
I think I understand what you mean about the balloons. Do you mean like some of the alternative arduino interfaces which are very visual based?

No, sorry, I was not clear enough.
The most important part about programming is leaning back and not thinking about code.
If you can split a real-world-problem into separate parts according to its functionality and see the logic of it, that is where the programming starts.
You can visualize balloons, or write down blocks, or write down a truth table or a flow diagram or solve the problem in your dreams while you are sleeping :wink: Once you have that, then you can fill in each part with code.
Your code will become better if you impose on yourself not to think about code, but lean back and think about separate parts and about what is doing what.

Only if you have a known problem and there is a know solution for it, then you can start typing like a madman as some advanced programmers do :confused:

I selected the balloons, because then I could talk about ropes, and then about data streams, and then about writing clean code and reading the buttons just once 8)

9

Just out curiosity why do you need the EEPROM?

10

Alright. Made a few changes.

Moved nearly all the code into seperate functions,
added more comments,
figured out why I was getting button carry overs
and replaced the mode if statement with an FSM.
I was able to condense this code:

if (TimerLedState == LOW) {
   TimerLedState = HIGH;
} else {
   TimerLedState = LOW;
}

and replace it with this:

TimerLedState = !TimerLedState;

Is the anything that can be done with states?

if (Mode == ON) {
   Mode = OFF;
} else {
   Mode = ON;
}

The EEPROM saves the timer setting for when power loss occurrs. It will be running off a power supply connected to mains power.

Whole Code:

//  --- Exhaust Fan Controller --- 
//  Board: SparkFun Micro Pro 5V/16MHz
//  IDE: 1.8.13
//  Author: nickthenack9
//  Date: 26-01-2021

//------------------------------------------------------------------------
//                    Libraries and Global Variables
//------------------------------------------------------------------------

#include <EEPROM.h>
const byte OnButtonPin = 2;
const byte TimerButtonPin = 3;
const byte OnLedPin = 4;
const byte TimerLedPin = 5;
const byte FanPin = 6;
bool OnButtonState;
bool OnButtonPreviousState = HIGH;
bool TimerButtonState;
bool TimerButtonPreviousState = HIGH;
bool OnLedState = LOW;
bool TimerLedState = LOW;
bool FanPinState = LOW;
unsigned long ButtonHoldStart = 0;
unsigned long ButtonHoldTime = 0;
unsigned long TimerTime = 0;
unsigned long TimerPreviousTime = 0;
unsigned long StartTime = 0;
byte TimerTempMinutes = 1;
byte TimerSetMinutes = 1;
int FlashCount = -5;
enum {
  OFF,
  ON,
  TIMER,
  PROGRAMMING
} Mode;

//------------------------------------------------------------------------
//                               Setup
//------------------------------------------------------------------------

void setup()
{
  Mode = OFF; // Intial state

  // Reset EEPROM if outside valid range
  if ((EEPROM.read(0) == 0) || (EEPROM.read(0) > 30)) {
    EEPROM.write(0, 1);
  }

  TimerTempMinutes = EEPROM.read(0);
  TimerSetMinutes = EEPROM.read(0);
  pinMode(OnButtonPin, INPUT_PULLUP);
  pinMode(TimerButtonPin, INPUT_PULLUP);
  pinMode(OnLedPin, OUTPUT);
  pinMode(TimerLedPin, OUTPUT);
  pinMode(FanPin, OUTPUT);
}

//------------------------------------------------------------------------
//                             Main Loop
//------------------------------------------------------------------------

void loop()
{
  OnButtonState = digitalRead(OnButtonPin);
  TimerButtonState = digitalRead(TimerButtonPin);

  // What mode are we in?
  if (Mode == PROGRAMMING) {
    ProgrammingMode();
  } else {
    RunMode();
  }

  HoldDetection();

  digitalWrite(OnLedPin, OnLedState);
  digitalWrite(TimerLedPin, TimerLedState);
  digitalWrite(FanPin, FanPinState);
  
  OnButtonPreviousState = OnButtonState;
  TimerButtonPreviousState = TimerButtonState;
}

//------------------------------------------------------------------------
//                              Run Mode
//------------------------------------------------------------------------

void RunMode()
{
  // On button release. falling edge
  if (OnButtonState != OnButtonPreviousState && OnButtonState == HIGH) {
    if (Mode == ON) {
      Mode = OFF;
    } else {
      Mode = ON;
    }

    // Timer button release. falling edge
  } else if (TimerButtonState != TimerButtonPreviousState && TimerButtonState == HIGH) {  
    if (Mode == TIMER) {
      Mode = OFF;
    } else {
      Mode = TIMER;
      StartTime = millis();
      TimerLedState = HIGH;
    }
  }

  // Action per mode
  switch(Mode)
  {
    case OFF:
      OnLedState = LOW;
      TimerLedState = LOW;
      FanPinState = LOW;
      TimerTime = 0;
      TimerPreviousTime = 0;
      break;

    case ON:
      OnLedState = HIGH;
      TimerLedState = LOW;
      FanPinState = HIGH;
      TimerTime = 0;
      TimerPreviousTime = 0;
      break;

    case TIMER:
      TimerTime = millis() - StartTime;
      OnLedState = LOW;
      FanPinState = HIGH;
      if ((TimerTime - TimerPreviousTime) >= 1000) {
        TimerLedState = !TimerLedState;
        TimerPreviousTime = TimerTime;
      }
      if (TimerTime >= (TimerSetMinutes*60000)) {
        Mode = OFF;
      }
      break;   
  }
}

//------------------------------------------------------------------------
//                          Programming Mode
//------------------------------------------------------------------------

void ProgrammingMode()
{
  FanPinState = LOW;
  TimerTime = millis() - StartTime; // elasped time since last button press

  // increment minutes for every on button press. falling edge
  if (OnButtonState != OnButtonPreviousState && OnButtonState == HIGH) {
    if (TimerTempMinutes < 30) {
      ++TimerTempMinutes;
    }
    StartTime = millis();
    FlashCount = -5;

    // decrement minutes for every timer button press. falling edge 
  } else if (TimerButtonState != TimerButtonPreviousState && TimerButtonState == HIGH) {  
    if (TimerTempMinutes > 1) {
      --TimerTempMinutes;
    }
    StartTime = millis();
    FlashCount = -5;
  }

  // Leds flash number of timer minutes
  LedFlash();

  // Programming mode time out
  if (TimerTime > 60000) {
    Mode = OFF;
    TimerTempMinutes = TimerSetMinutes;
  }
}

//------------------------------------------------------------------------
//                     Hold Both Buttons Detection
//------------------------------------------------------------------------

void HoldDetection()
{
  // Record time when a button is pressed. rising edge
  if ((TimerButtonState != TimerButtonPreviousState && TimerButtonState == LOW) || (OnButtonState != OnButtonPreviousState && OnButtonState == LOW)) {
    ButtonHoldStart = millis();
    StartTime = millis(); // Reset timeout
  }

  // Are both buttons depressed?
  if (TimerButtonState == LOW && OnButtonState == LOW) {
    ButtonHoldTime = millis() - ButtonHoldStart;

    // Have you held the buttons long enough?
    if (ButtonHoldTime > 10000) {

      // Enter or exit programming mode
      if (Mode == PROGRAMMING) {
        Mode = OFF;
        TimerSetMinutes = TimerTempMinutes;
        EEPROM.update(0, TimerTempMinutes);
      } else {
        Mode = PROGRAMMING;
        FlashCount = -5;
        StartTime = millis();
        TimerTempMinutes = TimerSetMinutes;
      }

      OnButtonState = HIGH;
      TimerButtonState = HIGH;
      digitalWrite(OnLedPin, HIGH);
      digitalWrite(TimerLedPin, HIGH);

      // Wait for both buttons to be released
      while(digitalRead(OnButtonPin) == LOW || digitalRead(TimerButtonPin) == LOW) {
        delay(20);
      }

    }
  }
}

//------------------------------------------------------------------------
//                   Leds Flash Number of Set Minutes
//------------------------------------------------------------------------

void LedFlash()
{
  if (FlashCount < (TimerTempMinutes+1)) {
    if (TimerTime - TimerPreviousTime >= 350) {
      if (TimerLedState == LOW && FlashCount > 0) {
        OnLedState = HIGH;
        TimerLedState = HIGH;
      } else {
        OnLedState = LOW;
        TimerLedState = LOW;
        ++FlashCount;
      }
      TimerPreviousTime = TimerTime;
    }
  } else {
    FlashCount = -5;
  }
}
11

It's extremely low hanging fruit that I caught on the first pass, but:

 TimerTempMinutes = EEPROM.read(0);
  TimerSetMinutes = EEPROM.read(0);

should really be

 TimerTempMinutes = EEPROM.read(0);
  TimerSetMinutes = TimerTempMinutes;

Also, you seem to have distributed your state machine handling, that's not the best way:

 // What mode are we in?
  if (Mode == PROGRAMMING) {
    ProgrammingMode();
  } else {
    RunMode();
  }

and some few other places inside support functions like HoldDetection():

      // Enter or exit programming mode
      if (Mode == PROGRAMMING) {
        Mode = OFF;

mode switching should be done in ONE (1) switch case statement (or ONE (1) if-else-if-else-if...), per machine (there can be multiple state machines) so in your case more like:

 // What mode are we in?
  if (Mode == PROGRAMMING) {
    ProgrammingMode();
  } else if (Mode == RUNNING){
    RunMode();
  } else if (Mode ==...

In your functions, like RunMode(), you should NEVER test the mode inside, like this:

void RunMode()
{
  // On button release. falling edge
  if (OnButtonState != OnButtonPreviousState && OnButtonState == HIGH) {
    if (Mode == ON) {
      Mode = OFF;

Instead you should only change the mode by assigning the appropriate mode value to the mode variable. I'm not saying you can't make it work, but it spaghettifies the logic and makes it difficult to understand and maintain.

To answer another question, if the modes always go in sequence, you can change modes by simply incrementing the mode variable (and constraining it to its bounds), like

mode++;
if (mode >= numberOfModes) mode = 0;
12

It's looking better. Getting into the realms of personal taste, I still think your functions are too long, especially RunMode, but that's edging into holy war territory :frowning:

There's not anything clever you can do with flipping Mode. You could write a little function to do it, but it doesn't really seem worth it.

On that topic, this:

TimerLedState = !TimerLedState;

Is mildly dirty. You're relying on the fact that HIGH and LOW are the same as true and false. They always will be for the foreseeable future, but it's a bad habit to get into if you rely on what you know about how something is implemented.

13

It's extremely low hanging fruit that I caught on the first pass, but:

 TimerTempMinutes = EEPROM.read(0);

TimerSetMinutes = EEPROM.read(0);



should really be

TimerTempMinutes = EEPROM.read(0);
 TimerSetMinutes = TimerTempMinutes;

Yeah fair enough. Is a single action setting a variable then variables setting variables usually preferred?

Also, you seem to have distributed your state machine handling, that's not the best way:

 // What mode are we in?

if (Mode == PROGRAMMING) {
   ProgrammingMode();
 } else {
   RunMode();
 }



and some few other places inside support functions like HoldDetection():

// Enter or exit programming mode
     if (Mode == PROGRAMMING) {
       Mode = OFF;




mode switching should be done in ONE (1) switch case statement (or ONE (1) if-else-if-else-if...), per machine (there can be multiple state machines) so in your case more like:

// What mode are we in?
 if (Mode == PROGRAMMING) {
   ProgrammingMode();
 } else if (Mode == RUNNING){
   RunMode();
 } else if (Mode ==...




In your functions, like RunMode(), you should NEVER test the mode inside, like this:

void RunMode()
{
 // On button release. falling edge
 if (OnButtonState != OnButtonPreviousState && OnButtonState == HIGH) {
   if (Mode == ON) {
     Mode = OFF;



Instead you should only change the mode by assigning the appropriate mode value to the mode variable. I'm not saying you can't make it work, but it spaghettifies the logic and makes it difficult to understand and maintain.

To answer another question, if the modes always go in sequence, you can change modes by simply incrementing the mode variable (and constraining it to its bounds), like

mode++;
if (mode >= numberOfModes) mode = 0;

Yeah having a single method of switching mode makes sense. I can merge the FMS with the if statement in the main loop. Should I have the FMS in the main loop or have it in it's own function?

Unfortunately the modes arn't sequential, it changes solely from user input.
For functions that run regardless of the mode, how would they determine the mode without testing for it?

wildbill:
It's looking better. Getting into the realms of personal taste, I still think your functions are too long, especially RunMode, but that's edging into holy war territory :frowning:

There's not anything clever you can do with flipping Mode. You could write a little function to do it, but it doesn't really seem worth it.

On that topic, this:

TimerLedState = !TimerLedState;

Is mildly dirty. You're relying on the fact that HIGH and LOW are the same as true and false. They always will be for the foreseeable future, but it's a bad habit to get into if you rely on what you know about how something is implemented.

I can continue to move code to seperate functions but how many functions is too many?
It seems silly to do so when some parts are only a few lines and arn't called on multiple times.

Yeah ok. Is using the previous if statement better?

14

nickthenack9:
I can continue to move code to seperate functions but how many functions is too many?
It seems silly to do so when some parts are only a few lines and arn't called on multiple times.

People have strong opinions about it - you can make your own mind up.

My personal view is that a function should be viewable without having to scroll. Further, it should be simple enough that I can see at a glance what it does, so there shouldn't be multiple ifs, loops, switches etc. More than three such control structures and I'll think about breaking some of it out into another function.

I have no concerns about having too many functions. I'll happily put a single line of code into one simply because I can give the function a meaningful name. The compiler may well in-line it for me anyway. For the same reasons, I'm not worried about having a function that's only called from one place. For me, code clarity trumps efficiency unless I have to tune for performance reasons.

Yeah ok. Is using the previous if statement better?

Technically, yes. In this specific case, I wouldn't worry about it.

15

This topic was automatically closed 120 days after the last reply. New replies are no longer allowed.