Hallo,
ich stelle hier mal ein Programm online, das ich geschrieben habe.
Da ich mit meinen C-Kenntnissen noch ziemlich am Anfang stehe, hoffe ich
auf diese Art und weise ein Paar Hinweise oder Tipps und Tricks zu
erfahren.
Ich danke vorab jedem, der sich die mühe macht diese (ca. 120) Zeilen
anzuschauen und mir ein Feedback dazu gibt.
Gruß an alle.
Michael
Das ist schon mal nicht so schlecht, aber das kann man noch besser
machen. Setze dir immer das Ziel, deinen Code so zu schreiben, dass du
Kommentare dieser Art nicht brauchst.
In deinem Fall geht das ganz leicht
if(moop!=0){// Ausgabe von Akustischem Signal wenn moop ungleich 0
23
moop--;// moop um 1 dekrementieren
24
PORTB|=(1<<HUPE)|(1<<KLAPPE_AUF);// Hupe ein
25
}
26
else{// Ausschalten des Signales wenn Z�hler auf 0 Zur�ckgelaufen
27
PORTB&=~((1<<HUPE)|(1<<KLAPPE_AUF));// Hupe aus
28
}
29
30
if(overflows==20){// Steuerung des Sekundenz�hlers
31
Sekunde++;// Wert des Sekundenz�hlers um 1 inkrementieren
32
overflows=0;// Interruptz�hler zur�cksetzen
33
}
34
}
Dadurch gewinnt der Code an Qualität, weil du nicht lange überlegen
musst, was denn jetzt an PB1 angeschlossen ist. Der Code sagt es dir,
bzw. umgekehrt: Du schaltest nicht mehr PB1, du schaltest die HUPE und
der Compiler weiß, dass die an PB1 angeschlossen ist.
Die Kommentare sind dann auch überflüssig, weil alles was es dazu zu
sagen gibt, im Code steht. Im schlimmsten Fall ist der Kommentar einfach
nur falsch, wie zb hier
1
ISR(TIMER2_COMP_vect){
2
overflows++;// Interruptz�hler um 1 inkrementieren
3
blinktimer++;// Z�hler f�r Blinkfrequenz um 1 inkrementieren
4
if(PINC&(1<<PINC0)){// Abfrage Start signal von Fernbedinung
5
start++;
6
}
7
else{
8
start=0;
9
}
10
if(PINC&(1<<PC1)){// Abfrage Start signal von Fernbedinung
11
reset++;
12
}
13
else{
14
reset=0;
15
}
Ist dir aufgefallen, dass im Kommentar in 2 Fällen behauptet wird, dass
hier von der Fernsteuerung das Start Signal kommt.
Und jetzt vergleiche mit
1
if(PINC&(1<<REMOTE_START))
2
start++;
3
else
4
start=0;
5
6
if(PINC&(1<<REMOTE_RESET))
7
reset++;
8
else
9
reset=0;
Da braucht es keinen Kommentar, damit ich dasselbe herauslesen kann.
Nur: Da hier der Code sein eigener Kommentar ist, kann er auch nicht
falsch sein.
if(moop!=0){// Ausgabe von Akustischem Signal wenn moop ungleich 0
8
moop--;// moop um 1 dekrementieren
9
PORTB|=(1<<PB0)|(1<<PB1);// Hupe ein
10
}
Daraus könnte man evtl. eine einzige If-Schlaufe machen..
Das sind die beiden Dinge, die mir beim Überfliegen grade so ins Auge
stachen. Mehr Zeit hab ich leider momentan nicht..
moop=9;// Signal "Ende der Zeit zum Fahrzeug ziehen" veranlassen
3
}
4
if((Sekunde==15)&&(overflows==0)){
5
PORTB|=(1<<PB4);// Lampe1 aus
6
}
7
if((Sekunde==30)&&(overflows==0)){
8
....
Diese ganze Sequenz ist nicht sehr schön.
Aber zumindest solltest du den Prozessor etwas entlasten (da geht es bei
dir noch nicht um Performance sondern dass du dich daran gewöhnst)
1
if((Sekunde==10)&&(overflows==0)){
2
moop=9;// Signal "Ende der Zeit zum Fahrzeug ziehen" veranlassen
3
}
4
elseif((Sekunde==15)&&(overflows==0)){
5
PORTB|=(1<<PB4);// Lampe1 aus
6
}
7
elseif((Sekunde==30)&&(overflows==0)){
8
....
Wenn Sekunde gleich 10 war (und Overflows gleich 0 ), dann ist völlig
klar, dass Sekunde nicht auch noch 15 sein kann, oder 30 oder was auch
immer
Die Abfrage auf overflows == 0 solltest du vorziehen und damit die
einzelnen if einfacher machen
1
...
2
3
if(overflows==0){
4
5
if(Sekunde==10)
6
moop=9;// Signal "Ende der Zeit zum Fahrzeug ziehen" veranlassen
7
8
elseif(Sekunde==15)
9
PORTB|=(1<<LAMPE_1);// Lampe1 aus
10
11
elseif(Sekunde==30)
12
PORTB|=(1<<LAMPE_2);// Lampe2 aus
13
14
elseif(Sekunde==45)
15
PORTB|=(1<<LAMPE_3);// Lampe3 aus
16
17
elseif(Sekunde==60)
18
PORTB|=(1<<LAMPE_4);// Lampe4 aus
19
20
...
Je einfacher du den jeweiligen Bedingungsteil gestalten kannst, desto
weniger Möglichkeiten gibt es, dort einen Fehler zu machen.
Du merkst vielleicht auch, dass ich kein Verfechter der 'überall muss
eine {} sein'-Schule bin. Für mich ist es wichtiger, dass die einzelnen
else-if Fälle einen optischen Block bilden. Wenn da nur eine Anweisung
steht UND wenn ich sbolut sicher sein kann, dass da auch nie irgendetwas
anderes stehen wird, dann lass ich die {} auch gerne weg.
volatileuint8_treset=0;// Ob gestartet werden soll oder nicht...
Der Variablenname sagt erst mal gar nix aus, außer dass es was mit dem
Reset zu tun hat. Der Kommentar ist doppelt falsch, benutzt wird die
Variable offensichtlich als Zähler zum Entprellen (und nicht als Flag
"ob oder nicht") des Reset-Tasters (und nicht des Start-Tasters). Warum
dann nicht
1
volatileuint8_tcntResetDebounce=0;
ohne Kommentar? Dann kann der Kommentar auch nicht mehr falsch sein
(Prinzip wurde bei den Portpins schon erläutert)
Für die Variable "start" gilt entsprechendes...
Kleine Zwischenbemerkung: Ich finds gut, dass sich Anfänger Gedanken um
die Qualität ihres Codes machen. Gibts selten ;-)
Würdest du dein Beispiel zur Verfügung stellen, dass wir ein Tutorial
"Sauber C codieren" oder so draus machen?
Wow... !!!
Ich bin absolut Überwältigt von der rießen Resonaz auf meine Anfrage.
Ein richtig großes
DD A NN N K K EEEEE
D D A A N N N K K E
D D A A N N N KK EEE
D D AAAAAAA N NN K K E
DD A A N N K K EEEEE
an alle!
@ Karl heinz Buchegger: Extra Dank für die gute Erklärung!
@ der mechatroniker: Ich verstehe noch nicht ganz wie du das mit dem
Tutorial meinst. Aber dagegen habe ich nichts.
Es wird wohl eine kleine Weile dauern alle Anregungen umzusetzen, aber
Ihr werdet dann von mir lesen.
Gruß Michael
Michael Zeller schrieb:
> Ich habe mal eben noch den Hinweis mit dem CASE aufgenommen...>> Könnte das in etwa so passen?>>
1
switch(Sekunde){
2
>case10:moop=9;break;
3
>case15:PORTB|=(1<<LAMPE_0);break;
4
>case30:PORTB|=(1<<LAMPE_1);break;
5
>//usw...
6
>}
Ja genau.
Der ganze Abschnitt, der sich im Original über viele Zeilen hingezogen
hat, müsste jetzt deutlich kürzer sein ohne dadurch kryptisch zu wirken.
Das ist gut so, denn viele Probleme in der Programmierung enstehen
dadurch dass wir Menschen den Code nicht mehr überblicken können. Viele
Programmierer benutzen so was wie eine Faustregel: Wenn der Inhalt einer
Funktion nicht mehr auf eine Bildschirmseite passt, ist es Zeit darüber
nachzudenken, ihn entweder zu kompremieren oder in mehrere Funktionen
aufzuteilen.
Hallo,
ich habe eure Vorschläge mal so gut ich es verstanden habe umgesetzt.
Ich freue mich auf eure Kritik und weitere Anregungen.
Gruß Michael
PS: Das Programm befindet sich im Anhang ;-)
Nächster Punkt:
Du hast noch einige konstante Werte in deinem Programm.
zb.
Dass deine ISR 20 mal in der Sekunde aufgerufen wird, hab ich nur
rausgefunden, indem ich den restlichen Sourcecode studiert habe.
Frage: Kann ich das leicht ändern?
Wie ist das, wenn der Prozessor nicht mit 4Mhz sondern mit 8Mhz
betrieben wird? Was muss dann alles im Programm geändert werden? (Ziel
ist es: Ich ändere nur eine Zahl, F_CPU, und alles andere passt sich von
alleine an!)
Wie kommen die hier
OCR2 = 194; // Vergleichswert 194 einstellen => 20,03 Hz
zustande?
Was ist, wenn ich die Dauer des Hupsignals länger oder kürzer machen
möchte? Wo muss ich dann drehen? Wie kommen die Zahlenwerte dort
zustande?
Schön wäre es, wenn am Anfang des Programms ein paar #define wären, mit
denen ich das einfach anpassen kann.
machst du zb
1
#define F_CPU 4000000 // darf nicht höher als 5.2 Mhz sein
2
#define TIME_RESOLUTION 20 // 20 ISR Aufrufe in der Sekunde
dann kannst du
1
OCR2=(F_CPU/1024/TIME_RESOLUTION)-1;
schreiben und der Compiler rechnet dir den Wert aus. Da das alles
konstante Zahlenwerte sind, wird das wirklich vom Compiler berechnet und
nciht zur Laufzeit des Programmes. Das kostet dir also nichts, ausser
ein bischen Tipparbeit. Aber du kannst F_CPU und TIME_RESOLUTION an
einer Stelle ändern und damit das gewünschte veranlassen. Natürlich
musst du dann noch:
1
if(overflows==TIME_RESOLUTION){// Steuerung des Sekundenz�hlers
2
Sekunde++;// Wert des Sekundenz�hlers um 1 inkrementieren
3
overflows=0;// Interruptz�hler zur�cksetzen
4
}
ändern, aber das Ziel ist erreicht: Jemand der einen 1Mhz Quarz
einsetzen will, kann das tun. Er ändert einfach F_CPU und fertig. Die
Variable Sekunde wird nach wie vor im Sekundentakt (zumindest genau
genug) hochgezählt.
Kannst du auch dafür
arithmetische Ausdrücke finden, so dass sich der Zahlenwert aus dem Rest
und einer vorgegebenen Zeit (oder Frequenz) ergibt? Selbiges für die
Dauer des Hupsignals.
Theoretisch könntest du noch all diese Vergleiche in Definitionen
packen, aber das ist dann eher noch Kosmetik als etwas anderes.
Also bei den Definitionen käme dann noch ein:
1
#define RUN_START_DEBOUNCE ((PINC&0x01) == 0x00)
Dann würde die If-Abfrage lauten:
1
if(RUN_START_DEBOUNCE){StartDebounce++;}
Mir hilft es, da ich dann auf den ersten Blick sehe, in welcher
Beziehung dieses START-Bit nun genutzt wird, gerade das '!' zu Beginn
des Vergleichs wird manchmal sehr gerne übersehen. Die Benennung bleibt
natürlich dir selbst überlassen, wie es für dich am eindeutigsten ist.
Aber jeder nach seinem Geschmack :)
@Peter: Ich steh gerade auf dem Schlauch: Was kann an meiner Variablen
cntResetDebounce als anstößig interpretiert werden?
(abgesehen davon, dass das Abkürzen von Namensbestandteilen, die ständig
vorkommen -- cnt für counter, id für identifier, ix für index usw. --
m.E. nicht den Lesefluss stört, sondern im Gegenteil dafür sorgt, dass
man sprechende Variablennamen vergeben kann, ohne dass diese allzu lang
werden; aber da gehen die Geschmäcker wohl auseinander).
Ich wollte gerade noch einen Punkt anmerken, aber ich sehe, das wurde
schon umgesetzt (das abenteuerliche "Einfangen" des Programmablaufs am
Ende wurde durch Rücksetzen der Variablen "lauft" ersetzt).
@ Karl heinz Buchegger
entspricht das erhoffte Ergebniss in etwa folgendem? :
1
fBlinklampe=(20*(1/gewueschte_Frequenz));break;// Blinklampe mi x Hz blinken lassen
bzw.
1
fBlinklampe=(10*(1/gewueschte_Frequenz));break;// Blinklampe mi x Hz blinken lassen
Da ich ja die lampe in der Angegebenen Frequenz toggle also die
blinkfrequenz nur halb so groß ist.
Falls das noch nicht passt brauche ich noch nen kleinen Schups runter
von dem Schlauch auf dem ich gerade stehe...
OK. Ich habe das Programm nun nochmal etwas umgerührt...
Bin gespannt es heute Nachmittag auf der Test-Hardware (STK600) mal
probelaufen zu lassen.
Falls noch jemand eine gute Idee hat dar natülich weiterhin kritik geübt
werden!
Gruß Michael
Hallo zusammen,
ich habe nun wie angekündigt das Programm mal etwas getestet.
Die besagten tests verliefen sogar (zumindest relativ) erfolgreich, was
ich bestimmt nicht zuletzt Eurer(!) Hilfe zu verdanken habe.
Dafür nochmal ein herzliches Dankeschön!
Natürlich habe ich noch den ein oder Anderen Denkfehler aufgedeckt und
auch ein Paar leichtsinnsfehler beheben müssen...
(copy & paste ist so ne sache ;-)
Dabei sind mir einige Fragen gekommen die ich hiermit einfach mal in die
Runde werfe, wer sich denen annehmen möchte darf das gerne tun. ;-)
1. Ich habe recht viel Code in meiner ISR. Ist dagegen etwas
einzuwenden? Bzw. macht es aus irgendwelchen Gründen sinn da etwas zu
verändern?
2. Würde es sinn machen Teile des Programmes in Subroutinen zu
verwandeln und diese dann in der Mainloop nurnoch aufzurufen?
3. Gibt es noch andere Dinge die zu einem (anständigen) C-Programm
gehören die ich momentan vergessen habe, oder die mir nicht bekannt
sind.
Gruß an alle
Michael
Michael Zeller schrieb:
> 1. Ich habe recht viel Code in meiner ISR. Ist dagegen etwas> einzuwenden? Bzw. macht es aus irgendwelchen Gründen sinn da etwas zu> verändern?
Du kannst das so lange machen, wie du willst - solange du dir sicher
bist, dass die ISR fertig ist, bevor sie wieder aufgerufen wird.
> 2. Würde es sinn machen Teile des Programmes in Subroutinen zu> verwandeln und diese dann in der Mainloop nurnoch aufzurufen?
Wichtig: Eine Funktion sollte immer einen Sinn ergeben. Hilfsfunktionen
mit 10+ Parametern die irgendeinen Zwischenschritt machen, der nur in
einem bestimmten Kontext einen Sinn ergibt, sollte man vermeiden.
Michael Zeller schrieb:
> 1. Ich habe recht viel Code in meiner ISR.
Halb so wild.
Das sieht nur soviel aus. Dein Programm nimmt fast überall eine von 2
Alternativen. Natürlich müssen beide Alternativen in der ISR stehen,
aber abgearbeitet wird ja immer nur 1 Zweig davon.
> Ist dagegen etwas> einzuwenden? Bzw. macht es aus irgendwelchen Gründen sinn da etwas zu> verändern?
Das hier würde ich verändern
Die Berechnung muss ja nicht jedesmal gemacht werden, verändert sich ja
so gut wie nie (aus Sicht des µC)
Ich würde das so machen:
(BTW: Bist du sicher, dass du 1 / Frequenz_Blinklampe berechnet haben
willst. Das sind alles ganze Zahlen. Da kommt immer 0 heraus. Manchmal
ist es hilfreich ein bischen Mathe einzusetzen :-)
Ohh... ,
dass das ja alles ganze Zahlen sein müssen hab ich total verpennt. :-X
(schande über mich)
@ Karl heinz Buchegger:
Danke für den guten Tipp.
Ich werde mal versuchen den umzusetzen.
Eine Frage dazu habe ich aber noch:
Die Blinkfrequenz kann nicht größer 500Hz sein da sonst das Ergebniss
von
1
ticks=INTERRUPTS_PER_SECOND/(2*Frequenz);
0 sein wird.
Also wird dieser Unerlaubte Zustand durch
1
if(ticks==0)// nur um sicher zu gehen
2
return1;
abgefangen, indem dann einfach immer 1 zurückgegeben wird.
Also der kleinst mögliche Zeitintervall.
...
Hmm...
Eigendlich wollte ich fragen wozu
1
if(ticks==0)// nur um sicher zu gehen
2
return1;
gut sein soll... :-D
hat sich wohl gerade erledigt... :-)
Michael Zeller schrieb:
> Die Blinkfrequenz kann nicht größer 500Hz sein da sonst das Ergebniss> von>
1
ticks=INTERRUPTS_PER_SECOND/(2*Frequenz);
> 0 sein wird.>> Also wird dieser Unerlaubte Zustand durch>
1
if(ticks==0)// nur um sicher zu gehen
2
>return1;
> abgefangen, indem dann einfach immer 1 zurückgegeben wird.> Also der kleinst mögliche Zeitintervall.
Und damit die größte mögliche Frequenz.
Bei dir spielt das keine Rolle, den den Unterschied zwischen 200Hz und
800Hz kann niemand sehen :-)
Das ist die ganze Idee dahinter.
Ein weiterer Nutzeffekt:
Man könnte die Variable Blinkticks damit gleichzeitig als Schalter
fungieren lassen.
Ist Blinkticks gleich 0, dann ist das Blinken ausgeschaltet
Ist es ungleich 0, dann ist es eingeschaltet und gibt die 'Frequenz' an.
Hallo alle zusammen,
im Anhang findet Ihr den (warscheinlich) Finalen- Stand des Programms.
Ich habe das Programm ausführlich auf meinem STK600 getestet und bin mit
der Funktion sehr zufrieden.
Und das verdanke ich zu einem erheblichen Teil eurer kompetenten Hilfe.
Dafür nochmal ein Herzlichens Dankeschön (Ihr seid klasse)!
Um nun das erlernte nicht gleich wieder zu vergessen steht auch schon
das nächste Projekt ins Haus... :-)
Ich werde dazu mal etwas Brain-Storming betreiben und schauen was so
dabei herauskommt.
Abschließende Frage: falls ich zum nächsten Programm eure Hilfe
nocheimal notwendig haben sollte ;-) , neuer Thread oder den bestehenden
vortsetzen?
(die Überschrift des bestehenden würde ja noch stimmen)
schönen Gruß an alle
Michael
Michael Zeller schrieb:
> Abschließende Frage: falls ich zum nächsten Programm eure Hilfe> nocheimal notwendig haben sollte ;-) , neuer Thread oder den bestehenden> vortsetzen?
Neues Programm -> Neuer Thread.