Autor Beitrag
Ralf Jansen
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starhalf ontopic star
Beiträge: 4700
Erhaltene Danke: 991


VS2010 Pro, VS2012 Pro, VS2013 Pro, VS2015 Pro, Delphi 7 Pro
BeitragVerfasst: Mo 30.05.22 14:03 
Zitat:
Was ist deine Meinung?

Ok. Andere Variante einfach weil es immer mehrere gibt. Wartbarkeit, Lesbarkeit, Testbarkeit hängen immer zusammen und sind keine absoluten Größen. Es hängt leider immer auch vom Publikum ab.

ausblenden C#-Quelltext
1:
2:
3:
private void SetColor(Control control, bool condition) => SetColor(control, condition, Color.Green, Color.LightGray);

private void SetColor(Control control, bool condition, Color colorTrue, Color colorFalse) => control.BackColor = condition ? colorTrue : colorFalse;


Zitat:
Was mich wieder ärgert. Probieren nicht wissen.

In der IT heißt es auch "Ich habe nur bewiesen das es geht aber noch nicht getestet." Nenne es nicht probieren sondern testen und schon ist der negative Klang weg.
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Mo 30.05.22 22:50 
Servus,

Vielen Dank für Eure Antworten.

Zitat:
(Ist RoboterDSLeer jetzt ein Schreibfehler bei dir, statt TrigSaveFiles?)

Jupp das war ein Schreibfehler.

Zitat:
Wenn du noch eine konkrete Frage bzgl. der Methodenaufrufe hast, dann stelle sie ruhig

Im Moment gerade nicht aber ich werde mir den Code nochmal anschauen und darüber nachdenken.

Aber ich hätte eine andere Frage. Und zwar gibt es ja die 3 Schichten Architektur.
Und wenn ich das richtig verstanden habe dann müsste ich alles was mit der reinen SPS zu tun hat in eine Klasse z.B. SPS auslagern. Das wollte ich schon immer machen und hatte auch mal angefangen. Das ist aber völlig in die Hose gegangen und ich habe es dann sein gelassen.
Es ist halt SPS Logik deswegen dachte ich das gehört ausgelagert. Ich muss aber auch dazu sagen das das für mich zu schwer ist und ich Eure Hilfe bräuchte. Aber was meint den Ihr dazu? Sinnvoll?
Von einer anderen Seite gesehen wäre es aber auch wieder doppelte Arbeit weil wir zum Beispiel erst vor kurzem die UIValues in der Methode Connect_Click untergebracht haben wenn ich die Methode jetzt in die Klasse SPS verschiebe dann entsteht ein sehr großer Rattenschwanz. Nicht Wahr?
Wenn Ihr der Meinung seit es ist auch für mich zu schaffen dann ok.

@ Ralf

Hattest du meinen Beitrag gelesen wo ich dich wegen dem OneShot-Timer was gefragt hatte?
Ich wollte halt nur wissen ob das so ok ist das ich den Timer stoppe oder gar nicht nötig ist.

So vielen Dank nochmal für die ganze Hilfe und Erklärungen.

Grüße Tom

Moderiert von user profile iconTh69: Beitragsformatierung überarbeitet.
Th69
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Moderator
Beiträge: 4764
Erhaltene Danke: 1052

Win10
C#, C++ (VS 2017/19/22)
BeitragVerfasst: Di 31.05.22 09:49 
Zuersteinmal zum One-Shot-Timer:
Es kommt darauf an, ob du die im Timer aufgerufenen Methoden während des Kopiervorgangs benötigst oder nicht (z.B. ShowPlcStatus()).
Ob der Kopiervorgang läuft, d.h. der BackgroundWorker, das fragst du ja ab.

Was ich jetzt aber noch im Code gesehen habe: Warum werden im Timer-Event TimerCopyAllFiles_Tick (statt einmalig im Connect) die UI-Werte ausgelesen, d.h. die Parse-Aufrufe, und dann noch in uiValues geschrieben (statt in s7VariablenResult)?
Die uiValues sollen ja nur die UI-Elemente abdecken, welche für den asynchronen Vorgang benötigt werden (und nicht schon in der anderen Klasse S7VariablenResult stehen - so hast du sie ja doppelt zu verwalten!?!).

Und dazu solltest du ein Objekt von S7VariablenResult als Membervariable anlegen und dessen Werte setzen (anstatt immer wieder lokale Objekte in ReadResult zu erzeugen - daher ja auch mein Vorschlag bzgl. zweier S7-Klassen, um die Trennung von Eingabe- und Ausgabewerten klarer zu machen).

Eigentlich habe ich angefangen, folgendes zu schreiben:
Zitat:
Und nun ist daher genau der richtige Zeitpunkt, um diese SPS-Funktionalität in eine eigene Klasse auszulagern.
Erzeuge dazu unabhängig vom bisherigen Code eine neue Klasse (in einer neuen Datei) und füge dort als Member ein Objekt von S7VariablenResult hinzu.
Und nun überlege dir, welche öffentlichen (public) Methoden du benötigst und erstellst dann die Signatur (Methodenkopf) dafür.

Aber dann ist mir anhand des Codes klargeworden, daß eigentlich nur die Methode ReadResult ausgelagert werden kann. Da diese aber eben noch die aus den UI-Controls gelesenen Werte benötigt, müßtest du dafür auch wieder eine Klasse (wie UIValues) als Parameter benutzen (oder aber alle Werte einzeln übergeben), und das macht den Code nicht einfacher.

Vom Design bzw. Architektur ist das aber selbstverständlich das Ziel.

Du solltest dir also überlegen, ob du nicht doch zwei getrennte Klassen für die S7-Eingabe und Ausgabe erzeugst (und auf die Gesamtklasse S7VariablenResult verzichtest). Alternativ könntest du deine SPS-Klasse mit den einzelnen Daten als Eigenschaften ausstatten (also statt getrennter Daten- und Codeklasse).

PS: Mir ist auch noch eingefallen, daß du für z.B. EnableTextBoxes und AreAllTextBoxesFilled auch noch ein Array anlegen könntest und diese dann per Schleife (oder LINQ) die Werte setzt bzw. abfragst. Dies ginge dann sogar auch für die Settings (wenn du die Namen der Controls identisch zu den Settingswerten machst, abgesehen vom Präfix txt).
Ralf Jansen
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starhalf ontopic star
Beiträge: 4700
Erhaltene Danke: 991


VS2010 Pro, VS2012 Pro, VS2013 Pro, VS2015 Pro, Delphi 7 Pro
BeitragVerfasst: Di 31.05.22 09:58 
Zitat:
Hattest du meinen Beitrag gelesen wo ich dich wegen dem OneShot-Timer was gefragt hatte?
Ich wollte halt nur wissen ob das so ok ist das ich den Timer stoppe oder gar nicht nötig ist.


Tatsächlich übersehen. Welche Sorte Timer man nimmt hängt natürlich von den Anforderungen ab.
Jetzt hast du einen "Mach was regelmäßig mit einer Pause dazwischen" Ding. Der Abstand hängt davon ab wie lange das kopieren dauert die einstellbare Dauer des Timers steuert jetzt die Pausenlänge nicht genau in welchem Abstand das kopieren initiiert werden soll. Wenn das ok ist ist das ok.

Wenn deine Anforderung wäre es muß wenn möglich haargenau alle 5 Minuten passieren oder so dann nicht. Wenn die Anforderung wäre kopiere immer wenn sich was geändert hat dann bräuchte man wieder was ganz anderes.

Kann die Frage ob das nötig ist oder nicht nicht wirklich beantworten ohne genauere Anforderungen zu kennen. Möglicherweise macht es für deinen Anwendungsfall keinen Unterschied ob du stoppst oder nicht.
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Mi 01.06.22 13:21 
Hallo,

Vielen Dank für die Antworten.

Zitat:
Zuerst einmal zum One-Shot-Timer:

Ich habe es getestet, ob ich den Timer laufen lassen kann. Kann ich nicht.
Es wird zwar kopiert, aber nicht so wie es soll und auf der Visu flackert alles.

Zitat:
Was ich jetzt aber noch im Code gesehen habe: Warum werden im Timer-Event TimerCopyAllFiles_Tick

Hattest du auch in der aktuellen Datei geschaut? Das ist nicht mehr so. Ich hänge noch eine neue Datei dran.

Zitat:
Die uiValues sollen ja nur die UI-Elemente abdecken, welche für den asynchronen Vorgang benötigt werden


Ich habe nur eine Klasse mit dem Namen S7VariablenResult, die enthält alle Variablen die aus dem Datenbaustein gelesen werden müssen.

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
namespace PDE
{
    // Variablen von der SPS
    class S7VariablenResult
    {
        public string PraegeCodeNr;     // Prägecode Hauptteil
        public string FolderName;       // Ordnername
        public string PraegeCode2Nr;    // Prägecode Zusatzteil
        public bool TrigSaveFiles;      // Daten kopieren
        public bool TypLinks;           // Typ Links
        public bool TypRechts;          // Typ Rechts
        public bool RoboterDSLeer;      // Roboter hat kein Teil im Greifer
        public bool FMBauteil;          // Fertigmeldung Bauteil
        public bool HeartBeat;          // Heartbeat
    }
}


Dann gibt es die Methode private S7VariablenResult ReadResult().
Diese Methode muss in einem Loop (Timer) an vielen Stellen im Code aufgerufen werden,
und zwar dann, wenn es notwendig ist, die Variablen im Datenbaustein abzufragen.

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
18:
19:
20:
21:
22:
 
        private S7VariablenResult ReadResult()
        {
            S7VariablenResult s7VariablenResult = new S7VariablenResult();
            // Reads the buffer.

            byte[] dbbuffersize = new byte[uivalues.DBSize];

            s7Client.DBRead(uivalues.DBNr, uivalues.DBOffset, uivalues.DBSize, dbbuffersize);

            s7VariablenResult.PraegeCodeNr = S7.GetStringAt(dbbuffersize, uivalues.PraegeCodeDBPos);
            s7VariablenResult.FolderName = S7.GetStringAt(dbbuffersize, uivalues.FolderNameDBPos);
            s7VariablenResult.PraegeCode2Nr = S7.GetStringAt(dbbuffersize, uivalues.PraegeCode2DBPos);
            s7VariablenResult.TrigSaveFiles = S7.GetBitAt(dbbuffersize, uivalues.TrigSaveFilesByte, uivalues.TrigSaveFilesBit);
            s7VariablenResult.TypLinks = S7.GetBitAt(dbbuffersize, uivalues.TypLinksByte, uivalues.TypLinksBit);
            s7VariablenResult.TypRechts = S7.GetBitAt(dbbuffersize, uivalues.TyprechtsByte, uivalues.TyprechtsBit);
            s7VariablenResult.RoboterDSLeer = S7.GetBitAt(dbbuffersize, uivalues.RoboterDSLeerByte, uivalues.RoboterDSLeerBit);
            s7VariablenResult.FMBauteil = S7.GetBitAt(dbbuffersize, uivalues.FMBauteilByte, uivalues.FMBauteilBit);
            s7VariablenResult.HeartBeat = S7.GetBitAt(dbbuffersize, uivalues.HeartByte, uivalues.HeartBit);

            return s7VariablenResult;
        }


Hier nochmal eine Erklärung, welche Variable wofür ist.

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
        public string PraegeCodeNr;     // Variable mit dem Wert, um die Dateien zu "suchen". z. B. 22060112573
        public string FolderName;       // Variable mit dem Wert für den Ordnernamen z. B. 22060112573R
        public string PraegeCode2Nr;    // auch eine Variable um die Dateien zu "suchen".

        public bool TrigSaveFiles;      // Triggersignal von der SPS zum Daten kopieren

        public bool TypLinks;           // Typ Links - Der Ordnername bekommt so den Buchstaben (L)
        public bool TypRechts;          // Typ Rechts - Der Ordnername bekommt so den Buchstaben (R)

        public bool RoboterDSLeer;      // Das Teil wurde abgelegt, das ist die Variable um die Visu zu aktualisieren.

        public bool FMBauteil;          // Fertigmeldung Bauteil -  Ist eine wichtige Variable für die if-Bedingung.

        public bool HeartBeat;          // Heartbeat - Für visuelle Zwecke nicht unbedingt notwendig.


Zitat:
Und dazu solltest du ein Objekt von S7VariablenResult als Membervariable anlegen und dessen Werte setzen (anstatt immer wieder lokale Objekte in ReadResult zu erzeugen - daher ja auch mein Vorschlag bzgl. zweier S7-Klassen, um die Trennung von Eingabe- und Ausgabewerten klarer zu machen).

Das verstehe ich gerade nicht so. Kannst du das anhand eines Beispieles bitte erklären?

Zitat:
und das macht den Code nicht einfacher.

Ich denke, dass ich das alles in der Main lassen sollte.

Zitat:
Du solltest dir also überlegen, ob du nicht doch zwei getrennte Klassen für die S7-Eingabe und Ausgabe erzeugst

Was ist für dich die S7-Eingabe (die Textboxen)? Und was die Ausgabe? Ich denke mit Sicherheit anders und bevor ich doppelte Arbeit mache, frage ich besser nach.

Zitat:
Alternativ könntest du deine SPS-Klasse mit den einzelnen Daten als Eigenschaften ausstatten (also statt getrennter Daten- und Codeklasse).

So wie die UIValues?

Zitat:
PS: Mir ist auch noch eingefallen, daß du für z.B. EnableTextBoxes und AreAllTextBoxesFilled auch noch ein Array anlegen
könntest und diese dann per Schleife (oder LINQ)

Du meinst das so?

ausblenden C#-Quelltext
1:
cboLocalApps = new ComboBox[] { cboLocalApp1, cboLocalApp2, cboLocalApp3, cboLocalApp4, cboLocalApp5, cboLocalApp6, cboLocalApp7, cboLocalApp8 };					


Ok, kann ich machen.

Zitat:
Dies ginge dann sogar auch für die Settings (wenn du die Namen der Controls identisch zu den Settingswerten machst, abgesehen vom Präfix txt).


Das habe ich nicht so gut verstanden.

So vielen Dank nochmal.

Grüße Tom
Einloggen, um Attachments anzusehen!
Th69
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Moderator
Beiträge: 4764
Erhaltene Danke: 1052

Win10
C#, C++ (VS 2017/19/22)
BeitragVerfasst: Mi 01.06.22 15:29 
Hallo,

OK, die einmalige Initialisierung der UIValues im Connect ist gut so. :zustimm:

Und bzgl. Eingabe- und Ausgabevariablen meinte ich genau auf die ReadResult()-Methode bezogenen Daten (für s7Client.DBRead und S7.GetStringAt).
Ich war bisher davon ausgegangen, daß auch die Eingabevariablen (PraegeCodeDBPos, FolderNameDBPos, ...), welche du ja nach UIValues verschoben hast, auch noch in der S7VariablenResult-Klasse drin sind und auch dort zum weiteren Bearbeiten benötigt werden (du hattest ja geschrieben, du kannst diese Klasse nicht ändern).

Wenn du doch frei die Klassen bestimmen kannst, dann ist es ja jetzt kein Problem diese auszulagern (nur daß du dann statt dem Namen UIValues eine eigene Klasse anlegen solltest, damit die SPS-Klasse komplett unabhängig [auch vom Namen her] von einer UI ist).

Und deine Frage dazu:
UserTom hat folgendes geschrieben:
Das verstehe ich gerade nicht so. Kannst du das anhand eines Beispieles bitte erklären?

Da bin ich auch noch davon ausgegangen, daß in der S7VariablenResult-Klasse mehr Daten stehen, als du in ReadResult setzt (und dann hättest du ja immer ein neues Objekt in ReadResult erzeugt, ohne die alten Werte zu behalten. Und dies hättest du mit einem Klassenmember (wie du ja auch uiValues nutzt) vermieden.

Wenn dem nicht (mehr) so ist, dann kannst du deinen Code diesbzgl. jetzt so lassen.

Mir geht es vorallem darum, daß du dir Gedanken über die Daten (Klassen) und den Datenfluß machst.

Und bzgl. dem Array: ja, wie die anderen Arrays auch, nur eben jetzt für deine TextBox-Objekte txtIPNr, .., txtHeartBit.

Bei den Settings:
Zitat:
Das habe ich nicht so gut verstanden.

Das habe ich mir fast gedacht. Schau dir einfach mal die Definition der Settings-Eigenschaften an (Datei "Properties/Settings.Designer.cs"). Dann wirst du (hoffentlich) feststellen, daß intern immer dieselbe Methode (bzw. genauer gesagt der Index-Operator) benutzt wird.
Und diesen kann man auch selber ansprechen, wenn man die Settings-Eigenschaften als string hat (wie z.B. der Name der zug. TextBox).
Somit dann analog zu den anderen Arrays in einer Schleife die Settings lesen und schreiben.

Ich habe gesehen, daß du var settings = Settings.Default; (überwiegend) benutzt. Das finde ich gut, da es auch wieder redundanten Code reduziert (du solltest es dann aber konsequent überall für den Zugriff auf die Settings machen ;- ).

PS: Du hast ja für EnableLocalAppControls() sowie EnableNetAppControls() wieder zwei Methoden angelegt (True/False), anstatt nur eine mit einem bool enable-Parameter (wie bei EnableTextBoxes(bool enable)).
Das tiefe Verständnis scheint dir dafür wohl noch zu fehlen?
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Mi 01.06.22 22:52 
Servus,

Zitat:
Ich war bisher davon ausgegangen, daß auch die Eingabevariablen (PraegeCodeDBPos, FolderNameDBPos, ...), welche du ja nach UIValues verschoben hast, auch noch in der S7VariablenResult-Klasse drin sind und auch dort zum weiteren Bearbeiten benötigt werden (du hattest ja geschrieben, du kannst diese Klasse nicht ändern).

Also mit nicht ändern meinte ich, wenn ich die S7VariablenResult Klassen-Datei ändere dann muss natürlich auch die dazugehörige
ReadResult()-Methode angepasst werden. Da aber die Methode so richtig ist wie sie ist (jetzt noch besser weil eingekürzt)
brauch nichts geändert werden. Ich denke wir reden zwecks meiner Idee eine SPS Klassen-Datei zu erstellen vielleicht ein bisschen aneinander vorbei.
Ich wollte eine SPS.cs Datei erstellen und alles was die SPS betrifft auslagern. Also z.B die Methode ConnectToPLC()weil damit die Verbindung
mit der CPU von der SPS hergestellt wird. Die Methode ShowPlcStatus() damit wird ein Status von der CPU abgerufen ob sie rennt oder gestoppt ist. Die Methode ShowS7VariablenStatus(S7VariablenResult s7VariablenResult) fragt die Bit Signale ab. Die Methode Ready() damit schreibe ich in den DB.
Und natürlich auch die Methode S7VariablenResult ReadResult() damit werden die DB-Daten von der SPS gelesen.

Ich hatte das ja schonmal probiert und als ich die Methode S7VariablenResult ReadResult() ausgelagert hatte habe ich nichts mehr zum laufen gebracht.
Und nur wegen der folgenden Methode.

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
18:
19:
20:
21:
22:
23:
24:
25:
26:
27:
private S7VariablenResult ReadResult()

        {

warum steht das hier in der Methode? Verstehe ich nicht.
##########################################################################            
S7VariablenResult s7VariablenResult = new S7VariablenResult();
##########################################################################

            // Reads the buffer.

            byte[] dbbuffersize = new byte[uivalues.DBSize];

            s7Client.DBRead(uivalues.DBNr, uivalues.DBOffset, uivalues.DBSize, dbbuffersize);

            s7VariablenResult.PraegeCodeNr = S7.GetStringAt(dbbuffersize, uivalues.PraegeCodeDBPos);
            s7VariablenResult.FolderName = S7.GetStringAt(dbbuffersize, uivalues.FolderNameDBPos);
            s7VariablenResult.PraegeCode2Nr = S7.GetStringAt(dbbuffersize, uivalues.PraegeCode2DBPos);
            s7VariablenResult.TrigSaveFiles = S7.GetBitAt(dbbuffersize, uivalues.TrigSaveFilesByte, uivalues.TrigSaveFilesBit);
            s7VariablenResult.TypLinks = S7.GetBitAt(dbbuffersize, uivalues.TypLinksByte, uivalues.TypLinksBit);
            s7VariablenResult.TypRechts = S7.GetBitAt(dbbuffersize, uivalues.TyprechtsByte, uivalues.TyprechtsBit);
            s7VariablenResult.RoboterDSLeer = S7.GetBitAt(dbbuffersize, uivalues.RoboterDSLeerByte, uivalues.RoboterDSLeerBit);
            s7VariablenResult.FMBauteil = S7.GetBitAt(dbbuffersize, uivalues.FMBauteilByte, uivalues.FMBauteilBit);
            s7VariablenResult.HeartBeat = S7.GetBitAt(dbbuffersize, uivalues.HeartByte, uivalues.HeartBit);

            return s7VariablenResult;
        }


Zitat:
Wenn du doch frei die Klassen bestimmen kannst, dann ist es ja jetzt kein Problem diese auszulagern

Ich kann alles tun aber ich weiß leider nicht wie. Das mit dem "= new" ist am schlimmsten für mich.
Manchmal macht es Ahaaa und manchmal stehe ich da wie blöde.

Zitat:
Und dies hättest du mit einem Klassenmember (wie du ja auch uiValues nutzt) vermieden.

Zum Beispiel hier da weiß ich nicht auf welchen Klassenmeber du dich beziehst. Das beneide ich Euch sehr weil ihr diese Wörter nutzen könnt.
Ich weiß das ein Klassenmember vieles sein kann. Aber was du mit Klassenmeber jetzt meinst weiß ich nicht.

Zitat:
Mir geht es vorallem darum, daß du dir Gedanken über die Daten (Klassen) und den Datenfluß machst.

Das mache ich aber das ist nicht so einfach.

Zitat:
Und bzgl. dem Array: ja, wie die anderen Arrays auch, nur eben jetzt für deine TextBox-Objekte txtIPNr, .., txtHeartBit.

Ich hänge noch ein paar Dateien mit dran. Ansonsten müsste es jetzt noch besser sein.

Zitat:
Bei den Settings:

Die Datei habe ich mir angeschaut es wiederholt sich alles. Aber keine Ahnung was ich davon nutzen könnte.

Zitat:
(bzw. genauer gesagt der Index-Operator)

Was ist denn ein Index-Operator.

Zitat:
Ich habe gesehen, daß du var settings = Settings.Default;

Ich habe alle überarbeitet.

Zitat:
PS: Du hast ja für EnableLocalAppControls()

Habe ich auch angepasst.

Vielen Dank nochmal für deine Hilfe.

Grüße Tom
Einloggen, um Attachments anzusehen!
Th69
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Moderator
Beiträge: 4764
Erhaltene Danke: 1052

Win10
C#, C++ (VS 2017/19/22)
BeitragVerfasst: Do 02.06.22 10:53 
UserTom hat folgendes geschrieben:
ausblenden C#-Quelltext
1:
2:
3:
4:
warum steht das hier in der Methode? Verstehe ich nicht.
##########################################################################
S7VariablenResult s7VariablenResult = new S7VariablenResult();
##########################################################################

Genau das meinte ich mit "anstatt immer wieder lokale Objekte in ReadResult zu erzeugen". Und mit Klassenmember meine ich Felder (fields) und Eigenschaften (properties) [ich benutze das Wort "Feld(er)" ungern in diesem Zusammenhang, da es in anderen Programmiersprachen, z.B. C oder C++, als Synonym für Array steht].

Hättest du also dieses Objekt außerhalb der Methode:
ausblenden C#-Quelltext
1:
private S7VariablenResult s7VariablenResult = new S7VariablenResult();					

so würde es nur einmalig angelegt werden und du könntest direkte dessen Werte ändern (dann könnte auch die Rückgabe entfallen und die aufgerufenen Methoden verwenden ebenfalls direkt diesen Klassenmember s7VariablenResult).

Das Auslagern aller (bisherigen) SPS-Methoden in eine eigene Klasse wird schwierig, da du in diesen ja auch auf die UI-Elemente der Form zugreifst.
Trotzdem würde man in einem größeren professionellem Projekt diese auslagern (Stichworte: Wrapper / Interface). Dies wäre erst einmal Mehraufwand, entkoppelt aber die Form- und die S7Client-Klasse.
Im Anhang mal ein Ansatz dafür:
- bei PlcGetStatus müßtest du die UI-relevanten Status-Abfragen trotzdem in der Form-Klasse durchführen, außer Text und BackColor würden über die SPS-Klasse gesetzt -> Hinzufügen zum Rückgabetyp (falls dir die Schreibweise (x, y, z) nichts sagt: Tupeltypen) - alternativ kannst du auch eine Datenklasse bzw. Eigenschaften dafür erzeugen, s. nächster Punkt.
- für ReadResult habe ich dir 2 Versionen erstellt, einmal mit den "Eingabe"-Werten als Eigenschaften und einmal als extra Klasse S7Input.
Die Erzeugung und Rückgabe des lokalen Objekts s7VariablenResult habe ich jetzt mal so gelassen (sofern S7VariablenResult auch nur die Werte umfaßt, die von ReadResult gesetzt werden - so wie du es ja in deinem letzten Code hast). Die entsprechenden Eigenschaften in UIValues solltest du dann entfernen, und nur noch z.B. die Pfadnamen dort drin haben.

Du solltest dir auch noch mal die ShowResult-Methode anschauen, denn nach dem Code-Umbau macht sie jetzt so keinen Sinn mehr, da sie den ErrorText nicht mehr direkt ausgibt, sondern in uiValues.Error schreibt, welches nirgendwo benutzt wird.

Und nochmal wegen der Settings:
Der Indexoperator ist hier ApplicationSettingsBase.Item[String], also der Zugriff mittels der eckigen Klammern [...] (in der Doku wird der Index-Operator immer "Item[]" genannt).
Da dich das alles zurzeit eher verwirrt, laß das mal mit den Settings (ich wollte dir nur weitere Codeverkürzungen aufzeigen - diese wäre aber nicht so performant wegen den Stringmethoden).
Einloggen, um Attachments anzusehen!
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Do 02.06.22 19:47 
Guten Abend TH69,

Vielen Dank für deine Antwort.

Man da habe ich was zu tun das zu alles durch zuarbeiten. Hat viel mit testen zu tun.

Aber ich hätte erstmal ein anderes Problem.

In der Methode ConnectBtn_Click werden ja die UIValues geladen. Mit AreAllSPSTextBoxesFilled() wird man erinnert wenn eine Textbox nicht
ausgefüllt ist. Und diese Methode, scheint von mir nicht richtig erstellt worden.

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
 private void ConnectBtn_Click(object sender, EventArgs e)
        {
####################################################################################         
   if (AreAllSPSTextBoxesFilled() == true// All text boxes must be filled out.
#####################################################################################
   {
       uivalues.Error = txtError.Text;

       uivalues.IPNr = txtIPNr.Text;
       uivalues.RackNr = int.Parse(txtRackNr.Text);
       uivalues.SlotNr = int.Parse(txtSlotNr.Text);

       uivalues.DBNr = int.Parse(txtDBNr.Text);
       uivalues.DBOffset = int.Parse(txtDBOffsetNr.Text);
       uivalues.DBSize = int.Parse(txtDBSizeNr.Text);



ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
private bool AreAllSPSTextBoxesFilled()
        {
            for (int i = 0; i < tbSPSConectionsValues.Length; i++)
            {
                if (!string.IsNullOrEmpty(tbSPSConectionsValues[i].ToString()))
                {
                    return true;
                }
            }

            return false;
        }


Keine Ahnung wo der Fehler ist. Könntest du mal da drüber schauen.

Zitat:
Du solltest dir auch noch mal die ShowResult-Methode anschauen, denn nach dem Code-Umbau macht sie jetzt so keinen Sinn mehr,

Ich habe sie gerade getestet sie zeigt mir aber die Werte an die sie mir anzeigen soll. Deswegen weiß ich jetzt nicht wirklich was daran
nicht richtig ist.

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
private void ShowResult(int Result)
        {
            // This function returns a text explanation of the error code.
            uivalues.Error = s7Client.ErrorText(Result);

            if (Result == 0)
            {
                uivalues.Error = uivalues.Error + " (" + s7Client.ExecutionTime.ToString() + " ms)";
            }
        }


Die Orginale sah so aus!

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
 private void ShowResult(int Result)
        {
            // Diese Funktion gibt eine Texterklärung des Fehlercodes zurück
            txtError.Text = s7Client.ErrorText(Result);
            if (Result == 0)
            {
                txtError.Text = txtError.Text + " (" + s7Client.ExecutionTime.ToString() + " ms)";
            }
        }


Vielen Dank nochmal für deine Hilfe und vor allem für die SPS.cs.

Schönen Abend noch.

Grüße Tom
Ralf Jansen
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starhalf ontopic star
Beiträge: 4700
Erhaltene Danke: 991


VS2010 Pro, VS2012 Pro, VS2013 Pro, VS2015 Pro, Delphi 7 Pro
BeitragVerfasst: Do 02.06.22 21:53 
Zitat:
Keine Ahnung wo der Fehler ist.


Logisch fast richtig nur falsch rum gedacht. Du brichst ab sobald ein Wert gesetzt ist und sagst alles in Ordnung(true). Du möchtest ab das sobald eine Wert NICHT gesetzt ist abbrechen und sagen es ist nicht in Ordnung(false).

Es sind übrigens TextBoxen und keine Values die enthalten vielleicht Text sind aber kein Text. Ein TextBox.ToString() liefert dir hier vermutlich den Typ Namen ala "System.Winforms.TextBox" bzw. den gesamten beschreibenden Context der TextBox aber nicht nur den TextInhalt eine TextBox kann halt viel mehr als den Text. Ein TextBox (wie eigentlich alle Controls) hat einfach eine Text Property.
Also eher
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
private bool AreAllSPSTextBoxesFilled()
{
    for (int i = 0; i < tbSPSConectionsValues.Length; i++)
        if (string.IsNullOrEmpty(tbSPSConectionsValues[i].Text))
            return false;
    return true;
}

oder wieder die Kurzform
ausblenden C#-Quelltext
1:
private bool AreAllSPSTextBoxesFilled() => tbSPSConectionsValues.All(tb=> !string.IsNullOrEmpty(tb.Text));					


Bei if (AreAllSPSTextBoxesFilled() == true) kann das == true weg. Aus der Methode kommt schon ein boolean raus und das vergleichen mit einem boolean ändert den kein bißchen.
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Fr 03.06.22 06:53 
Guten Morgen,

@ Ralf

Vielen Dank für deine Antwort und die Erklärung.
Funktioniert natürlich und sehr ärgerlich, dass ich das nicht gefunden habe, weil es war ja nicht wirklich schwer.
Fast schon peinlich.

Zitat:
oder wieder die Kurzform

Könntest du mir die Kurzform erklären? Ich weiß, dass sie funktioniert, weil du mir das als Option hier mit reingeschrieben hast, aber in Wirklichkeit hätte ich damit nichts anfangen können. return true / false sind auch weg.
Es ist, glaube ich, mit LINQ geschrieben? Die Pfeile sind Lambdaausdrücke. Die größere Variante ist verständlicher, was da passiert.

Vielen Dank für deine ganzen Erklärungen.

@ TH69

Nochmal.
Zitat:
Du solltest dir auch noch mal die ShowResult-Methode anschauen,

In nehme meine Aussage zurück. Funktioniert nicht. Ich bin wieder auf das Original gegangen.
txtError.Text braucht über die UIValues auch nicht geladen werden.

Auch dir Danke für deine tolle Hilfe.

Einen schönen Tag.
Grüße Tom

Moderiert von user profile iconTh69: C#-Tags hinzugefügt
Th69
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Moderator
Beiträge: 4764
Erhaltene Danke: 1052

Win10
C#, C++ (VS 2017/19/22)
BeitragVerfasst: Fr 03.06.22 09:35 
Hallo UserTom,

wegen der Kurzschreibweise: wenn du englisch kannst, s. Help understanding this code snippet (es sind also zwei verschiedene Bedeutungen von =>).

C# wird für Anfänger leider nicht einfacher, weil mit jeder Version (aktuell ja C#10, auf dem Weg ist schon C#11) neue Features und Syntax hinzukommt (da bin ich für mich froh, daß ich direkt mit C#1 [und .NET 1.1] angefangen hatte).


Zuletzt bearbeitet von Th69 am Fr 03.06.22 15:02, insgesamt 1-mal bearbeitet
Ralf Jansen
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starhalf ontopic star
Beiträge: 4700
Erhaltene Danke: 991


VS2010 Pro, VS2012 Pro, VS2013 Pro, VS2015 Pro, Delphi 7 Pro
BeitragVerfasst: Fr 03.06.22 13:44 
ausblenden C#-Quelltext
1:
private bool AreAllSPSTextBoxesFilled() => tbSPSConectionsValues.All(tb=> !string.IsNullOrEmpty(tb.Text));					

Das entspricht dem hier wenn man mal den Expression Body (In deutsch Ausdruckskörpermember :puke:) Syntax wegläßt und einfach einen klassischen Methodenblock benutzt so wie er dir vermutlich geläufiger ist.
ausblenden C#-Quelltext
1:
2:
3:
4:
private bool AreAllSPSTextBoxesFilled(List<string> tbSPSConectionsValues)
{
    return tbSPSConectionsValues.All(tb => !string.IsNullOrEmpty(tb));
}

All is dann tatsächlich eine Linq Methode. Die du dir in deinem Fall so implementiert vorstellen kannst. Ziemlich ähnlich zudem was du dir selbst mit einer Schleife gebaut hast.

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
public bool All(TextBox[] source, Func<TextBox, bool> predicate) {
   foreach (TextBox textBox in source) 
       if (!predicate(textBox)) 
          return false;
   return true;
}

Heißt Linq versteckt meist nur die Schleifen (for, foreach, while was auch immer) ist aber keine Magie die passieren natürlich trotzdem.


Eigentlicher Code für All referencesource.micr....cs,be4bfd025bd2724c

user profile iconTh69 Für einen "ach du Sch***e Moment bezüglich neuen Features empfehle ich einen Blick auf Raw Strings github.com/dotnet/csharplang/issues/4304
Th69
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Moderator
Beiträge: 4764
Erhaltene Danke: 1052

Win10
C#, C++ (VS 2017/19/22)
BeitragVerfasst: Fr 03.06.22 15:12 
Ja, ich weiß - kenne ich auch schon von C++: Raw String Literal in C++
Ich frage mich generell dabei, wer benutzt festkodierte mehrzeilige String-Literale in seinem Code (die meisten dieser Strings sollten doch entweder z.B. für Übersetzungen aus Ressourcen stammen oder aus Dateien o.ä. eingelesen werden).
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Fr 03.06.22 23:42 
Servus,


Zitat:
wegen der Kurzschreibweise: wenn du englisch kannst,

Also Spanisch kann ich besser. Aber Google wird mir schon dabei helfen.

@ Ralf

Vielen Dank für deine Erklärungen und die Beispiele. Wird nicht einfach sein das alles zu verstehen.
Aber es ist wichtiger das der Code so gut wie möglich wird.

Deshalb eine Bitte und mal sehen wie ihr darüber denkt.

@ TH69

Schon wieder.
Zitat:
Du solltest dir auch noch mal die ShowResult-Methode anschauen,

Als ich mich um diese Methode gekümmert habe, ist mir aufgefallen das sie oft gebraucht wird.
An einigen Stellen sind immer wieder ähnliche Abfragen für die Variablen der SPS.
Ich habe leider nur das Gefühl das man das zusammenfassen kann ich weiß aber leider nicht wie.

Also es geht um folgende Codes:

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
    private void ConnectToPLC()
        {
            int Result = s7Client.ConnectTo(uivalues.IPNr, uivalues.RackNr, uivalues.SlotNr);
            ShowResult(Result);
            if (Result == 0// connection successfull
            {
                EnableTextBoxes(false);


ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
    private void ShowPlcStatus()
        {
            int Status = 0;
            int Result = s7Client.PlcGetStatus(ref Status);
            ShowResult(Result);
            if (Result == 0)
            {
                switch (Status)
                {
                    case S7Consts.S7CPUSTATUSRUN


ausblenden C#-Quelltext
1:
2:
3:
4:
        byte[] buffersize = new byte[1];

            int Result = s7Client.WriteArea(S7AreaDB, dbnumber, dboffset, 1, S7WLBit, buffersize);
            ShowResult(Result);


Und zum Schluß die Methode die immer aufgerufen wird.

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
    private void ShowResult(int Result)
        {
            // Diese Funktion gibt eine Texterklärung des Fehlercodes zurück
            txtError.Text = s7Client.ErrorText(Result);
            if (Result == 0)
            {
                txtError.Text = txtError.Text + " (" + s7Client.ExecutionTime.ToString() + " ms)";
            }
        }

Ich würde gerne die ganzen Visu Sachen gleich in die ShowResult Methode mit integrieren.
Über die Argumentklammern wollte ich dann eine entsprechende Argumentliste mit übergeben.

Über mehrere If Bedingung wird dann die Ausgabe gesteuert.

z.B.
ausblenden C#-Quelltext
1:
private void ShowResult(int Result, bool PlcGetStatus, bool connectTo,....)					


So vielen Dank nochmal für Eure toole Hilfe.

Grüße Tom
Th69
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Moderator
Beiträge: 4764
Erhaltene Danke: 1052

Win10
C#, C++ (VS 2017/19/22)
BeitragVerfasst: Sa 04.06.22 09:33 
Das solltest du m.E. nicht so umsetzen, da diese Methode dann zuviele verschiedene Dinge macht (zuviele Abhängigkeiten).

Mir ist nur aufgefallen, daß du in ShowPlcStatus erst ShowResult() aufrufst und direkt danach noch jeweils txtError.Text direkt zuweist (diese Zuweisungen können doch dann gelöscht werden, oder?).

Und in ShowResult
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
if (Result == 0)
{
    // ...
}
else
{
    if (Result > 0)
    {
        // ..
    }
}

ist entweder die Abfrage if (Result > 0) überflüssig, oder es fehlt noch die Abfrage auf Result < 0 (je nachdem was negative Werte bedeuten).

Es gibt auch noch eine kleine Speicher- und Performanceverbesserung für ShowResult:
ausblenden C#-Quelltext
1:
2:
3:
4:
if (Result == 0)
{
    txtError.AppendText(" (" + s7Client.ExecutionTime + " ms)"); // .ToString() ist hier auch überflüssig, da der + Operator automatisch diese aufruft
}
(es entfällt die aufwendigere + Operation, s.a. TextBoxBase.AppendText).

Benutzt du denn jetzt die SPS-Klasse?
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: So 05.06.22 19:50 
Hallo TH69,

Vielen Dank für deine Antworten.


Zitat:
Das solltest du m.E. nicht so umsetzen,

Ok, deswegen habe ich gefragt.

Zitat:
Und in ShowResult


Result == 0 ist OK und alles über 0 sind Fehler.

Ein Ausschnitt:
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
  public string ErrorText(int Error)
        {
            switch (Error)
            {
                case 0return "OK";
                case S7Consts.ERRTCPSOCKETCREATION: return "SYS : Error creating the Socket";
                case S7Consts.ERRTCPCONNECTIONTIMEOUT: return "TCP : Connection Timeout";
                case S7Consts.ERRTCPCONNECTIONFAILED: return "TCP : Connection Error";
                case S7Consts.ERRTCPRECEIVETIMEOUT: return "TCP : Data receive Timeout";
                case S7Consts.ERRTCPDATARECEIVE: return "TCP : Error receiving Data";
                case S7Consts.ERRTCPSENDTIMEOUT: return "TCP : Data send Timeout";
                case S7Consts.ERRTCPDATASEND: return "TCP : Error sending Data";


Zitat:
Benutzt du denn jetzt die SPS-Klasse?


Ich habe mir eine Kopie von meinem Projekt erstellt und versuche die SPS.cs darin zu integrieren.
Zur Zeit ruht die Sache da ich verärgert bin weil ich einige Zusammenhänge nicht verstehe.

Zitat:
ausblenden C#-Quelltext
1:
public S7VariablenResult ReadResult2(S7Input s7Input)					

Mit ReadResult2 habe ich Probleme zu verstehen was ich damit machen soll.
Ich verstehe nicht warum ich "2 mal" aus dem Datenbaustein lesen soll.
Aber lass gut sein ich komme schon dahinter ich brauche halt meine Zeit dafür.

Also vielen Dank noch mal für die tolle Hilfe.

Schöne Pfingsten noch.

Grüße Tom
Th69
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Moderator
Beiträge: 4764
Erhaltene Danke: 1052

Win10
C#, C++ (VS 2017/19/22)
BeitragVerfasst: So 05.06.22 20:17 
ReadResult1 und ReadResult2 sind zwei verschiedene Möglichkeiten, die Methode ReadResult zu implementieren (einmal mit Setzen von Klasseneigenschaften und beim der zweiten mittels Übergabe eines Parameters der Klasse S7Input, für welche dann die Eigenschaften vor dem Aufruf gesetzt werden müssen).
Nimm die, welche du besser verstehst.
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Mo 06.06.22 23:40 
Servus Th69,

Vielen Dank für deine Antwort.

Ok, jetzt weiß ich bescheid, echt blöde das ich das nicht selber erkannt habe.

Ich habe da eine Situation gehabt die ich optimieren möchte aber bevor ich da was mache frage ich Euch mal um Rat.
Also es wäre möglich das das Tool gestartet ist aber nicht verbunden. Ich muss also in der SPS ein Bit mit einbinden das sicherstellt das das Trigger Signal nur gesetzt werden kann, wenn eine Verbindung aufgebaut ist.
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
 private void TimerCopyAllFiles_Tick(object sender, EventArgs e)
{
    S7VariablenResult s7VariablenResult = ReadResult();

    if (s7Client.Connected )
    {
        // set a Bit...?
    }
}

Meine erste Frage wäre ob ich den Backgroundworker auch mit einer Schleife und Thread.Sleep aufrufen könnte?
Es geht mir darum auf den Windows Forms Timer zu verzichten.
Der Intervall ist auf 500 eingestellt und welcher Wert da der Richtige ist weiß ich nicht. Das müsste man messen.
Meine erste Idee ist ein Heartbeat aufzubauen aber dafür bräuchte ich einen Timer in der S7 Schnittstelle.
Ich denke das würde sich mit dem Windows Forms Timer beißen. Ist dieser Timer eigentlich in Kombination mit dem Backgroundworker der richtige Timer?

Dann habe ich das hier gelesen: Muss es immer Timer sein?

Kann man mit INotifyPropertyChanged vielleicht eine Variable von der SPS überwachen?
Und wenn ja mit einem Event den Backgroundworker aufrufen?
Wenn ich einen Timer in der SPS habe und einen Timer im Programm dann kommt es zu komischen Reaktionen da die Schnittstelle nicht in Echtzeit überträgt.

Was sollte ich Eurer Meinung nach machen?

Vielen Dank für die bisherige Hilfe.

Grüße Tom

Moderiert von user profile iconTh69: C#-Tags hinzugefügt
Moderiert von user profile iconTh69: URL-Titel hinzugefügt.
Th69
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Moderator
Beiträge: 4764
Erhaltene Danke: 1052

Win10
C#, C++ (VS 2017/19/22)
BeitragVerfasst: Di 07.06.22 08:30 
Das hatte ich dir aber auch in meinem Beitrag vom 2.6. schon geschrieben:
Th hat folgendes geschrieben:
für ReadResult habe ich dir 2 Versionen erstellt, einmal mit den "Eingabe"-Werten als Eigenschaften und einmal als extra Klasse S7Input.


Leider verstehe ich dein Problem mit dem Trigger-Signal nicht so richtig. Es gibt also in der S7-Schnittstelle eine Methode zum Setzen dieses Bits, aber du mußt jetzt dieses in einem festen Intervall triggern (während das Programm damit verbunden ist).

Beim Windows-Timer ist zu wissen, daß dieser (auf den meisten Systemen, je nach Hardware) nur eine Präzision von ca. 15-16ms hat (d.h. max. 64x je Sekunde aufgerufen wird), s. z.B. PowerShell Sleep Duration Accuracy and Windows Timers (laß dich von dem "PowerShell" nicht verwirren, dieser Artikel gilt allgemein für .NET bzw. Windows).
Es gibt aber genauere, z.B. Multimedia Timer [for .NET Core] (bedenke aber, daß dieser dann im Gegensatz zum Windows.Forms.Timer nicht direkt auf die UI zugreifen kann, du also genauso wie für den BackgroundWorker programmieren müßtest).

Ich sehe aber jetzt ersteinmal (bis auf evtl. höhere Präzision) nichts, was gegen deinen bisherigen Code spricht (also Windows.Forms.Timer und BackgroundWorker).

Und wenn die S7-Schnittstelle selbst kein Event bereitstellt, dann hilft nur ein regelmäßiges Setzen (bzw. Abfragen).