Style and Best Practices for C99 Packetization Functions

Question

I just spent some time putting together a simple protocol and implementation for packetizing byte streams (on GitHub here). The project is aimed at embedded systems that need a very lightweight way to pass discrete messages across serial lines. Since I've never had any of my C reviewed before, would you mind taking a look at it and seeing if there's anything that could be improved in the style, design, or best practices?

s3p.h:

#ifndef S3P_H
#define S3P_H

#include <stdint.h>

#define S3P_OVERHEAD 3

typedef enum {
  S3P_SUCCESS = 0,
  S3P_BUF_TOO_SMALL = 1,
  S3P_PAYLOAD_TOO_LARGE = 2,
  S3P_CHECKSUM_ERR = 3,
  S3P_PARSE_FAILURE = 4,
} S3P_ERR;

/**
   Build a new packet from the bytes in "data" into "out".

   S3P packets are limited to 256 bytes of data; attempting to packetize more
   will result in a S3P_PAYLOAD_TOO_LARGE error. "out" must be at least
   S3P_OVERHEAD bytes longer than the data that needs to be packetized; and this
   can increase to twofold depending on the number of bytes that need
   escaping. In general, unless memory usage is a concern, allocate twice as
   much space as the data you wish to packetize.

   Params:
   data: a byte array of data to be packetized.
   dsize: the length of "data".
   out: a byte array which will hold the built packet.
   osize: the length of "out".
   psize: a pointer to an int which will contain the length of the
   built packet.
*/
S3P_ERR s3p_build(uint8_t const *data, int dsize, uint8_t *out, int osize, 
                    int *psize);

/**
   Read a packet from "in", and place unescaped data into "data".

   Breaking streams into packets is the responsibility of the 
   caller. This function will only read the first packet in the
   "in" buffer; all other data will be ignored. This function guarantees that
   the length of the data retrieved from the packet will be strictly less than
   the size of the input packet.

   Params:
   in: byte array of raw, packetized data.
   isize: the length of "in".
   data: byte array which will contain the retrieved data.
   dsize: the length of "data".
   psize: pointer to an int which will contain the length of the
   retrieved data.
*/
S3P_ERR s3p_read(uint8_t const *in, int isize, uint8_t *data, int dsize, 
                   int *psize);

#endif

s3p.c:

#include <s3p.h>

enum S3P_CONTROL_CHARS {
  S3P_START = 0x56,  // Marks the start of a packet
  S3P_ESCAPE = 0x25, // Marks escaped bytes
  S3P_MASK = 0x20,   // Mask used for escaping chars. echar = char ^ S3P_MASK
};

S3P_ERR s3p_build(uint8_t const *data, int dsize, uint8_t *out, int osize, 
                  int *psize){
  if((dsize + S3P_OVERHEAD) > osize){
    return S3P_BUF_TOO_SMALL;
  }
  if(255 < dsize){
    return S3P_PAYLOAD_TOO_LARGE;
  }

  /*
    S3P packets follow this structure:
    START | LENGTH (N) | DATA 1 | DATA 2 | ... | DATA N | CHECKSUM
   */

  out[0] = S3P_START;

  int dnext = 2; // Next data byte position in output buffer

  /*
    If the length of the data (dsize, in this case) is S3P_START or S3P_ESCAPE
    we need to escape it.
   */
  if(S3P_START == dsize || S3P_ESCAPE == dsize){
    out[1] = S3P_ESCAPE;
    out[2] = ((uint8_t) dsize) ^ S3P_MASK;
    dnext = 3;
  } else {
    out[1] = (uint8_t) dsize;
  }

  uint8_t check = 0; // Checksum
  int i;
  for(i=0; i<dsize; i++){
    // Check the size restrictions: dnext should contain the number of bytes
    // written so far less one (as the indices start at zero), and we need to
    // add a checksum byte after all is said and done.
    if(dnext + 2 > osize){
      return S3P_BUF_TOO_SMALL;
    }

    uint8_t dbyte = data[i];
    check += dbyte;

    if(S3P_START == dbyte || S3P_ESCAPE == dbyte){
      out[dnext] = S3P_ESCAPE;
      dnext++;
      out[dnext] = dbyte ^ S3P_MASK;
    } else {
      out[dnext] = dbyte;
    }
    dnext++;
  }

  out[dnext] = check;
  dnext++;
  *psize = dnext;
  return S3P_SUCCESS;
}

S3P_ERR s3p_read(uint8_t const *in, int isize, uint8_t *data, int dsize, 
                 int *psize){
  if(dsize < 1){
    return S3P_BUF_TOO_SMALL;
  }
  if(isize < S3P_OVERHEAD){
    return S3P_PARSE_FAILURE;
  }
  // The first byte should be a start byte. If not, this is not an S3P packet
  // and we should not try to parse it.
  if(S3P_START != in[0]){
    return S3P_PARSE_FAILURE;
  }

  int dnext = 2;
  int length;
  if(S3P_ESCAPE == in[1]){
    length = in[2] ^ S3P_MASK;
    dnext++;
  } else {
    length = in[1];
  }

  int dread = 0; // Number of data bytes read
  uint8_t check = 0; // Checksum
  while(dread < length){
    if(dread >= dsize){
      return S3P_BUF_TOO_SMALL;
    }
    if(dnext >= isize){
      return S3P_PARSE_FAILURE;
    }

    uint8_t dbyte = in[dnext];
    if(S3P_START == dbyte){
      return S3P_PARSE_FAILURE;
    }
    if(S3P_ESCAPE == dbyte){
      dnext++;
      dbyte = in[dnext];
      if(S3P_START == dbyte || S3P_ESCAPE == dbyte){
        return S3P_PARSE_FAILURE;
      }
      dbyte = dbyte ^ S3P_MASK;
    }

    data[dread] = dbyte;
    check += dbyte;
    dnext++;
    dread++;
  }
  *psize = dread;

  uint8_t pcheck;
  if(S3P_ESCAPE == in[dnext]){
    dnext++;
    pcheck = in[dnext] ^ S3P_MASK;
  } else {
    pcheck = in[dnext];
  }

  if(pcheck != check){
    return S3P_CHECKSUM_ERR;
  }

  return S3P_SUCCESS;
}

Update: I've incorporated some changes suggested by William Morris, such as adding end markers and changing size parameters to unsigned values (size_t). This simplified the code a bit. The latest version is up on GitHub now.

Answer 1

A few comments...

I'm no expert in data transmission, but there are sure to be some on StackOverflow who are. Perhaps you could discuss optimal encoding schemes there. Yours may or may not be optimal, but has some peculiarities. In particular, it is not easy to determine the packet length - only by looking for a start byte (or by counting more than 512 bytes) can you see that the last packet has ended. If you imagine packets being sent infrequently, perhaps once a second, the receiver cannot tell when to pass-on a completed packet to be decoded; it must wait until the next packet starts to know that the current one is complete. That implies a delay of a second! Ok, maybe some timeouts might help but that gets messy.

I also wondered where the 0x56/0x25 codes came from. They seem strange values. Are they particularly easy to recognise on a scope/analyser? Another minor issue is that binary data dumped onto a terminal is unreadable - hex-encoded data on the other hand is not.

To the code..

• First impressions - quite neat, but functions a bit stretched-out.

• negative input parameters are not trapped.

• a #include of your own header should use "" not <>

• exports stdint.h (i.e. stdint.h is part of the interface). This might be undesirable. Personally I see no advantage of uint8_t over unsigned char - CHAR_BIT is (I believe) guaranteed to be at least 8 and I can't see how an architecture that has CHAR_BIT > 8 could support uint8_t natively.

• comments within the functions look unnecessary.

s3p_build:

• check for buffer too small is done twice. Doesn't the second suffice?

• code to escape characters is repeated - use a function.

• if variable check needs a comment to make it clear that it is a checksum, why not call it checksum. Either way, delete the comment.

s3p_read:

• again, check for buffer too small is done twice. Doesn't the second suffice?

• code to un-escape characters is repeated 3 times - use a function

Answer 2

Have you considered #defining S3P_MASK as 0x00 instead of 0x20? It wouldn't change anything significant about your implementation, but it would make encoding and decoding more efficient, and it would make the protocol slightly less WTFy. Consider: When you want to embed a " character inside a C string, how do you escape it? \", of course, not \B. (I chose B by xoring " with an arbitrary mask. Couldn't you tell?)

At least change 0x56, 0x65, 0x25 (that's V,e,% in ASCII-speak) to something that looks reasonable in a dump. Perhaps 0x7B, 0x7D, 0x5C (that's {, }, \). I notice that when you had to choose the two characters that required double-width escaping, you specifically chose one of them to be e, the most common letter in English text. ;)

I'd also suggest swapping the "termination marker" and the "checksum". It doesn't really matter since they're only one byte apiece, but if the checksum were more complicated (say, CRC-32) or if the codepath to read some new input were more convoluted (say, it was coming from a pipe and you had to worry about EOF), then it would be kind of a hassle to have to deal with these bytes that might be data or might be checksum, we're not sure yet.

while (true) {
    dbyte = in[dnext++];
    if (in[dnext] != S3P_TERM) {
        out[out_idx++] = unescape(dbyte);
    } else {
        verify_checksum();
        break;
    }
}

would become simply

while (in[dnext] != S3P_TERM) {
    out[out_idx++] = unescape(in[dnext++]);
}
++dnext;
verify_checksum();

(where unescape and verify_checksum are pseudocode handwavey parts, of course).

Your documentation for s3p.build(str) says it raises ValueError if the payload to be encoded is longer than 255 bytes. This looks wrong to me (both in the sense that it shouldn't be true, and in the sense that it isn't currently true).

You have a potential overflow on the first line of s3p_build; and again on line 41. If you're going to sanity-check your inputs, make sure your sanity-checks are bulletproof.

In s3p_read, (dsize < 1) is equivalent to (dsize == 0). But that 1 should actually have been S3P_OVERHEAD, I bet.

I'd prefer to see function parameters consistently named in and out, or encoded and decoded, rather than in and data (resp. data and out).

Instead of the in-parameter osize and the out-parameter *psize, you could just have one in/out-parameter *osize. On entry to the function, it holds the size of the user's buffer. On exit, it holds how many bytes were actually written. I feel like this is a common pattern in Unix networking land, but I admit I can't find any examples off the top of my head.

You don't explicitly document that the user's output buffer is trashed on error — say, if we get halfway through decoding a packet and suddenly decide that it's bogus.

Finally, style: Your code feels horizontally compressed to an unnatural degree (no whitespace around control structures; 2-space tabs), and at the same time you're stretching it out vertically. That's backwards from how most of us have our monitors configured. :) Here's a particularly bad stretch of your code, followed by how I personally would have styled it:

S3P_ERR s3p_build(uint8_t const *data, size_t dsize, uint8_t *out, size_t osize, 
                  size_t *psize){
  if((dsize + S3P_OVERHEAD) > osize){
    return S3P_BUF_TOO_SMALL;
  }

  /*
    S3P packets follow this structure:
    START | DATA 1 | DATA 2 | ... | DATA N | CHECKSUM | TERM
   */

  out[0] = S3P_START;

  size_t data_next = 1;

  uint8_t checksum = 0;
  for(size_t i=0; i<dsize; i++){

Mine:

S3P_ERR s3p_build(uint8_t const *data, size_t dsize,
                  uint8_t *out, size_t osize, 
                  size_t *psize)
{
    // S3P packets follow this structure:
    // START | DATA 1 | DATA 2 | ... | DATA N | CHECKSUM | TERM

    if (dsize + S3P_OVERHEAD > osize) {
        return S3P_BUF_TOO_SMALL;
    }

    size_t data_next = 0;
    uint8_t checksum = 0;

    out[data_next++] = S3P_START;

    for (size_t i=0; i < dsize; ++i) {