Hallo Zusammen,
da ich aus der C# und Anwendungsentwicklung komme und mich in die C Welt
mit Mikrocontroller ebenfalls zurechtfinden möchte, bitte ich Euch auf
meinen Code zu sehen, ob ich vom Grundansatz her auf dem richtigen Weg
bin bzw. Ihr eure Augen verdreht.
Ich möchte mir nicht von Anfang an Murks einprägen :-).
Zur Info ich habe folgendes "Material":
- Atmel Studio 7
- STK600 mit Atmega1284-PU @ 16MHz
Main.c:
charTimer_PulseState;//Abrufen der einzelnen Pulse pro Zyklus.
13
charTimer_ToggleState;//Abrufen des toogle Statuses.
14
15
externTimer_InitTimer1with20ms();//Wird aufgerufen wenn das Programm initialisiert wird (einmalig). Interrupts müssen noch explizit über sei() aktiviert werden.
Ich hoffe das erschlägt jetzt nicht :). Es geht mir jetzt nicht um jedes
kleine Detail. Sondern mehr darum ob dieser Abstrahierungsgrad
vertretbar ist.
Freundliche Grüße
Christian
Als jemand der auch aus der C# Ecke kommt aber schon sehr lange C auf
AVRs programmiert:
Das sieht deutlich besser aus als 99% von dem was sonst hier im Forum
aufschlägt. Hab mir allerdings jetzt nicht jede Zeile genauestens
angeguckt.
Ich würde zumindest das andersrum machen und so nur einen Aussprungpunkt
aus der Routine haben:
1
if(lastTimerIntCounter!=timerIntCounter){
2
// tu was
3
lastTimerIntCounter=timerIntCounter;
4
}
Und nimm statt dieser Zu-Tode-Shifterei-Und-Zusammenpackerei hier als
Zähler einfach ein paar einzelne Bytes oder Worte. So braucht die ISR je
nach µC ewig (zumindest bei einem AVR, der keinen Barrelshifter hat):
1
Timer_InterruptAction(){
2
timerIntCounter=((timerIntCounter&0xFFFFFFF0)|((0x0F&timerIntCounter)+1));//1-4bit für 100ms Trigger
3
timerIntCounter=((timerIntCounter&0xFFFFFF0F)|(((0x0F&(timerIntCounter>>4))+1))<<4);//5-8bit für 200ms Trigger
4
timerIntCounter=((timerIntCounter&0xFFFF00FF)|(((timerIntCounter>>8)+1)<<8));//4-2bit für 500ms Trigger
5
timerIntCounter=((timerIntCounter&0xFF00FFFF)|(((timerIntCounter>>16)+1)<<16));//8-5bit für 1000ms Trigger
6
}
(Ok, ich geb zu, der Compiler kann das optimieren, aber trotzdem ist das
Zusammenpacken und Auseinandernehmen unleserlicher...)
Zudem wäre es sinnvoll, z.B. den 100ms-5er-Zähler nicht einfach
zurückzusetzen, sondern nach Setzen des Flags einfach 5 abzuziehen. Denn
wenn der Zähler aus irgendwelchen Gründen zwischenzeitlich schon bei 8
angekommen wäre, dann hast du mit deiner Methode 60ms "Verlust", ich mit
der Subtraktion nur einen höheren Jitter.
ich würde mal sagen gut gedacht, aber ja nach Compiler und Option nicht
optimal.
1
ISR(TIMER1_COMPA_vect)//20ms Timer
2
{
3
Timer_InterruptAction();
4
}
ein Funktionsaufruf in der ISR führ dazu das alles Register gesichert
werden müssen -> langsam. Mit Link-Time-Optimierung könnte es besser
werden.
auch das Schalten der LEDs mit dem LEDs makro find ich nicht wirklich
schön. müsste man mal genau schauen was der Compiler daraus macht.
Auch das setzen vom DDR bei jedem Schalten vom Ausgang ist nicht
sinnvoll.
Bei µC würde ich mehr als Optimierung achten, auch wenn es nicht immer
notwendig ist.
Hi Zusammen,
wow... vielen Dank für die schnellen Antworten (ich dachte schon ich
kann mich erstmal in´s Bett legen bevor es weiter geht :-)
Lothar M. schrieb:> Christian H. schrieb:>
> Ich würde zumindest das andersrum...
Ich habe das jetzt so umgesetzt:
1
Timer_CheckState()
2
{
3
if(Timer_InterruptCounter)//wenn die 20ms ISR den Counter erhöht hat.
4
{
5
6
counter_100ms+=Timer_InterruptCounter;
7
counter_200ms+=Timer_InterruptCounter;
8
counter_500ms+=Timer_InterruptCounter;
9
counter_1000ms+=Timer_InterruptCounter;
10
11
Timer_InterruptCounter=0;
12
13
Timer_PulseState|=(1<<Timer_20ms);
14
Timer_ToggleState^=(1<<Timer_20ms);
15
16
17
if(counter_100ms>=5)//100ms
18
{
19
Timer_PulseState|=(1<<Timer_100ms);
20
Timer_ToggleState^=(1<<Timer_100ms);
21
counter_100ms-=5;
22
}
23
24
if(counter_200ms>=10)//200ms
25
{
26
Timer_PulseState|=(1<<Timer_200ms);
27
Timer_ToggleState^=(1<<Timer_200ms);
28
counter_200ms-=10;
29
}
30
31
if(counter_500ms>=25)//500ms
32
{
33
Timer_PulseState|=(1<<Timer_500ms);
34
Timer_ToggleState^=(1<<Timer_500ms);
35
counter_500ms-=25;
36
}
37
38
if(counter_1000ms>=50)//1000ms
39
{
40
Timer_PulseState|=(1<<Timer_1000ms);
41
Timer_ToggleState^=(1<<Timer_1000ms);
42
counter_1000ms-=50;
43
}
44
45
}
46
}
die Variable "Timer_InterruptCounter" habe ich jetzt in den
TimerTrigger.h verschoben und zähle diese direkt aus der main.c
innerhalb der ISR hoch.
TimerTrigger.h:
1
...
2
volatileuint8_tTimer_InterruptCounter;// muss in ISR des Timer eingebunden werden: Timer_InterruptCounter += 1;
3
...
main.c:
1
ISR(TIMER1_COMPA_vect)//20ms Timer
2
{
3
Timer_InterruptCounter+=1;
4
}
Peter II schrieb:> auch das Schalten der LEDs mit dem LEDs makro find ich nicht wirklich> schön. müsste man mal genau schauen was der Compiler daraus macht.>> Auch das setzen vom DDR bei jedem Schalten vom Ausgang ist nicht> sinnvoll.
Ist es sinnvoll einen Ausgang/Eingang mit einem struct zu definieren?
1
typedefstruct{
2
volatileuint8_t*port;
3
volatileuint8_t*ddr;
4
uint8_tpin;
5
uint8_tlowOrHigh;
6
}Output;
und dann mit spezifischen Funktionen arbeiten z.B.:
- Initialisieren(Output value)
- Schalten(Output value)
- ...
Gruß Christian
Christian H. schrieb:> Ist es sinnvoll einen Ausgang/Eingang mit einem struct zu definieren?
würde ich auch nicht machen.
Ich habe mir nur ein paar Makros in der Art geschrieben.
1
#define LED1_PORT PORTD
2
#define LED1_PIN PIN1
3
#define LED1_INVERT 0
4
5
derAufrufdannmiteinpaarMakros
6
7
SET_OUTPUT(LED1);
8
9
SET_ON(LED1);
10
SET_OFF(LED1);
kann man zwar mit C++ auch über Templates machen, das sehe ich aber kaum
ein Vorteil.
Hallo Peter,
danke für Deine Antwort.
Wie finden in Deinem Code die Methoden SET_OUTPUT, SET_ON und SET_OFF
die Referenz zu den #define´s?
Du übergibst soviel ich sehen kann ja nur LED1 und nicht die einzelnen
LED_PORT usw...
Könntest Du mir bitte den Code von den Methoden aufzeigen?
Gruß Christian
Also ehrlich gesagt, finde ich Deinen Code viel zu unnötig abstrahiert.
a) Als Zeitbasis ist 1ms perfekt, 10ms oder 100ms ebenso, aber 5ms??
--> Timer_InterruptCounter+=2; oder +=20; //im 20ms Interrupt
Das der Kommentar an jedem Timer notwendig ist, zeigt, dass der Code
nicht gut ist.
b) die Bitschiebereien sind völlig unnötig. Deren Beherrschung wird zwar
vorausgesetzt, deren Verwendung hingegen zeugt von wenig Struktur.
Entweder mache Dir Bit-Strukturen (mit struct. im Code z.B.
Timer_PulseState.timer200 = 1;)
Oder mache Dir Funktionen/Makros zum Bitsetzen etc., z.B. SETB(addr,
Bit)
c) Nutze Timer mit je einem festen Wert bei insgesamt einem
Free-Running-Counter. Also z.B. msTick+=20 statt
"Timer_InterruptCounter". Der counter_1000ms wird dann beim ersten Start
auf (msTick+1000) gesetzt und in einer "Differenz"(ganz wichtig!)
ausgewertet, z.B.
[c]
if((msTick-counter_1000ms) > 0) {counter_1000+=1000; .... ;}
d) Versuche Deinen Code wirklich Stück für Stück kompakter und
verständlicher zu machen. Die LEDs beispielsweise sind ein Anfang, aber
die #defines Dafür sind ein Monster. Sie so allgemein zu halten scheint
"flexibel", ist aber absolut keine Hardware-Abstraktion, da genau der
Controller mit der Beschaltung vorausgesetzt wird.
Meine Tipps
Auf auf Mikrocontrollern gibt es C++. Nimm statt structs eine Klasse und
alles ist gut.
Verzichte auf Defines und Macros. Für Defines gibt es feine Sachen wie
static const int HighActive=0;
Aud für Makros normale Funktionen nehmen. Dann kann der Compiler damit
arbeiten und es gibt keine Seiteneffekte.
Und Zeilen länger als die Bildschirmbreit geht gat nicht. Der Code muss
mit einem Blick erfassbar sein, und man muss nicht erst nach rechts
scrollen.
void LampeSwitch(volatile uint8_t *port,
volatile uint8_t *ddr,
uint8_t pin,
uint8_t lowOrHighActive,
uint8_t OnOff
)
{
Ist übersichlicher, man kan die Parameter viel besser erkennen.
Christian H. schrieb:> Hallo Peter,>> danke für Deine Antwort.> Wie finden in Deinem Code die Methoden SET_OUTPUT, SET_ON und SET_OFF> die Referenz zu den #define´s?> Du übergibst soviel ich sehen kann ja nur LED1 und nicht die einzelnen> LED_PORT usw...>> Könntest Du mir bitte den Code von den Methoden aufzeigen?
nicht schön aber funktioniert:
1
#define _SET_INPUT_2( PORT, PIN ) DDR ## PORT &= ~(1<<PIN)
Jaja, hier wirst du auf 10 Leute 12 verschiedene Antworten bekommen. Der
eine will weniger Abstraktion, der andere kommt gleich mit
C++-Klassen...
Prinzipiell sieht der Code ganz gut aus, aber:
Kein Denglisch, bleib einfach überall bei Englisch
Sowas hab ich noch nie gesehen und verwendet auch keiner
1
#define LED3 &PORTB, &DDRB, PINB3, LOWACTIVE
Besser
1
#define LED3_PORT DDRB
In Interrupts die Befehle möglichst direkt abarbeiten und kurz halten.
Prinzipiell ist es eher üblich, einen 1ms "SysTick" zu verwenden. Hier
kannst du einen Counter hochzählen lassen und dann auswerten. Dann muss
man für 500ms auch nicht auf 25 prüfen.
Abstraktion kommt drauf an, was du machen willst. Willst du denn Code
wieder verwenden (auf einem anderen Prozessor), dann macht das Sinn. Ist
der Code nur für diesen einen Controller? Spar dir die Mühe.
Sowas kann man durchaus machen. Erhöht die Lesbarkeit und die
Portabilität, aber ein paar Assembler-Freaks hier rotieren gerade auf
ihrem Bürostuhl :)
Christian H. schrieb:> die Variable "Timer_InterruptCounter" habe ich jetzt in den> TimerTrigger.h verschoben
Das ist Käse. Da gehören Variaben nie hin.
Christian H. schrieb:> Ist es sinnvoll einen Ausgang/Eingang mit einem struct zu> definieren?typedef struct{> volatile uint8_t * port;> volatile uint8_t * ddr;> uint8_t pin;> uint8_t lowOrHigh;> } Output;
Damit würden für jeden einzelnen Pin 6 Byte SRAM belegt und der ist bei
den 8-Bittern nicht so üppig.
Ich hab mir daher für die Pinzugriffe Macros definiert.
Hier ein Beispiel:
Beitrag "Ooch nöö, jetzt ist der Thread weg"
Und die sbit.h:
https://www.mikrocontroller.net/attachment/highlight/254049
Felix F. schrieb:> Sowas hab ich noch nie gesehen und verwendet auch keiner#define LED3> &PORTB, &DDRB, PINB3, LOWACTIVE
Naja....
Vielleicht nicht genau so....
Aber doch sehr ähnlich!
Felix F. schrieb:> Sowas hab ich noch nie gesehen und verwendet auch keiner #define LED3> &PORTB, &DDRB, PINB3, LOWACTIVE
Nur, weil du es noch nicht gesehen hast, heißt ja nicht, dass es
keinen Sinn haben kann. ;-)
> Besser #define LED3_PORT DDRB
Ist nicht dasselbe. Das behandelt nämlich nur den Ausgabeport,
nicht aber die anderen Dinge.
Allerdings kann man so einen Makro nur sinnvoll da benutzen, wo
er all seine Argumente hinein substituieren kann. In dieser Hinsicht
wiederum kann man sich natürlich über den Programmierstil streiten,
wenn ein Makro mehrere (Funktions-)Argumente gleich auf einmal liefert.
Das ist auf den ersten Blick verwirrend und nicht ganz so toller Stil,
aber dafür ein guter Kompromiss zwischen Lesbarkeit und Effizienz.
PINB3 hätte ich aber besser PB3 genannt.
Jörg W. schrieb:> Felix F. schrieb:>> Sowas hab ich noch nie gesehen und verwendet auch keiner #define LED3>> &PORTB, &DDRB, PINB3, LOWACTIVE>> Nur, weil du es noch nicht gesehen hast, heißt ja nicht, dass es> keinen Sinn haben kann. ;-)
Ich lasse mich gerne eines Besseren belehren. Wo macht so etwas denn
Sinn? Und bitte verlinke mir doch ein entsprechendes Projekt, damit ich
es mal im Einsatz sehe :)
>> Besser #define LED3_PORT DDRB>> Ist nicht dasselbe. Das behandelt nämlich nur den Ausgabeport,> nicht aber die anderen Dinge.
Das sollte auch nur eine Anregung sein, wie er es Sinnvoll aufspalten
kann.
> PINB3 hätte ich aber besser PB3 genannt.
Wenn ich mich nicht irre, ist PINB3 ein Makro/Define von Atmel selbst.
mfg
Felix F. schrieb:>> Nur, weil du es noch nicht gesehen hast, heißt ja nicht, dass es>> keinen Sinn haben kann. ;-)> Ich lasse mich gerne eines Besseren belehren. Wo macht so etwas denn> Sinn?
1
#define LED_INIT_(port,ddr,pin,polarity) do { ddr |= (1<<(pin)); } while(0)
2
#define LED_INIT(x) LED_INIT_(x)
3
#define LED_SET_(port,ddr,pin,polarity) do { if(polarity) port |= (1<<(pin)); else port &= ~(1<<(pin)); } while(0)
4
#define LED_SET(x) LED_SET_(x)
5
6
...
7
LED_INIT(LED3);
8
LED_SET(LED3);
Das ist jetzt nur mal schnell aus dem Stegreif zusammengeschustert,
sollte aber verdeutlichen, warum es gar nicht so daneben ist, wenn
man alle Dinge für LED3 in einem Makro unterbringen kann.
>> PINB3 hätte ich aber besser PB3 genannt.> Wenn ich mich nicht irre, ist PINB3 ein Makro/Define von Atmel selbst.
Naja, von Atmel nicht direkt, sondern aus der avr-libc. Aber das
"PIN" darin deutet darauf hin, dass es sich dabei logisch um die
Pinbezeichnung bezüglich "PINB" handelt. Analog gibt es dann auch
noch PORTB3 für PORTB und DDRB3 für DDRB. PB3 dagegen ist eine Art
generische Pinnummer.
Ist natürlich weitgehend nebensächlich, da die Pinnummern in allen
Registern natürlich stets übereinstimmen. Man kann genausogut gleich
"3" schreiben.
Christian H. schrieb:> bzw. Ihr eure Augen verdreht.>> Ich möchte mir nicht von Anfang an Murks einprägen :-).
OK, das verstehe ich.
Also, dann fangen wir mal an mit der Manöverkritik:
1. Sowas wie
1
#define LED0 &PORTB, &DDRB, PINB0, LOWACTIVE
ist gar keine Abstraktion, es ist auch nicht wirklich nützlich. Deine
Absicht scheint es zu sein, in den höheren Programmschichten nicht mit
Pins, Ports und sowas herumfummeln zu müssen. Das ist der richtige
Gedanke, aber du hast ihn falsch umgesetzt.
Schreibe dir lieber einen Lowlevel-Treiber in separater Quelldatei,
deren .h garnichts vom Innenleben des Treibers verrät, außer dem
tatsächlichen Setzen der LED, also etwa so:
1
externvoidLED0_ein(void);
2
externvoidLED0_aus(void);
3
// allenfalls eine Rückmeldung:
4
externboolLED0_ist_an(void);
Wie das treiberintern gelöst ist, soll den Rest der Welt nichts angehen.
Und falls du dein Produkt mal auf eine andere Architektur umsetzt,
brauchst du nur die Innereien des Treibers anzupassen - der ganze Rest
kann so bleiben, denn er ist hardwareunabhängig.
2. zum Interrupt:
ISR(TIMER1_COMPA_vect) //20ms Timer
{
Timer_InterruptAction();
}
Gibt es einen Grund, den Interrupt nicht in der ISR zu erledigen,
sondern dazu eine andere Funktion aufzurufen? Nein, gibt es nicht. Außer
du beabsichtigst, eben diese Funktion auch mal von woanders aufzurufen,
was ein schlimmer Denk- bzw. Entwurfsfehler ist. Also laß sowas lieber
und geh nicht auf's Eis tanzen.
3. Funktionalitäten
1
voidLampeSwitch(volatileuint8_t*port,
2
volatileuint8_t*ddr,
3
uint8_tpin,
4
uint8_tlowOrHighActive,
5
uint8_tOnOff);
Du knetest hier so ziemlich alles hinein, was nicht zusammengehört.
Also noch einmal: Trenne Lowlevel und Highlevel, also Peripherietreiber
vom Algorithmus und halte die Hardwareschnittstelle einfach und auf's
Wesentliche beschränkt. Wenn du nicht mehr als Lampe ein oder aus machen
willst, dann gehören all die anderen Dinge nicht in den Header.Nicht
nur, daß sowas deinen Code unnötig kompliziert macht, es ist auch
unverständlich. Und du selber wirst dich in einem halben Jahr in deinem
eigenen Code nicht mehr zurechtfinden.
Verstehe mal die Herangehensweise: Du versuchst, das Ganze vom
göttlichen Standpunkt aus zu formulieren, etwa so:
void ErschaffeDieWelt ( param1, param2,...param9999999999999);
und wenn du mal bloß eine Mücke erschaffen willst, mußt du alle
Milliarden Parameter richtig setzen, sonst kommt ne Herde Elefanten
dabei heraus. Viel einfacher ist das mit nem Treiber, der nur eines
kennt:
void ErschaffeNeMücke(void);
Der ist natürlich überhaupt nicht universell und man kann damit eben
nicht eine Elefanteherde erschaffen, dafür aber ist er einfacher in der
Benutzung, wesentlich lesbarer und auch sicherer in jeder Weise,
insbesondere gegen Schusselfehler.
Ich skizziere hier mal ein Gegenbeispiel:
1
boolbeleuchtet;
2
intereignis;
3
4
ereignis=GetEreignis();
5
if(ereignis==isTastendruck)
6
{if(beleuchtet)
7
{Notbeleuchtung_ein();
8
LED0_aus();
9
beleuchtet=false;
10
}
11
else
12
{LED0_ein();
13
AddEreignis(isBesucherGekommen);
14
Notbeleuchtung_aus();
15
beleuchtet=true;
16
}
17
ereignis=0;
18
}
Sag jetzt nicht, die Logik so eines Programmstückes bliebe dir obskur.
W.S.
> 2. zum Interrupt:> ISR(TIMER1_COMPA_vect) //20ms Timer> {> Timer_InterruptAction();> }>> Gibt es einen Grund, den Interrupt nicht in der ISR zu erledigen,> sondern dazu eine andere Funktion aufzurufen? Nein, gibt es nicht. Außer> du beabsichtigst, eben diese Funktion auch mal von woanders aufzurufen,> was ein schlimmer Denk- bzw. Entwurfsfehler ist. Also laß sowas lieber> und geh nicht auf's Eis tanzen.
Ich habe mal eine Frage, bei einen Interrupt müssen die Register sowieso
gesichert werden, warum sollte da dann mehr als ein Subroutine Aufruf
sein und warum sollten dann Register gesichert werden?
Susi schrieb:> Ich habe mal eine Frage, bei einen Interrupt müssen die Register sowieso> gesichert werden, warum sollte da dann mehr als ein Subroutine Aufruf> sein und warum sollten dann Register gesichert werden?
den code direkt in der ISR überblickt der Compiler und sichert nur die
Register die verändert werden. Sobald eine Funktion enthalten ist, die
er nicht inline macht, muss er alle Register sichern.
Hallo Zusammen,
ich möchte mich nochmals bei ALLEN recht herzlich für die ganzen Tipps
und Wegweiser bedanken!
@W.S. Die Ausführung hat mir gut weitergeholfen!
@Fanboy Danke für die Variante.
Gruß Christian