Defining hardware components structure

Question

I'm writing firmware for my Arduino project, and I'm struggling with a clean, scalable hardware mapping structure. Initially, I had the following namespace:

namespace Motherboard {

extern DRV8825 xAxisMotor;
extern MP6500 yAxisMotor;
extern DRV8876 zAxisMotor;
extern ClickEncoder encoder;
extern SSD1331 display;
extern RGBLed rgbIndicator;
extern Relay heaterRelay;

void performMainSequence();
void begin(const EepromData &initialData);

}; // namespace Motherboard

Which I initialized like this:

namespace Motherboard {

//=================== Public ====================
DRV8825 xAxisMotor = {
    23, // ENABLE PIN
    25, // MS0 PIN
    27, // MS1 PIN
    29, // MS2 PIN
    31, // RESET PIN
    33, // SLEEP PIN
    35, // STEP PIN
    37, // DIRECTION PIN
    39, // FAULT PIN

    MOTHERBOARD_X_AXIS_MAX_SPEED,
    MOTHERBOARD_X_AXIS_MIN_POSITION,
    MOTHERBOARD_X_AXIS_MAX_POSITION,
};

MP6500 yAxisMotor = {
    22, // ENABLE PIN
    24, // MS1 PIN
    26, // MS2 PIN
    28, // SLEEP PIN
    30, // STEP PIN
    32, // DIRECTION PIN
    34, // FAULT PIN

    MOTHERBOARD_Y_AXIS_MAX_SPEED,
    MOTHERBOARD_Y_AXIS_MIN_POSITION,
    MOTHERBOARD_Y_AXIS_MAX_POSITION,
};

DRV8876 zAxisMotor = {
    44, // ENABLE PIN
    36, // PHASE PIN
    38, // PMODE PIN
    40, // SLEEP PIN
    42, // FAULT PIN

    MOTHERBOARD_Z_AXIS_MAX_SPEED};

ClickEncoder encoder = {
    45, // A PIN
    43, // B PIN
    41, // SW PIN
    2,  // STEPS PER NOTCH
};

SSD1331 display = {
    53, // CS PIN
    48, // DC PIN
    49, // RESET PIN
};

RGBLed rgbIndicator = {
    13,                     // R PIN
    12,                     // G PIN
    11,                     // B PIN
    RGBLed::COMMON_CATHODE, // COMMON
};

Relay heaterRelay = {
    47 // SWITCH PIN
};

void performMainSequence() {
    //
}

void begin(const EepromData &initialData) {
    //
}
//===============================================
}; // namespace Motherboard

A senior in my company saw this piece of code, suggested moving this to a class and create interfaces for hardware related classes. Therefore, I've created this:

struct MotherboardDevices {
    IStepperDriver* xAxisMotor;
    IStepperDriver* yAxisMotor;
    IDcDriver* zAxisMotor;
    ISwitch* heaterRelay;
    RGBLed* rgbIndicator;
    ClickEncoder* encoder;
    IDisplay* display;
};

class Motherboard {
public:
    MotherboardDevices devices;

    Motherboard();
    Motherboard(const MotherboardDevices _devices);

    void motherboardIsr();

    void performMainSequence();
    void begin(const EepromData &initialData);
};

I was struggling with the way of initializing this class. I eventualy created separate file:

namespace Hardware {
namespace {
DRV8825 initXAxisMotor() {
    DRV8825Pinout xAxisPinout = {
        23, // ENABLE PIN
        25, // MS0 PIN
        27, // MS1 PIN
        29, // MS2 PIN
        31, // RESET PIN
        33, // SLEEP PIN
        35, // STEP PIN
        37, // DIRECTION PIN
        39, // FAULT PIN
    };

    DRV8825 xAxisMotor = {
        xAxisPinout,
        MOTHERBOARD_X_AXIS_MAX_SPEED,
        MOTHERBOARD_X_AXIS_MIN_POSITION,
        MOTHERBOARD_X_AXIS_MAX_POSITION,
    };

    return xAxisMotor;
}

MP6500 initYAxisMotor() {
    MP6500Pinout yAxisPinout = {
        22, // ENABLE PIN
        24, // MS1 PIN
        26, // MS2 PIN
        28, // SLEEP PIN
        30, // STEP PIN
        32, // DIRECTION PIN
        34, // FAULT PIN
    };

    MP6500 yAxisMotor = {
        yAxisPinout,
        MOTHERBOARD_Y_AXIS_MAX_SPEED,
        MOTHERBOARD_Y_AXIS_MIN_POSITION,
        MOTHERBOARD_Y_AXIS_MAX_POSITION,
    };

    return yAxisMotor;
}

DRV8876 initZAxisMotor() {
    DRV8876Pinout zAxisPinout = {
        44, // ENABLE PIN
        36, // PHASE PIN
        38, // PMODE PIN
        40, // SLEEP PIN
        42, // FAULT PIN
    };

    DRV8876 zAxisMotor = {
        zAxisPinout,
        MOTHERBOARD_Z_AXIS_MAX_SPEED,
    };

    return zAxisMotor;
}

Relay initHeaterRelay() {
    Relay heaterRelay = {
        47 // SWITCH PIN
    };

    return heaterRelay;
}

SSD1331 initDisplay() {
    SSD1331 display = {
        53, // CS PIN
        48, // DC PIN
        49, // RESET PIN
    };

    return display;
}

ClickEncoder initEncoder() {
    ClickEncoder encoder = {
        45, // A PIN
        43, // B PIN
        41, // SW PIN
        2,  // STEPS PER NOTCH
    };

    return encoder;
}

RGBLed initRgbIndicator() {
    RGBLed rgbIndicator = {
        13,                     // R PIN
        12,                     // G PIN
        11,                     // B PIN
        RGBLed::COMMON_CATHODE, // COMMON
    };

    return rgbIndicator;
}

MotherboardDevices createDevices() {
    static auto xAxisMotor = initXAxisMotor();
    static auto yAxisMotor = initYAxisMotor();
    static auto zAxisMotor = initZAxisMotor();
    static auto heaterRelay = initHeaterRelay();
    static auto rgbIndicator = initRgbIndicator();
    static auto encoder = initEncoder();
    static auto display = initDisplay();

    return {
        &xAxisMotor,
        &yAxisMotor,
        &zAxisMotor,
        &heaterRelay,
        &rgbIndicator,
        &encoder,
        &display,
    };
}

MotherboardDevices devices = createDevices();

} // namespace

Motherboard motherboard = Motherboard(devices);

} // namespace Hardware

I feel like now it's messier than before and I could've done that better. It also creates a problem with my UI layer. I'm using ArduinoMenu library for creating UI menu system. This UI is built using macros and needs static, compile time access to hardware related methods. That means I can't use dependency injection and pass created Motherboard object to UI class. I would have to pass global Motherboard instance via extern which is ugly. It also doesn't make sense to create multiple instance of this class.

Would keeping this structure as namespace with hardware interfaces better? It would be statically available for the UI, there wouldn't be a way of creating multiple instances. I could initialize all the hardware in the related to header, cpp file. However, what I would essentially do here is create a bunch of global variables. I also don't like the idea of the possibility to modify this namespace from every place.

Edit, adding more context

Motherboard::devices would be accessed from non members, UI event handler example:

result updateZAxisSpeed(eventMask event) {
    if (event == enterEvent) {
        Hardware::motherboard.devices.zAxisMotor->start();
    } else if (event == updateEvent) {
        uint8_t updatedValue = EepromManager::currentSettings.zSpeed;
        Hardware::motherboard.devices.zAxisMotor->setSpeed(updatedValue);
    } else if (event == exitEvent) {
        EepromManager::syncEeprom();
        Hardware::motherboard.devices.zAxisMotor->stop();
    }
    return proceed;
}

Motherboard::begin is a late initialization method. It configures provided devices, like:

devices.encoder->setAccelerationEnabled(false);
devices.encoder->setButtonHeldEnabled(false);
devices.encoder->setDoubleClickEnabled(false);

Motherboard::performMainSequence method:

void Motherboard::performMainSequence() {
    devices.heaterRelay->toggleOn();
    delay(5000);

    long xHalfWay = devices.xAxisMotor->getEndPosition() / 2;

    devices.yAxisMotor->moveToEnd();
    devices.xAxisMotor->moveTo(xHalfWay);
    devices.zAxisMotor->start();
    devices.xAxisMotor->moveToEnd();
    delay(2000);
    devices.xAxisMotor->moveTo(xHalfWay);
    devices.zAxisMotor->stop();
    devices.heaterRelay->toggleOff();
    devices.xAxisMotor->moveToStart();
    devices.yAxisMotor->moveToStart();
}

Motherboard::motherboardIsr() is an interrupt routine:

void Motherboard::motherboardIsr() {
    devices.encoder->service();
}

When Motherboard was a namespace I could just directly asign service() method of my encoder to microcontroller's timer interrupt. Since interrupts needs global context and Motherboard now is a class I had to expose isr routine method and asign it globally:

Timer1.attachInterrupt([]() { Hardware::motherboard.motherboardIsr(); });

Answer 1

Namespace vs. struct

It's great that you created a namespace, that already avoids some of the problems global variables have. However, a struct has some advantages, and it's rather trivial to go from a namespace to a struct:

extern struct {
   DRV8825 xAxisMotor;
   MP6500 yAxisMotor;
   DRV8876 zAxisMotor;
   …
   void performMainSequence();
   void begin(const EepromData &initialData);
} Motherboard;

This declares a global variable Motherboard whose type is a struct holding all the actuator objects. Instead of Motherboard::zAxisMotor.stop(), you then just have to write Motherboard.zAxisMotor.stop().

However, now we can give the struct a name, pass it around, have multiple instances of it, add access control (public/private), and so on. These are all benefits that are not available with a namespace.

In particular, you can mostly avoid needing global variables this way, but since you have the interrupt service routine, you really do need something global; either the object itself, or a pointer to the object which can then be allocated elsewhere. For an embedded device, I would just make a global object, to avoid the overhead of pointer dereferencing.

Interfaces and inheritance

It's great if each device has a consistent interface, however if you go the route of abstract base classes, then you need to use pointers which have to be dereferenced, and those pointers point to vtables which contain the pointers to the actual functions. So that's more dereferencing, which adds some overhead. For an embedded project where you know exactly which type each actuator is, I think that is unnecessary.

You can write code so the compiler can check that all your classes follow a given interface (see this StackOverflow post).

Having a well-defined interface is handy if you want to write generic functions that work with multiple types of actuators. For example, if you want something that moves to start, then moves to end, then moves back to start (maybe for some initialization sequence where it finds the end stops and records encoder positions). You want to write this once so it works for all tyeps of motors. It would then either pass a pointer to an abstract base class:

void doInitSequence(IStepperMotor* motor) {
    motor->moveToStart();
    …
    motor->moveToEnd();
    …
    motor->moveToStart();
}

Or you can make it a template:

template<typename Motor>
void doInitSequence(Motor* motor) {
    motor->moveToStart();
    …
    motor->moveToEnd();
    …
    motor->moveToStart();
}

In the latter case, you can directly pass it a pointer to a DRV8825 without needing inheritance.

Answer 2

I'll expand a bit on your design by introducing an adapter.

Implementation

Firstly, you have somewhere an implementation. This could be a library from the internet that you don't want or can't change. Lets say this looks like this:

class Relay
{
public:
    void Set(bool value);
};

Interface

Now in our application we have defined an interface for working with relays. So there is one single way to work with relays throughout the application.

class IRelay
{
public:
    virtual void TurnOn() = 0;
    virtual void TurnOff() = 0;
};

Adapter

As you can see, the interface is different from the relay implementation. We can't change the implementation because this was a library from the internet. And who knows, we may get another relay in the future that has different methods. (Relay may not be the best example. But what about an IC that measures current?)

Also, we don't want to change the interface, because this would require us to change code all over the place. Besides, if we support 2 different relays with different implementations this isn't possible at all.

This is where the adapter comes in. It forms a link between the library and the interface. Now every time we need to support another relay. We just make another adapter and the relay can be used everywhere in our code.

class RelayAdapter : IRelay
{
    Relay& relay;
public:

    RelayAdapter(Relay& relay) 
        : relay(relay)
    {
    }

    void TurnOn() override {
        relay.Set(true);
        
    }
    void TurnOff()override {
        relay.Set(false);
    }
};

MyDevice

So, how would this look in your code? First, we need to instantiate the relay and the adapter, so it can be used.

class MyDevice
{
    Relay actualRelay1;
    Relay actualRelay2;
    RelayAdapter adapter1 {actualRelay1};
    RelayAdapter adapter2 {actualRelay2};
    
public:
    IRelay relay1 = adapter1;
    IRelay relay2 = adapter2;
}

This class can be used:

int main()
{
    MyDevice device;
    
    device.relay1.TurnOn();
    device.relay2.TurnOff();

    return 0;
}

---

Btw, like G. Sliepen mentioned, keep overhead in mind! For us the overhead is acceptable especially when keeping the advantages in mind. For the ISR, this is probably not preferred. I would do something with an semaphore and a different task to handle isr. Ofcourse, this is dependant on your situation.