1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ What is pSPIx in the first snippet? \$\endgroup\$ – vnp May 14 at 20:16
  • \$\begingroup\$ Typo, thanks for catching. The argument to the function should have been pSPIx, a pointer to a SPI_RegDef_t struct (the registers of the SPI peripheral). \$\endgroup\$ – Minh Tran May 15 at 0:56
3
\$\begingroup\$

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. :)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ For example, why is EnableStatus a typedef for uint8_t instead of bool. I thought C didn't define bool. (After some googling) I guess it does if the cross compiler uses C99. I will use bool from now on. That brings up another question: should you create macros for binary choices (True/False, Set/Clear, Set/Reset, On/Off, Enable/Disable), as you can imagine, using True to indicate the status of an LED or to say that clock has been enabled impairs readability. \$\endgroup\$ – Minh Tran May 15 at 1:12
  • \$\begingroup\$ Of course it's hard to know if we've done it right, when we don't know what top-level tasks need accomplishing. I'm a beginner and I haven't used SPI much to know how it would be called or the many different contexts in which it could be called (i.e., concurrently or inside an interrupt). Being a communication protocol, SPI could be used to communicate with sensors/actuators or to send data to another MCU. To test it, I'll probably use SPI1 to communicate with SPI2 (i.e., the MCU sends data to itself). \$\endgroup\$ – Minh Tran May 15 at 1:24
  • 1
    \$\begingroup\$ 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. That's a good suggestion. I'll refactor my code, code up a use case, and create a follow-up post. \$\endgroup\$ – Minh Tran May 15 at 1:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.