State machine implementation

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

#include 
#include 

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;
}

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.

StackExchange Code Review Q#104989, answer score: 3