Very basic shell on microcontroller in C

Objective: A lightweight shell that runs on a microcontroller (MSP430) and parses incoming data into a command and additional
parameters.


Requirements:

• Support of quoting in parameters using "

• Escaping special characters using \ in parameters

• Ending commands with \r or \n

• Separating parameters using ` or \t

• Sending NACK followed by a line break in the event of an error

• Locking (not accepting new commands) the shell during processing

NOT required:

• Support for entering special characters with leading backslash (\n, \015, ...)

• Support for backspace

• Support for ending commands using \r\n






Circumstances:


Though the microcontroller in use is a pretty powerful one, the shell
shall run in a low power mode and only activate the main program once
the command is fully read in. When an character is received an
interrupt is triggered, parsing should be done in that ISR routine
to leave the program in low power mode – therefore the implementation
used needs to be fast and small.

I thought a finite state machine is good enough for my parsing requirements and this is what I ended up with:

``
/ [...] /

// Control characters.
#define ACK 0x06
#define NACK 0x15

// Limitations for the shell.
#define SHELL_MAX_PARAMETER_LENGTH 32
#define SHELL_MAX_PARAMETERS 6
#define SHELL_MAX_CMD_LENGTH 32

// Shell signals.
#define SHELL_SIGNAL_OK {USART_Send_Data(ACK); USART_Send_String("\r\n");}
#define SHELL_SIGNAL_ERROR {USART_Send_Data(NACK); USART_Send_String("\r\n");}

/ [...] /

// Global variables.

/**
* @var
* The command that shall be executed by the shell.
*/
char Cmd[SHELL_MAX_CMD_LENGTH] = "";

/**
* @var
* An array of arguments for Cmd. Not including the command itself.
*/
char Parameters[SHELL_MAX_PARAMETERS][SHELL_MAX_PARAMETER_LENGTH];

/**
* @var
* The current index in Parameters.
*/
int Parameters_i = 0;

/**
* @var
* Flag th

Here are some things that may help you improve your code.

Simplify the buffer

The code currently has a 32-byte shell length, a maximum of 6 parameters, and a maximum parameter length of 32. This yields a maximum overall command length of 224 bytes (7 * 32). It seems to me that it would be much simpler to simply declare a contiguous buffer of 224 bytes rather than two separate buffers for command and parameters.

Avoid overly similar variable names

The interrupt code has both parameter_i and parameters_i. These only differ by one letter and it's not even the beginning or ending character, making it relatively difficult to spot the difference. The code has Parameters_i and parameters_i which differ only in capitalization. Better would be to ditch the _i suffix (which just makes the variable name harder to read) and rename to parameter_count and parameter_char or something similar. There is also no need to use both local and global variables since main is not running until the interpreter is complete anyway.

Keep tidy as your state machine progresses

The code currently waits to the end to add terminating NUL characters to strings but that should be part of the state machine processing instead. For example, in the CMD state, when any whitespace character is read, the terminating NUL character should immediately be added before moving to the next state.

Use a switch for the state machine

Rather than a long if ... else statement, it would make more sense to use a switch statement for the state machine. This also allows the code to use the default case to handle the problem that the state machine variable does not refer to a valid state. Even if your code is perfect, hardware isn't and an energetic gamma particle could flip a bit in the RAM that holds the state variable, so it's good to gracefully handle such invalid states.

Don't separate actions from states using goto

Having a proliferation of goto statements is usually a sign of bad design. Better would be to eliminate them entirely -- it makes the code easier to follow and less error-prone. Better would be to shorten the actions and put them inline rather than jumping to some other location to perform the action. Alternatively, have two separate state machines -- one for input state and one for actions.

Consolidate redundant variables

The variables NewCommand and Busy appear to be redundant. NewCommand could be eliminated and Busy used instead.

Factor out common code into subroutines

Several places use largely identical code which could be factored out into a routine instead. For example, you could use a subroutine (or at least a macro) to reset the state machine.

Consider implementing the state machine as a data structure

A somewhat more advanced technique is to implement the state machine as a structure. The input character advances the state, and depending on the state, some action ensues. This suggests a possible data structure consisting of state, input_char and action members. By expressing this all as a data structure, the code to implement the state machine can generally be made quite small and fast (desirable for an interrupt) and easy to understand and maintain on the part of the human programmer (desirable for any code).

StackExchange Code Review Q#77540, answer score: 4