Lately I’ve been programming on a project built around a safety-critical ARM microcontroller. When I started adding to the code base, I noticed again a few code smells that were all too common in embedded programming.

In this post, and maybe others in the future, I will try to enumerate them and add a fix that I used for any of it. Feedback welcome at my email. (Check the bottom of the page for it)

Hard coded string lengths

A function receives a text string, and the length of it as arguments. The function will not stop at the null byte so the programmer just hard codes the length of the string on the function call.

//I've seen this *way* too often
serialWrite(PORT1, 13, "Hello World\r\n");

This one is fairly common where puts() or printf() are not available, the one place where I see it at least a few times a week is on serial port write functions. The obvious issue with it is that, writing the wrong size or forgetting to change the length value when the string is changed, is far too easy. A better solution would be:

//This one is far less prone to error
{
  static const char msg[] = "Hello World!\r\n";
  //The -1 prevents sending the NULL character over the
  //wire, if we do want to send it, just remove the -1
  serialWrite(PORT1, sizeof(msg) - 1, msg);
}

Opening a new scope with { ... } allows us to declare a new string with any name we want, like msg in this example, even in c89. Also, the usage of sizeof(...) allows us to just change the string at any moment without having to worry about changing the string length. Opening a new scope also allows us to reuse the exact same code without changing any variable name:

{
  static const char msg[] = "Hello!\r\n";
  serialWrite(PORT1, sizeof(msg) - 1, msg);
}
//(...)
{
  static const char msg[] = "Bye!\r\n";
  serialWrite(PORT1, sizeof(msg) - 1, msg);
}

Global Local variables & functions

Sometimes you need a variable to exist outside a function call, usually to serve a few functions on the same translation unit, very common on ISRs. So the programmer just declares a global variable on the C file and be done with it:

#include "my_include.h"
uint32_t shared_value;
//(...)

Variables declared this way will be available to any and all code that wants to access it by using an extern statement, it also pollutes the symbol table for the linker, where someone else may declare a variable with the same name making the linking fail, and furthermore this will prevent many compilers from throwing an “unused variable” warning if that “local-global” variable stops being used within the translation unit.

The fix: Make it static and limit its scope to the current translation unit.

#include "my_include.h"
static uint32_t shared_value;
//(...)

The exact same happens with helper functions that are only used within the same translation unit. Generating the same problems and having the same solution. Instead of this:

int myHelperFunction(){
  //(...)
}

Use this:

static int myHelperFunction(){
  //(...)
}

This way if the function stops being used, the compiler will warn about it and you can remove useless code from the project.

Export all the things!!

Related to the previous “Global Local variables and functions” code smell. This one is the exact same thing with the added insult of declaring any and all helper functions on the header file. For example:

//File: sample.h
//This function provides something useful to other parts of the code
int myUsefulFunction();

//This one is only useful inside sample.c but it is declared anyway.
int someHelperFunction();

The evident solution is not exporting inner helper functions on the header file, and making all the inner helper functions static so they cannot be accessed even if using extern statements. If needed, a forward declaration of the static function inside the .c file may be used.

Variable constants

An obvious one yet exceedingly common. Declaring a constant, as a variable, usually as a “Global Local variable” to add insult to injury. This can result in accidental changes to a constant among other hard to debug problems. So this code:

//Declared globally
int BTU_TO_JOULES = 1055;

Should be rewritten as either a macro constant:

#define BTU_TO_JOULES 1055

Or as a real, proper, constant, depending on the scope:

static const int BTU_TO_JOULES = 1055;