0

I am writing firmware for a device in C. The Software allows the PC to communicate with this device over the serial interface (UART). The firmware contains multiple layers as following:

  1. Communication layer to send and receive data over UART.
  2. Block layer: This layer enables/disables certain blocks on the device by writing data to the device over UART.
  3. API layer: This contains a sequence of calls to the routines in the block layer. It is used to enable or disable a set of blocks on the device.

My problem is the error handling since in C there are no exceptions. Here is how I've implemented my firmware and I am trying to see if there is a more efficient and compact way to build it while still handling errors efficiently. I want to avoid checking at each layer the status of the call of the lower layer. The code below is very compact and in reality, I have a long sequence of send_uart_commands in the block layer.


// Communication layer

operation_status_t send_uart_command(command_id_t id, command_value_t value)
{
    // Send data over UART
    // Return success if the operation is successful; otherwise failure
}

// Block layer

operation_status_t enable_block1(void)
{
    if (send_uart_command(BLOCK1_COMMAND_1, 10) != operation_success)
           return operation_failure;

    if (send_uart_command(BLOCK1_COMMAND_2, 20) != operation_success)
           return operation_failure;

    // A list of sequences

    if (send_uart_command(BLOCK1_COMMAND_N, 15) != operation_success)
           return operation_failure;

    return operation_success;
}

operation_status_t enable_block2(void)
{
    if (send_uart_command(BLOCK2_COMMAND_1, 1) != operation_success)
           return operation_failure;

    if (send_uart_command(BLOCK2_COMMAND_2, 8) != operation_success)
           return operation_failure;

    return operation_success;
}

// API layer

operation_status_t initialize(void)
{
    if (enable_block1() != operation_success)
           return operation_failure;

    if (enable_block2() != operation_success)
           return operation_failure;

   // A list of calls to the functions in the block layer

    return operation_success;
}

IoT
  • 607
  • 1
  • 11
  • 23
  • What exactly is wrong with the presented code? I'm pretty sure that any reasonable compiler capable of inlining ang squashing gotos will generate an optimal assembly – tstanisl Oct 07 '21 at 13:02
  • @tstanisl My problem is the repetition of error handling that I have to do for each function. – IoT Oct 07 '21 at 13:04
  • 1
    See https://stackoverflow.com/questions/69361515/sequential-execution-of-functions-with-break-option – tstanisl Oct 07 '21 at 13:06
  • @tstanisl Eh I remember that one. There was lots of bad advise all over. I'd read everything there with a great deal of scepticism. – Lundin Oct 07 '21 at 13:17
  • Repetition is not necessarily bad; it can make code much easier to read and debug, than when trying to be "smart". – user694733 Oct 07 '21 at 13:35
  • Most of the code repetition here can be fixed with simple loops and tables though. That's the easiest part to fix. – Lundin Oct 07 '21 at 13:41

2 Answers2

2

One of the many big problems with exception handling like in C++ is that exceptions can crash through all layers like a cannonball. So when you are writing some completely unrelated code, you suddenly get that cannonball in your face: "UART framing error!" When you haven't even touched the UART code...

Therefore "I want to avoid checking at each layer the status of the call of the lower layer" is the wrong primise. Rather you should do like this:

  • Check errors at each layer.
  • Deal with errors as close to the error source as possible.
  • Only forward errors to the caller in case they are actually meaningful to them.
  • You can rename/change type of errors to suit the caller along the way.

For example: "UART framing error" might useful to the code calling the UART driver, but useless to the higher layer application. "Possible incorrect baudrate settings" might be a more relevant error description which you should pass along. Though in some cases you want detailed errors available even at the higher layers.

One reason why you might want that, is that it's common and often good design to have a centralized error handler on the top layer, which can make decisions of state changes, print/log errors etc from one single place in the code. Instead of doing that from all over the place. You'll often find the top layer of a microcontroller application looking something like this:

void main (void)
{
  /* init & setup code called */

  for(;;)
  {
    kick_watchdog(); // the only place in the program where you do this
      
    result = state_machine[state]();
    
    if(result != OK)
    {
      state = error_handler(result);
    }
  }
}

As for your specific code, it looks just fine and mostly doesn't contradict anything of what I've written above. It is always good to return an error code upon error - less confusing than goto, or even worse: massively nested statements and/or loops with error condition flags.

Lundin
  • 195,001
  • 40
  • 254
  • 396
-1

Your code is fine. Actually, it's a bad practice to avoid explicit error checking in C. But if you really want it you could use longjmp. But you should use it with a great care.

This feature allows to jump through the stack skipping arbitrary number of nested calls.

Below you can find an exemplary with mocked up send_uart_command().

#include <setjmp.h>
#include <stdio.h>

jmp_buf env;

void send_uart_command_impl(const char *cmd, int val) {
    static int left = 3;
    if (left-- == 0) {
        printf("%s(%d): failed\n", cmd, val);
        longjmp(env, 1);
    }
    printf("%s(%d): success\n", cmd, val);
}

#define send_uart_command(name, val) send_uart_command_impl(#name, val)

void enable_block1(void) {
    send_uart_command(BLOCK1_COMMAND_1, 10);
    send_uart_command(BLOCK1_COMMAND_2, 20);
    send_uart_command(BLOCK1_COMMAND_N, 15);
}

void enable_block2(void) {
    send_uart_command(BLOCK2_COMMAND_1, 1);
    send_uart_command(BLOCK2_COMMAND_2, 8);
}

int initialize(void) {
    if (setjmp(env)) return -1;
    enable_block1();
    enable_block2();
    return 0;
}

int main() {
    if (initialize() != 0)
        puts("initialize failed");
    else
        puts("initialize success");
}

The program is constructed to fail on the 4th invocation of send_uart_command(). Adjust variable left to select other invocation.

The program logic is very streamlined and it prints the expected output:

BLOCK1_COMMAND_1(10): success
BLOCK1_COMMAND_2(20): success
BLOCK1_COMMAND_N(15): success
BLOCK2_COMMAND_1(1): failed
initialize failed
Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
tstanisl
  • 13,520
  • 2
  • 25
  • 40
  • 1
    Or... you could manually hack the stack frame with inline assembler! I down voted since this teaches very bad practice. Seriously, don't teach people/your worst enemies to use `setjmp`. It makes spaghetti programming with goto look like state of the art code in comparison. And apart from spaghetti there's numerous forms of UB associated with these functions too. [Which functions from the standard library must (should) be avoided?](https://stackoverflow.com/a/46563868/584518) – Lundin Oct 07 '21 at 13:32
  • 1
    Also from a firmware point of view, setjmp can f*** up the program in exceptionally creative ways, such as disabling interrupts, dropping return addresses for interrupts so that they crash in the middle of nowhere when trying to return. Etc etc. Just _don't_ use it. – Lundin Oct 07 '21 at 13:34
  • @Lundin That is why I've added a warning that hiding flow logic is a bad practice, but the answer addresses the OP's question – tstanisl Oct 07 '21 at 13:36
  • 1
    I believe that the purpose of SO isn't just to blindly answer questions come hell or high water, but to point out related problems and steer people towards using good practices and away from bad practices. In this case you are steering them away from fairly decent code to spaghetti code with one of the most infamous language features of the whole C language... – Lundin Oct 07 '21 at 13:40
  • @Lundin, I agree that `setjmp` stuff is dangerous tool. But still if used correctly it can simplify a lot of code. It is useful for jumping out from a deep nested recursive calls. It is extensively used in PSIM simulator, a part of GCC tools. – tstanisl Oct 07 '21 at 13:45
  • Well... You shouldn't have deeply nested recursive calls either. – Lundin Oct 07 '21 at 13:54