Hallo. Im Anhang ein Programm-Auszug zur Spannungsüberwachung/-Steuerung. Dieses gesamte Programm soll später als "Zusatzmodul" in neue Programme eingebettet werden. Was ist hier eine saubere Programmarchitektur? Macht es Sinn, diesen gesamten main()-Code, in eine Funktion SpgUeberw() auszulagern. Somit würde die "neue" main(), sehr schlank und übersichtlich werden. Nur I/O-Deklaration und SpgUeberw() und neue Code.
:
Bearbeitet durch User
__Son´s B. schrieb: > Was ist hier eine saubere Programmarchitektur? > Macht es Sinn, diesen gesamten main()-Code, in eine Funktion SpgUeberw() > auszulagern. Gerade als Anfänger ist es mir wichtig, direkt eine saubere/übersichtliche Programm-Architektur zu gestalten. Dazu stelle ich mir vor, so viel wie möglich in einzelne spezielle Funktionenn zu stopfen um das main() zu entlasten und schlank zu halten. Ist der Gedanke falsch, das main() ausschliesslich/vorwiegend für die Konfig (Ports, Var, etc.) und der Ansteuerung der einzelnen Funktionen vorzusehen?
@ __Son´s Bersi__ (bersison) >> Macht es Sinn, diesen gesamten main()-Code, in eine Funktion SpgUeberw() >> auszulagern. Ja. >Gerade als Anfänger ist es mir wichtig, direkt eine >saubere/übersichtliche Programm-Architektur zu gestalten. Dann tu das. Aber wenns geht mit sinnvollen Namen für Funktionen. Da muss man heute nicht mehr krampfhaft abkürzen. Der Trend geht schon lange zu eher längeren, aber selbsterklärenden Namen. Strukturierte Programmierung auf Mikrocontrollern >Dazu stelle ich mir vor, so viel wie möglich in einzelne spezielle >Funktionenn zu stopfen um das main() zu entlasten und schlank zu halten. Ist OK. >Ist der Gedanke falsch, das main() ausschliesslich/vorwiegend für die >Konfig (Ports, Var, etc.) und der Ansteuerung der einzelnen Funktionen >vorzusehen? Nein. main() ist eher wie ein Inhaltsverzeichnis eines Buches. Selbst die Initialisierung von einzelenen Komponenten kann man in Funktionen auslagern, auch wenn diese nur einmalig aufgerufen werden.
__Son´s B. schrieb: > Macht es Sinn, diesen gesamten main()-Code, in eine Funktion SpgUeberw() > auszulagern. Nein. Der Grundsatz sollte sein: Mache es so einfach wie möglich. D.h. man macht nichts, was nicht fachlich begründet ist. Häufig hört man als Begründung: "Ich habe das schon mal so gemacht, damit man es später leichter erweitern kann." Und meistens wird es dann nie erweitert. Und wenn es erweitert wird, passt es nicht dort hin. Daher ist zumindest jene 'Begründung' und der damit verbundene das eigentliche Problem verschleiernde Aufwand Unsinn. Eine Begründung "Weil es übersichtlicher ist" kann gut sein, wenn aber jemand nachfragt, warum man das so gemacht hat, wurde es wohl nicht übersichtlicher, d.h. man lügt sich oft selbst etwas vor. Meist ist es am übersichtlichsten, wenn es so einfach wie möglich gemacht wurde. Womit wir wieder beim ersten Satz wären. Die Informatik hat sogar klare Massstäbe: Ein Algorithmus ist dann gut, wenn man ihn nicht weiter vereinfachen kann, d.h. er arbeitet das Problem mit der minimal notwendigen Anzahl von Operationen ab. Das schliesst natürlich alles ein, was 'notwendig' ist, z.B. Fehlerbearbeitung, und kann auch längeren Code bedeuten der aber statistisch weniger lange läuft (quicksort statt bubblesort). Ein Auslagern von Programmzeilen in eine Funktion, die nicht mehrmals aufgerufen wird (das wäre die fachliche Begründung) ist also Unsinn und macht das Programm weder effektiver noch übersichtlicher, es sei denn, die Funktion ist eine Standardfunktion wo man schon am Namen ablesen kann was sie tut, sich also die Funktion selbst zum Verständnis gar nicht mehr angucken muss.
Hab mal ein bischen am Code rumgefummelt, so finde ich es besser lesbar. - mMn. ist Code lesbarer, wenn er komplett in englisch verfasst wird, die Mixtur ist schwerer zu lesen - etwas kompakter gestalten, nicht unnütz viele Klammern - doppelten code vermeiden - Magic Numbers gegen #define SYM ersetzen - irgendwie unschön, in einer StateMachine ein Delay zu verwenden. Besser: state = DecodeState(prevState); oder so, und die DelayMs() ausserhalb starten.
1 | int main(void) |
2 | {
|
3 | DDRB |= (1<<PB4) | (1<<PB2) | (1<<PB1) | (1<<PB0); // Ausgang PB4=Batt_ON, PB2=low, PB1=mid, PB0=high |
4 | DDRB &= ~(1<<PB5); // Eingang PB5...Master_OFF wenn 1 |
5 | |
6 | uint16_t average; // Mittelwert |
7 | uint8_t activeLed = 2; // aktueller LED-Zustand, 1...OberBereich, 2...MittlerBereich, 3...UnterBereich |
8 | uint8_t state = 3; // StateMaschine-state |
9 | |
10 | ADC_Init(); |
11 | |
12 | while(1) { |
13 | average = ADCReadAverage(3, 100, 0); // Mess-Mittelwert, (Eingangs-Kanal (3=ADC3), Mess-Durchläufe, Intervall [us]) |
14 | |
15 | if(average >= MAX_RANGE && (activeLed==1 || activeLed==2 ) state = 1; // LED_high ... Oberer Bereich |
16 | if(average >= MAX_RANGE && activeLed==3 ) state = 2; // LED_high, von LED_low kommend |
17 | if(average < MAX_RANGE && average > MIN_RANGE ) state = 3; // LED_mid ... Mittlerer Bereich |
18 | if(average <= MIN_RANGE && (activeLed==1 ) state = 4; // LED_low, von LED_high kommend |
19 | if(average <= MIN_RANGE && (activeLed==3 || activeLed==2 ) state = 5; // LED_low ... Unterer Bereich |
20 | |
21 | switch(state) { |
22 | case 1: // high |
23 | activeLed = 1; // "OberBereich" |
24 | Batt(1); // Batterieausgang ein |
25 | break; |
26 | |
27 | case 2: // LED_high, erst über LED_mid, von LED_low kommend |
28 | SetLed(2); // LED/mid ein, Rest aus |
29 | DelayMs(500); // Mindest-Wartezeit [ms] |
30 | activeLed = 1; // "OberBereich" |
31 | Batt(1); // Batterieausgang ein |
32 | break; |
33 | |
34 | case 3: // mid |
35 | activeLed = 2; // "MittlerBereich" |
36 | DelayMs(500); // Mindest-Wartezeit [ms] |
37 | break; |
38 | |
39 | case 4: // LED_low, erst über LED_mid, von LED_high kommend |
40 | SetLed(2); // LED/mid ein, Rest aus |
41 | DelayMs(500); // Mindest-Wartezeit [ms] |
42 | activeLed = 3; // "UnterBereich" |
43 | Batt(0); // Batterieausgang aus |
44 | break; |
45 | |
46 | case 5: // low |
47 | activeLed = 3; // "UnterBereich" |
48 | Batt(0); // Batterieausgang aus |
49 | break; |
50 | }
|
51 | |
52 | SetLed(activeLed); |
53 | DelayMs(100); // [mSek], Wartezeit bis nächste Messwertauswertung |
54 | }
|
55 | }
|
:
Bearbeitet durch User
Ich würde die Magic Numbers (1,2,3,4,5, was ist das?) raus machen und dort eine enum oder konstanten nutzen. Dann kannste dir auch vieles der zum Teil doppelten Kommentare sparen. Ansonsten, wenn du für später etwas lernen willst in dieser Hinsicht, lies mal Clean Code, ein empfehlenswertes Buch, vor allem wenn man mit anderen zusammen arbeiten möchte.
Es gibt ein paar grundsätzliche Tipps: 1.) keinen Redundanten Code. 2.) Jede Funktion hat eine, genau EINE und nur EINE Aufgabe die man in einem kurzen Satz beschreiben kann. 3.) Komplexe Aufgaben in einfachere Teilaufgaben zu zerlegen ist DAS Grundkonzept jeder Programmierung und die Methode um überhaupt komplexe Aufgaben bewältigen zu können. 4.) Das ist nichts anderes als eine Methode zur Abstraktion (neben anderen). Abstrahiere über komplexe Aufgaben und behalte die Komplexität dort an einem Ort. Sie darf nicht ausbrechen ;-) 5.) Modulkonzept: Klare Schnittstellen definieren. Nichts geht daran vorbei. 6.) Lose Kopplung: So wenig Abhängigkeiten wie möglich. Jede Abhängigkeit von einer außenstehenden Ressource oder Konstruktion wird sich massiv negativ auf Erweiterungen und Veränderungen auswirken.
Michael B. schrieb: > Ein Auslagern von Programmzeilen in eine Funktion, die nicht mehrmals > aufgerufen wird (das wäre die fachliche Begründung) ist also Unsinn und > macht das Programm weder effektiver noch übersichtlicher Natürlich kann es dadurch übersichtlicher werden. Nicht gerade im gezeigten Beispiel wo alles nur eine Funktion ist aber wenn ich 100 Zeilen Code habe und ich kann daraus 5 sinnvolle Funktionen (also nicht myFuncStep1, myFuncStep2, ...) mit jeweils 20 Zeilen Code machen, welche einfach nacheinander aufgerufen werden, dann mache ich das so.
:
Bearbeitet durch User
Sebastian V. schrieb: > Michael B. schrieb: >> Ein Auslagern von Programmzeilen in eine Funktion, die nicht mehrmals >> aufgerufen wird (das wäre die fachliche Begründung) ist also Unsinn und >> macht das Programm weder effektiver noch übersichtlicher > > Natürlich kann es dadurch übersichtlicher werden. Nicht gerade im > gezeigten Beispiel wo alles nur eine Funktion ist aber wenn ich 100 > Zeilen Code habe und ich kann daraus 5 sinnvolle Funktionen mit jeweils > 20 Zeilen Code machen, welche einfach nacheinander aufgerufen werden, > dann mache ich das so. Richtig. Jemand der so was behauptet hat einfach überhaupt keine Ahnung von Softwareentwicklung. Der programmiert halt was hin und freut sich wenns läuft. Wer einmal wirklich komplexe Programme gemacht hat, weiß um die Schwierigkeiten. Wer nur kleine Blinky Programme macht kann sich die Notwendigkeit von solchen softwaretechnischen Konstrukten eben nicht vorstellen. Funktionen dienen eben nicht nur zum vermeiden von redundantem Code. Die bereits genannte Abstraktion ist das viel wichtigere.
:
Bearbeitet durch User
Sebastian V. schrieb: > Natürlich kann es dadurch übersichtlicher werden. Nicht gerade im > gezeigten Beispiel wo alles nur eine Funktion ist aber wenn ich 100 > Zeilen Code habe und ich kann daraus 5 sinnvolle Funktionen (also nicht > myFuncStep1, myFuncStep2, ...) mit jeweils 20 Zeilen Code machen, welche > einfach nacheinander aufgerufen werden, dann mache ich das so. Du machst das so. Übersichtlicher wird es dadurch nicht, denn neben den 100 Zeilen Code muss er auch noch zusätzlich rausfinden, in welcher Reihenfolge die nun drankommen, und warum irgendein Depp die aufgeteilt hat denn niemand denkt sich daß das ganz ohne fachliche Begründung erfolgte, man rätselt also an deinen verqueren Code. Klar, DU rätselst nicht, aber alle anderen. Sven S. schrieb: > Ich würde die Magic Numbers (1,2,3,4,5, was ist das?) raus machen Ich würde wohl vor allem den völlig falschen fachlichen Ansatz löschen, daß dort von "state machine" geredet wird und dann gar keine state machine benutzt wird. Der state wird ja jedesmal direkt vor dem switch/case neu berechnet. Ich finde den Code maximal unübersichtlich weil so viele meiner Meinung nach nutzlose Dinge gemacht werden. So wie ich den code (bisher) verstehe, gibt es 3 LEDs je nach gemessener Akkuspannung (whrscheinlich rot, gelb, grün), die jede 100ms gemessen wird und woraufhin die passende LED eingeschaltet wird. Die einzige Besonderheit ist eine Wartezeit von 500ms. Aber die wird idiotisch oft eingefügt: Im Normalzustand 3 MIN_RANGE<average<MAX_RANGE (LED 2) wird nach 100ms Schleifendelay auch IMMER 500ms Zustandsdelay eingefügt, es wird also effektiv nur alle 600ms gemessen und auf Veränderungen reagiert. Ob er das wollte ? Verändert sich die Spannung dann aus average<=MIN_RANGE oder MAX_RANGE<=average, dann wird sofort der Zustand eingekommen. Verändert sich die Spannung aber von LED 1 oder LED 3 auf den entgegengesetzten Zustand, wird mindestens für 500ms+100ms die LED2 aktiviert. Das geht einfacher zu formulieren:
1 | // notwendige Initialisierungen |
2 | while(1) |
3 | {
|
4 | av=ADCReadAverage(3, 100, 0); |
5 | if(av>=MAX_RANGE) |
6 | {
|
7 | if(led>1) led--; |
8 | } |
9 | else if(av<=MIN_RANGE) |
10 | {
|
11 | if(led<3) led++; |
12 | } |
13 | else led=2; |
14 | SetLED(led); |
15 | if(led==1) Batt(1); else if(led==3) Batt(0); |
16 | DelayMs(600); |
17 | } |
Meiner Meinung nach ist das der ganze Code, und ich denke mir, er wollte eigentlich einen Delay von 500ms statt 600ms was man aber erst sieht, wenn man den aufgeräumten code sieht. Auch ist die Frage, was passieren soll, wenn die Spannung innerhalb der 500ms Wartezeit schon wieder aus dem Überschreitungsbereich herauskommt. Soll dann trotzdem die LED 1 oder 3 für 100ms blitzen und vor allem die Batt kurz ausgeschaltet bzw. eingeschaltet werden oder besser nicht ?
:
Bearbeitet durch User
Falk B. schrieb: > Michael Bertrandt (laberkopp) schrieb: > ^^^^^^^^^ > Noch Fragen? Keine Fragen, wer fachlich doof ist, versucht halt persönliche Beleidigungen.
Hi Leute! Nocht Fragen >>> NEIN! Sind ausgezeichnette Ansätze und Gedankenmodelle dabei --- VIELEN DANK! Es gibt nicht nur DEN EINEN WEG, sondern viele Wege zu EINEM ZIEL. "Jeder Jeck ist Anders" - somit sollte persönliche Diskriminierungen/Angriffe in einem qualifiziertem Forum keinen Platz haben - habe ich daher gerne überlesen...
__Son´s B. schrieb: > Sind ausgezeichnette Ansätze und Gedankenmodelle dabei Vor Allem, werde ich zusammengehörige Funktionen zukünftig, mit dem gleichen Index zu beginnen und deutlicher/ausführlicher zu Bezeichnen; Bsp: SpgUeberwach_ADWandel() SpgUeberwach_Auswertung() SpgUeberwach_Mittelwert() SpgUeberwach_BereichZuordnung() etc. Somit ist klar erkennbar, warum es geht und vor allem, welche Funktionen zusammen gehören.
@__Son´s Bersi__ (bersison) >Somit ist klar erkennbar, warum es geht und vor allem, welche Funktionen >zusammen gehören. Eben, dann ist es fast schon C++ ;-) Klasse.Methode();
Thread beobachten
Seitenaufteilung abschaltenBitte melde dich an um einen Beitrag zu schreiben. Anmeldung ist kostenlos und dauert nur eine Minute.
Bestehender Account
Schon ein Account bei Google/GoogleMail? Keine Anmeldung erforderlich!
Mit Google-Account einloggen
Mit Google-Account einloggen
Noch kein Account? Hier anmelden.