4
\$\begingroup\$

I am developing a Firmware and I am using 3 layers: Driver, Platform and Application. In order to handle the communication between these layers, I am using an approach where each function will return a success/failure flag:

typedef enum {
  FW_ERROR = 0,
  FW_OK    = 1
}FWStatusTypeDef;

Every function in my FW looks like this:

FWStatusTypeDef function1(void)
{
  /* Do some stuff */
  ..
  ..

  if(something_wrong_happens)
    return FW_ERROR;
  return FW_OK;
}


FWStatusTypeDef function2(void)
{
  /* Do some stuff */
  ..
  ..
  if(something_wrong_happens)
    return FW_ERROR;

  return FW_OK;
}

The approach I am using is good for debugging but when running the application, I want to identify which function is sending an error flag. In other words, I want to keep track of errors and display them in a screen

Example:

FWStatusTypeDef OpenTheDoor(void)
{
  if (!KeyIsAvailable())
  {
    return FW_ERROR;
  }
  Door->Open();

  if (!DoorIsOpen())
  {
     return FW_ERROR;
  }

  return FW_OK;
}

FWStatusTypeDef KeyIsAvailable(void)
{
  return GetKeyStatus();
}

FWStatusTypeDef DoorIsOpen(void)
{
  return GetDoorStatus();
}

In this example, when I call OpenTheDoor() and it returns FW_ERROR. I won't be able to know if the problem is that the key is missing or there was another problem while trying to open the door (for example a cow is blocking it because it hates me).

I've been thinking and I came up with this solution: An Error code handler.

typedef enum {
  ERROR_1 = 0,
  ERROR_2 = 1,
  ERROR_k = k, // A cow is blocking the door
  ERROR_n = n  // The key is missing
}ErrorIdTypeDef;

static ErrorIdTypeDef ErrorCode;

// Getter
ErrorIdTypeDef GetErrorCode(void)
{
  return ErrorCode;
}

// Setter
void SetErrorCode(ErrorIdTypeDef code)
{
  ErrorCode = code;
}

Each function will use the error handler exposed Setter.

FWStatusTypeDef OpenTheDoor(void)
{
  if (!KeyIsAvailable())
  {
    SetErrorCode(ERROR_n);
    return FW_ERROR;
  }
  Door->Open();

  if (!DoorIsOpen())
  {
     SetErrorCode(ERROR_k);
     return FW_ERROR;
  }

  return FW_OK;
}

FWStatusTypeDef KeyIsAvailable(void)
{
  return GetKeyStatus();
}

FWStatusTypeDef DoorIsOpen(void)
{
  return GetDoorStatus();
}

In the application layer, I read the error code using the exposed getter API if an error occurs:

if (!OpenTheDoor())
{
  ErrorIdTypeDef theError;
  theError = GetErrorCode();
  switch(theError)
  {
    case ERROR_k:
      DisplayOnTheScreen("A cow is blocking the door.");
      break;

    case ERROR_n:
      DisplayOnTheScreen("You don't have a key.");
      break;

    ...
    ...

    default:
      break;
  }
}
else
{
  DisplayOnTheScreen("Door is open");
}

Now, my questions are not about the syntax, they're more about "good practices". Does this approach have any limitation ? Is there another known practice to get the same result ? In other words, Am I complicating things here ?

\$\endgroup\$
  • 5
    \$\begingroup\$ I don't think this is the right place for this question, yes asking about embedded firmware is on topic, but your question might be quite fitting on code review or some other more general programming SE- as it is nothing inherently embedded about it. Just point out some of the limitations (no exceptions etc.) and I'd guess you'd get better answers there. \$\endgroup\$ – Arsenal Feb 19 at 15:06
  • \$\begingroup\$ I'd ask if you really need to have that much error handling at the TOP level. I think you should be handling it at the levels you care about. IE, in the platform layer, do something specific to a specific error in the driver layer (USB Buffer underflow, better do something). The platform reports up to the application layer that there was an issue in the driver, but the applications layer can't really do anything to fix it. Think about what layer can do what action. \$\endgroup\$ – yhyrcanus Feb 19 at 18:37
  • \$\begingroup\$ @yhyrcanus The thing is: I need to display error messages on the screen. The user needs to know whats going on behind. Think about it like a smartphone. you can't just handle errors in the back and display "ERROR" to the user. You need to tell him where the problem comes from. \$\endgroup\$ – Pryda Feb 20 at 9:08
  • 2
    \$\begingroup\$ Any time you ask a best practices question or best anything question the answer is "primarily opinion based", as there is no such thing as a general best practice or best anything. Too many factors, in rare occasions you can make a short list of pretty goods, but each will beat out one of the others "depending" on other factors. And sometimes they will tie with no single one being "best". \$\endgroup\$ – old_timer Feb 20 at 13:44
  • 1
    \$\begingroup\$ In that case you just need to say "device not found; check connections." You don't need to say the very specific "device on Chip select 5 didn't respond to a SPI transaction." I'm also not saying don't make the detailed error message unreachable for debugging purposes. Just consider how each layer will use it. That is, your application layer shouldn't care if it's SPI or I2C or whatever. The answers here are great (ie use enums). The only thing I'd add is bitshifts/masks are your friend when you can run into multiple errors. \$\endgroup\$ – yhyrcanus Feb 20 at 16:36
3
\$\begingroup\$

Once the project complexity increases, I tend to develop embedded firmware like so:

  1. First, as you did, I set up a typedef to contain the "return status" values of any exposed functions:

    typedef enum
    {
        FLASH_SUCCESS = 0,
        FLASH_IS_BUSY,
        FLASH_BAD_ADDRESS,
        FLASH_DATA_TOO_LARGE,
        (etc)
    } flashReturn_t
    
  2. I have the calling routines handle any of these responses. Early in development, I simply call an error handling function which blinks an LED at me in an endless loop. The blinking pattern tells me which error happened. (My projects generally don't have convenient human-readable output devices...)

  3. As development progresses, the different error cases become managed in more appropriate ways. Eventually, there is nothing left to call the error handler and so it can be removed.

I like this because:

  1. It lets me work on larger program flow first, and fine tune the error handing later.
  2. It provides a running list of error cases I need to work on, and
  3. At any point, I could choose to call any remaining (unhandled) error cases "good enough", turn on a watchdog timer, and release the project.

In the unlikely event that an unexpected error happens, the WDT will timeout during the error handler loop and the system will reset. Which, of course, is what a WDT is supposed to do!

My error handler is set up like this:

typedef enum
{
    ERR_CC1101_TXFIFO_EMPTY = 1,
    ERR_CC1101_TXFIFO_FULL,
    ERR_CC1101_RXBYTES_TIMEOUT,
    ERR_CC1101_TXBYTES_TIMEOUT,
    ERR_FLASH_CORRUPTED,
    ERR_FLASH_CAL_BAD_DATA,
    ERR_STAT_EVENT_QUEUE_FULL,
    ERR_PB_INVALID_TYPE,
    ERR_IMU_INIT_FAIL,
    ERR_IMU_BAD_CONFIG
} errortype_t;

void ErrorHandler(errortype_t Error);

Notice that I've grouped errors by subsystem. Also, the first entry =1 (not 0) because those indexes are what causes my LED to flash. It doesn't help to flash zero times :)

\$\endgroup\$
  • 1
    \$\begingroup\$ "Notice that I've grouped errors by subsystem. Also, the first entry =1 (not 0) because those indexes are what causes my LED to flash. It doesn't help to flash zero times :)" Well, you could have something like ERR_NONE for no errors at index 0. And flash zero times for zero errors. \$\endgroup\$ – Lundin Feb 20 at 10:19
  • \$\begingroup\$ @Lundin True! But my routine is a dead-end in code, basically just an infinite loop with some indications. So in my case, I'd never call that routine unless there was something very wrong. \$\endgroup\$ – bitsmack Feb 20 at 19:29
3
\$\begingroup\$

Using enums for error codes is common practice. The whole purpose of using enum over for example bool is to get more information.

The pattern you suggest with a common "last error" handler is not recommended though. This has been tried historically many times, never with good results. Must infamously the Windows API GetLastError() function.

The problem with "last error" handlers are several: they are not thread/interrupt safe and they only remember the last error. If you set FW_ERROR_KEY from a function, from the time that happens until you print the error, another error could have happened. You then print the wrong reason and get the wrong diagnostic.

The common way to handle errors is instead rather something like this:

for(;;)
{
  kick_watchdog();

  result = state_machine[state]();

  if(result != OK)
  {
    error_handler(result);
  }
}

That's what the main loop looks like in many bare metal MCU projects. All errors are passed down from drivers up to the application layer, and error handling is centralized.

The application layer might then make the call to ignore certain errors, or handle them, or replace their error codes with another code. Etc.

\$\endgroup\$
  • \$\begingroup\$ Is it considered a code duplication to separate the state machine and error codes ? for example: CurrentState->Set(STATE_NOT_READY); if (!(MyDriver->Init(&initStruct) != FW_OK)) { ErrorHandler->Set(ERROR_CODE_INIT_FAILED); return FW_ERROR; } CurrentState->Set(STATE_READY); \$\endgroup\$ – Pryda Feb 20 at 10:25
  • 1
    \$\begingroup\$ @Pryda Not really. It is good design to centralize the code picking the next state to one single place in the program, to avoid "stateghetti". This can very well be merged with the error handler, so that one central piece of code makes a call on which state to execute next, based on results of the previous one. For example this is how safety-critical systems work: revert to a safe state in case of errors. \$\endgroup\$ – Lundin Feb 20 at 10:32
  • \$\begingroup\$ So. Let me get this straight. You decry Try/Catch blocks but you have basically coded a Try/Catch block, by hand, as your example of good error handling? Some people see the high-level patterns, other people just get caught up in the details... \$\endgroup\$ – Edgar Brown Feb 20 at 16:12
  • \$\begingroup\$ @EdgarBrown Do you know the difference between return from sub-routine and non-conditional branches? If so, you'll know that this is not exception handling, as there is no spaghetti code of any kind present. The C equivalent to C++ exception handling are the horrible setjmp/longjmp. \$\endgroup\$ – Lundin Feb 20 at 16:17
  • \$\begingroup\$ Not being able to understand or properly use the mechanisms provided by a programming language or operating system is no excuse. setjmp/longjmp is a far cry from the considerably cleaner and better structured Try/Catch blocks. Likewise, anyone that deals with general exceptions, reentrancy, interrupts, multiple threads, parallelism, semaphores, and mutexes should have absolutely no problem properly dealing with the comparatively trivial concepts behind a Try/Catch. No need for spaghettification. But, in its simplest from, a Try/Catch just does exactly what you wrote in your code. \$\endgroup\$ – Edgar Brown Feb 20 at 16:32
0
\$\begingroup\$

You will not get a definitive answer to this, just many opinions.

Error handling is one of the most problematic aspects of software development, it’s commonly deemed unnecessary by the inexperienced and nearly always the cause of nasty problems.

That’s why modern languages have striven to incorporate error handling at the most basic levels of their syntax.

Try/catch structures are now quite common. And languages like Swift have reduced what used to be a page of bounds checking into a single line.

There is nothing intrinsically wrong with your approach, the problem that you might not have considered is the increasing exponential complexity of the steps required to revert actions when errors occur. This is not a big issue when you are only one or two steps deep into a process, but it can quickly become overwhelming.

Try/Catch blocks, at the very least allow you to separate your straightforward application code from the messy tree that is handling errors when things don’t go as planned.

In your error handler, you don’t seem to have considered that a single error could trigger multiple errors in its wake.

\$\endgroup\$
  • 3
    \$\begingroup\$ "A glorified assembler at best." - Come now. No reason to troll the stack exchange! \$\endgroup\$ – bitsmack Feb 19 at 19:58
  • 1
    \$\begingroup\$ To each his own! I'm not interested in arguing. But I am quite proficient in both C and a few different assembly languages, and I see things differently. Have a nice day! (really!) :) \$\endgroup\$ – bitsmack Feb 19 at 20:33
  • 6
    \$\begingroup\$ Exception handling is one of the most heavily criticised parts of C++, very much unsuitable for embedded systems. try/catch is by no means "modern", but a language flaw inherited from the 1980s. See for example lighterra.com/papers/exceptionsharmful. And this is why we don't consider C++ for embedded systems: there's always some PC programmer aimlessly abusing the numerous bad but available features. In addition, those who can't separate program design from programming language should leave program design to someone else. \$\endgroup\$ – Lundin Feb 20 at 10:28
  • 2
    \$\begingroup\$ My point is that we should teach beginners to use a soldering iron rather than to master the blowtorch. And your reasoning is based on the assumption that the people who designed the blowtorch actually had a clue about de-soldering of electronics and designed it with that in mind, which is highly unlikely. And if one particular blowtorch model has been fuelled by hallucinogenic gas since 2011, I would stay clear of it for any purpose. As for exception handling in C++, it has absolutely nothing to do with MCU hardware exceptions, apart from the similar sounding-names. \$\endgroup\$ – Lundin Feb 20 at 14:17
  • 2
    \$\begingroup\$ Yes really. How would you solve an unimplemented op code exception or clock failure exception using C++ try/catch? \$\endgroup\$ – Lundin Feb 20 at 14:43
0
\$\begingroup\$

You may do one thing to make things simpler.

Take one global char buffer with sufficient size.(call it as error message string)

Fill it with error message if something goes wrong.

And then display error message.

For example:

FWStatusTypeDef OpenTheDoor(void)

{

  if (!KeyIsAvailable())

  {

    Fill the error message string with "You don't have a key."

    return FW_ERROR;

  }



//Remaining code 



  return FW_OK;

}


if (!OpenTheDoor())

{

Print error message string.

}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.