My first STM32 code - please criticize me

Question

I have just wrote my first code on STM32 - blinking LED. It combines a fragments from different sources; please criticize me so I can learn how to write proper code and don't learn stupid habits.

#include "stm32f30x.h"

void SysTick_Handler(void);
void TimingDelay_Decrement(void);
void Delay(__IO uint32_t nTime);

static __IO uint32_t TimingDelay;

int main(void)
{
  RCC->AHBENR  |=  ((1UL << 21) );                  // Enable GPIOE clock??
  GPIOE->MODER |= (1 << 2*8);                       // PIN 9 as output

//GPIOE->BSRR = 1 << 9;                             // LED ON
//GPIOE->BSRR = 1 << 9 << 16;                       // LED OFF
//GPIOE->BRR = 1 << 9;                              // LED OFF

    if (SysTick_Config(SystemCoreClock / 1000))
    { 
    /* Capture error */ 
    while (1);
  }

    while(1)
    {
        GPIOE->BSRR = 1 << 8;   
        Delay(50);
        GPIOE->BRR = 1 << 8;
        Delay(50);
    }
}

void SysTick_Handler(void)
{
  TimingDelay_Decrement();
}

void Delay(__IO uint32_t nTime)
{
  TimingDelay = nTime;

  while(TimingDelay != 0);
}

void TimingDelay_Decrement(void)
{
  if (TimingDelay != 0x00)
  { 
    TimingDelay--;
  }

}

Why I have to enable GPIOE clock? And what's the difference between 1UL << 21 and 1 << 21?

Answer 1

On the STM32s, all peripherals are by default disabled and don't get any clock signals. This saves a bunch of power (and forces you to think about interactions between pins as you code). If you want to use GPIOE, you need to tell the STM32 to enable its clock.

1UL << 21 shifts an unsigned long value of 1 21 bits to the left. 1 << 21 shifts some number valued at 1, but it's some default type, which may or may not be unsigned long, and you don't necessarily know how wide your system defaults are. It will vary by platform and compiler. In fact, UL can vary by platform and compiler as well. It is best to use the library typedefs, which are sure things.

(uint32_t)1 << 21 and (uint64_t)1 << 21 are unambiguous.

Answer 2

Overall, looks pretty nice. Could use some more comments.

Is SysTick_Handler an interrupt handler? If so, say so. I'm surprised there are no attributes declaring this (most embedded C's for other processors have the word interrupt somewhere i the definition of the handler name). I assume that must be set up in a #DEFINE in a system .h file somewhere. Too bad they hide it away like that.

How often does the SysTick_Handler get called? I know that's set up in the SysTick_Config call, but you don't say anything about it.

What are the units for the Delay call? Milliseconds? Say so.

Which LED on the board are you manipulating with GPIOE->BSRR = 1 << 8 ?

Is the function TimingDelay_Decrement really necessary? Why not just put its code inside SysTick_Handler? One usually tries to avoid unnecessary calls within a interrupt handler. Doesn't make a difference here, but could in some time critical handlers.

Answer 3

For the sake of readability, and to reduce the likelihood of mistakes, use macros instead of magic numbers.

stm32f30x.h contains macros for (hopefully) all the register values. So:

RCC->AHBENR  |=  ((1UL << 21) );                  // Enable GPIOE clock??
GPIOE->MODER |= (1 << 2*8);                       // PIN 9 as output

can be written as:

RCC->AHBENR  |= RCC_AHBENR_GPIOEEN;    // Enable GPIOE clock.
GPIOE->MODER |= GPIO_MODER_MODER8_0;   // PIN 8 - not 9! - as output.

(Indeed - the original comment was wrong here; using a macro makes this more obvious!)

Similarly, the pin toggling code could be written as:

GPIOE->BSRR = GPIO_BSRR_BS_8;   
Delay(50);
GPIOE->BRR = GPIO_BRR_BR_8;
Delay(50);

---

Going further, GPIO_MODER_MODER8_0 isn't all that clear, so I might add a macro of my own:

#define GPIO_MODER_MODER8_OUT   GPIO_MODER_MODER8_0

That could get a bit tiresome if you were setting up a lot of pins, so you could do:

#define GPIO_MODER_OUT(pin)     (((uint32_t) 1) << (2 * (pin)))

In addition the above line assumes that those bits in MODER were already 0x0 or 0x1. If we can't assume that, then we need to clear them first:

GPIOE->MODER &= ~GPIO_MODER_MODER8;
GPIOE->MODER |= GPIO_MODER_OUT(8);

---

Also:

If the functions declared at the top of the file are not used anywhere else, declare them static. This prevents them being used in any other file by mistake.

If the compiler allows, main should be declared as void, not returning an int, as it never returns.

This is a real nitpick: TimingDelay is just a regular number; it's not being used where a hexadecimal representation is appropriate. So in TimingDelay_Decrement, where it's compared to zero, use 0, not 0x00. This also makes it consistent with the comparison in Delay.

If I've understood correctly, __IO marks a variable as volatile, and is, I suspect, used to mark registers which are readable and writeable. TimingDelay does indeed need to be volatile, as it's updated in an interrupt - but I'd be inclined to use volatile rather than __IO, to indicate that it's not a register.

nTime does not need to be volatile (or __IO), as it's just a normal function parameter, passed by value, which is read once.

Answer 4

This technically should belong on code review.

My only recommendation is that you bit masking/shifting should either be macros or functions that describe what's going on. Comments are fine but they don't function and ultimately whatever the comments says means nothing; it's the code that counts (and they WILL get out-of-sync). Make the code BE the comments with functional decomposition:

RCC->AHBENR  |=  ((1UL << 21) );  

becomes

enableGpioe() {
    RCC->AHBENR  |=  (1UL << 21);  
}

and this

GPIOE->BSRR = 1 << 9; 

becomes

void setLEDStatus(LED led, Status status) {
     led = 1 << status;     // I don't know your struct members.
}

when

Led

and

Status

are enums. At a minimum, you can make them MACROS. This will alleviate you from having to remember the right bit masking to do the task.