8
\$\begingroup\$

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?

\$\endgroup\$
7
  • 1
    \$\begingroup\$ This wiki article is very helpful generically: en.wikipedia.org/wiki/Best_coding_practices \$\endgroup\$ Mar 5, 2015 at 13:39
  • 4
    \$\begingroup\$ There is a separate StackExchange site for code review, but they generally want more focused questions than "critique my code, please". \$\endgroup\$
    – Dave Tweed
    Mar 5, 2015 at 13:45
  • 2
    \$\begingroup\$ @Dave I believe this is a better place to review code written specifically for a microcontroller.. \$\endgroup\$
    – m.Alin
    Mar 5, 2015 at 16:01
  • 2
    \$\begingroup\$ "Please criticize me" Your code indentation is inconsistent. \$\endgroup\$
    – Cole Tobin
    Mar 5, 2015 at 19:44
  • 2
    \$\begingroup\$ Why don't you use the Standard Peripheral Library? It greatly improves readability and reduces the chance of errors that can occur with direct register access. \$\endgroup\$
    – realtime
    Mar 5, 2015 at 21:50

4 Answers 4

10
\$\begingroup\$

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.

\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ Curious: if I was worried about the call to TimingDelay_Decrement, could I make it an inline function? Would that help? \$\endgroup\$
    – Greg d'Eon
    Mar 5, 2015 at 14:46
  • 2
    \$\begingroup\$ It's a little different, but that's how the Cortex-M stuff is setup. The SysTick_Handler is a standard interrupt handler for the Cortex-M processors that is setup in the startup file that gets linked in when compiled. There is a default handler that is used if the user does not write their own which overrides the defaut handler. (I hope I worded all that correctly. I'm mainly a hardware engineer). The call: SysTick_Config(SystemCoreClock / 1000) will setup the SysTick_Handler to interrupt every millisecond. If it were written SysTick_Config(SystemCoreClock / 100), it would tick every 10ms. \$\endgroup\$
    – Tut
    Mar 5, 2015 at 15:03
  • \$\begingroup\$ ... standard Cortex-M stuff, but I agree that comments would help. \$\endgroup\$
    – Tut
    Mar 5, 2015 at 15:05
  • 1
    \$\begingroup\$ @m.Alin Good question and I'm not even sure if it is defined in the above example or not. In my systems using STM32F4, there is an initialization file system_stm32f4xx.c which sets up the system clocks and defines the global variable SystemCoreClock which gets set to a value equal to the core clock speed (for my application 168000000 ie 168Mhz). The call to SysTick_Config sets up a clock divider to define the tick rate. If you called SysTick_Config(SystemCoreClock), it would tick once a second. I must say that some of the Cortex-M stuff is setup in a confusing way (in my opinion). \$\endgroup\$
    – Tut
    Mar 5, 2015 at 16:16
  • 1
    \$\begingroup\$ @Kynit yes, making TimingDelay_Decrement would be the same as including the code within the handler as I suggested. \$\endgroup\$
    – tcrosley
    Mar 5, 2015 at 18:31
3
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ While it could indeed belong on CodeReview.SE, people over there are less likely to be helpful when it comes to embedded code and practices. Some people may be able to help, but probably less than here. \$\endgroup\$
    – Morwenn
    Mar 6, 2015 at 9:08
  • \$\begingroup\$ Code is code :) -- You might be right about the attitude there but you're much more likely to get response there too. \$\endgroup\$ Mar 6, 2015 at 22:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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