Abstraction for embedded C++ register operations

Question

I just started with embeddeded C++ since I only coded in C for the last year and I wanted to do/learn something while I'm in university.
First of the there is the Register manipulation class. I won't include the .cpp file since stuff done there is trivial.

register.hpp:

class Reg {
public:
    Reg(uint32_t *reg, uint32_t reset_value);
    Reg();
    uint32_t read();
    bool bit_set(int n);
    void reset();
    void set_bit(int n);
    void write(uint32_t n);
    void clear_bit(int n);

private:
    uint32_t *reg;
    uint32_t reset_value;
};

gpio_port.hpp:

class gpio_port {
public:
    gpio_port(GPIO_TypeDef *port);
    gpio_port();
    Reg moder;
    Reg otyper;
    Reg ospeedr;
    Reg pupdr;
    Reg idr;
    Reg odr;
    Reg bsrr;
    Reg lckr;
    Reg afrl;
    Reg afrh;
private:
    GPIO_TypeDef *port;
};

gpio_port.cpp:

gpio_port::gpio_port(GPIO_TypeDef* port) {
    this->port = port;
    this->moder     =   Reg((uint32_t *) &this->port->MODER, 0x0000);
    this->otyper    =   Reg((uint32_t *) &this->port->OTYPER, 0x0000);
    this->ospeedr   =   Reg((uint32_t *) &this->port->OSPEEDR, 0x0000);
    this->pupdr     =   Reg((uint32_t *) &this->port->PUPDR, 0x0000);
    this->idr       =   Reg((uint32_t *) &this->port->IDR, 0x0000);
    this->odr       =   Reg((uint32_t *) &this->port->ODR, 0x0000);
    this->bsrr      =   Reg((uint32_t *) &this->port->BSRR, 0x0000);
    this->lckr      =   Reg((uint32_t *) &this->port->LCKR, 0x0000);
    this->afrl      =   Reg((uint32_t *) &this->port->AFR[0], 0x0000);
    this->afrh      =   Reg((uint32_t *) &this->port->AFR[1], 0x0000);
}

gpio_port::gpio_port() {
    this->port = nullptr;
}

cosmic.hpp

namespace csmc {
    namespace peripherals {
        static gpio_port gpioa = gpio_port(GPIOA);
        static gpio_port gpiob = gpio_port(GPIOB);
        static gpio_port gpioc = gpio_port(GPIOC);
        static gpio_port gpiod = gpio_port(GPIOD);
        static gpio_port gpioe = gpio_port(GPIOE);
        static gpio_port gpioh = gpio_port(GPIOH);
    }
};

So here you see the Reg class is the abstraction for each Register. It can be applied to any u32 value.
gpio_port was a simple struct first. But initializing it in the cosmic.hpp for each GPIO port looked really messy so I moved it into a class.
The namespaces are the actual libray I want to write.

Here is some example usage:

#include "cosmic/cosmic.hpp"

using namespace csmc;

int main() {
    gpio_port gpioa = peripherals::gpioa;
    gpio_port gpiob = peripherals::gpiob;
    RCC->AHB1ENR |= RCC_AHB1ENR_GPIOAEN;
    RCC->AHB1ENR |= RCC_AHB1ENR_GPIOBEN;

    gpioa.moder.set_bit(16);
    gpiob.moder.set_bit(16);
    while (1) {
        gpioa.odr.set_bit(8);
        gpiob.odr.set_bit(8);
        delay();
        gpioa.odr.clear_bit(8);
        gpiob.odr.clear_bit(8);
        delay();
    }
}

My naming conventions are not that standard for embeddeded Development as far as I know. I'm open for all suggestions and better code structure. :D

Answer 1

Before you read my comments I did something similar, but just used #defines, what you have done is a good step forwards :)

In Register.hpp

• If a parameter to a function can't be changed make it const. It helps document the code.
• If a function doesn't change member data then make the function const.
• Make the class a template ad you could use it for 8/16/32/64 bit processors without having to change things. (Although templates might not work too well with all embedded compilers) Use an enum for the bit number. You will get compile time checking that the bit is in range. You could make this a template parameter too.

e.g.:

enum class ExampleBits
{
    bit0, bit1, bit2, bit3, bit4, bit5
};

template <class Storage, class Bits>
class Reg
{
public:
    Reg(const Storage* reg, const Storage reset_value);
    Reg();
    Storage read() const;
    bool is_bit_set(const Bits n) const;  // Changed name
    void reset();
    void set_bit(const int n);
    void write(const Storage n);
    void clear_bit(const int n);

private:
    Storage* reg;
    Storage reset_value;
};

gpio_port

• I think public members are almost always a mistake. I can't think of a reason why thats not true here, but I can see why you have done it with the code as it is.
• If you add a public enum to the class with the list of register names and then store the reg items in an "array".
• Provide a function that has a parameter of the above enum and it returns a reference to the reg object.
• You don't need this-> in the constructor.
• You do need to check that port is not null before you use it, otherwise you get a segmentation error.
• Use a static_cast rather than a C style cast.
• You have used a 16 bit reset value in your call to the Reg constructor.
• Naming the parameter and the member variable the same thing is a recipe for confusion, add m_ to all member variables.
• Not setting the Reg values in teh second constructor, means the Reg() constructor must make the Reg::reg value safe, i.e. null it and all the functions in Reg must protect against dereferencing that pointer when its null.

It depends on how you are going to use the code and if all the gpio_ports are always available, but I would be tempted to make a Peripherals class. That way you know they are all available and initialised all at the same time.

Using a using namespace csmc; statement means you don't need the csmc namespace. peripherals is quite a common name and I suggest that you do need csmc so stop being lazy and write it on both of the times you need it. Its actually less characters than the using statement, it means there is no doubt over which peripherals namespace you are using and it makes the code MUCH easier to read. Summary 'There is almost never any reason to write a using namespace xxx; statement, almost.'

Now stop your crying for a minute and go back to the top of this answer, what you have done is much better than my attempt. Its always easier to critic code than write it, and I am going to 'borrow' some ideas from your post to improve mine (when I get round to it:) )