Embedded C code doesn't have to be ugly

A contractor engineer we employed years ago was tasked with implementing a UVC-compliant functionality for the SUB2r camera we were working on. Around a year ago it became clear that this implementation was becoming increasingly unstable with the addition of new exciting capabilities and it was at that time that I had a chance to open the source files. Only to get disgusted at the dreadful state of the code. Proverbial “spaghetti code” was surely based on text in those files!

That UVC implementation was heavily based on an example implementation provided by the hardware manufacturer which was taken more or less verbatim by that said engineer and slightly masaged around to do “what the customer asked” (oh, there was one other major “improvement” - that engineer has made sure that there was a big banner with his name at the beginning of each and every file).

It was obvious that before continuing developing new code there was an obligation to pay the technical debt accumulated over all this time… This text is the observations made, lessons learnt, and guidelines emerged from that project of bringing the C code into the XXI century.

This one is not specific to the language of choice but is a more general best practice about anything an engineer of any kind should always keep in mind when working on a project: understand the problem and the current implementation before venturing into changing things.

It may not be obvious, especially at first, why certain decisions have been made that resulted in the choices made for the code you are about to modify. For example: the original implementation was producing MPEG frames programmatically, which required a synchronization between threads (which I'm not going to dig into right now). That, now unnecessary, code was left behind and built on top of. Which eventually resulted in stability issues caused by race conditions introduced by adding more code without understanding why the code was what it was in the first place.

Needless to say that debugging race conditions in embedded code is about as much “fun” as anyone should ever indure. Ever.

Even if unit tests are not something that pop to mind when working on firmware (embedded) code, one must always make sure to fully understand the code before modifying it.

The default SDK provided an IDE based around Eclipse. My personal preference is MS Visual Studio with its slew of features that I can no longer live without. Namely:

• My favorite keyboard shortcuts (A mouse? It often just slows me down when I'm working with text!)
• Code browsing database
• “Find all references”
• Jump to declaration/definition (including a “quick view”)
• View call hierarchy (both “from” and “to” up and down the call stack)
• Refactoring support
• Intelligent renaming (which includes renaming identifiers in comments and disabled code blocks)
• (A very intelligent!) syntax error check while editing text
• Code block comment/uncomment with a keyboard shortcut
• Style suggestions
• Clang-style formatting support (via .clang-format file)

In the end all that was needed to allow MSVS to work with the code was to add a single “settings” file named CppProperties.json to reside in the same directory as the rest of the source files and use the "Open Folder" feature to start editing code.

The plan is to eventually add “actions” to that setup so that it is possible to build without switching to that Eclipse's IDE. But for now this is what the CppProperties.json file looks like (with a future-use compiler flag for when the code is eventually transitioned into C++, but that part can easily be changed back to c99 or even a more advanced c17 if needed):

CppProperties.json

{
  "configurations": [
    {
      "name": "IoT-Release",
      "inheritEnvironments": [
        "linux_arm"
      ],
      "includePath": [
        "${env.FX3_INSTALL_PATH}/firmware/u3p_firmware/inc",
        "${projectRoot}",
        "${env.ARMGCC_INSTALL_PATH}/arm-none-eabi/include",
        "${env.ARMGCC_INSTALL_PATH}/lib/gcc/arm-none-eabi/${env.ARMGCC_VERSION}/include",
        "${workspaceRoot}\\**"
      ],
      "defines": [
        "__CYU3P_TX__=1"
      ],
      "intelliSenseMode": "linux-gcc-arm",
      "compilerSwitches": "-std=c++17"
    }
  ]
}

N.B.: There are quite a few compiler options for the actual compilation but as they don't affect the syntax analysis (like -fabi-version=0 -fno-exceptions -fno-rtti) I opted for not including them in the settings file for now.

Many smart individuals are working on improving the languages we use, providing better ways to express our goals and ideas in code, improving the code's readability and maintainability, reducing chances of making stupid mistakes. Allowing compilers to generate faster, smaller, more optimized code.

As such (barring external dependencies) I always advise on using the latest stable language standard supported by the toolchain that your organization is comfortable with. And yes, that means that engineers must continually improve their grasp of the language and be on top of the latest stable standard to efficiently take advantage of the improvements provided by that standard.

So much has been said about the many advantages of pushing as much work as possible away from the preprocessor and into the compiler that is amazes me to still see tons of #define's in the modern code where language standard features would provide numerous benefits over the plain old text-based code processing that knows nothing of the code structure or the data types used.

Consider a list of unique identifiers used for something, say event IDs. Specifying those with the preprocessor puts the burden of making sure the IDs are unique onto the developer. And let's be honest - the potential for silly mistakes¹⁾ is enormous here. The solution? Simple: use unnamed enums!

enum{
    event_start   // acquire resources and begin processing
  , event_end     // stop the processing and free up the resources
  , event_pause   // stop processing but keep the resources
  , event_resume
};

As an added benefit it helps with the code annotation²⁾ which, being on the same line as the code itself, prevents comments to get shifted around and get attached to an unrelated line of code³⁾.

Same goes for bitmap values, quite honestly:

typedef enum UsbIsoSyncAttr{
    transfer_type_iso  = (0x01 << 0)
  , sync_type_async    = (0x01 << 2)
  , sync_type_adaptive = (0x02 << 2)
  , sync_type_sync     = (0x03 << 2)
} UsbIsoSyncAttr;

That very suggestion (using enum's) is also applicable for places which are normally thought of a “const variables' space” but avoided to save some RAM runtime space⁴⁾. Using enum's of a single value makes sure the value is calculated during compile time, is appropriately type-cast, and doesn't get an address in RAM associated with it during runtime execution:

enum{ s2r_uvc_stream_buf_size = s2r_ep_bulk_video_pkts_count * s2r_ep_bulk_video_pkt_size };