State machine implementation

Question

I have written a simple state machine. Comments and feedback are welcome.

#include <stdio.h>
#include <stdlib.h>


typedef enum Event
{
    eEVENT_X,
    eEVENT_Y,
    eEVENT_Z,
}Event;

typedef enum States
{
    eSTATE_X,
    eSTATE_Y,
    eSTATE_Z
}States;

int e = eEVENT_X;
int s = eSTATE_X;

struct state_transition
{ 
  Event EventId;
  States StateId;
  void (*function)(void); //call back utility 
};

#define NSTATES 3
#define NEVENTS 3
#define STATE_END 3
#define EVENT_END 3

void function_eXsX(void)
{ 
    printf("\n xx \n");
    e = eEVENT_X;
    s = eSTATE_Y;

}
void function_eXsY(void)
{ 
    printf("\n xy \n");
    e = eEVENT_X;
    s = eSTATE_Z;
}

void function_eXsZ(void)
{ 
    printf("\n xz \n");
    e = eEVENT_Y;
    s = eSTATE_X;
}
void function_eYsX(void)
{ 
    printf("\n yx \n");
    e = eEVENT_Y;
    s = eSTATE_Y;
}
void function_eYsY(void)
{ 
    printf("\n yy \n");
    e = eEVENT_Y;
    s = eSTATE_Z;
}
void function_eYsZ(void)
{ 
    printf("\n yz \n");
    e = eEVENT_Z;
    s = eSTATE_X;
}
void function_eZsX(void)
{ 
    printf("\n zx \n");
    e = eEVENT_Z;
    s = eSTATE_Y;
}
void function_eZsY(void)
{ 
    printf("\n zy \n");
    e = eEVENT_Z;
    s = eSTATE_Z;
}
void function_eZsZ(void)
{ 
    printf("\n zz \n");
    e = EVENT_END;
    s = STATE_END;

}



struct state_transition stateTrans[NEVENTS][NSTATES] = 
{    
      {{eEVENT_X,eSTATE_X,function_eXsX},{eEVENT_X,eSTATE_Y,function_eXsY},{eEVENT_X,eSTATE_Z,function_eXsZ}},
      {{eEVENT_Y,eSTATE_X,function_eYsX},{eEVENT_Y,eSTATE_Y,function_eYsY},{eEVENT_Y,eSTATE_Z,function_eYsZ}},
      {{eEVENT_Z,eSTATE_X,function_eZsX},{eEVENT_Z,eSTATE_Y,function_eZsY},{eEVENT_Z,eSTATE_Z,function_eZsZ}},
};



int main(int argc,char *argv[])
{

    while(1)
    {
        stateTrans[e][s].function();
        if(e == STATE_END && s == EVENT_END)
            break;
    }
    return 0;
}

Answer 1

I see some things that may help you improve your code.

Avoid global variables

While you might only currently have a need for a single instance of your state machine, a better approach would eliminate the global variables. Instead, it would make more sense to me to have each function return the new (end) state, or even better, have the state_transition contain the transitions implicitly. Which you choose will depend on details of the real state machine. For example, if it's a state machine for a communications protocol, the event might be "received a message header" and the next state may depend on whether the message header is valid or not.

Make your state table complete

The STATE_END and EVENT_END are not represented within the state table. The problem with that is that anyone reading the code might easily overlook those two possibilities because they're not in the state table. On the other hand, the EventId and StateId members are never used and should be omitted.

Use the appropriate types

The s and e variables are both declared int but they're really not. They should instead be States and Event types.

Chose better names

The event enum is called Event (singular) but the state enum is called States (plural) which is confusing. I'd recommend using the singular form for both names.

Simplify your typedefs

In this case, there is no need to repeat the name Event or the name State in the typedef. You can make it an anonymous enum with a typedef:

typedef enum 
{
    eEVENT_X,
    eEVENT_Y,
    eEVENT_Z,
    NEVENTS
} Event;

Note that this construction makes this line unnecessary:

#define NEVENTS 3

That makes it a little easier to maintain the tables.

Implement range checking

What happens if some energetic gamma ray flips a bit in your embedded device's RAM and the value for the state goes from 1 to 129? The answer is that the code will use whatever arbitrary code is that far off the end of the state table. It's prudent instead to have a range checking function wrapper so that bad values (which might more plausibly be a result of a program error somewhere) will not cause the system to crash. Here's a range checking function that could be used:

bool applyEvent(State *s, Event *e) 
{
    if (*s >= NSTATES || *e >= NEVENTS)
        return false;
    *s = stateTrans[*e][*s].function(e);
    return true;
}

Note that it also assumes that each function is of this form:

State function_eZsY(Event *e)
{ 
    printf("\n zy \n");
    *e = eEVENT_Z;
    return eSTATE_Z;
}

Using this, there are no longer any global variables and the main function can look like this:

int main()
{
    State s = eSTATE_X;
    Event e = eEVENT_X;
    while(applyEvent(&s, &e))
    {
    }
}

Which brings us to the last point:

Eliminate return 0 at the end of main

Since C99, the compiler automatically generates the code corresponding to return 0 at the end of main so there is no need to explicitly write it.

Answer 2

For a state machine I expect the "event" to be independent of the current state (coming from an input).

This means I would expect the transition function to take an Event parameter.

I don't like the use of globals. Instead put all the mutable state in function scope. This means that the new state should be returned from the transition function.

The signature then becomes: State (*function)(Event) and the transition function then uses a switch to decide which state is the next one.

main then becomes:

int main(int argc,char *argv[])
{
    State s = eSTATE_X;
    while(s != STATE_END)
    {
        Event e = //read from the input
        s = stateTrans[s].function(e);
    }
    return 0;
}

Though just the state is a bit limited. Most real-life applications need additional state that isn't expressed as a single int. That means adding more input into the transition function that reads and manipulates a struct.

Answer 3

Thanks for the comments from @Edward, I have re-written the code ..I have posted the code here , I will later post a simple code with proper state diagrams ,

#include <stdio.h>

typedef enum
{
    eFALSE,
    eTRUE,
}BOOL;

typedef enum 
{
    eEVENT_X,
    eEVENT_Y,
    eEVENT_Z,
    NEVENTS
}Event;

typedef enum 
{
    eSTATE_X,
    eSTATE_Y,
    eSTATE_Z,
    NSTATES
}State;


struct state_transition
{ 
  State (*function)(Event *e); 
};



State function_eXsX(Event *e)
{ 
    printf("\n xx \n");
    *e = eEVENT_X;
    return eSTATE_Y;

}
State function_eXsY(Event *e)
{ 
    printf("\n xy \n");
    *e = eEVENT_X;
    return eSTATE_Z;
}

State function_eXsZ(Event *e)
{ 
    printf("\n xz \n");
    *e = eEVENT_Y;
    return eSTATE_X;
}
State function_eYsX(Event *e)
{ 
    printf("\n yx \n");
    *e = eEVENT_Y;
    return eSTATE_Y;
}
State function_eYsY(Event *e)
{ 
    printf("\n yy \n");
    *e = eEVENT_Y;
    return eSTATE_Z;
}
State function_eYsZ(Event *e)
{ 
    printf("\n yz \n");
    *e = eEVENT_Z;
    return eSTATE_X;
}
State function_eZsX(Event *e)
{ 
    printf("\n zx \n");
    *e = eEVENT_Z;
    return eSTATE_Y;
}
State function_eZsY(Event *e)
{ 
    printf("\n zy \n");
    *e = eEVENT_Z;
    return eSTATE_Z;
}
State function_eZsZ(Event *e)
{ 
    printf("\n zz \n");
    *e = NSTATES;
    return NSTATES;

}



struct state_transition stateTrans[NEVENTS][NSTATES] = 
{    
      {function_eXsX,function_eXsY,function_eXsZ},
      {function_eYsX,function_eYsY,function_eYsZ},
      {function_eZsX,function_eZsY,function_eZsZ},
};

BOOL applyEvent(State *s, Event *e) 
{
    if (*s >= NSTATES || *e >= NEVENTS)
        return eFALSE;
    *s = stateTrans[*e][*s].function(e);
    return eTRUE;
}


int main(int argc,char *argv[])
{
    State s = eSTATE_X;
    Event e = eEVENT_X;
    while(applyEvent(&s, &e));
}