1
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ I'm assuming "ARM CMSIS" means a bare metal Cortex M embedded system with microcontroller? Or is this for a high end, PC-like ARM? \$\endgroup\$
    – Lundin
    Dec 3 '20 at 7:43
1
\$\begingroup\$

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.
\$\endgroup\$
5
  • \$\begingroup\$ Thank you very very much for the review and the advice. \$\endgroup\$
    – SL7Bot
    Dec 3 '20 at 20:31
  • \$\begingroup\$ I will use the ARM Cortex M Boards for my project and many of the things will be reworked later on. This is indeed very very helpful to get used to design patterns. I just got into C and this is very useful to better know more professional design patterns. I will change the code accordingly. The while(1); is indeed just a placeholder. especially the advice with the state array is very helpful. I did not really know if this is good coding style. Would you say that the general design is good? \$\endgroup\$
    – SL7Bot
    Dec 3 '20 at 20:46
  • \$\begingroup\$ And really thanks for taking time and writing a review and advice \$\endgroup\$
    – SL7Bot
    Dec 3 '20 at 21:06
  • \$\begingroup\$ @SL7Bot This code that you have written can't be used in bare metal microcontrollers - you have written code for a PC. So my review is assuming that a PC will be used. The advise to use pthreads is made assuming you'd use Linux. \$\endgroup\$
    – Lundin
    Dec 4 '20 at 7:32
  • \$\begingroup\$ Yeah many of the used functions are just as placeholders, like the scanf() I will then rework the whole thing to fit in onto a microcontroller \$\endgroup\$
    – SL7Bot
    Dec 5 '20 at 20:19

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.