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:
- 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.
- 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.
pSPIx
in the first snippet? \$\endgroup\$pSPIx
, a pointer to aSPI_RegDef_t
struct (the registers of the SPI peripheral). \$\endgroup\$