Beurteilung der Architektur - C++ Neuling :)
-
Hallo Leute.
Ich komme aus C# - das mache ich nun schon ca. 15 Jahre. Nun soll ich für einen Kunden eine Architektur aufbauen und baue dazu gerade ein paar Prototypen in C++.
Das Ganze soll auf einem MikroController laufen, daher die while(1) Schleife und die FSM in der Main (mit einem Sleep(1) damit das auf Windows nicht alles lahm legt). Der Rest ist schmückendes Beiwerk - es geht darum einen Eingang (hier: Einen Tastendruck) auf einen Ausgang (Hier: Ein Virtuelles MagnetVentil) zu legen.
Die Eingänge werden durch die Schleife zyklisch abgefragt, deren State auf die Input Objekte gelegt und im anderen State den Ausgängen zugewiesen.
Da das Projekt schon ein wenig umfangreicher geworden ist von der Klassen-Anzahl, poste ich hier lediglich die Main() und der rest kann per ZIP geladen werden.
Ich brauche eine Einschätzung, ob der Prototyp so von der Schreibweise (Clean Code etc.) und Architektur her in C++ passt - und, wenn nicht, was geändert werden muss bzw kann.
Danke schonmal vorab!
#include <iostream> #include "magnetvalve.h"; #include "OutputDefinitions.h" #include <Windows.h> #include "Input.h" #include "InputDefinitions.h" #include "InputWatcher.h" #include "MachineState.h" #include "FiniteStateMachine.h" #include "ValveHandler.h" int main() { //************************************************************************* // //just a test how to handle a MagnetValve that is connected to an output. // //************************************************************************* //declare classes auto finiteStateMachine = FiniteStateMachine(); auto inputWatcher = InputWatcher(); auto valveHandler = ValveHandler(); //declare Inputs auto input1 = Input(INPUT_KEY_LEFTSHIFT); auto input2 = Input(INPUT_KEY_BACKSPACE); //add to watcher to monitor state inputWatcher.AddInputToWatch(&input1); inputWatcher.AddInputToWatch(&input2); //pass reference of output to magnetvalve auto magnetValve1 = MagnetValve(1, &(Output(OUTPUT_PIN1))); auto magnetValve5 = MagnetValve(2, &(Output(OUTPUT_PIN5))); while (1) { switch (finiteStateMachine.MachineState) { //poll the inputs for changes case MachineState::CheckInputs: inputWatcher.PollInputs(); finiteStateMachine.SetStateHandleValves(); break; //setting the output according to the state of the input. case MachineState::HandleValves: valveHandler.SetMagnetValve(&input1, &magnetValve5); valveHandler.SetMagnetValve(&input2, &magnetValve1); finiteStateMachine.SetStatePollInputs(); break; } Sleep(1); } return 0; }
Link zum TestProjekt:
-
Ich komme aus C# - das mache ich nun schon ca. 15 Jahre. Nun soll ich für einen Kunden eine Architektur aufbauen und baue dazu gerade ein paar Prototypen in C++. Das Ganze soll auf einem MikroController laufen,
Dieser Sprung ist ein wenig heftig. Solange du keinen großen Controller zu Verfügung hast, wirst du in eine komplett neue Welt einsteigen.
Ein Beispiel: Wer konfiguriert deine PINs? Ist z.B. Pin OUTPUT_PIN1 als Pullup, Pulldown, Input, Output, Floating definiert?
Ein paar kleine Dinge:
- Wie steuerst du deinen Controller? Benötigst du da nicht einen UART zur Übergabe von Befehlen?
-> Wie stellst du beispielsweise fest, welche Software Version auf deinem Controller läuft? - Sleep(1), #include <windows.h> -> Darf das auch auf dem Controller laufen?
- Der Name finiteStateMachine ist mir zu nichtsaussagend. Werde hier konkreter. Ist das der Programmzustand?
Ansonsten finde ich den Code ok.
- Wie steuerst du deinen Controller? Benötigst du da nicht einen UART zur Übergabe von Befehlen?
-
Hei!
Vielen Dank schonmal für deine erste Einschätzung! Wir haben wohl einen relativ grossen 32Bit PIC (ala PIC32MX795F512L) zur Verfügung. Ja, der Sprung ist krass, aber ich hab die stabilen Steigeisen an - die Lernkurve krieg ich schon noch gebacken
Zu deinen Fragen:
Die Konfiguration der Inputs wird über die MPLab X - Eigene Bibliothek für den PIC erledigt:
#include <plib.h> /* Include to use PIC32 peripheral libraries */ #include <stdint.h> /* For UINT32 definition */ #include <stdbool.h> /* For true/false definition */ #include <exception> /* Includes C++ try/catch functions */ #include "user.hpp" /* variables/params used by user.cpp */ void InitApp(void) { /* Initialize peripherals */ mPORTAClearBits(BIT_0); mPORTAClearBits(BIT_1); mPORTASetPinsDigitalIn(BIT_0|BIT_1); mPORTDClearBits(BIT_0); mPORTDClearBits(BIT_1); mPORTDSetPinsDigitalOut(BIT_0|BIT_1); }
Der Controller selbst ist quasi eine kleine SPS (wenn Eingang X kommt und dazu Status Y Anliegt, setze Ausgang Z für xxx ms) - die über CAN mit einem zweiten µC kommuniziert, welcher die Verbindung zur Aussenwelt (Display. CAN, Ethernet) zur Verfügung stellt. Auch gibt es eine Serielle Schnittstelle, über welche die Diagnose-Informationen abgefragt werden können.
=> Sleep(1), #include <windows.h> -> Darf das auch auf dem Controller laufen?
Natürlich nicht, das war vom Test auf Visual Studio "übrig", mittlerweile sieht die Main anders aus (am Ende dieses Posts).
=> Der Name finiteStateMachine ist mir zu nichtsaussagend. Werde hier konkreter. Ist das der Programmzustand?
Korrekt - das taktet quasi das Programm durch und steuert Abläufe. Hier könnte ich ProgramFlowControl als Namen einsetzen - ich habe das auch mal so umgesetzt.
Danke wie gesagt dir schon mal für deinen Input - nachfolgend der nun auf MPLab X (im PIC Simulator) lauffähige Code der Main():
/******************************************************************************/ /* Files to Include */ /******************************************************************************/ #ifdef __XC32 #include <xc.h> /* Defines special funciton registers, CP0 regs */ #endif #include <plib.h> /* Include to use PIC32 peripheral libraries */ #include <stdint.h> /* For UINT32 definition */ #include <stdbool.h> /* For true/false definition */ #include <exception> /* Includes C++ try/catch functions */ #include "system.hpp" /* System funct/params, like osc/periph config */ #include "user.hpp" /* User funct/params, such as InitApp */ #include "magnetvalve.h" #include "OutputDefinitions.h" #include "Input.h" #include "InputWatcher.h" #include "MachineState.h" #include "FiniteStateMachine.h" #include "ValveHandler.h" /* All the files in the C++ standard library declare its entities * within the std namespace. */ using namespace std; /* use the standard namespace */ /******************************************************************************/ /* Global Variable/Classes Declaration */ /******************************************************************************/ //declare classes auto programFlowControl = ProgramFlowControl(); auto inputWatcher = InputWatcher(); auto valveHandler = ValveHandler(); #define FOSC 60E6 #define PB_DIV 8 #define PRESCALE 256 #define T1_TICK (FOSC/PB_DIV/PRESCALE/4) /******************************************************************************/ /* Main Global Variable Declarations */ /******************************************************************************/ Input input1; Input input2; MagnetValve magnetValve1; MagnetValve magnetValve2; /******************************************************************************/ /* Main Program */ /******************************************************************************/ INT main(void) { #ifndef PIC32_STARTER_KIT /*The JTAG is on by default on POR. A PIC32 Starter Kit uses the JTAG, but for other debug tool use, like ICD 3 and Real ICE, the JTAG should be off to free up the JTAG I/O */ DDPCONbits.JTAGEN = 0; #endif /*Refer to the C32/XC32 peripheral library documentation for more information on the SYTEMConfig function. This function sets the PB divider, the Flash Wait States, and the DRM /wait states to the optimum value. It also enables the cacheability for the K0 segment. It could has side effects of possibly alter the pre-fetch buffer and cache. It sets the RAM wait states to 0. Other than the SYS_FREQ, this takes these parameters. The top 3 may be '|'ed together: SYS_CFG_WAIT_STATES (configures flash wait states from system clock) SYS_CFG_PB_BUS (configures the PB bus from the system clock) SYS_CFG_PCACHE (configures the pCache if used) SYS_CFG_ALL (configures the flash wait states, PB bus, and pCache)*/ /* TODO Add user clock/system configuration code if appropriate. */ SYSTEMConfig(SYS_FREQ, SYS_CFG_ALL); /* Initialize I/O and Peripherals for application */ InitApp(); //************************************************************************* // //just a test how to handle a MagnetValve that is connected to an output. // //************************************************************************* while (1) { switch (programFlowControl.State) { //enclosing the init state in brackets so that we can declare variables here case MachineState::Init: { //declare Inputs input1 = Input(IOPORT_A, BIT_0); input2 = Input(IOPORT_A, BIT_1); //add to watcher to monitor state inputWatcher.AddInputToWatch(&input1); inputWatcher.AddInputToWatch(&input2); Output output1 = Output(IOPORT_D, BIT_0); Output output2 = Output(IOPORT_D, BIT_1); //pass reference of output to magnetvalve magnetValve1 = MagnetValve(1, &output1); magnetValve2 = MagnetValve(2, &output2); //next state - start polling programFlowControl.SetStatePollInputs(); } break; //poll the inputs for changes case MachineState::CheckInputs: inputWatcher.PollInputs(); //next state - set the outputs programFlowControl.SetStateHandleValves(); break; //setting the output according to the state of the input. case MachineState::HandleValves: valveHandler.SetMagnetValve(&input1, &magnetValve1); valveHandler.SetMagnetValve(&input2, &magnetValve2); //next state - start polling - loop! (poll / set outputs) programFlowControl.SetStatePollInputs(); break; default:; } } }
-
Mir fällt auf, dass deine Statusmaschine eigentlich immer nur zwischen zwei Zuständen wechselt und dabei aktiv was macht. Dazu brauchst du sie eigentlich nicht, da tut´s auch eine normale Endlos-Schleife, ggf. mit Abbruchbedingung.
void initialize() { InitApp(); ... } bool terminated() { return false; } void run() { check_inputs(); handle_valves(); } int main() { Init(...); for( initialize(); !terminated(); run() ); }
Die Zustandmaschine braucht man häufig doch nur, um auf äußere Einflüsse zu reagieren (Benutzereingaben, eingehende Messdaten, etc.), die wird also eher passiv verändert. Ich benutze häufig sowas hier:
class StateMachine { public: enum State_t { InitialState, State1, State2, ... }; // Publisher/Subscriber Pattern StateChangedPublisher StateChanged; StateMachine() = default; State_t state() const { return CurrentState_; } void transition( State_t new_state ) { if( new_state != state() ) { State_t const old_state = state; CurrentState_ = new_state; std::invoke( StateChanged, new_state, old_state ); } } private: State_t CurrentState_ = InitialState; };
An den Stellen, an denen mich ein Statuswechsel interessiert, registriere ich einen Callback und werde dann von der Statusmaschine bei einem Zustandswechsel angesprungen.
-
@DocShoe sagte in Beurteilung der Architektur - C++ Neuling
:
Mir fällt auf, dass deine Statusmaschine eigentlich immer nur zwischen zwei Zuständen wechselt und dabei aktiv was macht. Dazu brauchst du sie eigentlich nicht, da tut´s auch eine normale Endlos-Schleife, ggf. mit Abbruchbedingung.
Aktuell ist das so, zumindest in diesem Prototypen. Jedoch wird sich das in Zukunft definitiv aendern, wenn mehr komplexitaet in das Programm kommt.
An den Stellen, an denen mich ein Statuswechsel interessiert, registriere ich einen Callback und werde dann von der Statusmaschine bei einem Zustandswechsel angesprungen.
Das ist eine sehr gute Idee, die Abhaengigkeiten zu reduzieren - ich werde das auf dem MikroProzessor testen.
-
Ok, ein paar Anmerkungen zum Code gerade in Bezug „CleanCode“
#pragma once
wird nicht von jedem Compiler unterstützt, die klassischen Include Guards funktionieren mit jedem Compiler.- Die include Direktiven sind gemischt, d.h. Systemheader und Projektheader erfolgen wild durcheinander, das
#include <windows.h>
, obwohl nur für den Test auf Windows benutzbar, wird ohne jeden Kommentar irgendwo dazwischen eingefügt. - Die Code Kommentare haben teilweise keinerlei Bezug zum Code der das steht. Der Codeblock nach
//declare classes
deklariert keine Klassen (das erfolgt in den *.h Dateien), es werden anschließend Objekte definiert, Exemplare von Klassen angelegt, classes instantiated, … es gibt da reichlich Möglichkeiten das korrekt zu beschreiben. while (1)
in C++ kann man direkt alswhile (true)
schreiben oder man nimmt die Versionfor (;;)
. Wobei letzteres von einigen abgelehnt wird, weil man das nicht direkt sähe.- Das Thema Ownership von Zeiger. Man kann das so wie im Test machen, allerdings muss dann immer ordentlich alles vorher definiert sein, und anschließend auch ordentlich zerstört werden. Rohe Zeiger können schnell zu einem Problem werden, wenn das Ownership damit einhergeht oder auf bereits zerstörte Resourcen zugegriffen wird.
return EXIT_SUCCESS
(definiert in cstdlib, da wird auch EXIT_FAILURE definiert) ist klarer alsreturn 0
, wenn man das schon explizit hinschreibt.- Die Konstruktoren sind unsauber geschrieben. Am Beispiel von (Code kompakter geschrieben)
class MagnetValve { bool _isOpen; int _openCounter; Output* _output; int _id; public: MagnetValve(int id, Output* output) : _isOpen (false), _openCounter(????), _output(output), _id(id) {} };
Der C++ Weg ist dieser hier, und bei der Gelegenheit _openCounter wird nicht gesetzt in Deinem Code.
- Der GenericAdder sitzt in einer cpp Datei, da hat er nichts zu suchen, weil Templates in C++ (bis auf Spezialisierungen) immer in einem Header definiert sein müssen.
-
Hi John,
vielen Dank für deine Anmerkungen. Da ich noch Anfänger auf dem Weg nach C++ bin, hilft mir das meinen Code besser zu machen und anderen den Weg zu besserem Code anschaulicher zu zeigen.
Ich werde die Tage das Projekt überarbeiten - und hier neu hochladen. Es soll ein Iterativer Prozess werden (wobei ich auch nicht zu viel Zeit in diesen Test-Ballon stecken will - das eigentliche Projekt steht ebenfalls in den Startlöchern).
Danke nochmals und eine schöne Woche!
-
@john-0 sagte in Beurteilung der Architektur - C++ Neuling
:
#pragma once wird nicht von jedem Compiler unterstützt, die klassischen Include Guards funktionieren mit jedem Compiler.
pragma once
steht zwar nicht im Standard, aber ist schon sehr weit verbreitet. GCC, CLang und MSC unterstützen das auf jeden Fall. Mir persönlich reicht dasAuf den Filehorst komme ich aktuell nicht, wegen ner Proxysperre, aber anhand des Beispiels was @john-0 raus gesucht hat, ich habe es lieber, wenn eine Klasse erst das
public
Interface deklariert und hinter dasprivate
. Ja, dann muss man einmalprivate
mehr schreiben, macht es aber, meiner Meinung nach, übersichtlicher.
-
@Schlangenmensch sagte in Beurteilung der Architektur - C++ Neuling
:
pragma once
steht zwar nicht im Standard, aber ist schon sehr weit verbreitet. GCC, CLang und MSC unterstützen das auf jeden Fall. Mir persönlich reicht dasEs geht hier um Embedded Programmierung, und man muss vorher abklären, ob der Compiler das überhaupt unterstützt.
Auf den Filehorst komme ich aktuell nicht, wegen ner Proxysperre, aber anhand des Beispiels was @john-0 raus gesucht hat, ich habe es lieber, wenn eine Klasse erst das
public
Interface deklariert und hinter dasprivate
. Ja, dann muss man einmalprivate
mehr schreiben, macht es aber, meiner Meinung nach, übersichtlicher.Ist das wirklich übersichtlicher? Ich habe da meine Zweifel, wenn man eine klare Trennung von Interface und Implementierungsdetails will muss man ohnehin auf das Pimpl-Idiom (auch Facade Pattern genannt) zurückgreifen.
Also anstatt
class MagnetValve { bool _isOpen; int _openCounter; Output* _output; int _id; public: MagnetValve(int id, Output* output) : _isOpen (false), _openCounter(????), _output(output), _id(id) {} };
willst Du dann die folgende Aufteilung
// Header File class MagnetValve { public: MagnetValve(int id, Output* output); private: bool _isOpen; int _openCounter; Output* _output; int _id; }; // Implementation File MagnetValve::MagnetValve (int id, Output* output) : _isOpen(false), _openCounter(?????), _output(output), _id(id) {}
lieber? Meiner Meinung nach soll jeder das machen was er will, ich schrieb ja extra „für den Codeblock kompakt geschrieben“.
-
@john-0 sagte in Beurteilung der Architektur - C++ Neuling
:
@Schlangenmensch sagte in Beurteilung der Architektur - C++ Neuling :
pragma once steht zwar nicht im Standard, aber ist schon sehr weit verbreitet. GCC, CLang und MSC unterstützen das auf jeden Fall. Mir persönlich reicht das
Es geht hier um Embedded Programmierung, und man muss vorher abklären, ob der Compiler das überhaupt unterstützt.
Es ging auch um Clean Code... ich musste tatsächlich erstmal die Wikipedia durchsuchen um einen Compiler zu finden der das nicht unterstützt. Kann natürlich bei speziellen Embedded Sachen anders aussehen, aber da @tbird das verwendet, scheint es sein Compiler auch zu unterstützen. Daher finde ich die Verwendung von
pragma once
vollkommen in Ordnung@john-0 sagte in Beurteilung der Architektur - C++ Neuling
:
Ist das wirklich übersichtlicher?
Ja, ist es. Ich könnte jetzt auf die Schnelle kein Projekt nennen, bei dem das nicht so gemacht wird. Das ist eine Gewohnheitssache, wo man nach was guckt und mit
public
anzufangen, ist, ich würde fast schon sagen, üblich. Aber ich erinnere mich dunkel, dass es hier im Forum da schon mal eine Diskussion zu gab.Meiner Meinung nach soll jeder das machen was er will,
Aber bitte konsistent in einem Projekt.
Ich habe da meine Zweifel, wenn man eine klare Trennung von Interface und Implementierungsdetails will muss man ohnehin auf das Pimpl-Idiom (auch Facade Pattern genannt) zurückgreifen.
Pimpl brauchst du doch nur, wenn du Binaries ausliefern willst und Implementierungsdetails verstecken möchtest. Wenn es um Trennung eines Interfaces gibt, kannst du immer noch mit Pure virtual classes arbeiten, oder mit CRTP, oder, inzwischen auch mit Concepts.
-
@Schlangenmensch sagte in Beurteilung der Architektur - C++ Neuling
:
Pimpl brauchst du doch nur, wenn du Binaries ausliefern willst und Implementierungsdetails verstecken möchtest. Wenn es um Trennung eines Interfaces gibt, kannst du immer noch mit Pure virtual classes arbeiten, oder mit CRTP, oder, inzwischen auch mit Concepts.
Würde ich jetzt nicht unterschreiben. Ich finde den Kommentar von john-0 hier völlig valide. Ist auch eine Möglichkeit, je nachdem.