2
\$\begingroup\$

This is a follow-up of: Very basic shell on microcontroller in C

A few days back I've asked for a review on a simple shell parser in c. Based on the answers, I've restructured my code to use a table-based Mealy machine.

shell.h

#ifndef SHELL_H
#define SHELL_H

#include <stdio.h>
#include "usart.h"
#include <string.h>
#include <intrinsics.h>
#include <io430x22x4.h>

// Shell limits.
#define SHELL_MAX_ARGUMENT_LENGTH 32
#define SHELL_MAX_ARGUMENTS       10

extern char Argv[SHELL_MAX_ARGUMENTS][SHELL_MAX_ARGUMENT_LENGTH];
extern int Argc;
extern int NewCommand;

// Functions.
void shell_error();
void shell_reset();
void shell_ok();
void shell_process_char(char current_char);

#endif

shell.c

/**
 * @file
 *  A Mealy machine that extracts ARGV and ARGC from input.
 */

#include "shell.h"

#define ACK 0x06
#define NACK 0x15

/**
 * Possible input tokens for the machine.
 */
enum TTokens {
    LITERAL = 0,
    WHITESPACE,
    BACKSLASH,
    QUOTE,
    LINEEND,
    TTOKENS_LENGTH
};

/**
 * Possible states the machine can be in.
 */
enum TStates {
    ARGUMENT,
    ARGUMENT_BACKSLASH,
    ARGUMENT_QUOTE,
    ARGUMENT_QUOTE_BACKSLASH,
    TSTATES_LENGTH
};

/**
 * Possible actions the machine can perform on transitions.
 */
enum TActions {
    NOTHING,
    NEXT_ARGUMENT,
    NEXT_CHAR,
    CALL,
    TACTIONS_LENGTH
};

/**
 * @var
 *  Lookup table to convert from characters to input token for the machine.
 */
const int CharacterMap[128] = {
    // Not explicit stated characters get initialized with 0
    // and therefore become LITERAL.
    /* [0 ... 127]  = LITERAL */

    ['\\']  = BACKSLASH,
    ['"']   = QUOTE,
    [' ']   = WHITESPACE,
    ['\t']  = WHITESPACE,
    ['\r']  = LINEEND,
    ['\n']  = LINEEND,
    ['\0']  = LINEEND,
    ['\4']  = LINEEND
};

/**
 * @var
 *  Lookup table that defines state transitions and actions to perform on
 *  transition.
 */
const int Transitions[TSTATES_LENGTH][TTOKENS_LENGTH][2] = {
    [ARGUMENT] = {
        [LITERAL]      = {ARGUMENT, NEXT_CHAR},
        [WHITESPACE]   = {ARGUMENT, NEXT_ARGUMENT},
        [BACKSLASH]    = {ARGUMENT_BACKSLASH, NOTHING},
        [QUOTE]        = {ARGUMENT_QUOTE, NOTHING},
        [LINEEND]      = {ARGUMENT, CALL}
    },
    [ARGUMENT_QUOTE] = {
        [LITERAL]      = {ARGUMENT_QUOTE, NEXT_CHAR},
        [WHITESPACE]   = {ARGUMENT_QUOTE, NEXT_CHAR},
        [LINEEND]      = {ARGUMENT_QUOTE, NEXT_CHAR},
        [BACKSLASH]    = {ARGUMENT_QUOTE_BACKSLASH, NOTHING},
        [QUOTE]        = {ARGUMENT, NOTHING},
    },
    [ARGUMENT_BACKSLASH] = {
        [LITERAL]      = {ARGUMENT, NEXT_CHAR},
        [WHITESPACE]   = {ARGUMENT, NEXT_CHAR},
        [BACKSLASH]    = {ARGUMENT, NEXT_CHAR},
        [QUOTE]        = {ARGUMENT, NEXT_CHAR},
        [LINEEND]      = {ARGUMENT, NEXT_CHAR}
    },
    [ARGUMENT_QUOTE_BACKSLASH] = {
        [LITERAL]      = {ARGUMENT_QUOTE, NEXT_CHAR},
        [WHITESPACE]   = {ARGUMENT_QUOTE, NEXT_CHAR},
        [BACKSLASH]    = {ARGUMENT_QUOTE, NEXT_CHAR},
        [QUOTE]        = {ARGUMENT_QUOTE, NEXT_CHAR},
        [LINEEND]      = {ARGUMENT_QUOTE, NEXT_CHAR}
    },
};

/**
 * @var
 *  The arguments array of the shell.
 */
char Argv[SHELL_MAX_ARGUMENTS][SHELL_MAX_ARGUMENT_LENGTH] = {0};

/**
 * @var
 *  The number of arguments that have been read in.
 */
int Argc = 1;

/**
 * @var
 *  Flag, that signals if a new command has been read in.
 */
int NewCommand = 0;

/**
 * @var
 *  The current state of the machine.
 */
enum TStates State = ARGUMENT;

/**
 * @var
 *  The current index in Argv[Argc -1].
 */
int ArgumentIndex = 0; 

/**
 * Process next character from the input.
 */
void shell_process_char(const char current_char)
{
    // Check that current_char is in ASCII range.
    if ((current_char < 0) || (current_char > 127)) {
        shell_error();
        return;
    }

    // Progress state and get action.
    const int* transition = Transitions[State][CharacterMap[current_char]];
    State = (enum TStates) (transition[0]);
    enum TActions action =  (enum TActions) (transition[1]);

    // Run action.
    switch (action)
    {
        case NEXT_CHAR:
            if (ArgumentIndex < SHELL_MAX_ARGUMENT_LENGTH) {
                Argv[Argc - 1][ArgumentIndex++] = current_char;
            } else {
                shell_error();
            }
            break;

        case NEXT_ARGUMENT:
            if (Argc < SHELL_MAX_ARGUMENTS) {
                Argc++;
                ArgumentIndex = 0;
            } else {
                shell_error();
            }

            break;

        case CALL:
            // Signal that a new command is received.
            NewCommand = 1;

            break;
    }
}

/**
 * Reset the machine.
 */
void shell_reset()
{
    State = ARGUMENT;

    // Reset counters.
    Argc = 1;
    ArgumentIndex = 0;

    // Refill Argv with 0.
    memset(Argv, '\0', (SHELL_MAX_ARGUMENTS * SHELL_MAX_ARGUMENT_LENGTH));

    // Reset new Command flag.
    NewCommand = 0;
}

/**
 * Signal that an error occured.
 */
void shell_error()
{
    shell_reset();

    USART_Send_Data(NACK);
    USART_Send_String("\r\n");
}

/**
 * Signal that the command has been successfully executed.
 */
void shell_ok()
{
    USART_Send_Data(ACK); 
    USART_Send_String("\r\n");
}

main.c

/** [...] **/

// Limit of input buffer.
#define INPUT_BUFFER_LENGTH 256

/**
 * @var
 *  Buffer for input.
 */  
char InputBuffer[INPUT_BUFFER_LENGTH] = {0};

/**
 * @var
 *  Index for writing into the input buffer.
 */
int InputBufferWriteIndex = 0;

/**
 * @var
 *  Index for reading from the input buffer.
 */
int InputBufferReadIndex = 0;

/**
 * Interrupt that gets triggered when a new character is read in.
 */
#pragma vector=USCIAB0RX_VECTOR
__interrupt void USCIAB0RX_ISR(void)
{
    // Save character in input buffer.
    InputBuffer[InputBufferWriteIndex] = USART_RxData_Reg;
    fUSART_Receive_Set;

    // Progress input buffer.
    InputBufferWriteIndex = (InputBufferWriteIndex + 1) % INPUT_BUFFER_LENGTH;

    // Wakeup main programm to process character.
    LPM3_EXIT;

    IFG2 &= ~0x01;  // Clear interrupt received flag.
}

/**
 * A sample main function.
 */
void main()
{
    /* [...] (hardware initialization) */

    while(1) {
        // Go into low power mode
        // and wait for interrupt to wake up the main program.
        LPM3;


        // Process the input buffer.
        for (
             ;
             InputBufferReadIndex != InputBufferWriteIndex;
             InputBufferReadIndex = (InputBufferReadIndex + 1) % INPUT_BUFFER_LENGTH
        ) {
             shell_process_char(InputBuffer[InputBufferReadIndex]);
        }

        // For demonstrational purposes just output all arguments
        // as CSV.
        if (NewCommand) {
             for (int i = 0; i < Argc; i++) {
                 // Seperate arguments with ",".
                 if (i != 0) {
                     USART_Send_String(",");
                 }

                 // Print current argument.
                 USART_Send_String(Argv[i]);
             }
             USART_Send_String("\r\n");
        }

        // Reset shell after processing arguments.
        shell_reset();
    }
}

/** [...] **/
\$\endgroup\$
  • 1
    \$\begingroup\$ Hello, what does /** [...] **/ mean ? \$\endgroup\$ – bhathiya-perera Jan 16 '15 at 8:08
  • \$\begingroup\$ It means that there is code that does other things but which is not of interest to the question. \$\endgroup\$ – Gellweiler Jan 16 '15 at 8:09
  • 2
    \$\begingroup\$ As you were advised for the previous version, the interrupt should not process the command. Is should do the minimum required to service the interrupting device - in this case just accumulate user input into a string. This may seem unimportant for your example code, but it is a terrible precedent to set. Embedded systems, especially those that have realtime constraints, rely on interrupt latencies that are as short and predictable as possible. What you were told before (read blocking on device input) is optimal. \$\endgroup\$ – William Morris Jan 17 '15 at 0:38
3
\$\begingroup\$

It is good that you have now simplified the processing done at interrupt level but I think it is unnecessary to wake the process for every character. It would be more normal for the ISR to accumulate the input string and to wake the process only when a \n is received or the buffer is full. Parsing a whole line is much easier than processing char by char and maybe makes the state machine redundant. For that reason I didn't give much attention to the state machine.

Some other things that I notice are, in no particular order:

  • lack of void parameter lists where functions take no parameters.

  • lack of static for functions and variables that are local to one translation unit.

  • macros that are functions should be written so that they can be called with brackets, e.g.

    LPM3();
    

    without seeing the definitions of these macros I can't tell just how bad they are. But they are certainly not right. Functions are usually preferred to macros.

  • variables that are used by both the ISR and by the process should be marked volatile to tell the compiler that it cannot optimize away access to the physical location of the variable.

  • some variable names are verbose, e.g. InputBufferWriteIndex. Others have a leading capital letter, which is not normal (leading capitals are widely used for type names).

  • your argv array and its related argc are global but probably should not be.

EDIT

Global variables should be used as little as possible so having a naming convention for them seems unnecessary to me - as there should be so few. The convention of using a leading upper case for types is not universal. I can't say your PascalCase is bad or wrong (although we are writing C, not Pascal) - consistency is more important; your opinion is as good as mine. When you work in a team, the team or company should define its own rules anyway, so your or my preference is of less significance.

Omitting void is wrong. I can call a function without a parameter list with any number of parameters and the compiler doesn't know whether I have made a mistake or not. Generally a good compiler will warn you if you miss out the parameter list.

As for InputBufferWriteIndex you might use InputBufferIn or InBufferIn or even just BufferIn if there is no output buffer. But I often get names too short, so maybe others will have something better to say.

\$\endgroup\$
  • \$\begingroup\$ Thank you for you feedback. I tend to write global variables in PascalCase to distinguish them from local variables for which I use underscore notation, isn't that good practice? Also I always thought using void in parameter list is a matter of style – or is there any benefit from explicitly writing void? What name would you suggest instead of InputBufferWriteIndex? \$\endgroup\$ – Gellweiler Jan 18 '15 at 0:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.