Interactive shell for Arduino

Question

Goal

I needed to interact with microcontrollers (ESP8266 & ESP32) via the Serial interface, so I wrote a small interactive shell with the command pattern.

The defined commands can either accept no parameter or one int32 parameter.

The API should be clear and easy to use, the code should only rely on Arduino.h, and data should not be stored in the heap, in order to avoid fragmentation.

Code

The complete code is available on this GitHub repository.

Header

The command_invoker.h header is:

#ifndef COMMAND_INVOKER_H_INCLUDED
#define COMMAND_INVOKER_H_INCLUDED
#include <Arduino.h>

#define MAX_COMMAND_SIZE 30

/** Other scripts can use this invoker, in order to define commands, via callbacks.
 * Those callbacks can then be used to send commands to the sensor (e.g. reset, calibrate, night mode, ...)
 * The callbacks can either have no parameter, or one int32_t parameter.
 */

namespace command_invoker {
  void defineCommand(const char *name, void (*function)(void), const __FlashStringHelper *doc_fstring);
  void defineIntCommand(const char *name, void (*function)(int32_t), const __FlashStringHelper *doc_fstring);

  void execute(const char *command_line);
}

#endif

Library

the corresponding command_invoker.cpp is:

#include "command_invoker.h"

namespace command_invoker {
  const uint8_t MAX_COMMANDS = 20;

  uint8_t commands_count = 0;

  struct Command {
    const char *name;
    union {
      void (*intFunction)(int32_t);
      void (*voidFunction)(void);
    };
    const char *doc;
    bool has_parameter;
  };

  Command commands[MAX_COMMANDS];

  //NOTE: Probably possible to DRY (with templates?)
  void defineCommand(const char *name, void (*function)(void), const __FlashStringHelper *doc_fstring) {
    const char *doc = (const char*) doc_fstring;
    if (commands_count < MAX_COMMANDS) {
      commands[commands_count].name = name;
      commands[commands_count].voidFunction = function;
      commands[commands_count].doc = doc;
      commands[commands_count].has_parameter = false;
      commands_count++;
    } else {
      Serial.println(F("Too many commands have been defined."));
    }
  }

  void defineIntCommand(const char *name, void (*function)(int32_t), const __FlashStringHelper *doc_fstring) {
    const char *doc = (const char*) doc_fstring;
    if (commands_count < MAX_COMMANDS) {
      commands[commands_count].name = name;
      commands[commands_count].intFunction = function;
      commands[commands_count].doc = doc;
      commands[commands_count].has_parameter = true;
      commands_count++;
    } else {
      Serial.println(F("Too many commands have been defined."));
    }
  }

  /*
   * Tries to split a string command (e.g. 'mqtt 60' or 'show_csv') into a function_name and an argument.
   * Returns 0 if both are found, 1 if there is a problem and 2 if no argument is found.
   */
  uint8_t parseCommand(const char *command, char *function_name, int32_t &argument) {
    char split_command[MAX_COMMAND_SIZE];
    strlcpy(split_command, command, MAX_COMMAND_SIZE);
    char *arg;
    char *part1;
    part1 = strtok(split_command, " ");
    if (!part1) {
      Serial.println(F("Received empty command"));
      // Empty string
      return 1;
    }
    strlcpy(function_name, part1, MAX_COMMAND_SIZE);
    arg = strtok(NULL, " ");
    uint8_t code = 0;
    if (arg) {
      char *end;
      argument = strtol(arg, &end, 10);
      if (*end) {
        // Second argument isn't a number
        code = 2;
      }
    } else {
      // No argument
      code = 2;
    }
    return code;
  }

  int compareCommandNames(const void *s1, const void *s2) {
    struct Command *c1 = (struct Command*) s1;
    struct Command *c2 = (struct Command*) s2;
    return strcmp(c1->name, c2->name);
  }

  void listAvailableCommands() {
    qsort(commands, commands_count, sizeof(commands[0]), compareCommandNames);
    for (uint8_t i = 0; i < commands_count; i++) {
      Serial.print("  ");
      Serial.print(commands[i].name);
      Serial.print(commands[i].doc);
      Serial.println(".");
    }
  }

  /*
   * Tries to find the corresponding callback for a given command. Name and number of arguments should fit.
   */
  void execute(const char *command_line) {
    char function_name[MAX_COMMAND_SIZE];
    int32_t argument = 0;
    bool has_argument;
    has_argument = (parseCommand(command_line, function_name, argument) == 0);

    for (uint8_t i = 0; i < commands_count; i++) {
      if (!strcmp(function_name, commands[i].name) && has_argument == commands[i].has_parameter) {
        Serial.print(F("Calling : "));
        Serial.print(function_name);
        if (has_argument) {
          Serial.print(F("("));
          Serial.print(argument);
          Serial.println(F(")"));
          commands[i].intFunction(argument);
        } else {
          Serial.println(F("()"));
          commands[i].voidFunction();
        }
        return;
      }
    }
    Serial.print(F("'"));
    Serial.print(command_line);
    Serial.println(F("' not supported. Available commands :"));
    listAvailableCommands();
  }
}

Sketch

Finally, here is a Arduino sketch in order to use this library:

/***************************************************************************************************
 * Small interactive shell via Serial interface for ESP32 and ESP8266, based on the command pattern.
 ***************************************************************************************************/

#include "command_invoker.h"

/**
 * Some example functions, which could be defined in separate libraries.
 */

void multiplyBy2(int32_t x) {
  Serial.print(x);
  Serial.print(" * 2 = ");
  Serial.println(2 * x);
}

void controlLED(int32_t onOff) {
  digitalWrite(LED_BUILTIN, onOff);
}

/**
 * Setup
 */

void setup() {
  Serial.begin(115200);
  pinMode(LED_BUILTIN, OUTPUT);

  // Define commands. Could be done in separate libraries (e.g. MQTT, LoRa, Webserver, ...)
  command_invoker::defineIntCommand("led", controlLED, F(" 1/0 (LED on/off)"));
  command_invoker::defineIntCommand("double", multiplyBy2, F(" 123 (Doubles the input value)"));

  // Commands can also be created with lambdas.
  command_invoker::defineCommand("reset", []() {
    ESP.restart();
  }, F(" (restarts the microcontroller)"));

  // Simple example. Turn LED on at startup.
  command_invoker::execute("led 1");

  Serial.println(F("Console is ready!"));
  Serial.print(F("> "));
}

/*
 * Saves bytes from Serial.read() until enter is pressed, and tries to run the corresponding command.
 *   http://www.gammon.com.au/serial
 */
void processSerialInput(const byte input_byte) {
  static char input_line[MAX_COMMAND_SIZE];
  static unsigned int input_pos = 0;
  switch (input_byte) {
  case '\n': // end of text
    Serial.println();
    input_line[input_pos] = 0;
    command_invoker::execute(input_line);
    input_pos = 0;
    Serial.print(F("> "));
    break;
  case '\r': // discard carriage return
    break;
  case '\b': // backspace
    if (input_pos > 0) {
      input_pos--;
      Serial.print(F("\b \b"));
    }
    break;
  default:
    // keep adding if not full ... allow for terminating null byte
    if (input_pos < (MAX_COMMAND_SIZE - 1)) {
      input_line[input_pos++] = input_byte;
      Serial.print((char) input_byte);
    }
    break;
  }
}

/**
 * Loop and wait for serial input. Commands could also come from webserver or MQTT, for example.
 */

void loop() {
  while (Serial.available() > 0) {
    processSerialInput(Serial.read());
  }
  delay(50);
}

Usage

It's now possible to send commands directly in PlatformIo monitor or in Arduino IDE Serial Monitor:

Console is ready!
> test
'test' not supported. Available commands :
  double 123 (Doubles value).
  led 1/0 (LED on/off).
  reset (restarts the microcontroller).
> led 1
Calling : led(1)
> led 0
Calling : led(0)
> double 12345
Calling : double(12345)
12345 * 2 = 24690
> double
'double' not supported. Available commands :
  double 123 (Doubles value).
  led 1/0 (LED on/off).
  reset (restarts the microcontroller).
> reset
Calling : reset()

Questions

This library works fine, but I'm still a C/C++ beginner and would appreciate any feedback.

In particular, I'd be happy to :

• remove the duplicate code in defineCommand and defineIntCommand
• allow commands to have a string argument
• allow commands to have a specific numerical argument (e.g. a bool or a uint8_t and not just a cast int32_t)
• be sure there is no buffer overflow anywhere
• use as little memory as possible.

Sadly, many good C++ suggestions (e.g. with std::string, std::vector or std::map) don't seem to apply to this project, because the data types are not available on Arduino. That's why the code might look more like C than C++.

Answer 1

Since everybody here has already provided great suggestions. I am going to try and answer only the asked questions.

1. Remove duplicate code: have internal function

   bool defineCommandInternal(const char *name, const __FlashStringHelper *doc_fstring) {
       const char *doc = (const char*) doc_fstring;
       if (commands_count < MAX_COMMANDS) {
           commands[commands_count].name = name;
           commands[commands_count].doc = doc;
           commands_count++;
           return true;
       } else {
           Serial.println(F("Too many commands have been defined."));
       }
       return false;
   }
   
   void defineCommand(const char *name, void (*function)(void), const __FlashStringHelper *doc_fstring) {
       if (defineCommandInternal(name, doc_fstring)) {
           commands[commands_count-1].voidFunction = function;
           commands[commands_count-1].param_type = 0;
       }
   }
   
   void defineIntCommand(const char *name, void (*function)(int32_t), const __FlashStringHelper *doc_fstring) {
       if (defineCommandInternal(name, doc_fstring)) {
           commands[commands_count-1].intFunction = function;
           commands[commands_count-1].param_type = 2;
       }
   }
   

2. Allow commands to have different arguments: replace has_argument to param_type

   struct Command {
       const char *name;
       const char *doc;
       union {
           void (*intFunction)(int32_t);
           void (*intFunction)(uint8_t);
           void (*voidFunction)(void);
       };
       uint8_t param_type;   // will be used as 0: void, 1: uint8_t, 2: int32_t ... etc.
   };
   

   Return argument type detected in shell from parseCommand(command_line, function_name, argument)

General Improvements:

1. List commands should not sort everytime. Once should be enough in setup as after the shell starts the commands are not added.

2. Why are you unnecessarily creating char array of [MAX_COMMAND_SIZE] in parseCommand? Better would be just loop through string character by character to get the function name. Gives you exact length to malloc (+1 for \0).

3. Are you supporting command overloading? If not there is no need to have has_argument or param_type that I suggested. Saves space reduces complexity as command names are unique.

Answer 2

#define

Don't use #define for constants!

#define MAX_COMMAND_SIZE 30

should be:

constexpr size_t MAX_COMMAND_SIZE = 30;

(I have some discussion on this point on a Code Project post)

string_view

void defineCommand(const char *name, 

Consider using std::string_view instead, which can efficiently take a lexical string literal like "MyName" or a std::string object. It's also much easier to use, e.g. matching with == rather than strcmp.

reserved names

__FlashStringHelper is a reserved name that should only be used by the compiler internals, like the standard library implementation and hidden built-in features. I don't see you define this symbol so maybe it's not your fault but in the supplied header. It's still Undefined Behavior unless the compiler your using was written by the same folks that supplied the header, and even then it should not be a user-exposed name.

pragma once

#ifndef COMMAND_INVOKER_H_INCLUDED
#define COMMAND_INVOKER_H_INCLUDED

Every compiler I've used accepts #pragma once even though it is not officially part of the standard. I use that instead of this kind of interlock, and figured if it ever becomes necessary it can be replaced with #ifndef etc. by a simple script; and in 30 years it has never been necessary.

what #includes do you need?

You're not including <cstdint> and I don't see a using std::int32_t; either. I suppose the Arduino.h is not only including some of the standard headers, but polluting the global namespace. Is it documented as pulling in certain standard includes? Headers generally don't define that officially and could change exactly what they pull in. You should include the standard headers you need in that file.

(void)

void (*function)(void)

To quote Bjarne Strustrup, "(void) is an abomination." Empty parameter lists are written (). The (void) construct is for compatibility with ANSI C which introduced it for reasons that don't apply to C++.

containers

if (commands_count < MAX_COMMANDS) {
  commands[commands_count].name = name;
  commands[commands_count].voidFunction = function;
  commands[commands_count].doc = doc;
  commands[commands_count].has_parameter = false;
  commands_count++;
} else {
  Serial.println(F("Too many commands have been defined."));

Don't refer to commands[commands_count] to add things to a list. Use std::vector rather than a fixed size array! You should not have a separate commands and commands_count variables, as they are one object and the collection code knows how to maintain itself. And MAX_COMMAND_SIZE is simply unnecessary, as the vector grows as needed.

Use a constructor or aggregate initializer to fill out a Command instance, and then push_back to the vector.

Your has_parameter member is keyed to the union usage, so make the different forms different constructors of Command rather than separate functions like you have it. Those functions implicitly deal with your "collection" which is the original thing I brought up here. It should be self-contained -- the constructor doesn't know about any further usage of the object, like what kind of global variables you have in the program!

Command (const char *name, void (*function)(void), const __FlashStringHelper *doc_fstring);  // one constructor

Command (const char *name, void (*function)(int32_t), const __FlashStringHelper *doc_fstring);  // another constructor

// ...

std::vector<Command> commands;

// ...

commands.push_back (Command{"led", controlLED, F(" 1/0 (LED on/off)")});
// or use `emplace_back`
commands.emplace_back ("double", multiplyBy2, F(" 123 (Doubles the input value)");

unions

You made your own discriminated union, with a plain C union and the has_parameter member acting as the key. Instead, use std::variant which does that for you.

parameters and pointers

uint8_t parseCommand(const char *command, char *function_name, int32_t &argument) {

Is function_name the output? Clearly argument is an "out" parameter. And you have a function return also, but it's not clear what it's used for.

You should have function results be returned as return values, and not use so-called "out" parameters.

The function_name is populating a string buffer supplied by the caller, which is subject to length issues and allocation issues. Just return that part of the result as a std::string ! This is C++, not C.

char split_command[MAX_COMMAND_SIZE];
strlcpy(split_command, command, MAX_COMMAND_SIZE);
char *arg;
char *part1;
part1 = strtok(split_command, " ");

yea... you should not need MAX_COMMAND_SIZE at all, and copying strings with C functions should not be necessary. I think you only needed to make a copy because strtok modifies the input... don't use that function, but use C++ standard library functions. There are members of std::string to find a specific character, as well as "STL" algorithms that are defined apart from any container.

compare names

int compareCommandNames(const void *s1, const void *s2) {
    struct Command *c1 = (struct Command*) s1;
    struct Command *c2 = (struct Command*) s2;
    return strcmp(c1->name, c2->name);
}

Why are you passing in the Command arguments as void*? Don't do that!

Ah... qsort(commands, commands_count, sizeof(commands[0]), compareCommandNames);

Use std::sort instead. It is not only type-safe, but much faster! Furthermore, if you started using things in the Command struct that are not simple C "PODS", then this will seriously malfunction.

just find it

for (uint8_t i = 0; i < commands_count; i++) {
  if (!strcmp(function_name, commands[i].name) && has_argument == commands[i].has_parameter) {

Just use std::find to locate the correct Command by name. Perhaps instead of a vector to replace the array, you want to use a map instead? Then make the name the key, and not part of the Command structure.

Answer 3

From my experience working with Arduino or the AVR architecture in general is an exercise in trying to make sure everything you need will actually fit in memory, both RAM and flash. The scary thing is you don't really know how much space your stack will require so there's a bit of a guessing game if your stack is going to overwrite your heap or static data segments without telling you, resulting in silent breakage or hard to debug errors.

With that in mind there are some things I'd like to suggest:

Don't reserve space for max commands

You set a maximum number of commands and allocate space for them in RAM currently this is 7*30=210 bytes of RAM which is almost 1/5th of the available RAM on the ATMega328p powering the most common Arduinos. If you don't use all 30 commands, you're wasting precious memory. In addition to this, the command names then selves take up RAM because you're not storing them in flash of you're actually using all 30 commands and have reasonably readable command names you might be using half your total RAM just to define static data.

There are several ways to address this, I think the easiest (but maybe not the prettiest) might be to let the user define the commands array. This is how it would work, in your command_invoker.cpp change the line:

 Command commands[MAX_COMMANDS]

to

 extern Command* commands;
 extern uint8_t max_commands;

extern tells the compiler that the array content is declared somewhere else and the linker will fix all references in the linking stage of the compilation.

And the user would then do something like this in their code:

 Command[] commands = {make_command("name", function, "doc"),
                       make_command("name1", function, "doc"),
                       ... };
 uint8_t max_commands = sizeof(commands)/sizeof(Command);

Obviously you now need to replace MAX_COMMANDS with max_commands everywhere...

You need helper functions to create the commands because initializer lists are not allowed for unions afair.

Just create two overloaded functions like below in your library:

Command make_command(const char* name, void (*)(), const char* doc);
Command make_command(const char* name, void (*)(int), const char* doc)

The compiler will choose (disambiguate) the correct one based on the second s argument in this case so you don't need different l names for different argument function types.

A further and final improvement here, although a bit trickier to implement, is to actually store the entire commands array and all strings in flash memory. This would be the best solution IMHO. Yes, reading from flash takes a little bit longer but I don't really think it matters in this use case. RAM is so precious on these micro controllers.

Including the doc string might be costly

Including the doc strings cost memory and you should closely consider if it's worth it. It'll be two bytes of RAM per command and then the matching amount of flash. Doesn't sound like much but in a constrained memory situation like the AVR it could be the difference between your thing working or not.

You could add a macro like DISABLE_DOC_STRINGS, and using #ifndef(DISABLE_DOC_STRING) ... #else ... #endif to disable the functionality and remove it from the Command struct and relevant pieces of code. The user would then pass this as a command line parameter when building (out from their makefile/what have you) like -DDISABLE_DOC_STRING. Note: the double D is not a typo, the first part -D means to define the variable following which is DISABLE_DOC_STRING.

Other aspects of the code are already treated by other answers.

Answer 4

Dealing with the lack of STL

You wrote:

@JDługosz's answer contains many good tips for generic C++ projects. Those suggestions don't seem to apply to Arduino projects, though, because the std:: objects are not available. I'd be glad to get suggestions specific to the Arduino platform, so that the code can be compiled for ESP32/ESP8266, at best without including any other header than 'Arduino.h'.

While for space constraints (especially on the lowest-end devices), the Arduino environment by default doesn't provide the STL, that doesn't mean you should fall back to C-style programming. I would follow JDługosz's advice, and find replacements for the STL functionality. There are ports of the STL for Arduino, for example ArduinoSTL, that might provide you with what you need. Another option is to just implement the parts of the STL you would want to use yourself. It is not that hard to implement rudimentary versions of std::string_view and std::vector that just do the minimum for what you need. To keep things compact, you can for example implement std::sort() by having it call std::qsort() internally.