Serial port write buffer

Question

I'm writing to some serial comm port with specific bytes set into buffer. My protocol for write buffer is written below. Platform is Windows and IDE is Visual C++.

1. Byte 0 will have message type indicator (Byte)
2. Byte 1-4 will have message number (Uint32)
3. Byte 5-8 will have unix epoch time (Uint32)
4. Byte 13-14 will have number of sample packets (Uint16)
5. Bytes 15+ (upto 60) will have sample data packets (Uint16)
6. Byte 15+(No of samples*2) will have end of message (Byte)

Now I'm concern about buffer building (raw Bytes) from integer or short values.

void ConvertIntToByte(int iValue, BYTE* ucRawBuffer, int iOffset)
{
  ucRawBuffer[iOffset]   = ((iValue >> 24) & 0xFF);
  ucRawBuffer[iOffset+1] = ((iValue >> 16) & 0xFF);
  ucRawBuffer[iOffset+2] = ((iValue >> 8) & 0xFF);
  ucRawBuffer[iOffset+3] = (iValue & 0xFF);
}

void ConvertShortToByte(short iValue, BYTE* ucRawBuffer, int iOffset)
{
  ucRawBuffer[iOffset]   = (iValue >> 8) & 0xFF;
  ucRawBuffer[iOffset+1] = iValue & 0xFF;
}

void DataThreadFunction()
{
  while(!m_bClosed)
  {
    DWORD dwRet = WaitForSingleObject(m_hDataSendEvent, INFINITE);
    if(dwRet == WAIT_OBJECT_0)
    {
      ResetEvent(m_hDataSendEvent);

      int iUTCTime = GetEpochTime();
      m_iMsgSequence++;
      ASSERT(m_iMaxSamples != 0);

      BYTE ucXmtdat[60] = {0};
      ucXmtdat[0] = 0x0B;
      ConvertIntToByte(iUTCTime, ucXmtdat, 1);
      ConvertIntToByte(m_iMsgSequence, ucXmtdat, 5);
      ConvertIntToByte(m_iMaxSamples, ucXmtdat, 13);
      int iOffset = 15;
      // m_data - member vector populating from some other method
      for(unsigned int i = 0; i < m_data.size(); i++)
      {
        short iCurrData = m_data[i];
        ConvertShortToByte(iCurrData, ucXmtdat, iOffset);
        iOffset += sizeof(unsigned short);
      }

      m_data.clear();
      iOffset = 15+(m_iMaxSamples*2);
      ucXmtdat[iOffset] = 0xCA;

      bool bSuccess = m_objComm.WriteCommPort(ucXmtdat, 60);
    }
  }
}

Answer 1

I see a number of things that may help you improve your program.

Don't use Hungarian notation

Read this for an explantion of why you shouldn't use Hungarian notation.

Use unsigned types where appropriate

It's unlikely that the values you are passing to ConvertShortToByte or ConvertIntToByte are intended to be signed values. Signed values are treated differently than unsigned values when shifted because of sign extension. In this case, that's almost certainly not what is needed.

Return something useful from functions

The conversion functions would be more useful if they returned a pointer just past the current value. Also rather than a pointer and index, why not just pass a pointer? Rewritten that would look like this:

BYTE *ConvertShortToByte(unsigned short value, BYTE* buffer)
{
  *buffer++ = value >> 8;
  *buffer++ = value;
  return buffer;
}

Eliminate "magic numbers"

This code has a number of "magic numbers," that is, unnamed constants such as 0x0B, 0xCA, 60 etc. Generally it's better to avoid that and give such constants meaningful names. That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "60" and then trying to determine if this particular 60 is relevant to the desired change or if it is some other constant that happens to have the same value.

Use an object

A packet is a natural for an object, so why not make a Packet class? Here's one way to do it:

class Packet {
public:
    Packet (unsigned seq, unsigned time, const std::vector<unsigned short>& data)
    {
      BYTE *buff = buffer;
      *buff++ = MSG_TYPE;
      buff = ConvertIntToByte(time, buff);
      buff = ConvertIntToByte(seq, buff);
      buff = ConvertIntToByte(0, buff); // filler
      buff = ConvertShortToByte(data.size(), buff);
      for (const auto item : data) {
          buff = ConvertShortToByte(item, buff);
      }
      *buff++ = MSG_END;
    }
    size_t size() const { return 16 + (((buffer[13] << 8) + buffer[14]) << 1); }
    static constexpr BYTE MSG_TYPE = 0x0B;  // start of packet
    static constexpr BYTE MSG_END = 0xCA;  // end of packet
    static constexpr size_t BUFF_SIZE = 60;
private:
    BYTE buffer[BUFF_SIZE];
};

If you need access to the underlying buffer, there are a few ways to do it. First, you could simply make the buffer a public data member (or alternatively make Packet a struct). Second, you could provide a handle that would return a const * reference to the internal buffer. Third, you could eliminate the need by providing, for example a write member function. Fourth, you could declare the buffer in place and then use "placement new" which is a common idiom for communications protocols in which a packet is built in a fixed location buffer. Since many people seem unfamiliar with the use of "placement new", I'll elaborate.

Use "placement new"

If a buffer is already allocated and you want to create a Packet in place, it can be done using "placement new":

BYTE ucXmtdat[60];
Packet *pkt = new (ucXmtdat) Packet(m_iMsgSequence, iUTCTime, m_data);

This creates a Packet at the location specified by ucXmtdat without allocating further memory. Note that when this form is used, it is entirely up to you to make sure that the memory area is really big enough and it's entirely up to you to destroy the object when finished. In this case, there's not really anything to do since ucXmtdat (which is not a very nice name, by the way) is local and will simply disappear when it goes out of scope.

Answer 2

Converting functions

These can be simplified a bunch. First, you don't need the & 0xFF part. That'll happen from the assignment itself. That takes us to:

void ConvertIntToByte(int iValue, BYTE* ucRawBuffer, int iOffset)
{
  ucRawBuffer[iOffset]   = iValue >> 24;
  ucRawBuffer[iOffset+1] = iValue >> 16;
  ucRawBuffer[iOffset+2] = iValue >> 8;
  ucRawBuffer[iOffset+3] = iValue;
}

Next, we can do better. All we want to do is write the int into the byte buffer in big-endian order. That's effectively an assignment. If you already are on a big-endian machine, that's as easy as:

void ConvertIntToByte(int iValue, BYTE* ucRawBuffer, int iOffset)
{
    *reinterpret_cast<int*>(ucRawBuffer + iOffset) = iValue;
}

On a little-endian machine, you'd have to byte swap. For that, there's htonl:

void ConvertIntToByte(int iValue, BYTE* ucRawBuffer, int iOffset)
{
    *reinterpret_cast<int*>(ucRawBuffer + iOffset) = htonl(iValue);
}

I'm going to assume little-endian here on out. The advantage here is that writing a short is really the same thing, the only difference between what the cast is and what the byte swap function is:

void ConvertShortToByte(short iValue, BYTE* ucRawBuffer, int iOffset)
{
    *reinterpret_cast<short*>(ucRawBuffer + iOffset) = htons(iValue);
}

So let's use a function template. Furthermore, having both ucRawBuffer and iOffset is redundant. Let's do away with the offset:

short hton(short value) { return htons(value); }
int   hton(int value)   { return htonl(value); }

template <class T>
void ConvertToByte(T value, BYTE* buffer)
{
    *reinterpret_cast<T*>(buffer) = hton(value);
}

Cool. Since we changed the signature, we now change this:

ConvertIntToByte(iUTCTime, ucXmtdat, 1);
ConvertIntToByte(m_iMsgSequence, ucXmtdat, 5);
ConvertIntToByte(m_iMaxSamples, ucXmtdat, 13);

to:

ConvertToByte(iUTCTime, ucXmtdat + 1);
ConvertToByte(m_iMsgSequence, ucXmtdat + 5);
ConvertToByte(m_iMaxSamples, ucXmtdat + 13);

Naming Conventions

Hungarian notation is awful. Do you really need the i to know that int iValue is an int? No, that's what the i is for. Similarly the BYTE* tells you that ucRawBuffer is an unsigned char, you don't need the uc. The extra characters give you no benefit, take longer to type, and lead to problems down the road if you ever decide to change types.

So the above really becomes:

ConvertToByte(utcTime, xmtDat + 1);
ConvertToByte(msgSequence, xmtDat + 5);
ConvertToByte(maxSamples, xmtDat + 13);

Unused/Barely used Variables

bSuccess (--> success) is unused.

dwRet and iCurrData are both used only on the line after they are declared, and only exactly once. They can both be gotten rid of. The latter for instance can change to just:

ConvertByte(data[i], xmtDat + offset);
offset += sizeof(data[i]); // don't use unsigned short here, that's brittle

Refactor

The whole function would read better if you factored out the inner part into its own function. That way the outer is:

void DataThread() { // obviously it's a Function
    while (!closed) {
        if (WaitForSingleObject(dataSendEvent, INFINITE) == WAIT_OBJECT_0)) {
            handleDataEvent();
        }
    }
}