Welcome to Software Development on Codidact!
Will you help us build our independent community of developers helping developers? We're small and trying to grow. We welcome questions about all aspects of software development, from design to code to QA and more. Got questions? Got answers? Got code you'd like someone to review? Please join us.
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 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.
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.
/* 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.
/* 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:
/* 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.
1 answer
You can replace it with a look-up table ("LUT").
There's a common manual optimization technique which we had to use a lot back in the days. For any switch applied to an enum where the enum values are in sequence (ideally from 0 to n, or at least adjacent), it can be replaced with a branch-free look-up table.
That is, instead of a switch or a long if else if, you can do a single look-up table check. That is, if we take that enum from my Q&A and modify it:
typedef enum
{
COFFEE_OK,
COFFEE_IDLE,
COFFEE_NO_BEANS,
COFFEE_NO_WATER,
COFFEE_UNEXPECTED,
COFFEE_N // lets add this one to the end
} coffee_result_t;
(Or in your case I guess the name might be OPCODE_N.)
Then for a sequential enum COFFEE_N equals the number of enumerations supported.
Similarly as I designed a function-pointer jump table in that Q&A, we can also do a data look-up table in that same manner. Create an array (in flash) that contains the outcome for all possible scenarios pre-programmed. Then we don't have to check it in run-time.
static const uint8_t OPCODE_results [OPCODE_N] =
{
[OPCODE_STATUS] = CMD_REQUEST,
};
Done. Huh how can this tiny bit code replace that whole switch/if else if?
In that Q&A, I mentioned that having the "OK" value as number zero is a great idea because that's the default implicit initializer.
What's important here is that the first state in an enum like this should correspond to OK/no errors, with the value 0. This is also a de facto standard way to write programs, so that any non-zero value corresponds to a specialized status/error code.
And so we see why that is so nice - now we don't have to write all of this code:
[OPCODE_CHANNEL_ON] = CMD_OK,
[OPCODE_CHANNEL_OFF] = CMD_OK,
[OPCODE_RELAY_ON] = CMD_OK,
[OPCODE_RELAY_OFF] = CMD_OK,
Because those are already set to zero by the default initialization of OPCODE_results and zero ought to equal CMD_OK. So we just have to create a look-up table where we explicitly initialize all the "not OK" scenarios.
And with that done, we can replace the whole switch with a simple
return OPCODE_results[received_opcode];
where received_opcode is assumed to be verified and sanitized in advance. This is pretty much entirely branch-free and if you keep reusing that same OPCODE_results[received_opcode] multiple times inside a function without changing received_opcode, then it will likely just get stored in a register. So if you check it twice - no biggie.
But obviously look-up tables are execution speed over memory use optimizations. But for MCUs you generally want to do execution speed > RAM use > flash use, where you got a ton of flash anyway.
However, please note that modern optimizing compilers actually do this very optimization internally and probably go further still. So you shouldn't replace switch with a look-up table to increase performance, but rather to make the code more readable and compact. Which may or may not be the case.
Also, a MCU which controls a bunch of relays doesn't sound like anything high-end with cache and branch prediction, so you probably don't need to take branching in consideration. But compile-time calculations over run-time do save some CPU no matter what MCU that is used.
switch does have the advantage of default in case you do want some defensive programming there even when the data is sanitized. Defensive programming is encouraged by MISRA C - always have a trailing else after if else if and a trailing default in switch. The equivalent of that in the look-up table version is the check that the received_opcode is in range of 0 to OPCODE_N.
0 comment threads
1 comment thread