Post History

Avoiding checking opcode multiple times in state machine for embedded system

I am implementing a state machine for an electrical relay control board and am trying to follow the principles/structure presented by Lundin [here](https://electrical.codidact.com/posts/295939) for doing that in C.

My machine has currently four states: init, poll, status, and switch. In the poll state the microcontroller waits to receive an opcode via uart and from that determines which state to go to next. My state diagram without error handling is shown below.

![Image_alt_text](https://electrical.codidact.com/uploads/jpuh84mn5517j9g4tbwnui4raves)

Here is a portion of my main function that incorporates the structure of the state machine and state transitions with almost all of the code taken from Lundin's example.

```C
/* main.c */
static cmd_func_t *const state_machine[]  =
{
    [CMD_INIT] = cmd_init,
    [CMD_POLL] = cmd_poll,
    [CMD_SWITCH] = cmd_switch,
    [CMD_STATUS] = cmd_request_status,
};

_Static_assert(sizeof state_machine/sizeof *state_machine == CMD_STATES_N,
		"cmd_state_t and state_machine does not match 1 to 1");

static cmd_state_t evaluate_result(cmd_state_t current_state, cmd_result_t result);
static void error_handler(cmd_result_t error_code);

int main(void)
{
    usart_init(BAUD, USART_PARITY_NONE, USART_STOP_1, 
               USART_BITS_8, USART_SPEED_NORMAL);
    pin_mode(&D24, OUTPUT); /* Enable indication LED */
    static cmd_state_t state = CMD_INIT;
    cmd_result_t result;

    for(;;)
    {
        result = state_machine[state]();	/* Execute function */
        state = evaluate_result(state, result); /* Change state */
    }
}

static cmd_state_t evaluate_result(cmd_state_t current_state, cmd_result_t result)
{
	cmd_state_t next_state = current_state;

	switch(current_state)
	{
		case CMD_INIT:
			if(result == CMD_OK)
			{
				next_state = CMD_POLL;
			}
			break;

		case CMD_POLL:
			if(result == CMD_OK)
			{
				next_state = CMD_SWITCH;
			}

/* More below */
```

One problem that I face, is that I'm checking the opcode that I receive twice. One time in order to determine which state to transition to from CMD_POLL. And once again in CMD_SWITCH when I need to determine if a relay or an entire channel of relays should be turned on/off. First check is in my `cmd_poll()` function.

```C
/* cmd.c */
static uint8_t received_opcode = 0x00u;
static uint8_t switch_number = 0u;

cmd_result_t cmd_poll(void)
{
	if(usart_available() > 0u)
	{
		received_opcode = usart_rx();
		switch_number = usart_rx();

		switch(received_opcode)
		{
			case OPCODE_CHANNEL_ON:
				return CMD_OK;
			case OPCODE_CHANNEL_OFF:
				return CMD_OK;
			case OPCODE_RELAY_ON:
				return CMD_OK;
			case OPCODE_RELAY_OFF:
				return CMD_OK;
			case OPCODE_STATUS:
				return CMD_REQUEST;
			default:
				return CMD_UNRECOGNIZED_OPCODE;
		}
		return CMD_UNEXPECTED;
	}
	else
	{
		return CMD_IDLE;
	}
	return CMD_UNEXPECTED; //Should never be reached
}
```

Next check is in my `cmd_switch()` function:

```C
/* cmd.c */
cmd_result_t cmd_switch(void)
{
    if(received_opcode == OPCODE_CHANNEL_ON)
    {
        /* Do something */
    }
    else if(received_opcode == OPCODE_CHANNEL_OFF)
    {
        /* Do something else*/
    }
    else if(...)
```

Is there a smarter way to do this such that I avoid this redundancy of having to check the opcode twice? Preferably a way that is MISRA compliant as well.