Simple state machine

Question

I tried implementing a state machine in C.
This is for a Stratosphere Balloon and I wanted to use a state machine to make it better.
This will be later implemented in ARM CMSIS.

The state function doesn't really do anything yet. And the while(1) loops will be replaced by the sbRepeat transition code.

#ifndef _SB_FSM
#define _SB_FSM

/**
 * Transition codes
 * used to navigate from current to the next state
 */
typedef enum state_transitions {
    sbStartupTrans,
    sbSelfTestTrans,
    sbStartupOK,
    sbErr,
    sbSelfTestOK,
    sbFlightStartOK,
    sbAscendOK,
    sbDescendOK,
    sbRecoveryOK,
    sbEndOK,
    sbAny,
    sbRepeat
}sbTransition;
/**
 * State Definitions
 */
typedef enum states {
    sbIdle,
    sbSelfTest,
    sbStartup,
    sbFlightStart,
    sbAscend,
    sbDescend,
    sbRecovery,
    sbError,
    sbEnd,
} sbState;

/**
 * State Transition definitions
 * when it encounter <trans> move to <state> State
 */
typedef struct state_trans {
    sbTransition trans;
    sbState state;
}sbStateTrans;

/**
 * State Definition
 * @param name - Name of the State
 * @param func - Function Handler of the State
 * @param transitions - Allowed Transition definitions 
 */
typedef struct state_def {
    char name[255];
    sbTransition (*func)(void);
    sbStateTrans transitions[16];
}sbStateDef;

sbTransition sbIdleState(void);
sbTransition sbSelfTestState(void);
sbTransition sbStartupState(void);
sbTransition sbFlightStartState(void);
sbTransition sbAscendState(void);
sbTransition sbDescendState(void);
sbTransition sbRecoveryState(void);
sbTransition sbErrorState(void);
sbTransition sbEndState(void);

sbState fetchNextState(sbState, sbTransition);

/**
 * Structure: {<state name>, <function pointer> , transitions{{<encounters> -> <next state>}}
 */
static sbStateDef states[9] = {
    {"Idle", sbIdleState,                {{sbStartupTrans, sbStartup}, {sbSelfTestTrans, sbSelfTest}}},
    {"Self Test", sbSelfTestState,       {{sbStartupTrans, sbStartup}, {sbErr, sbIdle}}},
    {"Startup", sbStartupState,          {{sbStartupOK, sbFlightStart}, {sbErr, sbIdle}}},
    {"Flight Start", sbFlightStartState, {{sbFlightStartOK, sbAscend}, {sbErr, sbError}}},
    {"Ascend", sbAscendState,            {{sbAscendOK, sbDescend}, {sbErr, sbError}, {sbRepeat, sbAscend}}},
    {"Descend", sbDescendState,          {{sbDescendOK, sbRecovery}, {sbErr, sbError}, {sbRepeat, sbDescend}}},
    {"Recovery", sbRecoveryState,        {{sbRecoveryOK, sbEnd}, {sbErr, sbError}, {sbRepeat, sbRecovery}}},
    {"Error", sbErrorState,              {{sbRepeat, sbError}}},
    {"End", sbEndState,                  {}}
};


#endif

This is the .c file

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "sb_fsm.h"

/**
 * Idle state to take commands
 * @returns sbTransition - Transition code
 */
sbTransition sbIdleState(void) {
    int err = 1;  
    while(1) {
        char *input = malloc(255 * sizeof(char));
        printf("Idle State\n");
        printf("> ");
        scanf("%s", input); 
        getc(stdin);
        if (strcmp(input, "startup") == 0){
            return sbStartupTrans;
        } else if (strcmp(input, "selftest") == 0) {
            return sbSelfTestTrans;
        } else if (strcmp(input, "help") == 0) {
            printf("<startup>   : Initialize the startup procedure\n");
            printf("<selftest>  : Initialize module check\n");
            continue;
        }
        printf("\ntype 'help' to get the commands\n");
    }
}

/**
 * Self Test or Self Diagnosis Check to test if all the 
 * components are working well
 * <br>
 * Returns <sbSelfTestOK> on success,
 * <sbErr> on failure
 * @returns sbTransition - Transition code
 */
sbTransition sbSelfTestState(void) {
    int err = 0;
    while (1) {
        printf("Self Test\n");
        if (err == 0) {
            return sbStartupTrans;
        } else if (err == 1) {
            return sbErr;
        }
    }
}

sbTransition sbStartupState(void) {
    int err = 0;
    while (1) {
        printf("Startup State\n");
        if (err == 0) {
            return sbStartupOK;
        } else {
            return sbErr;
        }
    }
}

sbTransition sbFlightStartState(void) {
    int err = 0;
    while (1) {
        printf("Flight Start\n");
        if (err == 0) {
            return sbFlightStartOK;
        } else {
            return sbErr;
        }
    }
}

sbTransition sbAscendState(void) {
    int err = 0;
    printf("Ascend State\n");
    if (err == 0) {
        return sbAscendOK;
    } else if (err == 1) {
        return sbRepeat;
    } else {
        return sbErr;
    }
}

sbTransition sbDescendState(void) {
    int err = 0;
    while (1) {
        printf("Descend State\n");
        if (err == 0) {
            return sbDescendOK;
        } else {
            return sbErr;
        }
    }
}

sbTransition sbRecoveryState(void) {
    int err = 0;
    while (1) {
        printf("Recovery State\n");
        if (err == 0) {
            return sbRecoveryOK;
        } else {
            return sbErr;
        }
    }
}

sbTransition sbErrorState(void) {
    int err = 0;
    while (1) {
        printf("Error State\n");
        return sbAny;
    }
}

sbTransition sbEndState(void) {
    printf("Accepted\n");
    char *input = malloc(255 * sizeof(char));
    while(1) {
        printf("> ");
        scanf("%s", input);
        getc(stdin); 
        if (strcmp(input, "restart") == 0) {
            return sbEndOK;
        }
    }
    return sbErr;
}


sbState fetchNextState(sbState state, sbTransition ret) {
    
    int i,j;
    i = 0;
    while (i < sizeof(states[state].transitions)/sizeof(sbStateTrans)) {
        if (ret == states[state].transitions[i].trans) {
            return states[state].transitions[i].state;
        }
        i++;
    }
    return sbError;
}

This is the main.c

#include <stdio.h>
#include <stdlib.h>
#include "sb_fsm.h"
#include <unistd.h>

void delay(unsigned long ms);

int main(void) {
    
    sbState curr_state = sbIdle;  
    
    sbTransition (*run_func)(void);
    while (1) {
        sbTransition ret = states[curr_state].func();

        curr_state = fetchNextState(curr_state, ret);
        delay(500);
    }

    return 0;
}

void delay(unsigned long ms) {
    usleep(ms * 1000);
}

Hoping on getting some advice on how to improve it.

Answer 1

Implementing state machines with a table with function pointers is very common. The table should however be read-only and preferably stored in flash. This does however seem to be some PC-like ARM with Linux?

The design with a centralized state transition decision-maker is particularly important and your program is well-written there. Picking next state, based on the result from the previous, should be done at one single place in the program, not in each individual state ("stateghetti programming"). With a centralized decision-maker, you can also easily implement a safe mode for the program where it goes upon certain critical errors.

Regarding the state machine/array:

• typedef enum state_transitions - the enum tag state_transitions is useless and can be removed.
• At the end of sbState enum, include a member for size such as sbStates or what you wish to call it. Then change the states[9] to states[] and static_assert(sizeof states/sizeof *states == sbStates, "states data inconsistent"); to guarantee data integrity.
• Assuming char name[255]; will never change, then 255 bytes per array item is wasteful. A char* to a string literal will save loads of read-only memory over this.
• states should be const.

So you could instead have something like:

static const sbStateDef states[] = {
    [sbIdle] =        {"Idle", sbIdleState,           {{sbStartupTrans, sbStartup}, {sbSelfTestTrans, sbSelfTest}}},
    [sbSelfTest] =    {"Self Test", sbSelfTestState,  {{sbStartupTrans, sbStartup}, {sbErr, sbIdle}}},
    ...
};
static_assert(sizeof states/sizeof *states == sbStates, "states data inconsistent"); // C11

You can do similar with transitions. Overall, designated array initializers like above, that make use of the enum is strongly recommended for readability and data integrity.

General comments:

• Never define variables in header files, ever. If you ever find yourself with the need to define a variable in a header, something has gone fundamentally wrong in your program design. In this case you could probably put both the allocation of the state machine and the calling code in main.c.
• All the while(1) loops mean that you can't change states... I'm assuming this is just place-holders? Such loops are unacceptable in a hosted multi-process system such as Linux. You should probably consider porting this to pthreads and put the thread to sleep when it isn't working.