MCU (Embedded C): Use nested if/elses or use an ugly function?

Question

I'm writing driver code for the SPI peripheral (as part of a tutorial) on a Nucleo64 (STM32F446RE). The instructor recommends writing this code:

// General macros
#define EnableStatus uint8_t
#define DISABLE      0
#define ENABLE       !DISABLE

void SPI_EnablePeripheralClock(SPI_RegDef_t *pSPIx, EnableStatus status)
{
    if (status) {
        if (pSPIx == SPI1)
            SPI1_PCLK_ENA();
        else if (pSPI == SPI2)
            SPI2_PCLK_ENA();
        // ... done for 4 SPI modules
    } else { 
        if (pSPIx == SPI1)
            SPI1_PCLK_DIS(); 
        else if (pSPI == SPI2)
            SPI2_PCLK_DIS();
        // ... done for 4 SPI modules
}

...
// You can treat RCC->APB2ENR as the value at some address. ENA() sets a bit. DIS() clears a bit.
#define SPI1_CLK_ENA()       (RCC->APB2ENR |= (1 << 12))
#define SPI2_CLK_ENA()       (RCC->APB1ENR |= (1 << 14))
#define SPI1_CLK_DIS()       (RCC->APB2ENR &= ~(1 << 12))
#define SPI2_CLK_DIS()       (RCC->APB1ENR &= ~(1 << 14))
...
#define IO volatile

typedef struct {
    IO uint32_t CR1;        // Control Reg. 1
    IO uint32_t CR2;        // Control Reg. 2
    IO uint32_t SR;         // Status Reg.
    IO uint32_t DR;         // Data Reg.
    IO uint32_t CRCPR;      // CRC Polynomial Reg.
    IO uint32_t RXCRCR;     // RX CRC Reg.
    IO uint32_t TXCRCR;     // TX CRC Reg.
    IO uint32_t I2SCFGR;    // I2S Configuration Reg.
    IO uint32_t I2SPR;      // I2S Prescaler Reg.
} SPI_RegDef_t;
#define SPI1 ((SPI_RegDef_t*)SPI1_BASEADDR)
#define SPI2 ((SPI_RegDef_t*)SPI2_BASEADDR)

Several things I don't like about this implementation:

1. The chain of if-elses in the function to set or clearing one bit. It's a lot of lines-of-code to express a simple task.
2. It's comparing peripheral device base addresses to figure out which of the four SPI peripheral it's working with.

I rewrote it like this to address the issues:

// Assigns the boolean value b to the p'th bit position (0-based indexing) of the number `num`.
void BitAssign(volatile uint32_t *num, uint8_t p, bool b)
{
    // https://stackoverflow.com/a/47990/3396951
    b = !!b; // booleanize b (if it was previously non-zero, it is now 1 otherwise it's 0)
    *num = (*num & ~(1UL << p)) | (b << p);
}

// To be used as first argument in SPI_EnablePeripheralClock() and other functions
#define SPI1 1
#define SPI2 2
#define SPI3 3
#define SPI4 4

void SPI_EnablePeripheralClock(uint8_t SPIx, EnableStatus status)
{
    // 12, 14, 15, 13 are values provided by the datasheet
    if (SPIx == SPI1)
        BitAssign(&RCC->APB2ENR, 12, status);
    else if (SPIx == SPI2)
        BitAssign(&RCC->APB1ENR, 14, status);
    else if (SPIx == SPI3)
        BitAssign(&RCC->APB1ENR, 15, status);
    else if (SPIx == SPI4)
        BitAssign(&RCC->APB2ENR, 13, status);
}

This avoids relying on the ENA()/DIS() functions (I can remove them) and utilizes a bit-operation to perform the bit assignment. I'm unsure if BitAssign() is a good idea since I might have to overload it for non-volatile parameters.

Answer 1

The variable names in both of those snippets are absolutely insane! But at least it seems clear on close inspection that ENA and DIS are abbreviations of "enable" and "disable." I would write those out.

I would rewrite the instructor's code like this:

typedef uint8_t EnableStatus;

void SPI_EnablePeripheralClock1(EnableStatus status) {
    if (status) {
        RCC->APB2ENR |= (1 << 12);
    } else {
        RCC->APB2ENR &= ~(1 << 12);
    }
}

void SPI_EnablePeripheralClock2(EnableStatus status) {
    if (status) {
        RCC->APB1ENR |= (1 << 14);
    } else {
        RCC->APB1ENR &= ~(1 << 14);
    }
}

void SPI_EnablePeripheralClock(SPI_RegDef_t *pSPI, EnableStatus status) {
    switch (pSPIx) {
        case SPI1: SPI_EnablePeripheralClock1(status); break;
        case SPI2: SPI_EnablePeripheralClock2(status); break;
        default: assert(false); break;  // what goes here?
    }
}

But it really depends on how this SPI_EnablePeripheralClock function is going to be used "upstream" in the caller. For example, why is EnableStatus a typedef for uint8_t instead of bool?

In fact, I would be tempted to write two convenience functions with English names:

void EnablePeripheralClock() {
    SPI_EnablePeripheralClock(&spi, true);
}
void DisablePeripheralClock() {
    SPI_DisablePeripheralClock(&spi, false);
}

Alternatively, it might make sense to refactor along those lines from the very beginning...

void EnablePeripheralClock() {
    switch (pSPIx) {
        case 1: RCC->APB2ENR |= (1 << 12); break;
        case 2: RCC->APB1ENR |= (1 << 14); break;
        default: assert(false);
    }
}

void DisablePeripheralClock() {
    switch (pSPIx) {
        case 1: RCC->APB2ENR &= ~(1 << 12); break;
        case 2: RCC->APB1ENR &= ~(1 << 14); break;
        default: assert(false);
    }
}

Here we're breaking down the code from the top down, focusing on "what task needs accomplishing" (e.g. "disable the clock"), rather than taxonomizing from the bottom up. Of course it's hard to know if we've done it right, when we don't know what top-level tasks need accomplishing. :)