Serial port library across different compilers

Question

I'm writing a C++ library with MinGW (4.8.0, DW2, POSIX) compiler. But this library must be used by another compiler. I've read this post, so I've re-written the C++ library interface in this way:

#ifndef SERIALPORT_H
#define SERIALPORT_H

#include <cstring>          // size_t
#include <memory>           // unique_ptr

#if defined( _MSC_VER ) || defined( __MINGW32__ ) || defined( __MINGW64__ )
#   define SERIALPORT_IMPORT __declspec( dllimport )
#   define SERIALPORT_EXPORT __declspec( dllexport )
#   define SERIALPORT_CALL __stdcall
#elif defined( __GNUG__ ) && !defined( WIN32 )
#   define SERIALPORT_IMPORT
#   define SERIALPORT_EXPORT __attribute__ ((visibility ("default")))
#else
#   define SERIALPORT_IMPORT
#   define SERIALPORT_EXPORT
#endif

#if defined( SERIALPORT_EXPORTS )
#   define SERIALPORT_API SERIALPORT_EXPORT
#else
#   define SERIALPORT_API SERIALPORT_IMPORT
#endif

namespace serialport {

/**
 * @brief Milliseconds_t rappresent a duration of time expressed in milliseconds
 */
typedef unsigned long Milliseconds_t;

/**
 * Possible read return value.
 */
enum ReadStatus
{
    SERIAL_OK,
    SERIAL_TIMEOUT,
    SERIAL_ERROR
};

/**
 * Interface to access to a serial port
 */
class ISerialPort {
public:    
    /**
     * @brief operator delete overload the delete operator so we can destroy
     *        class inside shared library bounds.
     */
    void operator delete( void* p )
    {
        if ( p )
        {
            ISerialPort* s = static_cast< ISerialPort* >( p );
            s->destroy();
        }
    }

    /**
     * Get serial port path
     */
    virtual const char* SERIALPORT_CALL getDeviceName() const = 0;

    /**
     * Get serial port baudrate
     */
    virtual int SERIALPORT_CALL baudrate() const = 0;


    /**
     * Flush serial port rx and tx buffers.
     */
    virtual void SERIALPORT_CALL flush() = 0;

    /**
     * @brief setTimeout set the serial port timeout
     * @param timeout_ms timeout in milliseconds
     */
    virtual void SERIALPORT_CALL setTimeout( Milliseconds_t timeout_ms ) = 0;


    /**
     * @brief getTimeoutGet serial port timeouts.
     * @return serial port timeouts in milliseconds
     */
    virtual Milliseconds_t SERIALPORT_CALL getTimeout() const = 0;


    /**
     * Read data from serial port in the specified timeout.
     * @param[out] buffer pointer to the destination buffer.
     * @param[in] size size in bytes of data that must be read.
     * @throw SerialTimeout if a timeout occured
     */
    virtual ReadStatus SERIALPORT_CALL read( void* buffer, size_t size ) const = 0;


    /**
     * Read data from serial port the aviable data.
     * @param[out] buffer storage where to put incoming data.
     * @param[in] size size in bytes of data that must be read.
     * @return SerialBuffer buffer containing the data.
     */
    virtual size_t SERIALPORT_CALL readSome( void* buffer, size_t size ) const = 0;


    /**
     * Read data from serial port in a infinite timeout.
     * @param[out] buffer pointer to the destination buffer.
     * @param[in] size size in bytes of data that must be read.
     */
    virtual void SERIALPORT_CALL read_n( void* buffer, size_t size ) const = 0;


    /**
     * Send data to the serial port.
     * @param[out] buffer pointer to the destination buffer.
     * @param[in] size size in bytes of data that must be read.
     */
    virtual void SERIALPORT_CALL send( const void* orig, size_t size ) const = 0;

protected:
    /**
     * @brief ~ISerialPort The destructor is protected to avoid that library is
     *        deleted outside dll bounds.
     */
    virtual ~ISerialPort() {}


    /**
     * @brief destroy need to free resource: we cannot use dctor so operator delete
     *        will call destroy() to release the resource
     */
    virtual void SERIALPORT_CALL destroy() = 0;
};


/**
 * @brief The SerialDeleter class is used by SerialPtr to safely delete the class
 */
class SerialDeleter
{
public:
    void operator() ( ISerialPort* s )
    {
       s->destroy();
    }
};

} // serialport namespace

typedef std::unique_ptr < serialport::ISerialPort, serialport::SerialDeleter > SerialPtr;

/**
 * Factory function for creating a serial port class
 * @param path path of the serial port.
 * @param baudrate baudrate of the serial port.
 */
extern "C" SERIALPORT_API serialport::ISerialPort* CreateSerialPort( const char* deviceName, int baudrate );

#endif // SERIALPORT_H

Assuming the compiler is C++11-compliant (I've tested with Visual C++ 2013), is this a good interface for cross-compiler integration?

Answer 1

• You should just include <cstddef> in order to use std::size_t. Including <cstring> seems unnecessary if you're not actually going to utilize anything else.

• Since this is C++11, you can now use strongly-typed enums over plain ones. One difference is that plain enums are implicitly cast to int, whereas strongly-typed ones must be explicitly cast in order to not be implicitly cast as well.

• Both of these classes have no private fields, so they can just become structs, which are public by default.

Answer 2

I have a number of questions that you might find useful to answer for yourself with the idea of improving your code.

Are read and send really const?

It would be a most unusual implementation of a serial port class that didn't actually modify internal state when doing a read or send operation. These should probably not be declared const, including variations such as readSome.

Does operator delete need a header?

I suspect that you will want to #include <new> because that is where your operator delete is defined.

What about constructors?

This interface provides no constructors, which is probably not realistic. Specifically, if you're using RAII, the class would probably need to acquire a particular serial port and initialize it for use. It may also be a useful place to document default baudrate and timeout if you have such for your class. Your factory function CreateSerialPort would be most useful if it returned an initialized instance and that initialization is probably more reasonably the domain of the class itself rather than the factory class.

Do readSome and read_n never throw exceptions?

You have nicely documented the interface, including noting that read can throw a SerialTimeout exception. It seems likely that the other functions could also throw that exception unless they're blocking functions, in which case it would be nice to explicitly say. For example, read_n says that it uses infinite timeout which implies blocking without specifically saying so.

Have you run a spell check on comments?

If you run a spell check on your comments, you'll find a number of things such as "aviable" instead of "available" and "rappresent" instead of "represent". Since your code is nicely commented, it's worth the extra step to eliminate spelling errors.

What about move constructors?

Although your use of the class through SerialPtr seems intended to eliminate the possibility of inadvertent copies, the class can still be moved but the class provides no such call. This means that the compiler-provided move constructor will be used. This may be fine, but it's worth documenting in the interface if that is really your intent.

Does the factory actually create a SerialPtr?

If your factory function actually creates a unique_ptr rather than a mere instance pointer, it's probably worth saying so explicitly in the comments or in the declaration. I understand that there may be some problems in doing so due to the fact that you're passing things through a "C" interface because of the limitations of the Windows interface.