Mein erster STM32-Code - bitte kritisieren Sie mich

7

Ich habe gerade meinen ersten Code auf STM32 geschrieben - blinkende LED. Es kombiniert Fragmente aus verschiedenen Quellen; Bitte kritisieren Sie mich, damit ich lernen kann, wie man richtigen Code schreibt und keine dummen Gewohnheiten lernt.

#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--;
  }

}

Warum muss ich die GPIOE-Uhr aktivieren? Und was ist der Unterschied zwischen 1UL << 21und 1 << 21?

Ekci
quelle
1
Dieser Wiki-Artikel ist allgemein sehr hilfreich: en.wikipedia.org/wiki/Best_coding_practices
Nick Williams
4
Es gibt eine separate StackExchange-Site für die Codeüberprüfung , aber sie möchten im Allgemeinen gezieltere Fragen als "Bitte kritisieren Sie meinen Code".
Dave Tweed
2
@ Dave Ich glaube, dies ist ein besserer Ort, um Code zu überprüfen, der speziell für einen Mikrocontroller geschrieben wurde.
m.Alin
2
"Bitte kritisieren Sie mich" Ihre Codeeinrückung ist inkonsistent.
Cole Johnson
2
Warum verwenden Sie nicht die Standard Peripheral Library? Es verbessert die Lesbarkeit erheblich und verringert die Wahrscheinlichkeit von Fehlern, die beim direkten Registerzugriff auftreten können.
Echtzeit

Antworten:

10

Bei den STM32 sind standardmäßig alle Peripheriegeräte deaktiviert und erhalten keine Taktsignale. Dies spart eine Menge Energie (und zwingt Sie, beim Codieren über Wechselwirkungen zwischen Pins nachzudenken). Wenn Sie GPIOE verwenden möchten, müssen Sie den STM32 anweisen, seine Uhr zu aktivieren.

1UL << 21verschiebt einen unsigned longWert von 1 21 Bit nach links. 1 << 21Verschiebt eine Zahl mit dem Wert 1, aber es handelt sich um einen Standardtyp, der möglicherweise nicht vorhanden ist unsigned long, und Sie wissen nicht unbedingt, wie breit Ihre Systemstandards sind. Es wird je nach Plattform und Compiler variieren. Tatsächlich kann UL auch je nach Plattform und Compiler variieren. Es ist am besten, die Bibliothekstypedefs zu verwenden, die sicher sind.

(uint32_t)1 << 21und (uint64_t)1 << 21sind eindeutig.

Scott Seidman
quelle
5

Insgesamt sieht es ganz gut aus. Könnte noch ein paar Kommentare gebrauchen.

Ist SysTick_Handler ein Interrupt-Handler? Wenn ja, sagen Sie es. Ich bin überrascht, dass es keine Attribute gibt, die dies deklarieren (die meisten eingebetteten Cs für andere Prozessoren haben das Wort Interrupt irgendwo in der Definition des Handlernamens). Ich gehe davon aus, dass das in einer #DEFINE in einer System .h Datei irgendwo eingerichtet sein muss. Schade, dass sie es so verstecken.

Wie oft wird der SysTick_Handler aufgerufen? Ich weiß, dass dies im SysTick_Config-Aufruf eingerichtet wurde, aber Sie sagen nichts darüber.

Was sind die Einheiten für den Verzögerungsaufruf? Millisekunden? Sag es.

Welche LED auf der Platine manipulieren Sie mit GPIOE-> BSRR = 1 << 8?

Ist die Funktion TimingDelay_Decrement wirklich notwendig? Warum nicht einfach den Code in SysTick_Handler einfügen? Normalerweise versucht man, unnötige Aufrufe innerhalb eines Interrupt-Handlers zu vermeiden. Macht hier keinen Unterschied, könnte aber in einiger Zeit kritische Handler sein.

tcrosley
quelle
Neugierig: Wenn ich mir Sorgen über den Aufruf von TimingDelay_Decrement mache, kann ich ihn dann zu einer Inline-Funktion machen? Würde das helfen?
Greg d'Eon
2
Es ist ein bisschen anders, aber so wird das Cortex-M-Zeug eingerichtet. Der SysTick_Handler ist ein Standard-Interrupt-Handler für die Cortex-M-Prozessoren, der in der Startdatei eingerichtet wird, die beim Kompilieren verknüpft wird. Es gibt einen Standardhandler, der verwendet wird, wenn der Benutzer keinen eigenen schreibt, der den defaut-Handler überschreibt. (Ich hoffe, ich habe das alles richtig formuliert. Ich bin hauptsächlich Hardware-Ingenieur). Der Aufruf: SysTick_Config (SystemCoreClock / 1000) richtet den SysTick_Handler so ein, dass er jede Millisekunde unterbricht. Wenn SysTick_Config (SystemCoreClock / 100) geschrieben würde, würde es alle 10 ms ankreuzen.
Tut
... Standard-Cortex-M-Zeug, aber ich stimme zu, dass Kommentare helfen würden.
Tut
1
@ m.Alin Gute Frage und ich bin mir nicht mal sicher, ob es im obigen Beispiel definiert ist oder nicht. In meinen Systemen, die STM32F4 verwenden, gibt es eine Initialisierungsdatei system_stm32f4xx.c, die die Systemuhren einrichtet und die globale Variable SystemCoreClock definiert, die auf einen Wert gesetzt wird, der der Kerntaktrate entspricht (für meine Anwendung 168000000, dh 168 MHz). Der Aufruf von SysTick_Config richtet einen Taktteiler ein, um die Tickrate zu definieren. Wenn Sie SysTick_Config (SystemCoreClock) aufrufen, wird dies einmal pro Sekunde angekreuzt. Ich muss sagen, dass einige der Cortex-M-Inhalte (meiner Meinung nach) verwirrend eingerichtet sind.
Tut
1
@Kynit Ja, TimingDelay_Decrement zu machen wäre dasselbe wie den Code in den Handler aufzunehmen, wie ich vorgeschlagen habe.
Tcrosley
3

Verwenden Sie zur besseren Lesbarkeit und zur Verringerung der Fehlerwahrscheinlichkeit Makros anstelle von magischen Zahlen.

stm32f30x.h enthält Makros für (hoffentlich) alle Registerwerte. Damit:

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

kann geschrieben werden als:

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

(In der Tat - der ursprüngliche Kommentar war hier falsch; die Verwendung eines Makros macht dies offensichtlicher!)

In ähnlicher Weise könnte der Pin-Umschaltcode wie folgt geschrieben werden:

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

Weiter zu gehen GPIO_MODER_MODER8_0ist nicht ganz klar, also könnte ich ein eigenes Makro hinzufügen:

#define GPIO_MODER_MODER8_OUT   GPIO_MODER_MODER8_0

Das könnte etwas lästig werden, wenn Sie viele Stifte einrichten, also könnten Sie Folgendes tun:

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

Außerdem wird in der obigen Zeile davon MODERausgegangen, dass diese Bits bereits 0x0 oder 0x1 waren. Wenn wir das nicht annehmen können, müssen wir sie zuerst löschen:

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

Ebenfalls:

Wenn die oben in der Datei deklarierten Funktionen nirgendwo anders verwendet werden, deklarieren Sie sie static. Dies verhindert, dass sie versehentlich in einer anderen Datei verwendet werden.

Wenn der Compiler dies zulässt, mainsollte deklariert werden als void, ohne ein zurückzugeben int, da es niemals zurückgibt.

Dies ist ein echter Trottel: TimingDelayist nur eine reguläre Zahl; Es wird nicht verwendet, wenn eine hexadezimale Darstellung angemessen ist. Also TimingDelay_Decrement, wo es mit Null verglichen wird, verwenden Sie 0nicht 0x00. Dies macht es auch konsistent mit dem Vergleich in Delay.

Wenn ich richtig verstanden habe, __IOmarkiert eine Variable als volatileund wird, wie ich vermute, verwendet, um Register zu markieren, die lesbar und beschreibbar sind. TimingDelaymuss in der Tat sein volatile, da es in einem Interrupt aktualisiert wird - aber ich würde volatileeher verwenden als __IO, um anzuzeigen, dass es kein Register ist.

nTimemuss nicht volatile(oder __IO) sein, da es sich nur um einen normalen Funktionsparameter handelt, der als Wert übergeben wird und einmal gelesen wird.

Steve Melnikoff
quelle
1

Diese technisch sollte gehören Code - Review .

Meine einzige Empfehlung ist, dass Sie Bitmaskierung / Verschiebung entweder Makros oder Funktionen sein sollten, die beschreiben, was los ist. Kommentare sind in Ordnung, funktionieren aber nicht und was auch immer die Kommentare sagen, bedeutet letztendlich nichts. Es ist der Code, der zählt (und sie werden nicht mehr synchron sein). Machen Sie den Code zu den Kommentaren mit funktionaler Zerlegung:

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

wird

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

und das

GPIOE->BSRR = 1 << 9; 

wird

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

wann

LED

und

Status

sind Aufzählungen. Zumindest können Sie sie MACROS machen. Dadurch müssen Sie sich nicht mehr an die richtige Bitmaskierung erinnern, um die Aufgabe auszuführen.

Christian Bongiorno
quelle
2
Während es tatsächlich zu CodeReview.SE gehören könnte, sind die Leute dort weniger hilfreich, wenn es um eingebetteten Code und Praktiken geht. Einige Leute können vielleicht helfen, aber wahrscheinlich weniger als hier.
Morwenn
Code ist Code :) - Sie haben vielleicht Recht mit der Einstellung dort, aber es ist viel wahrscheinlicher, dass Sie dort auch eine Antwort erhalten.
Christian Bongiorno