2
\$\begingroup\$

I have a protocol where I send lots of different messages using two or three different header types (let's call them "Type A" and "Type B" messages).

To simplify unit testing, my idea was to do something like:

typedef struct
{
    u32 timestamp; // unix
    u32 deviceId; 
}
CommonHeader;

void buildMessage(Buffer * output, u16 cookie, CommonHeader * header, 
   void (*bodySerializerFn)(Buffer*,void*), void * bodySerializerArgs)
{
     // create header
     Buffer_append_u16(output, cookie);
     Buffer_append_u16(output, (u16)0); // dummy length, update at the end
     Buffer_append_u32(output, header->timestamp); 
     Buffer_append_u32(output, header->deviceId); 

     // invoke callback to fill the body
     bodySerializerFn(output, bodySerializerArgs);

     // finish wrapping (write length into the header and append crc)
     u32 totalLen = Buffer_getLen(output) + 2; // length so far + 16-bit crc
     Buffer_write_u16(output, /*offset*/ 2, /*len*/ totalLen);
     Buffer_append_crc(output);
}

Buffer is a struct (or ADT) which supports various overflow-safe operations for appending bytes in big endian or little endian format:

typedef struct
{
    const u8 * bufferStart; // points to start of  buffer
    const u8 * bufferEnd;   // points one byte after end of buffer
    u8 * currentPos;  // points to current byte
}
Buffer;

void Buffer_init(Buffer *buffer, u8 * array, u32 arraySize)
{
    buffer->bufferStart = array;
    buffer->currentPos = array;
    buffer->bufferEnd = array + arraySize;
}

void Buffer_append_u8 (Buffer *buffer, u8 singleByte)
{
    if (Buffer_isNotFull(buffer))
        *buffer->currentPos++ = singleByte;
}

void Buffer_append_u16 (Buffer *buffer, u16 value)
{
    Buffer_append_u8(buffer, (u8)(value));
    Buffer_append_u8(buffer, (u8)(value >> 8));
}

// etc.

So using this I can create data messages:

// append data message to the output buffer
CommonHeader hdr = { .timestamp = getTime(), .deviceId = getId() };
Measurement measurements[100]; // struct Measurement { .timestamp, .value}
buildMessage(output, COOKIE_DATAMSG, &hdr, appendDataBody, &measurements);

or config messages:

// append config message to output
CommonHeader hdr = { .timestamp = getTime(), .deviceId = getId() };
Config config = { 
    .deviceId = getId(), 
    .deviceName = "PressureSensor_1", 
    .reportingFrequency = 10 /* Hz */,
    .scalingFactor = 10 /* x0.001 */
};
buildMessage(output, COOKIE_CONFIG, &hdr, appendConfigBody, &config);

appendConfigBody and appendDataBody are functions which fill the buffer using the supplied parameter:

void appendConfigBody(Buffer * output, void * args)
{
    Config * cfg = args;
    Buffer_append_u16(output, cfg->deviceId);
    Buffer_append_string(output, cfg->deviceName, MAX_DEVICE_NAME);
    Buffer_append_u16(output, cfg->reportingFrequency);
    Buffer_append_u32(output, cfg->scalingFactor);
}

Since there are many messages using the same header, I wanted to decouple the part where the header is being created from the body serialization, which can be tested separately.

This makes testing buildMessage trivial (just create a temporary buffer, pass an empty bodySerializerFn and check the result), and allows me to quickly test each "body serializer" function with a separate buffer (since it simply appends to the buffer), without having to check the header for each individual message.

But my concerns are:

  1. Since parameters for the passed function pointer are void*, there is not much compile-time type safety, I need to take care to pass the correct struct type to the function, I am not sure if this is something that can be improved?

  2. This will run on an embedded system (ARM based), and the bodies of these messages are often rather small. Perhaps the callback approach is not the most performant way to do it?

  3. I am not sure if this code can be considered "idiomatic C code" by people who code in C for living, i.e. future code maintainers?

So, is there a better (i.e. more "idiomatic") way to compose messages which share common parts, which would:

  1. Allow me to unit test the whole thing properly?

  2. Provide as much compile-time type safety as possible?

  3. Ideally have no code duplication?

  4. Feel like a natural C code for future maintainers?

\$\endgroup\$
  • \$\begingroup\$ @Edward: sorry, I didn't know about this, I'll fix the question (although it's almost equal to the actual working code). Does this mean I have to post all the Buffer_xxx functions too? Or are you talking about the contents of appendMessageTypeA? \$\endgroup\$ – Groo Apr 16 '17 at 19:36
  • \$\begingroup\$ I don't know whether this is suitable, but have you tried c-faq.com/varargs/vprintf.html? \$\endgroup\$ – Neil Apr 18 '17 at 5:28
2
\$\begingroup\$

Here are some thoughts on how to improve this code.

Provide complete compilable code for review

When the code is missing many pieces, as with this question, it makes it more difficult for reviewers to review the code. When the code is difficult to review, it tends to draw fewer and less thorough reviews. Posting a full, compilable version for review tends to allow for more and better reviews.

Use standard types

The code uses u32, u8 etc. which I am guessing are equivalent to uint32_t and uint8_t. If that's so, I would strongly recommend using the standard names from <stdint.h> rather than custom synonyms. It will make the code more portable and allow quicker understanding of the code.

Add error checking and handling

Right now, the code does not appear to provide any signal back to the calling code if the buffer has overflowed within Buffer_append_u8. A useful addition might be to return a bool that indicates success of the operation. This would allow for simple and consistent checks so that calling code would be signaled in the event of a buffer overflow.

Use const where practical

In a number of cases in this code, such as appendConfigBody some of the pointer arguments are not and should not be modified. For that reason, they should be declared const as in:

void appendConfigBody(Buffer * output, const void *args)
{
    const Config *cfg = args;
    Buffer_append_u16(output, cfg->deviceId);
    Buffer_append_string(output, cfg->deviceName, MAX_DEVICE_NAME);
    Buffer_append_u16(output, cfg->reportingFrequency);
    Buffer_append_u32(output, cfg->scalingFactor);
}

This also means a change to the declaration of buildMessage:

void buildMessage(Buffer * output, u16 cookie, CommonHeader * header,
                  void (*bodySerializerFn) (Buffer *, const void *),
                  const void *bodySerializerArgs)

Consider a different approach

The approach of passing function pointers and void * for any necessary data is not very idiomatic C in my experience. Instead, what is more often done is to simply call the functions in sequence. That is, instead of your current generic buildMessage, I'd suggest one of two possible approaches. One is to break things into chunks such as buildHeader(), buildDataMsgBody()/buildConfigMsgBody(), and finishMessage() and make three calls where you currently call one function. This would allow better type checking at compile time rather than potentially having a problem only exposed at runtime. Since the bodies of the functions would need to exist anyway, this requires little to no additional ROM space for the code.

The other possible approach, is to create specific functions such as buildDataMessage() and buildConfigMessage() that would create the specific type of message, and would be passed a specific kind of pointer (which would be checked at compile time). This, too, would require little to no additional ROM space but confers the benefit of compile-time type checking.

Either of these approaches are likely to be as fast or faster than the current code because composition of functions at compile time allows for additional optimizations (e.g. register allocations) that are not generally possible when passing function pointers about which the compiler can make no assumptions.

\$\endgroup\$
  • \$\begingroup\$ Reasonable suggestions, thanks. As you suggested, I will probably abandon the fn pointer approach due to possible time/memory constraints and put each sequence of calls (header, body, footer) into a separate function. I still believe the "fn pointer" approach provides looser coupled, is easier to test and to prevent runtime mistakes, but in this case performance and legibility for future maintainers might be more important than some slight code duplication. \$\endgroup\$ – Groo Apr 21 '17 at 8:41

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.