Loop structure recommendations

Using Arduino Programming Questions
1

This is a generic (or best practice) question.

I usually have many functions in a program.
What I often end up with are 1..3 function calls in the loop, and that's it.

For example:

void loop()
{
    identify_button_pressed();
}

... while identify_button_pressed(x) calls
measure_button_resistor(y), which calls
actuate_light(z)
... and so on.

Would it be better practice to write the loop like so:

void loop()
{
   x = identify_button_pressed();
   y = measure_button_resistor(x);
    ...
   actuate_light(n);
}

I suspect the latter example shows more what is going on, rather than having to read thought the code where it is going.
So better legibility, but more intermediate variables.

For those needing a more concrete example:

/*! -------------- Prototypes ----------------------------------------------- */
// common static code functions
void mac_address_build(uint8_t pin);
void mac_address_publish();
void ip_address_publish();
void ethernet_connection_maintain();
void message_publish(const char *topic, const char *msg);
void error_publish(uint8_t error_type, char *error_msg);
char *deblank(char *str);

void  temperature_collect();
float temperature_get(uint8_t pin);
float temperature_validate(uint8_t pin, float temp_sensed);
void  temperature_publish(uint8_t pin, float temp);

// modify as required
bool b_connected_to_broker();
void mqtt_call_back(char *topic, uint8_t *payload, unsigned int length);

// program-specific functions
void setup_input_pins();
void setup_output_pins();
void send_mqtt_reset_messages();
void actuate_relay(uint8_t relay, uint8_t state);
uint8_t which_button_has_been_pressed(uint16_t button_voltage, uint8_t button_group);
void actuate_rollerdoor(uint8_t door_id);
void toggle_light(uint8_t light_id);
void perform_button_action(uint8_t button_box, uint8_t button_pressed);
void button_press_detect();
void motion_detect();
void smoke_detect();
float measure_button_resistor(uint8_t input_pin);

void loop()
{
    wdt_reset();
    ethernet_connection_maintain();
    mqtt_client.loop();
    button_press_detect();
    smoke_detect();
    motion_detect();
}

Maybe the bigger scopes are in the loop, and the helpers remain hidden (not in the loop).
It's just something I pondered about.

Any hints appreciated.

2

The compiler will make the best sequence regardless of your sequence. Just make the code readable for you. A function calling a function makes the complete program modular. That is good.

3

A good rule of thumb is to have each function perform only one operation. This doesn't mean you need to write a function for every line of code - operations can be compound and quite complex. However, logically it should be one action.
Relatively speaking, if you can say “the procedure does X”, then this is the correct organization of the code. And if you have to write something like “the function is designed to do Y and Z” - then it means it’s better to divide the function into two :slight_smile:

2 Likes
4

Perfect. But it should not activate the light. Or it should have a different name...

x is a bad name for a variable with wide scope. There is significant chance it will be used later on for a different purpose...
Inside a 5 line function it is ok...

5

Totally agree; I typed it for simplicity sake... I usually use the name of the parameter being passed in, or something close.

Keep it coming; I am still (in need of heavy) learning; wrote my first class (and header) a couple of days ago :slight_smile:

6

This. Any function, but the loop() in particular, has an opportunity to tell a story.

The loop should make the purpose of the sketch obvious, and the place to look for what makes the steps happen clear.

Think of it like the table of contents of a book, or a synopsis.

Often the loop written with the IPO model in mind works well. Read about it in the abstract here:

The IPO Model

An obligatory tip o' the hat to @paulpaulson who showed me the embedded systems name for read, evaluate and print.

As for more variables or your other cons, I did not look at your code, but if you write relentlessly forcing yourself to give variables the least scope you can, it should not be a problem.

Write good functions that use a few parameters and return values of appropriate types. You can reduce global variable use substantially; most boob noob code is lazy and ignorant around these languages features. Again, not saying that's you, I did not look at the code on offer, just wanted to say.

a7

2 Likes
7

In my opinion it's the cleanest to separate input handling from logic and from output control

void loop()
{
  // handle any type of input
  handleInputs();
  // do the logic resulting in output values
  doLogic();
  // control the outputs based on the result of the logic
  controlOutputs();
}
1 Like
8

Yes. Look like IPO in action.

And +1 to your emphasis on the separation not only into I, P, and O, but the idea that the P part can be (wait for it)… ~100 percent free of any references to real hardware, and if done carefully, entirely possible to write to a high degree in isolation to real life issues.

a7

9

What the others have said bears repeating relentlessly. Reduce complexity of your functions. IPO, absolutely. KISS.

Others may suggest "make loop() your loop, as much as you can". It took me a while to see the sense in that, but it's now very present in my code. If processing an array is bogging down, consider doing one element, or one row of a 2D array, in each pass through loop(). As a personal challenge, toggle the onboard LED each time through loop(), and strive to keep that LED appearing like it's constantly on, no blinks. For most of what we do with these small micros, it can be done. When you suddenly see blinks, think about what you've changed that slowed your code down.

Functions should get their job done, and exit promptly. No waiting around for input, no ginormous array processing. Short, sweet and snappy.

Remove delay() from your toolset. Button debounces, for example, don't get done with delay(), but can be done with millis() tests.

Sign me,
Just another slow learner.

1 Like
10

Tom Demarco in Structured Analysis and System Specification: Tom DeMarco, P. J. Plauger: 9780138543808: Amazon.com: Books referenced the paper, [The Magic Number 7 +/- 2](The Magical Number Seven, Plus or Minus Two - Wikipedia.

make sense to keep the # of calls in loop within that range as you've suggested

1 Like
11

Which board is this? You should not have to call a wdt_reset manually. The platform's main function that calls setup and loop should do it. For example, with "esp32"

  setup();
#if ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_DEBUG
  printAfterSetupInfo();
#else
  if (shouldPrintChipDebugReport()) {
    printAfterSetupInfo();
  }
#endif
  for (;;) {
#if CONFIG_FREERTOS_UNICORE
    yieldIfNecessary();
#endif
    if (loopTaskWDTEnabled) {
      esp_task_wdt_reset();
    }
    loop();
    if (serialEventRun) {
      serialEventRun();
    }
  }