Software drivers for the mcu peripherals in C++

Question

I have been developing software drivers for the mcu peripherals in C++. My approach how to implement the drivers is following.

Each peripheral has its software driver modeled by C++ class with its interface

Peripheral.h

#include <cinttypes>
#include "DriversCfg.h"
class Peripheral {
public:
 
 Peripheral(const PeripheralCfg& peripheral_cfg, uint32_t base_address);
 
 void enable();
 void toggleReset();
 bool isEnabled();
 
private:
 
 struct ControlReg
 {
 uint32_t enable_bit: 1;
 uint32_t reset_bit: 1;
 uint32_t cfg_bit_01: 1;
 uint32_t cfg_bit_02: 1;
 uint32_t cfg_bit_03: 1;
 };
 
 struct PeripheralRegs
 {
 volatile ControlReg control_reg;
 };
 
 PeripheralRegs* regs; 
 
};

and its implementation

Peripheral.cpp

#include <new>
#include "Peripheral.h"
Peripheral::Peripheral(const PeripheralCfg& peripheral_cfg, uint32_t base_address) {
 regs = new (reinterpret_cast<void*>(base_address)) PeripheralRegs;
 
 regs->control_reg.cfg_bit_01 = static_cast<uint32_t>(peripheral_cfg.cfg_bit_1);
 regs->control_reg.cfg_bit_02 = static_cast<uint32_t>(peripheral_cfg.cfg_bit_2);
 regs->control_reg.cfg_bit_03 = static_cast<uint32_t>(peripheral_cfg.cfg_bit_3);
}
void Peripheral::enable()
{
 regs->control_reg.enable_bit = 1;
}
 
bool Peripheral::isEnabled()
{
 return ((regs->control_reg.enable_bit) != 0);
}
void Peripheral::toggleReset()
{
 if(regs->control_reg.reset_bit)
 {
 regs->control_reg.reset_bit = 0;
 }
 else
 {
 regs->control_reg.reset_bit = 1;
 }
}

Each peripheral has fixed part of its configuration i.e. part of configuration which is not changeable during run-time

PeripheralCfg.h

enum class FunctionUsage
{
 kNotUsed,
 kUsed
};
struct PeripheralCfg
{
 FunctionUsage cfg_bit_1;
 FunctionUsage cfg_bit_2;
 FunctionUsage cfg_bit_3;
};

There is a "container" containig the fixed part of the configuration for all the peripherals

DriversCfg.h

#include "PeripheralCfg.h"
struct DriversCfg 
{
 PeripheralCfg periph_cfg_01 = {FunctionUsage::kAdcNotUsed, FunctionUsage::kAdcUsed, 
 FunctionUsage::kAdcUsed};
 PeripheralCfg periph_cfg_02 = {FunctionUsage::kAdcNotUsed, FunctionUsage::kAdcNotUsed, 
 FunctionUsage::kAdcUsed};
};

There is a "container" containing instances of all the drivers

Hal.h

#include "Peripheral.h"
#include "DriversCfg.h"
class Hal {
public:
 
 Peripheral *periph_01;
 Peripheral *periph_02;
 
 Hal();
 
private:
 
 DriversCfg configuration;
 
};

Hal.cpp

#include "Hal.h"
#include "DriversCfg.h"
#include "Registers.h"
Hal::Hal() {
 periph_01 = new Peripheral(configuration.periph_cfg_01, PERIPH_01_BASE_ADDRESS);
 periph_02 = new Peripheral(configuration.periph_cfg_02, PERIPH_02_BASE_ADDRESS);
}

The intended usage is

main.cpp

int main(int argc, char** argv)
{
 Hal hal;
 
 hal.periph_01->enable();
 hal.periph_02->enable();
}

I have tested this approach and it works. Nevertheless I have some doubts regarding namely following aspects:

• container DriversCfg with configuration for the peripherals in which the individual configurations are public - is it a problem here?
• order of instantiation of the private attribute DriversCfg configuration in the class Hal more precisely whether there is a chance that the DriversCfg configuration will not be created at the moment when a reference to it will be passed into the peripheral drivers?

Thanks in advance for any remarks and suggestions.

• Reinderien: What is the target platform for this code?
• Reinderien: And what compiler is this for?

Answer 1

Avoid the placement new

The placement new you are doing looks redundant. PeripheralRegs is a POD type, so its member values will probably not be initialized, but to avoid any doubt I would just write:

Peripheral::Peripheral(const PeripheralCfg& peripheral_cfg, uint32_t base_address):
 regs{reinterpret_cast<PeripheralRegs*>(base_address)}
{
 regs->control_reg.cfg_bit_01 = static_cast<uint32_t>(peripheral_cfg.cfg_bit_1);
 regs->control_reg.cfg_bit_02 = static_cast<uint32_t>(peripheral_cfg.cfg_bit_2);
 regs->control_reg.cfg_bit_03 = static_cast<uint32_t>(peripheral_cfg.cfg_bit_3);
}

Toggle using XOR

You can rewrite toggleReset() by using the XOR operation:

void Peripheral::toggleReset()
{
 regs->control_reg.reset_bit ^= 1;
}

Why the k prefix for enum class values?

I see the values of enum class FunctionUsage are prefixed with a k. This looks like Hungarian notation, but I don't see Hungarian notation used anywhere else in this code. I suggest you drop it. Since it's an enum class, there is no chance that these values would conflict with anything else.

Use arrays where appropriate

I see several variables being repeated with just a different number at the end. Those are often a candidate to be replaced by arrays. Consider writing:

struct PeripheralCfg
{
 FunctionUsage cfg_bits[3];
};
...
struct DriversCfg 
{
 PeripheralCfg periph_cfgs[2] = {
 {FunctionUsage::kAdcNotUsed, FunctionUsage::kAdcUsed, FunctionUsage::kAdcUsed},
 {FunctionUsage::kAdcNotUsed, FunctionUsage::kAdcNotUsed, FunctionUsage::kAdcUsed},
 };
};
...
class Hal {
 ...
 Peripheral *peripherals[2];
 ...
};

And so on. You can always rename the variables you used before to refer to an array element, for example periph_01 becomes periph[0]. Also, it opens up possibilities to use for-loops to avoid repetition:

for (auto &periph: hal.peripherals)
 periph->enable();

Although it looks silly now with an array of only two elements, it will quickly become more useful if you have to support hardware which supports more peripherals.

Make Hal::configuration static const

It looks like Hal::configuration is only meant to be read once, and never changed. You should change it into a static const variable:

class Hal {
 ...
 static const DriversCfg configuration;
 ...
};

Although it does look a bit weird this way. If it's only used by the constructor of Hal, then this should not be a (static) class member at all. You could move it completely into Hal.cpp:

static const struct
{
 PerpheralCfg periph_configs[2] = {...};
} configuration;
Hal::Hal() {
 perhiperal[0] = new Peripheral(configuration.periph_cfgs[0], PERIPH_01_BASE_ADDRESS);
 perhiperal[1] = new Peripheral(configuration.periph_cfgs[1], PERIPH_02_BASE_ADDRESS);
}

You could also keep the configuration in DriversCfg.h, but then still exactly as I've shown here.

As for the order of instantiation: global variables are instantiated before main() runs, so configuration will be valid before the variable hal is constructed. If it was still a regular member variable, then it would also have been instantiated before the body of the constructor was run, so your code is fine in that respect.

Avoid unnecessary memory allocations

Why does Hal need to allocate the Peripherals on the heap? Especially on an MCU you want to avoid dynamic memory allocations. So instead of having pointers to Peripherals inside class Hal, just have them by value:

class Hal {
public:
 Peripheral peripherals[2];
 Hal();
};
...
Hal::Hal():
 peripherals{
 {configuration.periph_cfgs[0], PERIPH_01_BASE_ADDRESS},
 {configuration.periph_cfgs[1], PERIPH_02_BASE_ADDRESS},
 }
{}

And then use it like so:

int main(...)
{
 Hal hal;
 for (auto &periph: hal.peripherals)
 periph.enable();
}

• Student: thank you very much for your remarks. Please can you explain to me why it is necessary to use the initialization part of the constructor of the Hal for initializing the instances of the Peripheral class in the peripherals array instead of initializing the instances in the constructor body?
• G. Sliepen: @L3sek In this case it doesn't really matter. If the variables you initialize are more complex, with their own constructors/destructors, then it might be slightly more efficient, or the only way possible; if it's in the body the variables will first be default constructed, then copy-assigned (and not all classes can be default constructed and/or copy-assigned to). And if something in the constructor body can throw, then it becomes really important that the member variables are properly initialized before the body runs, to avoid things being left in an inconsistent state.