Code Review

I have written code for a project that I am working on. The logic is as follows:

  1. Set the timer to 5 minutes when button 1 is pressed.

  2. Set the timer to 10 minutes when button 2 is pressed.

  3. When button 3 is pressed, checked if both microswitches are closed. If both are closed, turn on relay and wait until the time elapses or button 4 is pressed to turn off the relay.

  4. If relays are ON and microswitches are opened, turn off relay and activate buzzer until it is closed.

  5. At any point if button 4 is pressed, turn off the relays and reset the timer.

How can I optimize the code to make it run faster? I do I deal with edge cases and make the code fail-safe?

Thanks

#include <digitalWriteFast.h>

const int buzzer = 2;
const int startButton = 3;
const int stopButton = 4;
//const int pauseButton = 5;
const int switchState1 = 6;
const int switchState2 = 7;
const int relay = 8;
const int greenLed = 9;
const int redLed = 10;
const int interval1 = 11;
const int interval2 = 12;

int startButtonState;
int stopButtonState;
int interval1State;
int interval2State;
//int pauseButtonState;
int startButtonLastState = LOW;
int stopButtonLastState = LOW;
int interval1LastState = LOW;
int interval2LastState = LOW;
//int pauseButtonLastState = LOW;
int relayState;

unsigned long startDebounceTime = 0;
unsigned long stopDebounceTime = 0;
unsigned long interval1DebounceTime = 0;
unsigned long interval2DebounceTime = 0;
//unsigned long pauseDebounceTime = 0;
unsigned long debounceDelay = 50;
unsigned long relayStartTime = 0;
unsigned long intervalTime1 = 480000;
unsigned long intervalTime2 = 900000;
unsigned long interval;


void setup() {
  pinMode(buzzer, OUTPUT);
  pinMode(startButton, INPUT);
  pinMode(stopButton, INPUT);
  //pinMode(pauseButton, INPUT);
  pinMode(switchState1, INPUT);
  pinMode(switchState2, INPUT);
  pinMode(relay, OUTPUT);
  pinMode(greenLed, OUTPUT);
  pinMode(redLed, OUTPUT);
  pinMode(interval1, INPUT);
  pinMode(interval2, INPUT);
  pinMode(redLed, OUTPUT);
  digitalWrite(greenLed, HIGH);
    
}

void loop() {
    while(1){
      int readStartButtonState = digitalReadFast(startButton);
      int readStopButtonState = digitalReadFast(stopButton);
      int readInterval1State = digitalReadFast(interval1);
      int readInterval2State = digitalReadFast(interval2);
      //int readPauseButtonState = digitalReadFast(pauseButton);
      
      if (readStartButtonState != startButtonLastState) {
        startDebounceTime = millis();
      }
      if (readStopButtonState != stopButtonLastState) {
        stopDebounceTime = millis();
      }
      if (readInterval1State != interval1LastState) {
        interval1DebounceTime = millis();
      }
      if (readInterval2State != interval2LastState) {
        interval2DebounceTime = millis();
      }
      /*if (readPauseButtonState != pauseButtonLastState) {
        pauseDebounceTime = millis();
      }*/
      if ((millis() - startDebounceTime) > debounceDelay) {
        if (digitalReadFast(switchState1) == HIGH && digitalReadFast(switchState2) == HIGH){
          if (readStartButtonState != startButtonState) {
            startButtonState = readStartButtonState;
            if (startButtonState == HIGH) {
              digitalWriteFast(buzzer,LOW)
              digitalWriteFast(relay,HIGH);
              relayStartTime = millis();
              relayState = HIGH;
              digitalWriteFast(greenLed,LOW);
              digitalWriteFast(redLed,HIGH);
              tone(2,2000,1000);
              
            }
          }
        }
      }
      if ((millis() - stopDebounceTime) > debounceDelay) {
        if (readStopButtonState != stopButtonState) {
          stopButtonState = readStopButtonState;
          if (stopButtonState == HIGH) {
            digitalWriteFast(relay,LOW);
            relayState = LOW;
            digitalWriteFast(greenLed,HIGH);
            digitalWriteFast(redLed,LOW);
            tone(2,2000,500);            
          }
        }
      }
      if ((millis() - interval1DebounceTime) > debounceDelay) {
        if (readInterval1State != interval1State) {
          interval1State = readInterval1State;
          if (interval1State == HIGH) {
            interval = intervalTime1;            
          }
        }
      }
      if ((millis() - interval2DebounceTime) > debounceDelay) {
        if (readInterval2State != interval2State) {
          interval2State = readInterval2State;
          if (interval2State == HIGH) {
            interval = intervalTime2;            
          }
        }
      }
      
      
      
      if (relayState == HIGH){
        if((millis() - relayStartTime) > interval){
          digitalWriteFast(relay,LOW);
          relayState = LOW;
          relayStartTime = 0;
          tone(2,2000,500);
        }
        if((millis() - relayStartTime) < interval){
          if ((millis() - stopDebounceTime) > debounceDelay) {
            if (readStopButtonState != stopButtonState) {
              stopButtonState = readStopButtonState;
              if (stopButtonState == HIGH) {
                digitalWriteFast(relay,LOW);
                relayState = LOW;
                digitalWriteFast(greenLed,HIGH);
                digitalWriteFast(redLed,LOW);
                tone(buzzer,2000,500);
                
              }
            }
          }
          if (digitalReadFast(switchState1) == LOW || digitalReadFast(switchState2) == LOW){
            tone(buzzer,2000);
          }
          
        }
        
      }
      
      
    }
}

It doesn't need any optimization. In fact, you can remove the fastWrite stuff, none of the devices you use come even close to fast enough, for that to matter. Just use digitalRead and digitalWrite.

To improve reliability and maintainability, you should focus more on structure, formatting, and modularization, and also add a lot more inline documentation (comments). At this point, the nested timing statements may work, but are extremely difficult to read because of the monolithic approach - use more functions to isolate unrelated or loosely related parts of the code and give it a more top-down structure. Your loop() should contain mainly just functions.

  • have you tested your code? can you post the results (we did during reviews)

  • why aren't there any comments at least explaining what each conditional block is intended to do?

  • why is there a while (1) loop in loop()? the arduino main repeatedly calls loop(), doing the same thing?

  • i don't understand why there are so many conditional timing blocks (e.g. millis() - someTime > some Delay). there should only be one.

  • that conditional can depend on whether timing is active (e.g. someTime is set and not zero)

  • the delay is either 5 mins or increased to 10 mins

  • when checking for button presses, where is startButtonLastState, for example, set to readStartButtonState?

  • shouldn't checking for microswitch closures NOT be conditional, shouldn't it always be performed and if the timing is enabled (i.e. someTime != 0), the relays turned on?

  • debouncing can be handled with a 10 msec delay each iteration.

  • i don't see where startDebounceTime is ever reset, or something, to prevent

if ((millis() - startDebounceTime) > debounceDelay) {

from continually being true

enough for now

aarg:
It doesn't need any optimization. In fact, you can remove the fastWrite stuff, none of the devices you use come even close to fast enough, for that to matter. Just use digitalRead and digitalWrite.

To improve reliability and maintainability, you should focus more on structure, formatting, and modularization, and also add a lot more inline documentation (comments). At this point, the nested timing statements may work, but are extremely difficult to read because of the monolithic approach - use more functions to isolate unrelated or loosely related parts of the code and give it a more top-down structure. Your loop() should contain mainly just functions.

Thank you. I will make those changes. I am planning to replace the relays with SSR or TRIAC which will have a faster response time, so maybe then I will need to use the fastWrite?

gcjr:

  • have you tested your code? can you post the results (we did during reviews)

  • why aren't there any comments at least explaining what each conditional block is intended to do?

  • why is there a while (1) loop in loop()? the arduino main repeatedly calls loop(), doing the same thing?

  • i don't understand why there are so many conditional timing blocks (e.g. millis() - someTime > some Delay). there should only be one.

  • that conditional can depend on whether timing is active (e.g. someTime is set and not zero)

  • the delay is either 5 mins or increased to 10 mins

  • when checking for button presses, where is startButtonLastState, for example, set to readStartButtonState?

  • shouldn't checking for microswitch closures NOT be conditional, shouldn't it always be performed and if the timing is enabled (i.e. someTime != 0), the relays turned on?

  • debouncing can be handled with a 10 msec delay each iteration.

  • i don't see where startDebounceTime is ever reset, or something, to prevent

if ((millis() - startDebounceTime) > debounceDelay) {

from continually being true

enough for now

Thanks.

  • No, a component is yet to arrive.
  • I will add comments.
  • I read that it is more efficient to use a infinite loop inside loop() than exiting and entering loop().
  • Oops, yea. One would have been enough.
  • You mean Checking for debouncing only if the timer is set?
  • Alright.
  • The rest are just blunder from me. Lol. Thanks.

You mean the debouncing can be handled by just using 10ms delay, without having to do these many comparisons?

ismyname:
You mean the debouncing can be handled by just using 10ms delay, without having to do these many comparisons?

yes

gcjr:
yes

Great! Thanks!

ismyname:

  • I read that it is more efficient to use a infinite loop inside loop() than exiting and entering loop().

It will because the processor doesn't do the housekeeping tasks, such as updating millis. I would think that you want millis to be updated.

Loop() runs about 16 Million times a second- isn't that fast enough for you?

#include <Arduino.h>

int main(void)
{
    init();

#if defined(USBCON)
    USBDevice.attach();
#endif
    
    setup();
    
    for (;;) {
        loop();
        if (serialEventRun) serialEventRun();
    }
        
    return 0;
}

millis() is not updated by main.

What Arduino runs loop at 16 million times a second and how did you determine this? or did you just mean very frequently?

a7

SteveMann:
It will because the processor doesn't do the housekeeping tasks, such as updating millis. I would think that you want millis to be updated.

Loop() runs about 16 Million times a second- isn't that fast enough for you?

Wouldn't it depends on the instructions inside the loop?

ismyname:
I have written code for a project that I am working on. The logic is as follows:

  1. Set the timer to 5 minutes when button 1 is pressed.

  2. Set the timer to 10 minutes when button 2 is pressed.

  3. When button 3 is pressed, checked if both microswitches are closed. If both are closed, turn on relay and wait until the time elapses or button 4 is pressed to turn off the relay.

  4. If relays are ON and microswitches are opened, turn off relay and activate buzzer until it is closed.

  5. At any point if button 4 is pressed, turn off the relays and reset the timer.

There are many questions I could ask about this "specification" of how the program is supposed to behave,
as is very open to interpretation. A specification should be precise and crystal clear.

For instance what should happen if button 1 is pressed, then 4 minutes later is pressed again - is the timer
now at 1 minute or reset to 5 ?

I'd recommend learning to draw state-machine transition diagrams for describing and specifying behaviour,
as this allows you to fill in all the missing transitions you forgot to think about.

State machine formalism makes you describe states and events explicitly so you can consider what should
happen for every event in every state. Currently only rule 5) in your description considers every state for
a particular input event. Also its not clear if setting a timer to a value starts it running or not, and what
events stop or cancel the timer.

And state transition diagrams are easy to code up.

(deleted)

SteveMann:
Loop() runs about 16 Million times a second- isn’t that fast enough for you?

That’s nonsense, and very misleading

MarkT:
There are many questions I could ask about this "specification" of how the program is supposed to behave,
as is very open to interpretation. A specification should be precise and crystal clear.

For instance what should happen if button 1 is pressed, then 4 minutes later is pressed again - is the timer
now at 1 minute or reset to 5 ?

I'd recommend learning to draw state-machine transition diagrams for describing and specifying behaviour,
as this allows you to fill in all the missing transitions you forgot to think about.

State machine formalism makes you describe states and events explicitly so you can consider what should
happen for every event in every state. Currently only rule 5) in your description considers every state for
a particular input event. Also its not clear if setting a timer to a value starts it running or not, and what
events stop or cancel the timer.

And state transition diagrams are easy to code up.

Thank you. Makes sense. Let me come back with a state-machine transition diagram.

smarts-jb:
Yes, but the idea is to break things up so that you can do loads of stuff seemingly at the same time, with each thing getting a tiny slice of time each visit through loop(). Very often, a thing may not happen on many passes through loop(): if you blink an led delay()-lessly for example, you'll check each time if a blink is due, but only actually toggle the led once in a gazillion passes.

Alright. Thanks! So you suggest I remove the while loop inside the loop().

I am planning to work with a different ATmega chip, the 808 or 1608. I read that Arduino IDE outputs code that are not as efficient as the ones outputs by Atmel Studio. Should, switch to AS and code the logic in C or is there a way to make Arduino IDE output more efficient code?

(deleted)

smarts-jb:
Must confess I haven't been following the thread or read your code; just picked up on that thing about the speed of loop().

Oops. Ok

ismyname:
Thank you. I will make those changes. I am planning to replace the relays with SSR or TRIAC which will have a faster response time, so maybe then I will need to use the fastWrite?

Not unless the device you are controlling with the SSR or TRIAC itself requires a faster response time. If you turn a light on or off, or a motor, does it really make any difference if it is 1/10,000 of a second faster or slower? No.

SteveMann:
It will because the processor doesn't do the housekeeping tasks, such as updating millis.

Nonsense. millis() updates are interrupt-driven.

aarg:
Not unless the device you are controlling with the SSR or TRIAC itself requires a faster response time. If you turn a light on or off, or a motor, does it really make any difference if it is 1/10,000 of a second faster or slower? No.

Thanks. Understood