1
\$\begingroup\$

I have wrote a small C++ class that provides some basic serial I/O functionality (it uses the standard GNU/Linux API).

At this stage is working fine, before adding new functionality I want to get some feedback for the current code.

I structured the classes like this:

There's a pure virtual class called AbstractIOHandler which provides the most basic functionality (like read(), write(), etc).

From this abstract class UARTIOHandler publicly inherits and actually implements all the functionality.

AbstractIOHandler.h:

#ifndef ABSTRACTIOHANDLER_H
#define ABSTRACTIOHANDLER_H

#include <vector>
#include <initializer_list>

/*!
 * \class Abstract class (interface) for performing general purpose I/O with various devices.
 */

class AbstractIOHandler {
public:
    using uchar8 = unsigned char;

    /*!
     * \brief Default constructor (explicit), since this is an interface, there's nothing to construct.
     */
    AbstractIOHandler() { };

    /*!
     * \brief Destructor, performs all the required cleanup (no cleanup since this is an interface).
     */
    virtual ~AbstractIOHandler() { };

    /*!
     * \brief Pure virtual method used for sending data out over a communication channel.
     * \param[in] A pointer to a constant C-like array of bytes where the data is stored.
     * \param[in] A `int` value that represents the size of `data` C-like array.
     * \return Same as the standard UNIX `write()` function (check `man 3 write` for details).
     */
    virtual int write(const uchar8 *data, int size) = 0;

    /*!
     * \brief Pure virtual method used for sending one byte over a communication channel.
     * \param[in] A `uchar8` value which represent the data which should be sent.
     * \return 1 if the byte was successfully sent, 0 otherwise.
     */
    virtual int write(uchar8 byte) = 0;

    /*!
     * \brief Pure virtual method used for sending data out over a communication channel.
     * \param[in] data: A reference to a const `std::vector<uchar8>` object that represents the vector of values.
     * \return Same as the standard UNIX `write()` function (check `man 3 write` for details).
     */
    virtual int write(const std::vector<uchar8> &data) = 0;

    /*!
     * \brief Pure virtual method used for sending data out over a communication channel.
     * \param[in] data: A reference to a constant `std::initializer_list<uchar8>` object that
     *            represents the data to be wrote.
     * \return Same as the standard UNIX `write()` function (check `man 3 write` for details).
     */
    virtual int write(const std::initializer_list<uchar8> &data) = 0;

    /*!
     * \brief Pure virtual method used for receiving data over a communication channel.
     * \param[out] data: A const pointer to a `uchar8` object (C-like array of `uchar8`s) where the
     *             received data will be stored.
     * \param[in] size: A `int` value which represents the number of maximum bytes to be read.
     * \return Same as the standard UNIX `read()` function (check `man 3 read` for details).
     */
    virtual int read(uchar8 *const data, int size) = 0;

    /*!
     * \brief Pure virtual method used for sending a BREAK sequence over a communication channel.
     * \return 0 if the BREAK sequence was successfully sent, -1 otherwise.
     */
    virtual int sendBreakSignal() = 0;

    virtual void flush() = 0;
};

#endif

UARTIOHandler.h:

#ifndef UARTIOHANDLER_H
#define UARTIOHANDLER_H

//! Standard C++ libraries.
#include <iostream>
#include <string>
#include <chrono>

//! Standard C (POSIX) libraries.
#include <termios.h>
//#include <asm/termios.h>
#include <sys/ioctl.h>
#include <unistd.h>
#include <errno.h>
#include <fcntl.h>


/*!
 * \class UARTIOHandler: Class used for implementing the actual UART I/O functionality.
 * \remark This class publicly inherits from `AbstractIOHandler` for being able to offer the minimum functionality.
 */

class UARTIOHandler: public AbstractIOHandler {
public:
    /*!
     * \brief Explicit constructor, used for constructing a valid `UARTIOHandler` object.
     * \param[in] port: A `std::string` object that represents the location of the UART-mapped tty* file.
     * \param[in] baud: A `speed_t` value that represents the baud rate value.
     * \remark If the UART-mapped tty*-like file can't be opened, `std::ios_base::failure` will be thrown.
     */
    explicit UARTIOHandler(std::string port, speed_t baud);

    /*!
     * \brief Destructor, will perform all the necessary cleanup.
     */
    virtual ~UARTIOHandler();

    /*!
     * \brief Overridden method from the parent class, functionality described there.
     */
    virtual int write(const uchar8 *data, int size) override;

    /*!
     * \brief Overridden method from the parent class, functionality described there.
     */
    virtual int write(uchar8 byte) override;

    /*!
     * \brief Overridden method from the parent class, functionality described there.
     */
    virtual int write(const std::vector<uchar8> &data) override;

    /*!
     * \brief Overridden method from the parent class, functionality described there.
     */
    virtual int write(const std::initializer_list<uchar8> &data) override;

    /*!
     * \brief Overridden method from the parent class, functionality described there.
     */
    virtual int read(uchar8 *const data, int size) override;

    /*!
     * \brief Overridden method from the parent class, functionality described there.
     */
    virtual int sendBreakSignal() override;

    /*!
     * \brief Method used for setting the UART communication baud rate.
     * \param[in] baud: A `speed_t` value representing the desired baud rate.
     */
    virtual void setBaudRate(speed_t baud);

    /*!
     * \brief Will change the hardware flow control setting to ON or OFF.
     * \param[in] mode: A `bool` object, if is set to `true` HW control is ON, OFF otherwise.
     */
    virtual void setHardwareFlowControl(bool mode);

    /*!
     * \brief Getter, will get the location of the tty*-like UART-mapped file.
     * \return A `std::string` value that represents the file's location and name.
     */
    std::string getUARTPort() const;

    void discardBufferedData() const;
protected:
    virtual void flush();
    virtual void applyTermiosSettings();
private:
    //! tty*-like file that's mapped to the UART peripheral.
    std::string UARTport;

    //! UART's communication baud rate.
    speed_t baud;

    //! File descriptor used for performing `write(...)` operations.
    int fd_write;

    int fd_write_break;

    //! File descriptor used for performing `read(...)` operations.
    int fd_read;

    //! File descriptor used for logging debug data.
    int fd_debug;

    //! Structure used for setting the port's baud rate.
    struct termios settings;
};

#endif

UARTIOHandler.cpp:

#include "UARTIOHandler.h"

UARTIOHandler::UARTIOHandler(std::string port, speed_t baud):
            AbstractIOHandler{ },
            UARTport{ port },
            baud{ baud },
            fd_write{ 0 },
            fd_write_break{ 0 },
            fd_read{ 0 },
            fd_debug{ 0 },
            settings()
    {

    if(-1 == (fd_debug = open("gateway_debug.out", O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR))) {
            throw std::ios_base::failure("Unable to open debug descriptor.");
        }


    if(-1 == (fd_write = open(port.c_str(), O_WRONLY | O_NOCTTY | O_SYNC))) {
        throw std::ios_base::failure("Unable to open UART descriptor.");
    }


    if(-1 == (fd_write_break = open(port.c_str(), O_WRONLY | O_NOCTTY | O_SYNC))) {
        throw std::ios_base::failure("Unable to open UART descriptor.");
    }

    this->setBreakBaudParameters();


    if(-1 == (fd_read = open(port.c_str(), O_RDONLY))) {
        throw std::ios_base::failure("Unable to open UART descriptor.");
    }

    tcgetattr(fd_write, &settings);
    tcgetattr(fd_read, &settings);

    /*!
     * Set some default settings.
     */
    cfsetospeed(&settings, baud);       /* baud rate                */
    settings.c_cflag &= ~PARENB;        /* no parity                */
    settings.c_cflag &= ~CSTOPB;        /* 1 stop bit                           */
    settings.c_cflag &= ~CSIZE;
    settings.c_cflag |= CS8 | CLOCAL;   /* 8 bits                       */
    settings.c_oflag &= ~OPOST;         /* raw output               */
    settings.c_cflag &= ~CRTSCTS;           /* disable HW flow control              */
    settings.c_iflag |= (INPCK | PARMRK | IGNBRK);
    settings.c_iflag &= ~IGNBRK;
    settings.c_cc[VMIN]  = 0;
    settings.c_cc[VTIME] = 1;


    /*!
     * Apply the settings for both: the writing and reading file descriptors.
     */
    this->applyTermiosSettings();
}

UARTIOHandler::~UARTIOHandler() {
    close(fd_write);
    close(fd_read);
    close(fd_debug);
}

int UARTIOHandler::write(const uchar8 *data, int size) {

    auto output = ::write(this->fd_write, data, size);

    this->flush();

    return output;
}

int UARTIOHandler::write(uchar8 byte) {
    auto output = ::write(this->fd_write, &byte, sizeof(uchar8));

    this->flush();

    return output;
}

int UARTIOHandler::write(const std::vector<uchar8> &data) {
    uchar8 buffer[data.size()];

    int counter = 0;
    for(auto i : data) {
        buffer[counter++] = i;
    }


    auto output = ::write(this->fd_write, buffer, data.size());

    this->flush();

    return output;
}

int UARTIOHandler::write(const std::initializer_list<uchar8> &data) {
    uchar8 buffer[data.size()];

    int counter = 0;
    for(auto i : data) {
        buffer[counter++] = i;
    }

    auto output = ::write(this->fd_write, buffer, data.size());

    this->flush();

    return output;
}

int UARTIOHandler::read(uchar8 *const data, int size) {
    return ::read(fd_read, data, size);
}

int UARTIOHandler::sendBreakSignal() {
    /*!
     * Break character.
     */
    char bbreak = 0;

    /*!
     * Decreasing baud rate to 9600 so one byte will last ~900qs.
     */
    this->setBaudRate(B9600);

    /*!
     * Send the break on the bus.
     */
    ::write(this->fd_write_break, &bbreak, sizeof(char));

    /*!
     * Force the write operation (for preventing the kernel buffering).
     */
    tcflush(this->fd_write_break, TCIOFLUSH);

    /*!
     * Reset the baud rate to the previous set value.
     */
    this->setBaudRate(this->baud);
}

void UARTIOHandler::setBaudRate(speed_t baud) {
    cfsetospeed(&settings, baud);

    this->applyTermiosSettings();
}

void UARTIOHandler::setHardwareFlowControl(bool mode) {
    if(false == mode) {
            settings.c_cflag &= ~CRTSCTS; // disable HW flow control.
    }
    else {
            settings.c_cflag &= CRTSCTS; // enable HW flow control.
    }

    this->applyTermiosSettings();
}

std::string UARTIOHandler::getUARTPort() const {
    return this->UARTport;
}

void UARTIOHandler::discardBufferedData() const {
    tcflush(this->fd_write, TCIOFLUSH);
    tcflush(this->fd_read, TCIOFLUSH);
}

void UARTIOHandler::flush() {
    tcdrain(this->fd_write);
}

void UARTIOHandler::applyTermiosSettings() {
    tcsetattr(fd_write, TCSANOW, &settings);
    tcflush(fd_write, TCOFLUSH);

    tcsetattr(fd_read, TCSANOW, &settings);
    tcflush(fd_read, TCOFLUSH);
}

Any tips on how to do it better?

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Why did you not write it to behave like a standard stream? \$\endgroup\$ Sep 12, 2016 at 19:27
  • \$\begingroup\$ Can you please elaborate a little bit? (I'm kinda new to c++). Standard stream library is using some global objects like std::cin, std::cout, etc. And all this objects are using the operator<< and operator>> for performing writings/readings. Did I understood right? \$\endgroup\$ Sep 12, 2016 at 19:40
  • \$\begingroup\$ They can use operator<< and operator>> but they also have read() write() interface. In addition to std::cin and std::cout the same interface is used to write to files std::ifstream/std::ofstream and additionally to strings std::stringstream. There are also multiple libraries that allow you to write to sockets. By using the standard stream interface you could simply plug your stream into existing code that uses streams. Additionally you would be able to debug code that uses your serial interface by replacing it with a stringstream to validate they are writing the correct data \$\endgroup\$ Sep 12, 2016 at 20:52

1 Answer 1

0
\$\begingroup\$
  • Break

    You forgot to close(fd_write_break). Speaking of which, I don't see why do you need it at all: breaking through fd_write works just fine.

    That said, I don't like the way you generate the break. 900 usec is way less than enough for a receiver to detect break; it will most likely see a framing error. Besides, fiddling with baud rate makes the program thread unsafe. A recommended way is to tcsendbreak(write_fd, 0).

  • write

    Data in the vector are guaranteed to occupy contiguous memory, so

    ::write(fd_write, &data[0], data.size())
    

    is just fine.

    I also recommend to deal with the details of uart in a single place, e.g

    int UARTIOHandler::write(uchar8 byte) {
        return write(&byte, 1);
    }
    
    int UARTIOHandler::write(const std::vector<uchar8> &data) {
        return write(&data[0], data.size());
    }
    
    int UARTIOHandler::write(const std::initializer_list<uchar8> &data) {
        std::vector<uchar8> v(data);
        return write(data);
    }
    
  • Misc

    The first call in

    tcgetattr(fd_write, &settings);
    tcgetattr(fd_read, &settings);
    

    is effectively no-op: its result is immediately overwritten by the second. BTW, it is perfectly fine to read and write UART through the same fd. Just open it with O_RDWR.

    You don't need to prefix members with this->. If you do so, be consistent.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.