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
• Mouse-over function/variable comment display
• Datatype information (especially useful for C++'s auto variables, IMHO)
• “Find all references”
• Search text in: block, file, all open files, all files for the project/solution (that last one will include all the #include files as well)
• 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 };

Working with the low-level code⁵⁾ quite often requires direct bit manipulation. And as much as every C programmer is familiar and is comfortable with bit operations in the language, using those results in the ugliest code - it is unnecessarily verbose, prone to errors⁶⁾, makes the code that much harder to port⁷⁾.

Here's a more concrete example: working with 16/32 bit integers, where depending on the context it is either a CPU-addressable word, or a stream of bytes on the wire (be it USB or I²C). Traditionally that would result in the 1970's style code like this:

uint32_t = pageAddress * glSpiPageSize;
uint8_t location[4];
location[1] = (byteAddress >> 16) & 0xFF; /* MS byte */
location[2] = (byteAddress >> 8) & 0xFF;
location[3] = byteAddress & 0xFF; /* LS byte */

Do you see an obvious bug in that code⁸⁾?

Here's a much more modernized version of this, which also (automagically!!!) takes care of that idiotic bug:

typedef union PackedUint16{
    uint16_t v;
    struct{ uint8_t l, h; };
    uint8_t a[2];
} PackedUint16;
typedef union PackedInt16{
    int16_t v;
    struct{ uint8_t l, h; };
    uint8_t a[2];
} PackedInt16;
typedef union PackedUint32{
    uint32_t v;
    struct{ PackedUint16 l, h; };
    uint8_t a[4];
} PackedUint32;
 
///////////////////////////////////
 
PackedUint32 byteAddress = {pageAddress * glSpiPageSize};
uint8_t location[4] = {};      /* uninitialized memory is evil */
location[1] = byteAddress.h.l; /* compiler error, of course - must be PackedUint32, not 16 */
location[2] = byteAddress.l.h;
location[3] = byteAddress.l.l; /* LS byte */
 
/* alternatively the code above can be written much more to-the-point, clearly expressing the intent  */
const uint8_t * const location_arr = pageAddress.a;

Incoming USB requests have a standard “setup” header which results in old-style code looking something like the following⁹⁾:

static CyBool_t CyFxUVCApplnUSBSetupCB(uint32_t setupdat0, /* SETUP Data 0 */
                                       uint32_t setupdat1 /* SETUP Data 1 */
) {
    CyU3PIoMatrixConfig_t io_cfg;
    CyBool_t uvcHandleReq = CyFalse;
    uint32_t status;
    CyU3PReturnStatus_t apiRetStatus = CY_U3P_SUCCESS;
    uint8_t address, data;
 
    /* Obtain Request Type and Request */
    bmReqType = (uint8_t) (setupdat0 & CY_FX_USB_SETUP_REQ_TYPE_MASK );
    bRequest = (uint8_t) ((setupdat0 & CY_FX_USB_SETUP_REQ_MASK ) >> 8);
    bType = (bmReqType & CY_U3P_USB_TYPE_MASK);
    bTarget = (bmReqType & CY_U3P_USB_TARGET_MASK);
    wValue = (uint16_t) ((setupdat0 & CY_FX_USB_SETUP_VALUE_MASK ) >> 16);
    wIndex = (uint16_t) (setupdat1 & CY_FX_USB_SETUP_INDEX_MASK );
    wLength = (uint16_t) ((setupdat1 & CY_FX_USB_SETUP_LENGTH_MASK ) >> 16);
 
    /* Handle USB vendor requests */
    if (bType == CY_U3P_USB_VENDOR_RQT) {

The function goes on for HUNDREDS of lines of deep nesting!!!¹⁰⁾.

Don't know about you, but all this bit shifting and byte extraction just screams “fruitful field for errors!” to me. Consider having abstracted all this ugliness into a structure like the following:

enum UsbRequestTarget{usb_tgt_dev, usb_tgt_ifc, usb_tgt_ep, usb_tgt_other};
enum UsbRequestType{usb_rt_std, usb_rt_class, usb_rt_vendor, usb_rt_reserved};
typedef struct UsbSetupPacket{
    union{
        uint32_t d0;
        struct{
            struct{
                struct{
                    enum UsbRequestTarget bTarget : 5;
                    enum UsbRequestType bType : 2;
                    uint8_t dirIn : 1; // "IN" is the direction towards USB host (reading data)
                };
                uint8_t bRequest;
            };
            union{
                PackedUint16 wValue;
                uint16_t featureSelector;
            };
        };
    };
    union{
        uint32_t d1;
        struct{
            union{
                PackedUint16 wIndex;
                struct{
                    union{
                        uint8_t all;
                        struct{
                            uint8_t num : 4;
                            uint8_t res3 : 3;
                            uint8_t dirIn : 1;
                        };
                    };
                    uint8_t res8 : 8;
                } ep;
                struct Infc{
                    uint8_t num;
                    uint8_t res;
                } infc;
            };
            PackedUint16 wLength;
        };
    };
} UsbSetupPacket;

With that little prep work the same code becomes so much more readable and maintainable:

static CyBool_t CyFxUVCApplnUSBSetupCB(uint32_t setupdat0, uint32_t setupdat1)
{
    UsbSetupPacket req = {};
    // the next 2 lines is all it takes to distribute the bits into proper NAMED variables
    req.d0 = setupdat0;
    req.d1 = setupdat1;
 
    // dispatch to appropriate handler based on request type and return status of request handling to the USB driver
    switch(req.bType){
        case usb_rt_vendor: return handleVendorRqt(&req);
        case usb_rt_std:    return handleUsbStdRqt(&req);
        case usb_rt_class:  return handleUvcRqt(&req);
        default: return CyFalse;
    }
}

Not only the code becomes that much more readable - it also opens up opportunities for compilers to do platform-specific optimizations which are otherwise just not available to them as the intent of the code is not clearly expressed with all that manual bit shifting…