Skip to main content
Code Review

Return to Answer

added 878 characters in body
Source Link
G. Sliepen
G. Sliepen
  • 61.7k
  • 3
  • 61
  • 152

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);

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);
Source Link
G. Sliepen
G. Sliepen
  • 61.7k
  • 3
  • 61
  • 152

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)) {
}

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.