Autor Beitrag
UserTom
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Mo 16.05.22 22:50 
Hallo Gemeinde,

Ich bin neu hier bei Euch. Ich bin SPSler und habe mit C# normalerweise nichts zu tun.
Mit einigen gut bebilderten Internetseiten habe ich es geschafft mit einem Winforms Projekt mich mit einer SPS zu verbinden. (Sharp7.dll)
Über ein Trigger Signal von der SPS möchte ich einige Kopieraktionen durchführen. Es gibt auch verschiebeaktionen. Innnerhalb Lokal und von anderen PCs über das Netzwerk auf Lokal. Das kopieren der Dateien und auch Ordner habe ich umsetzen können. Das Ergebenis ist aber eher schlecht als recht. Mit einem Timer rufe ich einen Backgrounworker auf um die ganzen kopieraktionen im Hintergrund auszuführen. Gui blockiert aber trotzdem. Ich habe auch schon probiert mit Task.Run das kopieren auszulagern.
Hat auch nicht geholfen. Die Oberfläche blockiert (keine Rückmeldung) in dem Moment wo das kopieren beginnt bis es fertig ist. Auf einer Internetseite habe ich gelesen das gleichzeitige kopieren eh nichts bringen soll wegen den gleichzeitigen schreibzugriffen.
Auf einer anderen Seite habe ich gelesen Threads ist besser wie der Backgroundworker.
Hat jemand von Euch damit Erfahrung und kann mir sagen wie ich das umsetzen muss?
Vielleicht hat auch der Timer schuld daran, aber den brauch ich als Loop für die Verbindung mit der SPS. Alle 65s steht ein Triggersignal an und das darf ich ja nicht verpassen.

ausblenden volle Höhe 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:
28:
29:
30:
31:
32:
33:
34:
35:
36:
37:
38:
39:
40:
41:
42:
43:
44:
45:
46:
47:
48:
49:
50:
51:
52:
53:
54:
55:
56:
57:
58:
59:
60:
61:
62:
63:
64:
65:
66:
67:
68:
69:
70:
71:
72:
73:
74:
75:
76:
77:
78:
79:
80:
81:
82:
83:
84:
85:
86:
87:
88:
89:
90:
91:
92:
93:
94:
95:
96:
97:
98:
99:
100:
101:
102:
103:
104:
105:
106:
107:
108:
109:
110:
111:
112:
113:
114:
115:
116:
117:
118:
119:
120:
121:
122:
123:
124:
125:
126:
127:
128:
129:
130:
131:
132:
133:
134:
135:
136:
137:
138:
139:
140:
141:
142:
143:
144:
145:
146:
147:
148:
149:
150:
151:
152:
153:
154:
155:
156:
157:
158:
159:
160:
161:
162:
163:
164:
165:
166:
167:
168:
169:
170:
171:
172:
173:
174:
175:
176:
177:
178:
179:
180:
181:
182:
183:
184:
185:
186:
187:
  private void EventLoop_Tick(object sender, EventArgs e)
        {
            EventLoop.Enabled = false;

            if (BW_Event.IsBusy != true)
            {
                BW_Event.RunWorkerAsync();
            }
        }

        private void BW_Event_DoWork(object sender, DoWorkEventArgs e)
        {
            BW_Event.ReportProgress(0);
        }

        private void BW_Event_ProgressChanged(object sender, ProgressChangedEventArgs e)
        {
            try
            {
                S7VariablenResult s7VariablenResult = LeakResult();

                string createTargetPath = "";

                if (s7VariablenResult.TrigSaveFiles && (s7VariablenResult.TypLinks || s7VariablenResult.TypRechts) && !s7VariablenResult.RoboterDSLeer && !s7VariablenResult.FMBauteil && !s7VariablenResult.RoboterDSLeer)
                {

                    lblPraegeCode.Text = s7VariablenResult.PraegeCodeNr; // It’s already a string
                    lblFolderName.Text = s7VariablenResult.FolderName;
                    lblPraegeCode2.Text = s7VariablenResult.PraegeCode2Nr;

                    createTargetPath = Path.Combine(@"D:\S7_Export\AppData", lblFolderName.Text);
                    try
                    {
                        //Ordner erstellen
                        if (!Directory.Exists(createTargetPath))
                        {
                            Directory.CreateDirectory(createTargetPath);
                        }
                    }
                    catch (Exception ex)
                    {
                        MessageBox.Show(this, ex.Message, "Ordner erstellen fehlgeschlagen!", MessageBoxButtons.OK, MessageBoxIcon.Error);
                    }

                    
                    // 10R01 Tox Clinchnietbolzen

                    //IEnumerable<string> Tox10R01files = Directory.EnumerateFiles(@"\\tox10r01\AppData", "*", System.IO.SearchOption.TopDirectoryOnly).Where(x => x.Contains(lblPraegeCode.Text));
                    IEnumerable<string> Tox10R01files = Directory.EnumerateFiles(@"\\computer\PDE\tox10r01\AppData""*", System.IO.SearchOption.TopDirectoryOnly).Where(x => x.Contains(lblPraegeCode.Text));

                    foreach (string file in Tox10R01files)
                    {

                        try
                        {
                            File.Copy(file, $"{Path.Combine(@"D:\S7_Export\AppData", lblFolderName.Text + "\\")}{Path.GetFileName(file) }");

                        }
                        catch (DirectoryNotFoundException ex)
                        {
                            ex.SaveErrorMessage();
                        }
                        catch (UnauthorizedAccessException ex)
                        {
                            ex.SaveErrorMessage();
                        }
                    }


                    
                    //15R01 Tox Kalottenprägen

                    //IEnumerable<string> ToxKPr15R01files = Directory.EnumerateFiles(@"\\tox15r01\AppData\", "*", System.IO.SearchOption.TopDirectoryOnly).Where(x => x.Contains(lblPraegeCode.Text));
                    IEnumerable<string> ToxKPr15R01files = Directory.EnumerateFiles(@"\\computer\PDE\tox15r01\AppData""*", System.IO.SearchOption.TopDirectoryOnly).Where(x => x.Contains(lblPraegeCode.Text));

                    foreach (string file in ToxKPr15R01files)
                    {

                        try
                        {
                            File.Copy(file, $"{Path.Combine(@"D:\S7_Export\AppData", lblFolderName.Text + "\\")}{Path.GetFileName(file) }");

                        }
                        catch (DirectoryNotFoundException ex)
                        {
                            ex.SaveErrorMessage();
                        }
                        catch (UnauthorizedAccessException ex)
                        {
                            ex.SaveErrorMessage();
                        }
                    }

                    
                    //15R02 Tox Clinchnietbolzen

                    //IEnumerable<string> Tox15R02files = Directory.EnumerateFiles(@"\\tox15r02\AppData\", "*", System.IO.SearchOption.TopDirectoryOnly).Where(x => x.Contains(lblPraegeCode.Text));
                    IEnumerable<string> Tox15R02files = Directory.EnumerateFiles(@"\\computer\PDE\tox15r02\AppData""*", System.IO.SearchOption.TopDirectoryOnly).Where(x => x.Contains(lblPraegeCode.Text));

                    foreach (string file in Tox15R02files)
                    {

                        try
                        {
                            File.Copy(file, $"{Path.Combine(@"D:\S7_Export\AppData", lblFolderName.Text + "\\")}{Path.GetFileName(file) }");

                        }
                        catch (DirectoryNotFoundException ex)
                        {
                            ex.SaveErrorMessage();
                        }
                        catch (UnauthorizedAccessException ex)
                        {
                            ex.SaveErrorMessage();
                        }
                    }

                    
                    //15R02 Intec Kleben

                    //IEnumerable<string> Kle15R02dirs = Directory.EnumerateDirectories(@"\\kl15r02\AppData\", "*", System.IO.SearchOption.AllDirectories).Where(x => x.Contains(lblPraegeCode.Text));
                    IEnumerable<string> Kle15R02dirs = Directory.EnumerateDirectories(@"\\computer\PDE\kle15r02\AppData""*", System.IO.SearchOption.AllDirectories).Where(x => x.Contains(lblPraegeCode.Text));

                    foreach (string dir in Kle15R02dirs)
                    {
                        try
                        {
                            // ohne Backslash
                            //FileSystem.CopyDirectory(dir, dir.Replace(@"\\kl15r02\AppData", Path.Combine(@"D:\S7_Export\AppData", lblFolderName.Text)), true);
                            FileSystem.CopyDirectory(dir, dir.Replace(@"\\computer\PDE\kle15r02\AppData", Path.Combine(@"D:\S7_Export\AppData", lblFolderName.Text)), true);

                        }
                        catch (DirectoryNotFoundException ex)
                        {
                            ex.SaveErrorMessage();
                        }
                        catch (UnauthorizedAccessException ex)
                        {
                            ex.SaveErrorMessage();
                        }
                    }
 private S7VariablenResult LeakResult()
        {
            S7VariablenResult s7VariablenResult = new S7VariablenResult();
            // Reads the buffer.
            int dbnumber = short.Parse(txtDBNr.Text);
            int dboffset = short.Parse(txtDBOffsetNr.Text);
            int dbsize = short.Parse(txtDBSizeNr.Text);

            int PraegeCodeDBPos = short.Parse(txtPraegeCodeDBPos.Text);
            int FolderNameDBPos = short.Parse(txtFolderNameDBPos.Text);
            int PraegeCode2DBPos = short.Parse(txtPraegeCode2DBPos.Text);

            int TrigSaveFilesByte = short.Parse(txtTrigSaveFilesByte.Text);
            int TrigSaveFilesBit = short.Parse(txtTrigSaveFilesBit.Text);

            int TypLinksByte = short.Parse(txtTypLinksByte.Text);
            int TypLinksBit = short.Parse(txtTypLinksBit.Text);

            int TypRechtsByte = short.Parse(txtTypRechtsByte.Text);
            int TypRechtsBit = short.Parse(txtTypRechtsBit.Text);

            int RoboterDSLeerByte = short.Parse(txtRoboterDSLeerByte.Text);
            int RoboterDSLeerBit = short.Parse(txtRoboterDSLeerBit.Text);

            int FMBauteilByte = short.Parse(txtFMBauteilByte.Text);
            int FMBauteilBit = short.Parse(txtFMBauteilBit.Text);

            int HeartByte = short.Parse(txtHeartByte.Text);
            int HeartBit = short.Parse(txtHeartBit.Text);

            byte[] dbbuffersize = new byte[dbsize];

            s7Client.DBRead(dbnumber, dboffset, dbsize, dbbuffersize);

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

            return s7VariablenResult;
        }


Es wäre echt toll wenn einer von Euch eine Idee für mich hätte.

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: Di 17.05.22 09:18 
Hallo und :welcome:,

asynchrone Programmierung kann ersteinmal ein großer Stolperstein für Anfänger sein.

Dein Hauptfehler ist, daß du die eigentliche Kopieraktion im ProgressChanged durchführst, anstatt im DoWork (das ProgressChanged-Ereignis läuft im UI-Thread und blockiert daher die GUI - es sollte nur für kurze Ausgabeaktualisierungen benutzt werden)!

PS: Der Backgroundworker wird in modernem C# eigentlich nicht mehr benutzt und stattdessen mit Tasks und den Schlüsselwörtern async und await gearbeitet. Für (einfache) WinForms-Projekte halte ich es jedoch noch OK.

PPS: Dateipfade solltest du besser konfigurierbar halten (anstatt fest im Code als Stringliterale zu verwenden)!
Und den eigentlichen Kopiercode solltest du in eine eigene Methode auslagern und mit passenden Parametern aufrufen (anstatt Copy&Paste-Code).
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Di 17.05.22 10:58 
Hallo TH69,

Vielen Dank für deine Antwort.

Zitat:
asynchrone Programmierung kann ersteinmal ein großer Stolperstein für Anfänger sein.

Das kannst du laut sagen. Dazu gleich eine Frage das ich den RunWorkerAsync im Timer aufrufe ist ok? mit viele Sekunden sollte er ticken?
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
 private void EventLoop_Tick(object sender, EventArgs e)
        {
            EventLoop.Enabled = false;

            if (BW_Event.IsBusy != true)
            {
                BW_Event.RunWorkerAsync();
            }
        }


Zitat:
Dein Hauptfehler ist, daß du die eigentliche Kopieraktion im ProgressChanged durchführst, anstatt im DoWork (das ProgressChanged-Ereignis läuft im UI-Thread und blockiert daher die GUI - es sollte nur für kurze Ausgabeaktualisierungen benutzt werden)!

Ich hatte alles im DoWork
Wegen dem Label: lblPraegeCode.Text = s7VariablenResult.PraegeCodeNr; habe ich alles in das ProgressChanged getan.
Ich muss also nur drauf aufpassen das keine Zugriff auf Gui-Elemente im DoWork passieren.

Wann und wo muss ich BW_Event.ReportProgress(0); aufrufen? Im DoWork nach dem kopieren?

Zitat:
PS: Der Backgroundworker wird in modernem C# eigentlich nicht mehr benutzt und stattdessen mit Tasks und den Schlüsselwörtern async und await gearbeitet. Für (einfache) WinForms-Projekte halte ich es jedoch noch OK.

Hättest du ein Beispiel oder ein guten Link in Zusammenhang Winforms und kopieren und Timer als Loop.

Zitat:
PPS: Dateipfade solltest du besser konfigurierbar halten (anstatt fest im Code als Stringliterale zu verwenden)!

Ja da hast du recht, es gibt auch eine Variante, aber wegen des Backgroundworker habe ich es fest reingeschrieben.

Zitat:
Und den eigentlichen Kopiercode solltest du in eine eigene Methode auslagern und mit passenden Parametern aufrufen (anstatt Copy&Paste-Code).

Ich frage lieber nach als dass ich mir noch weitere doppelte Arbeit mache, die ich zur Genüge hatte.
Jeder Kopiercode in eine eigene Methode oder alle Kopieraktionen in eine Methode.
Könntest du mir das mit den passenden Parametern verdeutlichen?

Na da bin ich ja mal gespannt, ob es jetzt besser funktioniert.

Vielen Dank nochmal für deine Hilfe.

Grüße Tom

Moderiert von user profile iconTh69: C#-Tags hinzugefügt
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 17.05.22 13:13 
Zitat:
Das kannst du laut sagen. Dazu gleich eine Frage das ich den RunWorkerAsync im Timer aufrufe ist ok? mit viele Sekunden sollte er ticken?

Das kann man kaum beantworten weil das Muster das du da benutzt eher undurchsichtig ist ;) Möglicherweise steckt da zuviel SPS denkweise drin.
Ein Timer der was aynchron startet wie hier ist schwer zu kontrollieren. Du versuchst dich ja schon mit abfragen von IsBusy dich zu schützen.
Möglicherweise erklärst du uns kurz warum da regelmäßig und scheinbar oft kopiert werden muß. Dann haven wir vermutlich ein passenderes Muster parat.


Zitat:
Wann und wo muss ich BW_Event.ReportProgress(0); aufrufen? Im DoWork nach dem kopieren?

Jedesmal wenn du die UI updaten willst. Der eigentlich Code gehört ja in DoWork und da bleibt für ReportProgress nur die Aufgabe irgendwo mitzuteilen wie weit der Fortschritt ist.
Wenn du nirgendwo den Fortschritt anzeigen willst mußt du ReportProgress im Zweifel gar nicht aufrufen/benutzen.
Zitat:
Jeder Kopiercode in eine eigene Methode oder alle Kopieraktionen in eine Methode.
Könntest du mir das mit den passenden Parametern verdeutlichen?

Deine Kopierschleifen sehen immer gleich aus bis auf das sie jedemal eine andere File Liste benutzen.
Dieser Code sollte einfach eine eigene Methode sein wo die FileListe reingeht. Dann hast du den immer gleichen Code auch tatsächlich nur einmal.

Und möglicherweise den Zielordner reinreichen bzw. aus einer Klassenvariablen so das du den vorher festlegen kannst. Im Moment holst du den direkt aus dem UI Control. Und wie wir schon festgestellt haben muß dieser Code später wieder aus DoWork aufgerufen wo keine UI Zugriffe stattfinden sollten.
Also am besten den Zielordner vor DoWork in einer Variablen speichern und von dort benutzen.

Da stellt sich mir auch die Frage ob du das überhaupt so willst. Würde das mit dem im Hintergrund ausführen tatsächlich so funktionieren könnte jeder zu jedem Zeitpunkt parallel das Ziel ändern.
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Di 17.05.22 14:02 
Hallo Ralf,

Vielen Dank für deine Hilfe.

Zitat:
Möglicherweise erklärst du uns kurz warum da regelmäßig und scheinbar oft kopiert werden muß.

Alle 65s ist ein Bauteil fertig. Und mit dem Fertigsignal (Trigger) müssen von allen Applikations-PCs über Netzwerk
die Daten abgeholt werden. Auf dem PC wo die Winform läuft sind auch Daten die abgeholt werden müssen.
Der Zielpfad ist Path.Combine(@"D:\S7_Export\AppData", lblFolderName.Text). Das Label lblFolderName.Text ändert sich mit jedem Bauteil.

Der Timer ist dafür gedacht das er ständig die Variablen von der SPS überwacht damit das Triggersignal das kopieren anstossen kann.

Diese Methode ist zum abrufen der SPS Signal zuständig.
ausblenden volle Höhe 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:
28:
29:
30:
31:
32:
33:
34:
35:
36:
37:
38:
39:
40:
41:
42:
43:
44:
45:
46:
        private S7VariablenResult LeakResult()
        {
            S7VariablenResult s7VariablenResult = new S7VariablenResult();
            // Reads the buffer.
            int dbnumber = short.Parse(txtDBNr.Text);
            int dboffset = short.Parse(txtDBOffsetNr.Text);
            int dbsize = short.Parse(txtDBSizeNr.Text);

            int PraegeCodeDBPos = short.Parse(txtPraegeCodeDBPos.Text);
            int FolderNameDBPos = short.Parse(txtFolderNameDBPos.Text);
            int PraegeCode2DBPos = short.Parse(txtPraegeCode2DBPos.Text);

            int TrigSaveFilesByte = short.Parse(txtTrigSaveFilesByte.Text);
            int TrigSaveFilesBit = short.Parse(txtTrigSaveFilesBit.Text);

            int TypLinksByte = short.Parse(txtTypLinksByte.Text);
            int TypLinksBit = short.Parse(txtTypLinksBit.Text);

            int TypRechtsByte = short.Parse(txtTypRechtsByte.Text);
            int TypRechtsBit = short.Parse(txtTypRechtsBit.Text);

            int RoboterDSLeerByte = short.Parse(txtRoboterDSLeerByte.Text);
            int RoboterDSLeerBit = short.Parse(txtRoboterDSLeerBit.Text);

            int FMBauteilByte = short.Parse(txtFMBauteilByte.Text);
            int FMBauteilBit = short.Parse(txtFMBauteilBit.Text);

            int HeartByte = short.Parse(txtHeartByte.Text);
            int HeartBit = short.Parse(txtHeartBit.Text);

            byte[] dbbuffersize = new byte[dbsize];

            s7Client.DBRead(dbnumber, dboffset, dbsize, dbbuffersize);

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

            return s7VariablenResult;
        }


Zitat:
Wenn du nirgendwo den Fortschritt anzeigen willst mußt du ReportProgress im Zweifel gar nicht aufrufen/benutzen.

Nein einen Fortschrittbalken brauche ich nicht unbedingt. Aber ein paar SPS Variablen werden in Labels angezeigt.
Das kann vor dem kopieren passieren weil diese Variablen werden auch für das kopieren gebraucht. Oder danach so zu sagen als Complet Meldung.

Zitat:
Dieser Code sollte einfach eine eigene Methode sein wo die FileListe reingeht. Dann hast du den immer gleichen Code auch tatsächlich nur einmal.

Wenn ich dich richtig verstanden habe nehme ich alle meine Kopierbefehle und packe sie in eine Methode und kann so zum Beispiel auch die Variable für den Zielpfad mit übergeben. Die Quellpfade sollen über Textboxen als Variablen übergeben werden.

Vielen Dank nochmal für deine Antwort

Grüße Tom

Moderiert von user profile iconTh69: C#-Tags hinzugefügt
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 17.05.22 20:07 
Zitat:
Wenn ich dich richtig verstanden habe nehme ich alle meine Kopierbefehle und packe sie in eine Methode und kann so zum Beispiel auch die Variable

Nein du packst einen Kopierbefehl in eine neue Methode und rufst diese Methode mehrmals auf. Ziel ist Code zu deduplizieren den du einfach fast identisch mehrmals hast.
Ein Beispiel mit Teilen deines Codes (nicht kompilierend weil hier im Editor "programmiert")

ausblenden volle Höhe 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:
28:
29:
30:
31:
32:
33:
34:
35:
36:
   private const string DefaultPath = "D:\S7_Export\AppData";
   private S7VariablenResult s7VariablenResult;
   
   private void EventLoop_Tick(object sender, EventArgs e)
   {
        if (!BW_Event.IsBusy)
        {
            // der ganze Quatsch mit UI Bezug hierhin 
            // kann sich so konkurierend zum Backgroundworker thread nicht ändern und ist damit Threadsafe
            s7VariablenResult = LeakResult(); 
        
            BW_Event.RunWorkerAsync();
        }
   }
   
   private void BW_Event_DoWork(object sender, DoWorkEventArgs e)
   {
        var bw = sender as Backgroundworker;

        var filter = $"*{s7VariablenResult.PraegeCodeNr}";
        var targetPath = Path.Combine(DefaultPath, s7VariablenResult.FolderName)
        Directory.CreateDirectory(targetPath);    
   
        bw.ReportProgress(0);
        CopyFiles(@"\\computer\PDE\tox10r01\AppData", targetPath , filter);
        bw.ReportProgress(10);
        CopyFiles(@"\\computer\PDE\tox15r01\AppData", targetPath , filter);      
        bw.ReportProgress(20);  
        // .usw
   }   
   
   private static void CopyFiles(string source, string target, string filter)
   {
        foreach (FileInfo file in new DirectoryInfo(source).GetFiles(filter))
            file.CopyTo(Path.Combine(target, file.Name));
   }
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Mi 18.05.22 10:51 
Hallo Ralf,

Vielen Dank für deine Antwort und für den Code.

Dein Beispiel ändert alles und es war wieder falsch nur einen kleinen Abschnitt zur Verfügung zu stellen.
Allein wegen DoWork müsste ich so viel umschreiben. Meine ganze Arbeit umsonst, ich hätte euch früher fragen sollen.

Zitat:

ausblenden C#-Quelltext
1:
private const string DefaultPath = "D:\S7_Export\AppData";					



Kann ich über Konstanten alle meine Oberflächenelemente in eine Variable speichern?
Oder wie würdet Ihr das machen?

Ich habe in jeder Methode Zugriffe auf die Gui-Elemente, die ich gegen Variablen austauschen muss.
Sonst kann ich die Methoden ja nicht in Dowork aufrufen, oder?

Ich hänge mal mein eigentliches Projekt mit an, vielleicht kannst du mal hineinschauen und mir sagen, was ich ändern muss.
Ich habe 4 große Methoden drin für Dateien kopieren, Ordner kopieren, Dateien verschieben und Ordner verschieben.
Eine Methode heißt Ready, die ein Signal an die SPS zurückgibt, die wird nach dem Kopieren aufgerufen.
Ob noch im Backgroundworker oder anders weiß ich nicht wirklich.
Bitte nicht erschrecken über die vielen Wiederholungen ich hatte keine Ahnung von Deduplizieren oder besser gesagt
Ich wusste nicht, wie ich es hätte besser machen sollen. Ich bin froh über das, was ich geschafft habe.
Es läuft und macht genau, was es soll. Nur das, wenn das Kopieren angestoßen wird gibt es für 30 sek. keine Rückmeldung.
Es wäre schön, wenn du mir nur dabei hilfst den Backgroundworker da rein zu bekommen. Das Hauptproblem für mich sind
die Variablen für die Gui-Elemente, wenn ich das umgebaut habe kann ich die Methoden für das Kopieren im DoWork aufrufen.

Vielen Dank nochmal für deine Unterstützung.

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 18.05.22 12:21 
user profile iconUserTom hat folgendes geschrieben Zum zitierten Posting springen:
Dein Beispiel ändert alles und es war wieder falsch nur einen kleinen Abschnitt zur Verfügung zu stellen.
Allein wegen DoWork müsste ich so viel umschreiben. Meine ganze Arbeit umsonst, ich hätte euch früher fragen sollen.

Das Refactoring gehört zur täglichen Arbeit eines Software-Entwicklers dazu und manchmal sind es eben auch größere Codeänderungen (VS unterstützt aber z.B. das Auslagern in eigene Methoden sehr gut: Refactoring von Code).

Zitat:
Kann ich über Konstanten alle meine Oberflächenelemente in eine Variable speichern?
Oder wie würdet Ihr das machen?

Ich habe in jeder Methode Zugriffe auf die Gui-Elemente, die ich gegen Variablen austauschen muss.
Sonst kann ich die Methoden ja nicht in Dowork aufrufen, oder?

So wie Ralf schon geschrieben hat, merke dir vor dem BW_Event.RunWorkerAsync() alle aus der GUI benötigten Werte in Membervariablen (bzw. erstelle dafür eine eigene Hilfsklasse).

PS: Gib mal keine Zahlen in deine Textboxen ein... (was ich meine: du solltest statt short.Parse besser TryParse verwenden, um Eingabefehler abzufangen).
Und warum verwendest du short.Parse, wenn du es einer int-Variablen zuweist? Benutze int.Parse.

PPS: Dein Code, gerade bzgl. deiner Control-Arrays, kommt mir irgendwie bekannt vor (Stichwort: dynamisches Erstellen von Controls). Du solltest dann aber auch die Arrays in deinem Code konsequent benutzen (also Schleifen, anstatt manuell die Controls, Zeile für Zeile, anzusprechen)!
Das würde dir deinen Code um mind. die Hälfte reduzieren. Merke dir außerdem, daß der Code in einer Klasse nur so max. 500-1000 Zeilen groß sein sollte, um noch überschaubar zu sein.
Die eigentlichen IO-Operationen solltest du in eine eigene Klasse auslagern und dann nur von deiner Form (mit passenden Parametern) aufrufen.
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Mi 18.05.22 13:29 
Hallo TH69,

Vielen Dank für deine Antwort und die Ratschläge.

Zitat:

So wie Ralf schon geschrieben hat, merke dir vor dem BW_Event.RunWorkerAsync() alle aus der GUI benötigten Werte in Membervariablen


Ok, werde ich versuchen, umzusetzen.
Eine Frage noch dazu, wenn ich keine Informationen über das Kopieren haben möchte, kann ich doch BW_Event_ProgressChanged weglassen, oder?

Zitat:

(bzw. erstelle dafür eine eigene Hilfsklasse).


Das wäre perfekt aber nichts für mich ich bekomme es nicht hin wie die Klassen mit der Form kommunizieren.
Alle Klassendateien, die in diesem Projekt sind, habe ich als Copy-and-paste aus dem Internet.
Außer die Settings.cs, da hat mir jemand aus einem Forum bei geholfen.

Zitat:

Und warum verwendest du short.Parse, wenn du es einer int-Variablen zuweist? Benutze int.Parse.


Vielen Dank, das habe ich von einer Internetseite so übernommen. Ich habe es angepasst.
Da es keinen Fehler ausgelöst hat, bin ich davon ausgegangen, dass das so richtig ist.

Zitat:

PPS: Dein Code, gerade bzgl. deiner Control-Arrays, kommt mir irgendwie bekannt vor (Stichwort: dynamisches Erstellen von Controls). Du solltest dann aber auch die Arrays in deinem Code konsequent benutzen (also Schleifen, anstatt manuell die Controls, Zeile für Zeile, anzusprechen)!
Das würde dir deinen Code um mind. die Hälfte reduzieren. Merke dir außerdem, dass der Code in einer Klasse nur so max. 500-1000 Zeilen groß sein sollte, um noch überschaubar zu sein.


Wie mit den Hilfsklassen bin ich mit Schleifen erstellen überfordert. Ich bastle jetzt schon seit über 1 Jahr an diesem Projekt.
Ich habe nicht immer Zeit dafür. Dann wollte ich in den Foren nicht so viel nachfragen, weil ich wollte es alleine schaffen.
Das Ergebnis siehst du. Ich hoffe, ich bekomme das mit dem Backgroundworker noch hin.

Zitat:

Die eigentlichen IO-Operationen solltest du in eine eigene Klasse auslagern und dann nur von deiner Form (mit passenden Parametern) aufrufen.

Würde ich gerne, aber daran verzweifle ich. Habe ich ja weiter oben schon erwähnt.

Ich beneide jeden von euch. Wenn ich das vor 30 Jahren schon gewusst hätte, dann hätte ich einen anderen Weg eingeschlagen.


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: Mi 18.05.22 15:20 
Zitat:
Eine Frage noch dazu, wenn ich keine Informationen über das Kopieren haben möchte, kann ich doch BW_Event_ProgressChanged weglassen, oder?

Ja. Wenn du kein Progress brauchst mußt du die Funktion für den Progress nicht aufrufen ;)
Zitat:
Das wäre perfekt aber nichts für mich ich bekomme es nicht hin wie die Klassen mit der Form kommunizieren.

a.) Vermutlich eher andersherum. Die Form kommuniziert mit der Klasse die die Funktionalität enthält.
b.) Und das geht dann ungefähr genauso wie du das jetzt schon tust ;) Deine S7VariablenResult Klasse ist sowas. Diese enthält vermutlich gerade nur Daten könnte aber genauso gut Logik enthalten.
Zitat:
Da es keinen Fehler ausgelöst hat, bin ich davon ausgegangen, dass das so richtig ist.

Das besagt das es mal gehen kann ist aber was anderes als immer ;)


Zuletzt bearbeitet von Ralf Jansen am Mi 18.05.22 15:24, insgesamt 1-mal bearbeitet
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 18.05.22 15:23 
Ja, das ProgressChanged-Event kannst du weglassen, wenn du es nicht brauchst. Da der Kopiervorgang jedoch bei dir ca. 30s benötigt, wie du schreibst, wäre es doch trotzdem gut, diesen (als Rückmeldung) anzuzeigen (und wenn es nur vor bzw. nach jeden der Kopierschleifen ist).

Und mit Hilfsklasse für die Speicherung der UI-Elemente meine ich einfach soetwas:
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
// als Unterklasse in deiner Form-Klasse
class UIValues
{
  public bool Checked1 { get; set; }
  // ...
}

private UIValues UIValues = new();

Und dann (genauso wie bei der S7VariablenResult-Klasse) entsprechend die Werte setzen:
ausblenden C#-Quelltext
1:
UIValues.Checked1 = chkActivateNetApp1.Checked;					

(auch hier solltest du dann ein Array für Checked benutzen, s.u.).
Und bei den Kopierschleifen greifst du dann auf UIValues.CheckedX (bzw. als Array auf UIValues.Checked[n] zu).

Und bzgl. der Schleifen hier zwei Code-Verbesserungen:
1. statt Z. 119-165
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
const int MaxLocalApps = 8;
for (int n = 0; n < MaxLocalApps; n++)
{
    cboLocalApps[n].Enabled = true;
    tbSourceFilePathLocalApps[n].Enabled = true;
    cbActivateLocalApps[n].Enabled = true;
    cbLocalSubFolderUses[n].Enabled = true;
    cbActivateCode2LocalApps[n].Enabled = true;
}

(also Code-Ersparnis (= 47 - 9) 38 Zeilen!)
Und dasselbe dann für die NetApps.

2. statt 223 -233:
ausblenden C#-Quelltext
1:
2:
3:
4:
for (int n = 0; n < MaxLocalApps; n++)
{
    cboLocalApps[n].Items.AddRange(txtLocalApplication.Lines);  
}

Und dasselbe dann für die NetApps darunter.

Und die gleiche Art von Schleife dann in SaveLocalPathConfigSettings() sowie SaveNetPathConfigSettings(). Idealerweise, da es derselbe Code ist, diese Schleife(n) in eine eigene Methode auslagern.

Und auch die ganzen txtXXX.Enabled-Aufrufe könnte man einfacher mittels einer Schleife über das Controls-Array lösen.
Aber als erstes würde es reichen, den Code dafür in eine eigene Methode auszulagern (da du den Codeblock mehrfach hast):
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
private void EnableTextBoxes(bool enable)
{
  txtIPNr.Enabled = enable;
  // ...
  txtHeartBit.Enabled = enable;
}

Und dann nur EnableTxtBoxes(true) bzw. EnableTextBoxes(false) aufrufen. ;-)

Wenn du dann noch die von Ralf geschriebene Code-Auslagerung der Kopierfunktionen machst, dann ist dein Code (einigermaßen) aufgeräumt und lesbar.

Schau dir am besten in deinem C#-Buch bzw. Arrays (C#-Programmierhandbuch) ff., wie man Arrays im Zusammenhang mit Schleifen benutzt.
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Do 19.05.22 09:58 
Servus,

Vielen Dank an Euch für Eure Hilfe, Ideen und die Codebeispiele.

Zitat:
a.) Vermutlich eher andersherum. Die Form kommuniziert mit der Klasse, die die Funktionalität enthält.

Da kannst Du mal sehen, was für eine Ahnung ich habe.

Zitat:
Und dann (genauso wie bei der S7VariablenResult-Klasse) entsprechend die Werte setzen:

Wie Du siehst, sind da nur Variablen drin. Ich habe leider keine Klassendatei, die ich als Vorlage nehmen könnte.
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;      // PDE 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
    }
}


Zitat:
Und mit Hilfsklasse für die Speicherung der UI-Elemente meine ich einfach soetwas:


Also um ganz klein anzufangen könntest Du mir bitte erklären wie ich für die Textbox txtDBNr.Text über die Klasse UIValues eine Variable zur
Verfügung stelle.
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
private void Ready()
        {
            int S7AreaDB = 0x84;
            int dbnumber = int.Parse(txtDBNr.Text);
            int TrigSaveFilesByte = int.Parse(txtTrigSaveFilesByte.Text);
            int TrigSaveFilesBit = int.Parse(txtTrigSaveFilesBit.Text);
            int dboffset = (TrigSaveFilesByte * 8) + TrigSaveFilesBit;
            int S7WLBit = 0x01;
            byte[] buffersize = new byte[1];

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

            EventLoop.Enabled = true;
        }


Hier habe ich Deinen Anfang. Ich werde es mit Google weiter versuchen.

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
namespace PDE
{
    class UIValues
    {
       public int DBNr { get; set; }
      
    }

    private UIValues UIValues = new();
}



ausblenden C#-Quelltext
1:
private UIValues UIValues = new();					

Das kann ich nicht benutzen, mein Projekt ist mit NET Framework 4.8 erstellt. Dafür brauche ich C#9 .NET Core 5
Wie würdest Du NET Framework 4.8 in .NET Core 5 konvertieren?

Zitat:
Und bzgl. der Schleifen hier zwei Code-Verbesserungen:

Vielen Dank, das ist sehr zuvorkommend von Dir.
Das habe ich mir auch als Anreiz für die for Scheifen für das Kopieren genommen. Siehe weiter unten.

Zitat:
Und die gleiche Art von Schleife dann in SaveLocalPathConfigSettings() sowie SaveNetPathConfigSettings(). Idealerweise, da es derselbe Code ist, diese Schleife(n) in eine eigene Methode auslagern.

Da weiß ich nicht genau, worauf Du Dich beziehst. Kannst Du mir die Zeilennummer bitte sagen?

Zitat:
Und auch die ganzen txtXXX.Enabled-Aufrufe könnte man einfacher mittels einer Schleife über das Controls-Array lösen.

Ich tue mich sehr schwer damit, ich habe gute 20min gerätselt, wie Du das meinst. Das = enable hat mich total verwirrt.
Im Anhang ist dann der neu komplette Code. Du kannst ja mal schauen, ob Du das so gemeint hattest.

Zitat:
Wenn du dann noch die von Ralf geschriebene Code-Auslagerung der Kopierfunktionen machst, dann ist dein Code (einigermaßen) aufgeräumt und lesbar.

Ich habe schon angefangen, habe aber mit einer Variable Probleme.
Die Idee mit der Konstante von Ralf für den Defaultpath ist super und habe ich auch umgesetzt.

Da ist das i von der For-Schleife mit drin und somit geht das nicht mit Konstanten. Oder?
ausblenden C#-Quelltext
1:
string sourceFilePath = tbSourceFilePathLocalApps[i].Text;					


ausblenden volle Höhe 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:
28:
29:
30:
31:
32:
33:
34:
35:
36:
37:
38:
39:
40:
41:
42:
43:
44:
45:
46:
47:
48:
49:
50:
51:
52:
53:
54:
55:
56:
57:
58:
59:
60:
61:
62:
63:
64:
65:
66:
67:
68:
        private const string DEFAULTTARGETPATH = @"D:\S7_Export\AppData";

        private void BW_Event_DoWork(object sender, DoWorkEventArgs e)
        {
            try
            {
                S7VariablenResult s7VariablenResult = LeakResult();

                string createTargetPath = "";

                if (s7VariablenResult.TrigSaveFiles && (s7VariablenResult.TypLinks || s7VariablenResult.TypRechts) && !s7VariablenResult.FMBauteil)
                {
                    createTargetPath = Path.Combine(@"D:\S7_Export\AppData", s7VariablenResult.FolderName);

                    try
                    {
                        //Ordner erstellen
                        if (!Directory.Exists(createTargetPath))
                        {
                            Directory.CreateDirectory(createTargetPath);
                        }
                    }

                    catch (Exception ex)
                    {
                        MessageBox.Show(this, ex.Message, "Ordner erstellen fehlgeschlagen!", MessageBoxButtons.OK, MessageBoxIcon.Error);
                    }

                    for (int i = 0; i < cbActivateLocalApps.Length; i++)
                    {
                        if (cbActivateLocalApps[i].Checked == true)
                        {
                            string praegeCodeNrFolderName = s7VariablenResult.FolderName;
                            string praegeCodeNrFileName = s7VariablenResult.PraegeCodeNr;
                            string praegeCode2NrFileName = s7VariablenResult.PraegeCode2Nr;
                            string sourceFilePath = tbSourceFilePathLocalApps[i].Text;
                            string targetPath = DEFAULTTARGETPATH;

                            MoveFilesfromLocal(praegeCodeNrFolderName, praegeCodeNrFileName, sourceFilePath, targetPath);
                            MoveFilesCode2fromLocal(praegeCodeNrFolderName, praegeCode2NrFileName, sourceFilePath, targetPath);
                        }
                    }

                    for (int i = 0; i < cbActivateNetApps.Length; i++)
                    {
                        if (cbActivateNetApps[i].Checked == true)
                        {
                            string praegeCodeNrFolderName = s7VariablenResult.FolderName;
                            string praegeCodeNrFileName = s7VariablenResult.PraegeCodeNr;
                            string praegeCode2NrFileName = s7VariablenResult.PraegeCode2Nr;
                            string sourceFilePath = tbSourceFilePathNetApps[i].Text;
                            string targetPath = DEFAULTTARGETPATH;

                            CopyFilesfromNetwork(praegeCodeNrFolderName, praegeCodeNrFileName, sourceFilePath, targetPath);
                        }
                    }

                }
            }
            catch (Exception ex)
            {
                MessageBox.Show("Error:\n\n" + ex.Message, "System", MessageBoxButtons.OK, MessageBoxIcon.Error);
            }

            Ready();

            BW_Event.ReportProgress(0);
        }

Ich weiß nicht, ob das so gut ist, wie ich das umgesetzt habe. Ich habe zwar nur noch 3 Methoden aber könnte es nicht sein das
durch diese ganzen For-Schleifen der Code langsamer wird???


Vielen Dank nochmal für alles.

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 19.05.22 12:35 
Statt nur new() mußt du bei älteren C#-Versionen noch den Typen hinschreiben:
ausblenden C#-Quelltext
1:
private UIValues UIValues = new UIValues();					

Aber ich schrieb
ausblenden C#-Quelltext
1:
// als Unterklasse in deiner Form-Klasse					
(also direkt in deiner MainForm-Klasse einfügen, damit UIValues auch ein Member der Klasse wird - nicht außerhalb nur im Namespace, das erzeugt einen Compilerfehler).

Und für dbnumber erzeugst du dann eine weitere Eigenschaft (oder simpel als Feld) in der UIValues und setzt diesen Wert entsprechend.

In SaveLocalPathConfigSettings() und SaveNetPathConfigSettings() hast du doch schon die Schleife (mit MAXLOCALAPPS bzw. MAXNETAPPS) jetzt eingebunden. Aber da du hier auch den Code einfach kopiert hast, könntest du eben auch dafür passende Methoden EnableLocalAppControls() und EnableNetAppControls() erstellen.

Und das mit dem enable ist doch einfach zu verstehen. Dort kommt beim Aufruf entweder true oder false rein, d.h. du benötigst nicht zwei Methoden zum Setzen, sondern eben nur eine.
Mit EnableTextBoxes(true) setzt du alle Textboxen textBoxX.Enabled = true und mit EnableTextBoxes(false) entsprechend textBoxX.Enabled = false (d.h. = disabled).
Du kannst also die Methode DisableTextBoxes() entfernen und statdessen EnableTextBoxes(false) benutzen.

Bei
ausblenden C#-Quelltext
1:
string sourceFilePath = tbSourceFilePathLocalApps[i].Text;					
kannst du keine Konstante benutzen, sondern müßtest auch diesen Wert in die UIValues-Klasse packen, d.h. hier als Array
ausblenden C#-Quelltext
1:
public string[] SourceFilePathLocalApps = new string[MaxLocalApps]; // die Konstante MaxLocalApps dafür als Klassenmember anlegen!					

Und jetzt dann entsprechend vorher (in einer Schleife) die Werte zuweisen (und dasselbe für alle anderen Controlwerte, welche du für den Kopiervorgang benötigst - so wie du es ja für die S7VariablenResult-Werte auch gemacht hast).

Und die Frage bzgl. "Code wird langsamer" finde ich immer wieder erstaunlich bei Anfängern. Die meiste Prozessorzeit wird hier doch sowieso durch die IO-Operationen benötigt.
Es geht um die Lesbarkeit von Code (und je kürzer und eleganter, desto besser - im Hinblick auch auf Wiederverwendbarkeit und Wartbarkeit von Code).

PS: Ansonsten sieht dein Code aber schon aufgeräumter und lesbarer aus (hast ja auch nur noch ca. 900 statt 2500 Zeilen)!
Als letzte größere Codeoptimierung könntest du noch den !string.IsNullOrEmpty(...) && ...-Abfrageblock als eigene Methode bool AreAllTextBoxesFilled() auslagern (da du ihn bisher 4x benutzt).
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Do 19.05.22 21:33 
Hallo Th69,

Vielen Dank für deine Hilfe.

Zitat:
(also direkt in deiner MainForm-Klasse einfügen, damit UIValues auch ein Member der Klasse wird)

Also ich habe jetzt für die Textbox txtDBNr.text eine Variable über die Klasse erstellt und hoffe ich habe das richtig gemacht.

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
namespace PDE
{
    class UIValues
    {
        private string dbnr;
        public string DBNr
        {
            get { return dbnr; }
            set { dbnr = value; }
        }        
    }
}


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:
namespace PDE
{

    public partial class MainForm : Form
    {
        private S7Client s7Client;

        private ComboBox[] cboNetApps;
        private TextBox[] tbSourceFilePathNetApps;
        private CheckBox[] cbActivateNetApps;
        private CheckBox[] cbNetSubFolderUses;
        private CheckBox[] cbActivateCode2NetApps;

        private const string DEFAULTTARGETPATH = @"D:\S7_Export\AppData";  
      
################################################################################
        private UIValues uivalues = new UIValues();        
################################################################################

        public MainForm()
        {
            InitializeComponent();


Ist das richtig hier im MainForm_Load die Variable zu initialsieren?
Ich habe mit einer MessageBox kontroliert ob ein Wert drin steht.
Es funktioniert aber das heißt noch lange nicht das es richtig ist, so wie es sein sollte.

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:
 private void MainForm_Load(object sender, EventArgs e)
        {
            GetLocalPathConfigSettings();
            GetNetPathConfigSettings();
            GetSPSConfigurationSettings();

            const int MAXLOCALAPPS = 8;
            for (int n = 0; n < MAXLOCALAPPS; n++)
            {
                cboLocalApps[n].Items.AddRange(txtLocalApplication.Lines);
            }

            const int MAXNETAPPS = 8;
            for (int n = 0; n < MAXNETAPPS; n++)
            {
                cboNetApps[n].Items.AddRange(txtNetApplication.Lines);
            }

            BW_Event.WorkerReportsProgress = true;
            BW_Event.WorkerSupportsCancellation = true;

            uivalues.DBNr = txtDBNr.Text;
            MessageBox.Show(uivalues.DBNr);
        }



Hier wird sie dann später genutzt.
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
18:
19:
 private void Ready()
        {
            int S7AreaDB = 0x84;

####################################################################################
            int dbnumber = int.Parse(uivalues.DBNr);
####################################################################################

            int TrigSaveFilesByte = int.Parse(txtTrigSaveFilesByte.Text);
            int TrigSaveFilesBit = int.Parse(txtTrigSaveFilesBit.Text);
            int dboffset = (TrigSaveFilesByte * 8) + TrigSaveFilesBit;
            int S7WLBit = 0x01;
            byte[] buffersize = new byte[1];

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

            EventLoop.Enabled = true;
        }


Zitat:
In SaveLocalPathConfigSettings() und SaveNetPathConfigSettings() hast du doch schon die Schleife (mit MAXLOCALAPPS bzw. MAXNETAPPS) jetzt eingebunden. Aber da du hier auch den Code einfach kopiert hast, könntest du eben auch dafür passende Methoden EnableLocalAppControls() und EnableNetAppControls() erstellen.

Und die Schleifen packe ich in die Methoden?

Zitat:
Und das mit dem enable ist doch einfach zu verstehen.

Ich habe es ja jetzt verstanden. Vielen Dank. Du beherrscht das alles aus dem FF. Ich brauche sehr lange. Du kannst mir glauben,
wenn es am einfachsten ist, mache ich es zu kompliziert.

Zitat:
Und jetzt dann entsprechend vorher (in einer Schleife) die Werte zuweisen

Hier möchte ich dich bitten es mir genauer zu beschreiben. Was gehört in die Klasse was in die Main und wo muss ich was für eine Schleife aufbauen?

Zitat:
Als letzte größere Codeoptimierung könntest du noch den !string.IsNullOrEmpty(...) && ...-Abfrageblock als eigene Methode bool AreAllTextBoxesFilled() auslagern (da du ihn bisher 4x benutzt).

Ja habe ich um setzen können.

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:
28:
29:
        private bool AreAllTextBoxesFilled()
        {
            if (!string.IsNullOrEmpty(txtIPNr.Text)
                && !string.IsNullOrEmpty(txtRackNr.Text)
                && !string.IsNullOrEmpty(txtSlotNr.Text)
                && !string.IsNullOrEmpty(txtDBNr.Text)
                && !string.IsNullOrEmpty(txtDBOffsetNr.Text)
                && !string.IsNullOrEmpty(txtDBSizeNr.Text)
                && !string.IsNullOrEmpty(txtPraegeCodeDBPos.Text)
                && !string.IsNullOrEmpty(txtFolderNameDBPos.Text)
                && !string.IsNullOrEmpty(txtPraegeCode2DBPos.Text)
                && !string.IsNullOrEmpty(txtTrigSaveFilesByte.Text)
                && !string.IsNullOrEmpty(txtTrigSaveFilesBit.Text)
                && !string.IsNullOrEmpty(txtTypLinksByte.Text)
                && !string.IsNullOrEmpty(txtTypLinksBit.Text)
                && !string.IsNullOrEmpty(txtTypRechtsByte.Text)
                && !string.IsNullOrEmpty(txtTypRechtsBit.Text)
                && !string.IsNullOrEmpty(txtRoboterDSLeerByte.Text)
                && !string.IsNullOrEmpty(txtRoboterDSLeerBit.Text)
                && !string.IsNullOrEmpty(txtFMBauteilByte.Text)
                && !string.IsNullOrEmpty(txtFMBauteilBit.Text)
                && !string.IsNullOrEmpty(txtHeartByte.Text)
                && !string.IsNullOrEmpty(txtHeartBit.Text))
            {
                return true;
            }

            return false;
        }


ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
private void Connect_Click(object sender, EventArgs e)
{
    ######################################################################
    if (AreAllTextBoxesFilled() == true)
    ######################################################################
{
    int Result;
    int Rack = Convert.ToInt32(txtRackNr.Text);
    int Slot = Convert.ToInt32(txtSlotNr.Text);
    Result = s7Client.ConnectTo(txtIPNr.Text, Rack, Slot); // 0 means Success
    ShowResult(Result); // The status bar result display
}


Eine Frage noch die for-Schleifen für das kopieren aufrufen und die Methoden für das Kopieren sind in Ordnung so?

So nochmals vielen Dank für deine Unterstützung

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: Do 19.05.22 23:35 
Die UIValues-Werte solltest du erst kurz vor Aktivierung des BackgroundWorkers ermitteln (am besten wohl in Connect_Click bevor EventLoop.Enabled = true gesetzt wird). In MainForm_Load ist ja gerade erst die Form erzeugt worden und der Anwender kann noch gar nicht die TextBoxen gefüllt haben.

Ansonsten ist dein Code aber vom Aufbau richtig.

Das Parsen der Eingabe solltest du aber auch schon vorher machen, d.h. speichere den Wert schon direkt als int ab (anstatt string).
Wenn du jedoch generell nur Zahlen in der Eingabe zulassen willst, dann solltest du gleich das NumericUpDown-Steuerelement benutzen (da kannst du dann auch Min- und Maxwerte vorgeben). Oder aber du benutzt meine dazu erstellte Komponente NumericTextBox (basierend auf dem Windows-Standard-Control).

Läuft denn dein Programm bisher ohne Exceptions? Es kann sein, daß ein lesender Zugriff auf die Controls aus dem BackgroundWorker funktioniert - auch wenn es nicht zu empfehlen ist und du den Weg über die UIValues konsequent umsetzen solltest (auch wenn es einige Daten sind).
Eigentlich würde ich dafür sogar empfehlen, den gesamten BackgroundWorker-Code in eine eigene Klasse auszulagern, weil dann würde man zuerst Fehlermeldungen bei der Verwendung der Form-Controls erhalten (welche man dann sukzessive ersetzen würde), aber das mute ich dir jetzt (noch) nicht zu...

UserTom hat folgendes geschrieben:
Und die Schleifen packe ich in die Methoden?

Ja, genau. Wie schon geschrieben kannst du dafür auch das VS-Refactoring benutzen. Selektiere den gesamten Schleifencode und öffne dann das Kontextmenü und wähle "Quick Actions and Refactorings" (Ctrl+.) und dann "Extract Method" und schon wird der Code in eine neue Methode erstellt, dessen Namen man gleich vergeben kann.

Und an der anderen (äquivalenten) Codestelle mußt du dann selbstverständlich diese Schleife löschen und die Methode stattdessen aufrufen.

Der bisherige Umbau der Kopieraktionen mit den for-Schleifen und das Aufteilen in die einzelnen Methoden sieht gut aus (auch wenn man ihn sicherlich noch optimieren könnte). :zustimm:
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Fr 20.05.22 08:40 
Servus Th69,

Vielen Dank für deine Antwort.

Zitat:
Die UIValues-Werte solltest du erst kurz vor Aktivierung des BackgroundWorkers ermitteln

Ok habe ich geändert.

Ich kann das leider nicht richtig abschließen, weil ich nicht weiß wie ich
ausblenden C#-Quelltext
1:
string sourceFilePath = tbSourceFilePathLocalApps[i].Text;					


In die UIValues.cs einbinden muss.

Zitat:

ausblenden C#-Quelltext
1:
public string[] SourceFilePathLocalApps = new string[MaxLocalApps]; // die Konstante MaxLocalApps dafür als Klassenmember anlegen!					


Ich vermute, das kommt in die Klasse?

Kannst du mir das bitte genauer erklären?

Zitat:
Das Parsen der Eingabe solltest du aber auch schon vorher machen, d.h. speichere den Wert schon direkt als int ab (anstatt string).

Wie meinst du das mit vorher?
Du meinst die s7VariablenResult oder?
Wenn ich die gleich als int abspeichere, dann muss ich aber die Textboxen gegen NumericUpDown austauschen.
Ahh toll die Werte vom NumericUpDown sind decimal die müssen auch konvertiert werden. Da haben wir ja nichts gewonnen!

Du meinst das bestimmt anders mit den direkt abspeichern? Ein Beispiel wäre nicht schlecht!

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:
            S7VariablenResult s7VariablenResult = new S7VariablenResult();
            // Reads the buffer.
            int dbnumber = int.Parse(txtDBNr.Text);
            int dboffset = int.Parse(txtDBOffsetNr.Text);
            int dbsize = int.Parse(txtDBSizeNr.Text);

            int PraegeCodeDBPos = int.Parse(txtPraegeCodeDBPos.Text);
            int FolderNameDBPos = int.Parse(txtFolderNameDBPos.Text);
            int PraegeCode2DBPos = int.Parse(txtPraegeCode2DBPos.Text);

            int TrigSaveFilesByte = int.Parse(txtTrigSaveFilesByte.Text);
            int TrigSaveFilesBit = int.Parse(txtTrigSaveFilesBit.Text);

            int TypLinksByte = int.Parse(txtTypLinksByte.Text);
            int TypLinksBit = int.Parse(txtTypLinksBit.Text);

            int TypRechtsByte = int.Parse(txtTypRechtsByte.Text);
            int TypRechtsBit = int.Parse(txtTypRechtsBit.Text);

            int RoboterDSLeerByte = int.Parse(txtRoboterDSLeerByte.Text);
            int RoboterDSLeerBit = int.Parse(txtRoboterDSLeerBit.Text);

            int FMBauteilByte = int.Parse(txtFMBauteilByte.Text);
            int FMBauteilBit = int.Parse(txtFMBauteilBit.Text);

            int HeartByte = int.Parse(txtHeartByte.Text);
            int HeartBit = int.Parse(txtHeartBit.Text);


Zitat:
Läuft denn dein Programm bisher ohne Exceptions?


Es startet und ich kann es auch mit der SPS verbinden (Connect_Click).
Da ich noch das UIValues Problem habe, konnte ich noch nicht mehr testen.

Zitat:
Möglicherweise steckt da zu viel SPS Denkweise drin.

Ralf hatte recht. Ich weiß nicht, ob ihr schon mal einen AWL Code gesehen habt, der geht von oben nach unten.
Der jetzt optimierte Code ist natürlich besser aber ich muss mich jetzt erstmal dran gewöhnen das sehr viele schleifen enthalten sind
und mir das Nachverfolgen erschwert.

Vielen Dank nochmal für deine Tipps.

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: Fr 20.05.22 10:52 
UserTom hat folgendes geschrieben:
Ich vermute, das kommt in die Klasse?

Ja, in die UIValues-Klasse.

Und anstatt eines einzelnen Wertes setzt du dann in einer Schleife alle Array-Werte:
ausblenden C#-Quelltext
1:
2:
for (int n = 0; n < UIValues.SourceFilePathLocalApps.Length; n++)
  UIValues.SourceFilePathLocalApps[n] = tbSourceFilePathLocalApps[n].Text;


Und bzgl. int:
Bisher speicherst du ja DBNr als String ab und benutzt int.Parse(uivalues.DBNr) erst in der Ready()-Methode.
Wenn du aber direkt schon den geparsten Wert als int in die UIValues abspeicherst, brauchst du nachher keine Umwandlung mehr (gilt auch für die anderen Werte).
Und wie ich schon schrieb, solltest du besser int.TryParse(...) benutzen, damit du Eingabefehler besser abfangen kannst (ansonsten kommt nämlich eine InvalidCastException und das gesamte Programm wird beendet, wenn du keinen try..catch-Block benutzt).

NumericUpDown verwendet decimal für Value, weil man es auch mit Nachkommastellen (DecimalPlaces) konfigurieren kann.
Man kann aber einfach int value = (int)numericUpDown.Value schreiben (also eine explizite Umwandlung [Cast]).

Dies alles dient auch dazu UI und Logik (Datenmodellierung) voneinander zu trennen (eines der wichtigsten Konzepte in der Software-Entwicklung).

PS: Was meinst du eigentlich mit Leak in LeakResult?

PPS: Ich habe deine Zitate jeweils editiert, damit kein unnötiger Zeilenumbruch dort angezeigt wird (der Text sollte direkt nach dem `Quote`-Tag stehen, nicht in einer neuen Zeile).
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 20.05.22 12:47 
Zitat:
PS: Was meinst du eigentlich mit Leak in LeakResult?


Da ich so gerne spekuliere. (Un)Dichteprüfung in einem Rohrwerk?
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Fr 20.05.22 13:58 
Hallo an alle,

Vielen Dank für Eure Antworten.

Was Leak bedeutet, weiß ich auch nicht. Ich habe den Beispielcode von einer Internetseite. Siehe im Anhang das Bild.

Zitat:
Und anstatt eines einzelnen Wertes setzt du dann in einer Schleife alle Array-Werte:

Ich habe das jetzt folgendermaßen gelöst. Ich hoffe, es ist richtig.

In der Klasse.
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
namespace PDE
{
    class UIValues
    {
        private string[] _sourcefilepathlocalapps;
        public string[] SourceFilePathLocalApps
        {
            get { return _sourcefilepathlocalapps; }
            set { _sourcefilepathlocalapps = value; }
        }

        private string[] _sourcefilepathnetapps;
        public string[] SourceFilePathNetApps
        {
            get { return _sourcefilepathnetapps; }
            set { _sourcefilepathnetapps = value; }
        }


Im Connect_Click:
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
      uivalues.FMBauteilByte = txtFMBauteilByte.Text;
      uivalues.FMBauteilBit = txtFMBauteilBit.Text;
      uivalues.HeartByte = txtHeartByte.Text;
      uivalues.HeartBit = txtHeartBit.Text;

      for (int i = 0; i < uivalues.SourceFilePathLocalApps.Length; i++)
        uivalues.SourceFilePathLocalApps[i] = tbSourceFilePathLocalApps[i].Text;

      for (int i = 0; i < uivalues.SourceFilePathNetApps.Length; i++)
        uivalues.SourceFilePathNetApps[i] = tbSourceFilePathNetApps[i].Text;

      ScanDBOffset.Enabled = true;
      EventLoop.Enabled = true;


In der Schleife im DoWork.
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
for (int i = 0; i < cbActivateLocalApps.Length; i++)
{
    if (cbActivateLocalApps[i].Checked == true)
    {
        string praegeCodeNrFolderName = s7VariablenResult.FolderName;
        string praegeCodeNrFileName = s7VariablenResult.PraegeCodeNr;
        string praegeCode2NrFileName = s7VariablenResult.PraegeCode2Nr;
        string sourceFilePath = uivalues.SourceFilePathLocalApps[i];
        string targetPath = DEFAULTTARGETPATH;

        MoveFilesfromLocal(praegeCodeNrFolderName, praegeCodeNrFileName, sourceFilePath, targetPath);
        MoveFilesCode2fromLocal(praegeCodeNrFolderName, praegeCode2NrFileName, sourceFilePath, targetPath);
    }
}


Zitat:
Und bzgl. int:

Ich habe Folgendes probiert, aber ich bekomme es nicht hin.

In der Klasse UIValues.TryParse verstehe ich noch weniger, da muss ich noch einen Wert mehr übergeben??
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
private string _dbnr;
public int DBNr
{
    get { return int.Parse(_dbnr); }

    set { _dbnr = value.ToString(); }
}


ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
uivalues.IPNr = txtIPNr.Text;
uivalues.RackNr = txtRackNr.Text;
uivalues.SlotNr = txtSlotNr.Text;

#########################################################################
uivalues.DBNr = txtDBNr.Text;  << Ist rot unterstrichen
########################################################################

uivalues.Offset = txtDBOffsetNr.Text;
uivalues.DBSize = txtDBSizeNr.Text;


In der Methode Ready():
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
private void Ready()
{
    int S7AreaDB = 0x84;

    #####################################################
    int dbnumber = uivalues.DBNr;
    #####################################################

    int TrigSaveFilesByte = int.Parse(uivalues.TrigSaveFilesByte);


Vielen Dank für die Antworten.

Grüße Tom

Moderiert von user profile iconTh69: Leerzeile aus Zitaten entfernt
Moderiert von user profile iconTh69: C#-Tags hinzugefügt
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: Fr 20.05.22 14:30 
Bei deinen String-Arrays fehlt noch die Erzeugung des Arrays (mittels new[...]).
Ich habe doch extra folgendes geschrieben:
ausblenden C#-Quelltext
1:
public string[] SourceFilePathLocalApps = new string[MaxLocalApps]; // die Konstante MaxLocalApps dafür als Klassenmember anlegen!					


Für einfache Datenhaltungsklassen, wie eben UIValues, solltest du die automatischen Eigenschaften ({ get; set; }) benutzen (solange du keine eigene Logik im Getter oder Setter hast).

Auch bei DBNr. Dein umgeänderter Code kann so nicht funktionieren, da du jetzt einen String einem int-Wert zuweisen möchtest.

Ich meinte einfach
ausblenden C#-Quelltext
1:
2:
3:
4:
class UIValues
{
  public int DBNr { get; set; }
}

Und dann in der Zuweisungsmethode
ausblenden C#-Quelltext
1:
uivalues.DBNr = int.Parse(txtDBNr.Text);					

bzw. mit TryParse
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
if (int.TryParse(txtDBNr.Text, out int dbnr))
{
  uivalues.DBNr = dbnr;
}
else
{
  // Fehlermeldung ausgeben
}


Noch einfacher ginge es mit einer reinen Membervariablen
ausblenden C#-Quelltext
1:
2:
3:
4:
class UIValues
{
  public int DBNr;
}

ausblenden C#-Quelltext
1:
2:
3:
4:
if (!int.TryParse(txtDBNr.Text, out uivalues.DBNr))
{
   // Fehlermeldung ausgeben
}
(out-Parameter müssen Variablen sein, keine Eigenschaften)