Parsing oscilloscope binary data file with C++

Question

Preliminaries

I have OWON oscilloscope that is able to record waveform in file in three formats:

• Text format .txt: the first lines are some general information such as peak-to-peak value, period, frequency etc. Remaining lines are waveform points that is space-separated values.
• Comma-separated-values format .csv: the first lines are some general information such as peak-to-peak value, period, frequency etc. Remaining lines are waveform points that is comma-separated values.
• Binary format .bin: binary file with unknown (for now) with the following structure:
  1. Prefix. Several bytes of some information such as header and setup information. Unknown for now.
  2. Data bytes. 8-bit signed integers that represent ordinates of waveform points.
  3. New line character 0x0a.

I am not interested in the first two options (at least for know) so I will concentrate on dealing with binary data only.

Another fact is you are able to choose how to record the data. There are two options:

• Using a software.
• Using an oscilloscope itself.

The structure of a binary file is different depending on what option was used. For example the size of the prefix is 78 and 278 bits respectively.

My purpose is to provide user-friendly interface to parse data from oscilloscope.

Result

I chose to place all stuff in the namespace which I named owon.

1. owon namespace structure

The following is just a short description of the owon namespace.

namespace owon
{
    /*Wafeform point*/
    struct Point;

    /*Storage of useful information such as position in a file where data
     *starts from or where it ends*/
    struct FilePosition;

    /*Binary data is being produced either by the OWON software
     *or directly by pushing the button on an oscilloscope*/
    enum FileCreator { Software, Device };

    /*Abstract class to parse file*/
    class Parser;

    /*Concrete parser
     *that parses only binary files
     *though depending on source of the file (software or device)
     *it uses different parse algorithm*/
    template<FileCreator creator>
    class BinaryParser : public Parser;

    /*Iterator class
     *that iterates through points in the waveform
     *Does not depend on concrete parser (format) or source of the file (file creator)*/
    class PointIterator;

2. owoh.h

2.1 Preamble

//owon.h
#ifndef OWON_H
#define OWON_H

#include <iostream>
#include <fstream>
#include <string>
#include <stdexcept>//for std::runtime_error

#include <stdint.h>//for int8_t
#include <stddef.h>//for NULL

#define MY_WARNING(msg) std::cout << "WARNING: " << msg << std::endl

namespace owon
{
//continued below...

2.2 struct Point

This structure represents a data point.

//owoh.h continued
    struct Point
    {
        float x;
        float y;
    };
//continued below...

2.3 struct FilePosition

This structure is to store some useful info about bytes in a file.

//owon.h continued
    struct FilePosition
    {
        size_t dataPosition;//position in file where data begins from
        size_t lastPosition;//position in file where data ends
    };
//continued below...

2.4 enum FileCreator

This enumearation represents two possible option how to record data: through the software or from the oscilloscope.

//owon.h continued
    /*Binary data is being produced either by the OWON software
     *or directly by pushing the button on an oscilloscope*/
    enum FileCreator { Software, Device };
//continued below...

2.5 class Parser

Abstract class that provides a protocol for all parsers (there is only one for now). It

//owon.h continued
    class Parser
    {
        protected :
            /*CTORS (objects of this class are not copiable and assignable)*/
            Parser( std::string fileName ) :
                _fileName( fileName ),
                _isParsed( false )
            { }
            Parser( const Parser& );
            Parser& operator=( const Parser& );
           //DTOR
            ~Parser()
            {
                if( _fStream.is_open() ) { _fStream.close(); }
            }

            /*Member data: */
            std::string _fileName;//name of file to be parsed
            FilePosition _filePosition;//storage of useful info about data position in file
            std::ifstream _fStream;//file stream
            /*Flags*/
            bool _isParsed;

            /*This method should not be accessible from the outside*/
            virtual Point ConstructPoint( size_t index ) = 0;

        public :
            inline std::string GetFileName() { return _fileName; }
            PointIterator* CreatePointIterator();
            /*Main method that parse file
             *used for determine data positions in file
             *depending on in what format (bin, csv or txt)
             *and from what source (software or device) file is*/
            virtual void Parse() = 0;

            friend class PointIterator;
    };//Parser
//continued below...

2.6 template class BinaryParser

This is concrete parser - binary data parser.

//owon.h continued
    /*Concrete parser
     *that parses only binary files
     *though depending on source of the file (software or device)
     *it uses different parse algorithm*/
    template<FileCreator creator>
    class BinaryParser : public Parser
    {
        protected :
            FileCreator _creator;

            Point ConstructPoint( size_t index );

        public :
            BinaryParser( std::string fileName ) : Parser( fileName ),
                _creator( creator )
            {
                /*Check if fileName contains an extension suffix*/
                if( fileName.rfind( ".bin" ) == std::string::npos )
                {
                    MY_WARNING( "Binary file without '.bin' extension was specified." );
                }
            }

            void Parse();
    };//BinaryParser
//continued below...

2.7 class PointIterator

This class is developed in order to provide user-friendly iteration over the data points e.g. inside a for loop.

//owon.h continued
    /*Iterator class
     *that iterates through points in the waveform
     *Does not depend on concrete parser (format) or source of the file (file creator)*/
    class PointIterator
    {
        protected :
            Parser* _parser;
            size_t _index;

        public :
            PointIterator( Parser* parser ) : _parser( parser ) { }

            void First() { _index = _parser->_filePosition.dataPosition; }
            void Next()  { _index++; }
            bool Last() { return ( _index == _parser->_filePosition.lastPosition ); }

            Point CurrentPoint() { return _parser->ConstructPoint( _index ); }
    };//PointIterator
};//owon

3. owon.cpp

3.1 BinaryParser::Parse method

Parsing algorithm for binary data

//owon.cpp
#include "owon.h"

namespace owon
{
    /*Main method that parses the file*/
    template<FileCreator creator>
    void BinaryParser<creator>::Parse()
    {
        _isParsed = true;
        /*Try to open file in binary mod*/
        _fStream.open( _fileName.c_str(), std::ios::out | std::ios::binary );
        if( !_fStream )
        {
            _isParsed = false;
            throw std::runtime_error( "Could not open file.\n" );
        }

        else
        {
            /*Set the start position depending on the source of the file*/
            if     ( creator == Software ) { _filePosition.dataPosition = 278; }
            else if( creator == Device )   { _filePosition.dataPosition = 78;  }

            /*Determine the end of the data
             *NOTE: the very last byte is omitted*/
            _fStream.seekg(-1, _fStream.end );
            _filePosition.lastPosition = _fStream.tellg();
            if( _filePosition.lastPosition < _filePosition.dataPosition )
            {
                _isParsed = false;
                throw std::runtime_error( "No data in the file.\n" );
            }

            /*Place cursor at the start of the data*/
            _fStream.seekg( _filePosition.dataPosition, _fStream.beg );
        }
    }
//continued below...

3.2 BinaryParser::ConstructPoint method

This method constructs Point object from binary data

//owon.cpp continued
    /*Method that returns current point of parsed waveform*/
    template<FileCreator creator>
    Point BinaryParser<creator>::ConstructPoint( size_t index )
    {
        Point a = { 0., 0. };

        /*check if index is out of range*/
        if( index < _filePosition.dataPosition  ||  index > _filePosition.lastPosition )
        {
            MY_WARNING( "Requesting position is out of range. Returned (0,0)" );
            return a;
        }
        else if( _fStream.is_open() )
        {
            /*Read a byte from file*/
            int8_t intY;
            _fStream.read( reinterpret_cast< char* >(&intY), sizeof(intY) );
            /*Assign coordinates
             *using a strange conversion rule :D*/ 
            a.x = (float)index - _filePosition.dataPosition;//User does not care about position of data, let it start from zero
            a.y = 0.4 * intY + 53.2;//No comments for a while
        }
        else
        {
            MY_WARNING( "File is not open." );
        }

        return a;
    }
//continued below...

3.3 Parser::CreatePointIterator method

This method creates a PointIterator object.

    PointIterator* Parser::CreatePointIterator()
    {
        if( _isParsed )
        {
            return new PointIterator( this );
        }
        else
        {
            MY_WARNING( "Bad parsing occured." );
            return NULL;
        }
    }
};//owon

4. Using

This is how I use it.

//Test.cpp
#include <iostream>

#include "owon_parser.cpp"

using namespace owon;

int main()
{
    BinaryParser<Software> bps( "wf_from_software.bin" );
    try
    {
        bps.Parse();
    }
    catch (const std::runtime_error& e )
    {
        std::cout << e.what();
    }

    PointIterator* it = bps.CreatePointIterator();

    for( it->First(); !it->Last(); it->Next() )
    {
        Point a = it->CurrentPoint();
        std::cout << a.x << " " << a.y << std::endl;
    }

    return 0;
}

Problem

The problem that I can see is that PointIterator class has access to all stuff of class Parser. I cannot specify const pointer to Parser class inside PointIterator class because of method ConstructPoint (sec. 3.2) that reads from a file and therefore cannot be declared as const.

Sorry for a lot of code and what is your advice to me?

Answer 1

I think this is a really good start! Most of what I see are minor things. Here are some thoughts:

C++

Most of your use of C++ is great, but I see you slipping into some C-isms. For example, you have:

#include <stddef.h>//for NULL

You shouldn't be using NULL. You should prefer nullptr instead in C++. See this StackOverflow Question for the details of why.

Also, it's best to avoid macros. (That's true even in C.) I would change this:

#define MY_WARNING(msg) std::cout << "WARNING: " << msg << std::endl

to an inline function. Also, I'd avoid std::endl as it flushes the entire output stream. That will slow down performance of your code. I'd just use "\n" in its place. You might also want to pass in the output stream rather than assuming std::cout. A caller might want the output to go to std::err or to a file.

As a rule, I almost never put more than 1 class declaration in a single header file. It becomes difficult to find things when there are several classes in a header. I would put Parser in one header, BinaryParser in another, and probably PointIterator in a third, though there's an argument to be made to keep it with Parser since they are tightly coupled. I could go either way on that.

Iterators

You have an iterator class which is a great idea, but it doesn't have the traditional iterator interface, which makes it confusing. I would recommend making the interface for it be more like other iterators where begin() returns the first object in the container, end() returns one past the last element, and operator++() increments the iterator to the next element.

The problem that I can see is that PointIterator class has access to all stuff of class Parser. I cannot specify const pointer to Parser class inside PointIterator class because of method ConstructPoint (sec. 3.2) that reads from a file and therefore cannot be declared as const.

I agree that this is a problem. One way to solve it would be to have accessors in the Parser class that return the data start and end positions. Then you wouldn't have to access them directly.

A bigger problem is how ConstructPoint() works. The interface for ConstructPoint() is quite deceptive. It takes an index, but doesn't actually look at the index when reading the desired point. Which means it's not reading the desired point - it's always reading the next point. There are also a number of errors that can occur in the function, and they aren't really handled. Returning (0,0) seems problematic. What if the first point has a value of 0? How does a caller distinguish that from an error on the first point?

One solution would be to change the Parse() method of BinaryParser read in all the points during parsing. That's what I would expect from a Parse() method. Otherwise, it's really a BeginParsing() method, or a ParseHeader() method, or something like that. If you read in all the points during Parse(), then the ConstructPoint() method can simply look up the existing data, convert it to the correct format and return it, meaning it can be const.

Naming

In owon.h you write:

struct FilePosition
{
    size_t dataPosition;//position in file where data begins from
    size_t lastPosition;//position in file where data ends
};

The fact that you have to explain what these values represent probably means they should be renamed, and the new names should explain what they represent. I'd name dataPosition to dataStartIndex and lastPosition to dataEndIndex.

I dislike the name ConstructPoint(). I would rename it GetPoint() or PointAtIndex(). (As mentioned above, right now it's really a NextPoint() method.) Also, if you are going to have it do things which can fail, I'd either have it throw an exception or return an error value so the caller can distinguish when it fails.

Also, you use ConstructPoint(), but CreatePointIterator(). I would try to be consistent and call them both Create* or Construct* if you're going to keep the names.

Comments

Many of your comments are unhelpful. It's clear that the member data is member data. You don't need to point that out. It just clutters the file. Same with the descriptions of the member variable names. I would change _fStream to _fileStream though.

Likewise, Flags tells a reader nothing about what the flags are for. Their names do that, so you don't need to point out that they are flags.

The one place where you have a chance to make comments useful, you simply don't:

        /*Assign coordinates
         *using a strange conversion rule :D*/ 
        a.x = (float)index - _filePosition.dataPosition;//User does not care about position of data, let it start from zero
        a.y = 0.4 * intY + 53.2;//No comments for a while

Um... could you please explain the "strange conversion rule"? And why "No comments for a while"? Is there a page in the documentation that explains this oddness? If so, at least put the page number (if it's a physical manual) or page address (if it's a URL) in the comment so I can look it up when reading the code. Either that, or just put the explanation in the code as a comment.

Overall, this is pretty well written. I think if you clean up the above things, it should be easy to read and maintain going forward.

Answer 2

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

Understand templates vs. inheritance

The use of templates here for BinaryParser is quite strange. Normally, we use templates when the intent is to have a generic class such as std::vector<> which is a vector of almost any arbitrary type. However in this code, there are only two possibilities defined here:

enum FileCreator { Software, Device };

Further, although the Parser is a virtual base class for BinaryParser, there seems to be no reason for this since there are no other derived classes. Finally, the only difference between the Software and Device versions appears to be the starting offset of the data. This could all be made much simpler, as I'll demonstrate at the end.

Avoid exceptions for unexceptional events

The default behavior of std::ifstream is to simply set failbit rather than throw an exception. This is generally a good approach, because trying to open a file that doesn't exist or reading from an empty file are not that uncommon. Rather than an exception, I'd be inclined to mimic the standard library and simply set a status bit. If you really want to throw exceptions, you can always call exceptions(std::ifstream::failbit) to turn on exception throwing for a particular ifstream.

Think carefully about object copies

The code has an explicit copy function declared, but not defined, for the base Parser class. The effect is that a copy cannot be made, which, given the current construction of the class, is probably a good thing. However, the better way to express that is to write this:

Parser(const Parser& ) = delete;
Parser& operator=(const Parser& ) = delete;

Now the intent is very clear.

Avoid user I/O in general classes

The MY_WARNING macro could, of course be redefined to do nothing, but in general, it's better to simply omit such run-time I/O in a general class (one whose prime function is not user I/O). Indicate errors to the calling code and let it decide how to deal with this. Printing a message to std::cout might not be the best way to handle it.

Use the C++ forms of C includes

Instead of <stdint.h> and <stddef.h>, use <cstdint> and <cstddef>. The latter forms essentially put the important things into the std namespace.

Delegate to standard iterators

Instead of writing a custom iterator (which doesn't follow the standard iterator form), I'd suggest instead to read the entire file into a std::vector. That way, a number of convenient things such as iterators, std::max(), std::min(), etc. are already automatically available and users won't have to wonder how to use the data.

Minimize the use of system resources

Keeping a file open uses system resources, so it's often better to keep an input data file open for as little time as possible. One easy way to do that would be to defer opening the file until the request to actually parse the file. This not only minimizes the time the file is open, but also means there is no longer a problem with allowing copies of the object.

Eliminate redundant semicolons

The end of a class declaration requires a semicolon, but the end of a namespace declaration does not. For that reason, you should eliminate the redundant semicolons following the closing brace of a namespace.

Avoid transferring ownership via raw pointer

Right now, the main function leaks memory. This is because it obtains a raw pointer *it via the call to bps.CreatePointIterator(). That's almost always a bad way to do things. Instead, simply create and return a new object on the stack. Don't rely on the caller cleaning up if you can avoid it.

Don't include a .cpp file

Your sample main has this deeply suspect line:

#include "owon_parser.cpp"

A .cpp file implies compiled code, while a .h file is a header. There is very rarely (I'd say never) a situation in which a .cpp file should be included in another. Instead, if they need to be separate compilation units, they should be compiled separately and linked.

A revised version

Using all of these ideas, this is a revised version:

#include <string>
#include <vector>
#include <fstream>
#include <iomanip>
#include <iostream>

namespace owon {
    struct Point {
        Point(float x, float y) : x{x}, y{y} {}
        friend std::ostream& operator<<(std::ostream& out, const Point& p) {
            out.unsetf(std::ios_base::floatfield);
            out << std::setprecision(5) << std::setw(7) << std::left << 1+p.x << " ";
            return out << std::setprecision(2) << std::fixed << p.y;
        }
        float x;
        float y;
    };
    class BinaryParser {
        public:
        enum class FileCreator { Software, Device };
        BinaryParser(const std::string &filename, FileCreator creator = FileCreator::Software) :
            filename{filename},
            offset{(creator == FileCreator::Software) ? 278u : 78u}
        {
        }
        std::vector<Point> Parse() {
            std::vector<Point> points;
            std::ifstream in{filename};
            in.seekg(offset);
            std::size_t x{0};
            if (in) {
                char y;
                while (in.read(&y, sizeof(y))) {
                    // apply scaling and translation
                    points.emplace_back(x++, 0.4 * y + 53.2);
                }
                // we ignore the last byte in the file
                points.pop_back();
            }
            return points;
        }
        private:
            std::string filename;
            std::size_t offset;
    };
}

int main()
{
    owon::BinaryParser bps("Waveform.bin");
    auto points = bps.Parse();
    for(const auto &point : points) {
        std::cout << point << '\n';
    }
}

This code produces output which is exactly identical to the "txt" format that the oscilloscope creates, which I used for testing.