usart driver wrapper

Question

Could you review my usart driver wrapper please?

It contains the C language Xilinx usart driver, but I write C++ programs and need to C++ usart driver. How it was designed correctly and what I have to chage or add in the one on your view?

I will be thankful for your the strictest appraisal.

Thank you.

//**********************************************************************
// name:           usart.hpp
//
// test:           non.
//
// description:    a usart driver wrapper.
//
// author:         Artyom Shimko
//
// contact:        [email protected]
//
// created:        28.12.19
//
// release:        28.12.19
//
// changes:        non.
//
#ifndef USART_HPP
#define USART_HPP


#include "platform.h"                      // Platform depending settings.
#include "xuartps_hw.h"                    // The Xilinx header file for XUartPs device.

namespace UsartSapce {

    enum usartNumber { usart0 = XPAR_PS7_UART_0_BASEADDR,
                       usart1 = XPAR_PS7_UART_1_BASEADDR};
}


template <typename T>
class Usart : public Platform {

private:
   u32 usartNumber;

public:
   Usart(u32 usartNumber);                 // The set usart number method.

   status transmit(T *pData, u32 dataValue);
   status receive (T *pData, u32 dataValue);

   void reset(void);
};


//**********************************************************************
// The class constructor definition, it sets the usart number.
//**********************************************************************
// id:             A required usart number.
//
template <typename T>
Usart<T>::Usart (u32 usartNumber ) {

   this->usartNumber = usartNumber;
}


//**********************************************************************
// The usart reset method.
//**********************************************************************
// parameters:     non.
//
// return:         non.
//
template <typename T>
void Usart<T>::reset(void) {

   XUartPs_ResetHw(usartNumber);
 }


//**********************************************************************
// The data transmit usart method.
//**********************************************************************
// pData:          The pointer to transmited data.
//
// dataValue:      Value of transmited data.
//
// return:         ok aka 0 or error aka -1. The error occurs then pData
//                 is equal to nullptr or/and dataValue is equal to null.
//
template <typename T>
status Usart<T>::transmit(T *pData, u32 dataValue) {

   u8 *pByteData = nullptr;

   if(parameterCheck_(pData,dataValue) != StatusSpace::ok) {

       return StatusSpace::error;
   }

   pByteData = reinterpret_cast<u8*>(pData);

   for(u32 byteCounter = 0; byteCounter < dataValue; ++byteCounter) {

       XUartPs_SendByte(usartNumber, *(pByteData + byteCounter));
   }

    return StatusSpace::ok;
}

//**********************************************************************
// The data receive usart method.
//**********************************************************************
// pData:          The pointer to an array for received data.
//
// dataValue:      Value of received data.
//
// return:         ok aka 0 or error aka -1. The error occurs then pData
//                 is equal to nullptr or/and dataValue is equal to null.
//
template <typename T>
status Usart<T>::receive(T *pData, u32 dataValue) {

   u8 *pByteData = nullptr;

   if(parameterCheck_(pData,dataValue) != StatusSpace::ok) {

       return StatusSpace::error;
   }

   pByteData = reinterpret_cast<u8*>(pData);

   for(u32 byteCounter = 0; byteCounter < dataValue; ++byteCounter) {

       *(pByteData + byteCounter) = XUartPs_RecvByte(usartNumber);
   }

   return StatusSpace::ok;
}

#endif

Answer 1

Consider using enum class

Instead of having a regular enum inside its own namespace, use an enum class:

enum class UsartSpace: u32 {
    usart0 = XPAR_PS7_UART_0_BASEADDR,
    usart1 = XPAR_PS7_UART_1_BASEADDR,
};

Then, to ensure someone cannot instantiate a Usart with the wrong base address, make the constructor take this enum as a parameter:

template <typename T>
Usart<T>::Usart(UsartSpace usartSpace): usartNumber(static_cast<u32>(usartSpace)) {
}

Also, "Space" is a bit of a generic word that doesn't say much, but you can't call the enum Usart since you already have class Usart. Consider moving the enum to inside class Usart, and since it refers to a port, just call it Port. And perhaps keep the distinction between port number and address clear:

template <typename T>
class Usart: public Platform {
private:
    u32 baseAddress;
    static const u32 addresses[] = {
        XPAR_PS7_UART_0_BASEADDR,
        XPAR_PS7_UART_1_BASEADDR,
    };

public:
    enum class Port {
        usart0,
        usart1,
    };

    Usart(Port port);
    ...
};

...

template <typename T>
Usart<T>::Usart(Port port):
    baseAddress(addresses[static_cast<size_t>(port)])
{
}

In code that uses this class, you would then write:

Usart<sometype> usart(Usart::Port::usart0);

Use better names

What you call a usartNumber looks like an base address to me, so call it baseAddress instead. In any case, don't repeat the name of the class in the member variable names.

Also, dataValue is not the value of some piece of data, it's the size of the data you want to read or write. So call it size, and if possible use size_t as its type:

status transmit(T *pData, size_t size);
status receive (T *pData, size_t size);

Is it status or StatusSpace?

I feel this is another case of an enum in its own namespace being used as a way to declare constants of another type. I would instead define:

enum class Status {
    ok,
    error,
    ...
};

Use array notation where appropriate

Instead of writing *(pByteData + byteCounter), just write the more ideomatic pByteData[byteCounter].

Use const where appropriate

You wouldn't expect the function transmit() to modify the data that you want to send. So make this explicit:

template <typename T>
class Usart: public Platform {
    ...
    status transmit(const T *pdata, size_t size);
    ...
}

Consider templating transmit() and receive() instead of the whole class

The only things that depend on the template parameter T are the transmit() and receive() functions. By making the class templated, you basically lock the type of data you can send and receive when you instantiate the class. Consider instead templating just those two functions, so you can send and receive different data types on the same UART without having to reinstantiate the class:

class Usart: public Platform {
    ...
    template <typename T>
    status transmit(const T *pData, size_t size);

    template <typename T>
    status receive(T *pData, size_t size);
    ...
};

Use Doxygen to document your code

You are documenting your code, which is good practice, but consider doing it in Doxygen's format, so you can have Doxygen create cross-referenced documentation in HTML, PDF and other forms. Doxygen can also check that you documented all the functions and all the parameters to the functions.