Binary protocol print utility in C++

Question

Please review this protocol printer/decoder in C++.

I also attached a sample binary file to use to test.

Main program:

/*
program to parse a binary protocol layer with following form:
header: x bytes - example uses DEVx: of 5 bytes
timestamp: uses excel format timestamp of 8 bytes (double)
payload size: 4 bytes
payload: payload bytes

program just prints hex of payload, but could be extended to parse
the inner data protocol

Program reads from stdin.
Example usage: parse <file.bin
*/

#include "ring_buffer.hpp"

#include <stdio.h>
#include <string.h>
#include <ctype.h>

#include <string>
#include <iostream>
#include <iomanip>
#include <cassert>

// handling for embedded Ctrl-Z in stream
#if defined(WIN32)
#include <io.h>
#include <fcntl.h>
#endif

// forward declares
size_t get_payload_size(const char* stream, size_t length);
void print_line(const char* line, const size_t length, const size_t protocol_header_length);
void handle_line(ring_buffer& data, const bool print_all, const size_t protocol_header_length);
std::string get_timestamp(double timestamp, char date_delimeter = '/', char time_delimiter = ':');


size_t get_payload_size(const char* stream, size_t length) {
    unsigned payload_size = 0;
    for (size_t i = 0; i < length; ++i) {
        payload_size += ((stream[i] & 0xFF) << i * 8);
    }
    return payload_size;
}

void print_line(const char* line, const size_t length, const size_t protocol_header_length) {

    const int timestamp_length = 8;
    const int data_length = 4;

    // get protocol header part
    char* start = new char[protocol_header_length + 1]();
    memcpy(start, line, protocol_header_length);
    start[protocol_header_length] = '\0';
    std::cout << start << " ";
    delete[] start;

    // get timestamp part
    double alltime = 0.0;

    // return if insufficient data to print timestamp
    if (length < protocol_header_length + timestamp_length)
        return;

    memcpy(&alltime, &line[protocol_header_length], timestamp_length);
    std::cout << get_timestamp(alltime) << " ";

    // return if insufficient data to get payload size
    if (length < protocol_header_length + timestamp_length + data_length)
        return;

    // get size of data payload 
    size_t data_size = get_payload_size(&line[protocol_header_length + timestamp_length], data_length);

    // for data just print binary
    const size_t start_pos = protocol_header_length + timestamp_length + data_length;

    // correct data_size for incomplete line
    if (data_size > length - start_pos)
        data_size = length - start_pos;

    assert(length - start_pos == data_size);

    for (size_t i = start_pos; i < start_pos + data_size; ++i) {
        std::cout << std::hex << std::setfill('0') << std::setw(2) << (int)line[i] << ' ';
    }

    std::cout << '\n';
}


void handle_line(ring_buffer& data, const bool print_all, const size_t protocol_header_length) {

    // if print_all not specified, don't remove next protocol header
    size_t stopSize = print_all ? 0 : protocol_header_length;  
    char* message = new char[data.size()]();

    size_t idx = 0;
    while (data.size() > stopSize) {
        message[idx++] = data.get();
    }
    print_line(message, idx, protocol_header_length);

    delete[] message;
}

// returns Excel DATE format time as a string in UK DD/MM/YYYY HH:MM:SS:MSEC format
// TODO parameterise format
std::string get_timestamp(double timestamp, char date_delimeter, char time_delimiter) {

    // get integer part
    unsigned days = (int)timestamp;

    // date part implementation taken from:
    // https://www.codeproject.com/Articles/2750/Excel-serial-date-to-Day-Month-Year-and-vise-versa

    int day, month, year;

    // Excel/Lotus 123 have a bug with 29-02-1900. 1900 is not a
    // leap year, but Excel/Lotus 123 think it is...
    // TODO - work out how this works and what magic nos represent
    if (days == 60) {
        day = 29;
        month = 2;
        year = 1900;
    } else {
        if (days < 60) {
            // Because of the 29-02-1900 bug, any serial date 
            // under 60 is one off... Compensate.
            days++;
        }

        // Modified Julian to DMY calculation with an addition of 2415019
        int l = days + 68569 + 2415019;
        int n = int((4 * l) / 146097);
        l = l - int((146097 * n + 3) / 4);
        int i = int((4000 * (l + 1)) / 1461001);
        l = l - int((1461 * i) / 4) + 31;
        int j = int((80 * l) / 2447);
        day = l - int((2447 * j) / 80);
        l = int(j / 11);
        month = j + 2 - (12 * l);
        year = 100 * (n - 49) + i + l;
    }

    // fractional part holds hours, minutes, seconds and milliseconds.  eg 0.25 is midday ie 06:00:00:000
    double fraction = timestamp - ((int)timestamp);

    const int milliseconds_per_hour = 60 * 60 * 1000;
    const int milliseconds_per_minute = 60 * 1000;

    // total no. milliseconds since start of day
    // add 1/2 to +ve double so conversion to int rounds correctly
    int days_milliseconds = (int)(fraction * (24.0 * milliseconds_per_hour) + 0.5);

    int hour = days_milliseconds / milliseconds_per_hour;

    //Calculate Minutes
    int minute = (days_milliseconds - hour * milliseconds_per_hour) / milliseconds_per_minute;

    //Calculate Seconds
    int second = (days_milliseconds - (hour * milliseconds_per_hour) - (minute * milliseconds_per_minute)) / 1000;

    //Calculate MilliSeconds
    int msec = (days_milliseconds - (hour * milliseconds_per_hour) - (minute * milliseconds_per_minute) - (second * 1000));

    char buffer[100] = {};
    sprintf_s(buffer, 100, "%02d%c%02d%c%04d %02d%c%02d%c%02d%c%03d", 
        day, date_delimeter, month, date_delimeter, year, 
        hour, time_delimiter, minute, time_delimiter, second, time_delimiter, msec);
    std::string s(buffer);
    return s;
}


int main() {

// on Windows switch to binary mode for handling stdin
// otherwise embedded Ctrl-Z will cause getchar to assume end of file
#if defined(WIN32)
    _setmode(_fileno(stdin), _O_BINARY);
#endif

    // configuration constants
    static const char* header1 = "DEV1:";
    static const char* header2 = "DEV2:";
    static const int prefix_size = 5;  // size of header

    // processing vars
    int ch = 0;
    bool contains_next_prefix = false; 
    ring_buffer buffer(100);  // buffer size dependent on protocol
    uint8_t prefix_check[prefix_size] = {};

    // case where first bytes are not protocol prefix - seek to start of line
    while ((ch = getchar()) != EOF) {
        for (int i = 1; i < prefix_size; i++)
            prefix_check[i - 1] = prefix_check[i];

        prefix_check[4] = (uint8_t)ch;

        // check if we have a start of line
        if (memcmp(prefix_check, header1, prefix_size) == 0
            || memcmp(prefix_check, header2, prefix_size) == 0) {

            // now copy previous prefix_size bytes to ring buffer
            for (size_t i = 0; i < prefix_size; ++i) {
                buffer.push_back(prefix_check[i]);
            }
            break;
        }
    }

    while ((ch = getchar()) != EOF) {
        // check if last 5 bytes are start of protocol - copy each byte 1 to
        // the left and add new byte on end
        for (int i = 1; i < prefix_size; i++)
            prefix_check[i - 1] = prefix_check[i];

        prefix_check[4] = (uint8_t)ch;

        buffer.push_back((uint8_t)ch);

        // check if we have a start of line
        if (memcmp(prefix_check, header1, prefix_size) == 0
            || memcmp(prefix_check, header2, prefix_size) == 0) {
            contains_next_prefix = true;
        }

        if (contains_next_prefix) {
            // we have both start and end markers - a whole line
            handle_line(buffer, false, prefix_size);
            contains_next_prefix = false;
        }
    }

    // case end of buffer - no start of next line, so handle here
    if (ch == EOF) {
        handle_line(buffer, true, prefix_size);
    }

    std::cout << "end of stream\n";
}

Supporting ring buffer class:

ring_buffer.hpp

#ifndef RING_BUFFER_HPP_
#define RING_BUFFER_HPP_

#include <stdint.h>

class ring_buffer
{
public:
    ring_buffer(size_t size);
    ~ring_buffer();
    void reset();
    bool empty() const;
    bool full() const;
    size_t capacity() const;
    size_t remaining() const;
    size_t size() const;
    void push_back(uint8_t data);
    uint8_t get();
    // do not allow copy
    ring_buffer(const ring_buffer&) = delete;
    ring_buffer& operator=(const ring_buffer&) = delete;

private:
    uint8_t* buffer_;
    size_t put_idx_, get_idx_, capacity_, count_;
};

#endif // RING_BUFFER_HPP_

ring_buffer implementation:

ring_buffer.hpp

#include "ring_buffer.hpp"

#include <stdint.h>
#include <cassert>
#include <string.h>

ring_buffer::ring_buffer(size_t size) 
    : get_idx_(0), put_idx_(0), count_(0), capacity_(size), buffer_(new uint8_t[size+1]()) 
{
}

ring_buffer::~ring_buffer() {
    delete[] buffer_;
}

void ring_buffer::reset() {
    get_idx_ = put_idx_ = count_ = 0;
}

bool ring_buffer::empty() const {
    return count_ == 0;
}

bool ring_buffer::full() const {
    return count_ == capacity_;
}
size_t ring_buffer::capacity() const {
    return capacity_;
}
size_t ring_buffer::remaining() const { 
    return capacity_ - count_; 
}

void ring_buffer::push_back(uint8_t data) {
    assert(!full());
    buffer_[put_idx_++] = data;
    put_idx_ %= (capacity_ + 1);
    ++count_;
}

//get the oldest element, move read pointer
uint8_t ring_buffer::get() {
    assert(!empty());
    const uint8_t ret(buffer_[get_idx_++]);
    get_idx_ %= (capacity_ + 1);
    --count_;
    return ret;
}

size_t ring_buffer::size() const {
    return count_;
}

Example file in hex:

44 45 56 32 3A E5 EC 4E 45 47 FA E4 40 01 00 00 00 06 44 45 56 32 3A BC B3 4F 45 47 FA E4 40 01 00 00 00 02 44 45 56 32 3A BC B3 4F 45 47 FA E4 40 04 00 00 00 53 31 30 2C 44 45 56 32 3A 28 17 50 45 47 FA E4 40 01 00 00 00 4E 44 45 56 32 3A 28 17 50 45 47 FA E4 40 0C 00 00 00 30 31 2C 41 30 35 2C 45 30 34 03 37

Answer 1

This is not C++. This is C with some completely avoidable C++ elements. Proper way would be completely get rid of C++ elements and post this code as standard C for reviewing. I do not feel the current version is worth reviewing as C++ program.

For example

// get protocol header part
char* start = new char[protocol_header_length + 1]();
memcpy(start, line, protocol_header_length);
start[protocol_header_length] = '\0';
std::cout << start << " ";
delete[] start;

in C++ would be

std::string start(line, line + protocol_header_length);
std::cout << start << " ";

In C it should be

char* start = malloc(protocol_header_length + 1);
memcpy(start, line, protocol_header_length);
start[protocol_header_length] = '\0';
printf("%s ", start);
free(start);

See for yourself which version is closer to the original code

Of course the C version then could be simplified into

printf("%*.*s ", protocol_header_length, protocol_header_length, line);

So decide which language you are going to use. Rewrite your code into this language. Add some error handling and then post something for review.

Answer 2

From my point of view:

• Raw pointers are bad

• Comments in code are bad - use functions instead

Main:

• Are these forward declarations really needed?

  // forward declares
  size_t get_payload_size(const char* stream, size_t length);
  void print_line(const char* line, const size_t length, const size_t protocol_header_length);
  void handle_line(ring_buffer& data, const bool print_all, const size_t protocol_header_length);
  std::string get_timestamp(double timestamp, char date_delimeter = '/', char time_delimiter = ':');
  

• This should be a separate function:

  // get protocol header part
  char* start = new char[protocol_header_length + 1]();
  memcpy(start, line, protocol_header_length);
  start[protocol_header_length] = '\0';
  std::cout << start << " ";
  delete[] start;
  

  I would use std::vector instead of raw pointers. With std::vector I would use std::copy.

• Why do you have assert in your code?

  assert(length - start_pos == data_size);
  

  What does it mean when it fails? What should the user do with it? What would happen when you compile your project with NDEBUG and assertion should fail, but it's not here?

• This should be a separate function:

  // Modified Julian to DMY calculation with an addition of 2415019
  int l = days + 68569 + 2415019;
  int n = int((4 * l) / 146097);
  l = l - int((146097 * n + 3) / 4);
  int i = int((4000 * (l + 1)) / 1461001);
  l = l - int((1461 * i) / 4) + 31;
  int j = int((80 * l) / 2447);
  day = l - int((2447 * j) / 80);
  l = int(j / 11);
  month = j + 2 - (12 * l);
  year = 100 * (n - 49) + i + l;
  

  Why do you have so many magic numbers? What do 2415019 and 146097 mean?

ring_buffer.hpp

• This comment is not necessary: // do not allow copy

• I would replace the RING_BUFFER_HPP_ guard with #pragma once. (please read the discussion in comments)

• Do you really need the raw pointer uint8_t* buffer_ with the new operator? Maybe it could be replaced with std:vector?

• #include <stdint.h> is already included in the header file.