I have a TCP Listener with NetworkStream
that reads 530 bytes from the Client once a second.
Within the TCP Listener BackgroundWorker
, I write those bytes into a MemoryStream
, and invoke a method that breaks down the MemoryStream
into smaller arrays.
Then, I have another BackgroundWorker
that constantly compares those smaller arrays to a set of predefined byte arrays- based on the outcome it performs different tasks
While this works fine for what I'm doing, I wonder if there's a better, more efficient way of doing it.
Here's my code:
namespace TcpServerTest
{
public partial class Form1 : Form
{
public TcpListener server = null;
public TcpClient client;
public Int32 port = 6100;
public IPAddress localAddr = IPAddress.Parse("127.0.0.1");
public MemoryStream memStream = new MemoryStream(2000);
Byte[] bytesFromTCP = new Byte[530];
byte[] SendtoPlcZeroCalPASS = { 0x0A, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x32, 0x2B };
byte[] SendtoPlcZeroCalFAIL = { 0x0A, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x30, 0x58 };
byte[] SendtoPlcSmokeCalPASS = { 0x0A, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x03, 0x4C };
byte[] SendtoPlcSmokeCalFAIL = { 0x0A, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x46, 0xC9 };
public byte[] arrInt1 = new byte[2];
public byte[] _arrInt1 = { 0x10, 0x02 };
public byte[] arrBool1 = new byte[2];
public byte[] _arrBool1 = { 0x01, 0x00 };
public byte[] arrBool2 = new byte[2];
public byte[] _arrBool2 = { 0x01, 0x00 };
public byte[] arrBool3 = new byte[2];
public byte[] _arrBool3 = { 0x01, 0x00 };
public byte[] arrBool4 = new byte[2];
public byte[] _arrBool4 = { 0x01, 0x00 };
public byte[] arrString1 = new byte[256];
public byte[] _arrString1 = new byte[256];
public byte[] arrString2 = new byte[256];
public byte[] arrBool5 = new byte[2];
public byte[] _arrBool5 = { 0x01, 0x00 };
public byte[] arrBool6 = new byte[2];
public byte[] _arrBool6 = { 0x01, 0x00 };
public byte[] arrBool7 = new byte[2];
public byte[] _arrBool7 = { 0x01, 0x00 };
public byte[] arrCrc = new byte[2];
public Form1()
{
InitializeComponent();
bgWorkerSocketListener.DoWork += bgWorkerSocketListener_DoWork;
bgWorkerSocketListener.WorkerSupportsCancellation = true; //Allow for the background worker process to be cancelled
bgWorkerSocketListener.WorkerReportsProgress = true;
bgWorkerSocketListener.RunWorkerAsync();
bgWorkerTcpProcessor.DoWork += bgWorkerTcpProcessor_DoWork;
bgWorkerTcpProcessor.WorkerSupportsCancellation = true; //Allow for the background worker process to be cancelled
bgWorkerTcpProcessor.WorkerReportsProgress = true;
bgWorkerTcpProcessor.RunWorkerAsync();
}
public void bgWorkerSocketListener_DoWork(object sender, DoWorkEventArgs e) //Start the TCP Listener
{
try
{
AppendTBoxTcpBytes("Waiting for a connection... " + Environment.NewLine);
//Create a listener
server = new TcpListener(localAddr, port);
//Start listening for client requests.
server.Start();
String data = null;
//Enter the listening loop.
while (true)
{
//Perform a blocking call to accept requests.
client = server.AcceptTcpClient();
AppendTBoxTcpBytes("Connected!" + Environment.NewLine);
data = null;
//Get a stream object for reading and writing
NetworkStream stream = client.GetStream();
int i;
while (client.Connected == true)
{
try
{
//Loop to receive all the data sent by the client.
while ((i = stream.Read(bytesFromTCP, 0, bytesFromTCP.Length)) != 0)
{
//Translate data
data = BitConverter.ToString(bytesFromTCP);
AppendTBoxTcpBytes(" Data: " + Environment.NewLine + data + Environment.NewLine + "Length: " + data.Length
+ Environment.NewLine + DateTime.Now.ToString("dd MMMM yyyy HH:mm:ss"));
memStream.Write(bytesFromTCP, 0, bytesFromTCP.Length);
this.Invoke(new EventHandler(processMessageMemStr)); //Process incoming message
memStream.Position = 0;
}
}
catch (Exception ex){}
}
}
}
catch (SocketException ex)
{
MessageBox.Show("SocketException: " + ex);
}
}
private void processMessageMemStr(object sender, EventArgs e) //Process received messages from Memory Stream
{
try
{
memStream.Position = 0;
memStream.Read(arrInt1, 0, 2);
memStream.Read(arrBool1, 0, 2);
memStream.Read(arrBool2, 0, 2);
memStream.Read(arrBool3, 0, 2);
memStream.Read(arrBool4, 0, 2);
memStream.Read(arrString1, 0, 256);
memStream.Read(arrString2, 0, 256);
memStream.Read(arrBool5, 0, 2);
memStream.Read(arrBool6, 0, 2);
memStream.Read(arrBool7, 0, 2);
memStream.Read(arrCrc, 0, 2);
}
catch (Exception err)
{ MessageBox.Show(err.ToString()); }
}
public void bgWorkerTcpProcessor_DoWork(object sender, DoWorkEventArgs e) //Start the TCP Listener
{
ASCIIEncoding ascii = new ASCIIEncoding();
while (true)
{
try
{
if (string.Join(",", arrBool1) == string.Join(",", _arrBool1))
pictureBox2.Image = Resources.green_on;
else
pictureBox2.Image = Resources.led_off;
if (string.Join(",", arrBool2) == string.Join(",", _arrBool2))
pictureBox1.Image = Resources.green_on;
else
pictureBox1.Image = Resources.led_off;
if (string.Join(",", arrBool3) == string.Join(",", _arrBool3))
pictureBox3.Image = Resources.red_on;
else
pictureBox3.Image = Resources.led_off;
if (string.Join(",", arrBool4) == string.Join(",", _arrBool4))
pictureBox4.Image = Resources.red_on;
else
pictureBox4.Image = Resources.led_off;
if (string.Join(",", arrBool5) == string.Join(",", _arrBool5))
pictureBox5.Image = Resources.green_on;
else
pictureBox5.Image = Resources.red_on;
if (string.Join(",", arrBool6) == string.Join(",", _arrBool6))
pictureBox6.Image = Resources.green_on;
else
pictureBox6.Image = Resources.led_off;
if (string.Join(",", arrBool7) == string.Join(",", _arrBool7))
{
pictureBox7.Image = Resources.green_on;
pictureBox8.Image = Resources.led_off;
}
else
{
pictureBox7.Image = Resources.led_off;
pictureBox8.Image = Resources.green_on;
}
AppendTBoxAlarm(ascii.GetString(arrString1, 0, 256));
AppendTBoxStep(ascii.GetString(arrString2, 0, 256));
}
catch { }
}
}
private void btnSendToClient_Click_1(object sender, EventArgs e)
{
try
{
server = new TcpListener(localAddr, port);
NetworkStream stream = client.GetStream();
if (cBoxSendToPLC.SelectedItem == "Test1 PASS")
stream.Write(SendtoPlcZeroCalPASS, 0, SendtoPlcZeroCalPASS.Length);
if (cBoxSendToPLC.SelectedItem == "Test2 PASS")
stream.Write(SendtoPlcSmokeCalPASS, 0, SendtoPlcZeroCalPASS.Length);
if (cBoxSendToPLC.SelectedItem == "Test1 FAIL")
stream.Write(SendtoPlcZeroCalFAIL, 0, SendtoPlcZeroCalPASS.Length);
if (cBoxSendToPLC.SelectedItem == "Test2 FAIL")
stream.Write(SendtoPlcSmokeCalFAIL, 0, SendtoPlcZeroCalPASS.Length);
else{}
}
catch (Exception err)
{
MessageBox.Show("No client is yet connected to the server.\n" + "Exception: " + err.Message, "Client not connected", MessageBoxButtons.OK, MessageBoxIcon.Error);
}
}
public void AppendTBoxTcpBytes(string value) //Appends text, prevents UI from freezing
{
if (InvokeRequired)
{
this.Invoke(new Action<string>(AppendTBoxTcpBytes), new object[] { value });
return;
}
tBoxTcpBytes.Clear();
tBoxTcpBytes.Text += value;
}
public void AppendTBoxAlarm(string value) //Appends text, prevents UI from freezing
{
if (InvokeRequired)
{
this.Invoke(new Action<string>(AppendTBoxAlarm), new object[] { value });
return;
}
tBoxAlarm.Clear();
tBoxAlarm.Text += value;
}
public void AppendTBoxStep(string value) //Appends text, prevents UI from freezing
{
if (InvokeRequired)
{
this.Invoke(new Action<string>(AppendTBoxStep), new object[] { value });
return;
}
tBoxStep.Clear();
tBoxStep.Text += value;
}
}
}
1 Answer 1
There's a lot of issues in the code. If i'll try to review all of them, the post would be too long.
Common tips:
- Give variable understandeable names. What is
arrBool1
means? Write code to avoid such questions. - When you're in multithreading or asynchronous environment, avoid using global variables. This is strongly recommended. Unless ones is thread-safe or you're using it in a thread-safe manner. Pass data to methods as arguments, use Producer/Consumer collections from
System.Collections.Concurrent
, etc. - A monster
string.Join(",", arrBool1) == string.Join(",", _arrBool1)
can be replaced with simplearrBool1.SequenceEqual(_arrBool1)
.
I'll focus on data receiving avoiding the war with arrays.
Here's a data structure
public struct Packet
{
public bool bool1;
public bool bool2;
public bool bool3;
public bool bool4;
public string text1;
public string text2;
public bool bool5;
public bool bool6;
public bool bool7;
public short crc;
public static Packet FromStream(Stream stream)
{
using BinaryReader br = new BinaryReader(stream, Encoding.ASCII, leaveOpen: true);
Packet p = new Packet();
p.header = br.ReadInt16();
p.bool1 = br.ReadBoolean();
br.ReadByte();
p.bool2 = br.ReadBoolean();
br.ReadByte();
p.bool3 = br.ReadBoolean();
br.ReadByte();
p.bool4 = br.ReadBoolean();
br.ReadByte();
p.text1 = FromStringZ(br.ReadChars(256));
p.text2 = FromStringZ(br.ReadChars(256));
p.bool5 = br.ReadBoolean();
br.ReadByte();
p.bool6 = br.ReadBoolean();
br.ReadByte();
p.bool7 = br.ReadBoolean();
br.ReadByte();
p.crc = br.ReadInt16();
return p;
}
private static string FromStringZ(ReadOnlySpan<char> buffer)
{
int i = 0;
while (i < buffer.Length && buffer[i] != '0円')
i++;
return new string(buffer[0..i]);
}
}
You can use BinaryWriter
in the same way to write data to some Stream
.
You can receive data in the following way then.
byte[] headerBuffer = new byte[2];
int bytesReceived;
while (client.Connected == true)
{
try
{
while ((bytesReceived = stream.Read(headerBuffer, 0, headerBuffer.Length)) > 0) // reads 2 bytes if available
{
if (headerBuffer.SequenceEqual(Packet.Id))
{
Packet packet = Packet.FromStream(stream); // reads 528 bytes
this.Invoke((Action)(() => Display(packet)));
}
else
{
// received another packet type but as this loop doesn't contain
// any other type packets handler, it goes to abnormal state
stream.Close();
}
}
}
catch (Exception ex) { }
}
The Display
method is simple then
private void Display(Packet packet)
{
pictureBox2.Image = packet.bool1 ? Resources.green_on : Resources.led_off;
pictureBox1.Image = packet.bool2 ? Resources.green_on : Resources.led_off;
pictureBox3.Image = packet.bool3 ? Resources.red_on : Resources.led_off;
pictureBox4.Image = packet.bool4 ? Resources.red_on : Resources.led_off;
pictureBox5.Image = packet.bool5 ? Resources.green_on : Resources.red_on;
pictureBox6.Image = packet.bool6 ? Resources.green_on : Resources.led_off;
pictureBox7.Image = packet.bool7 ? Resources.green_on : Resources.led_off;
pictureBox8.Image = packet.bool7 ? Resources.led_off : Resources.green_on;
AppendTBoxAlarm(packet.text1);
AppendTBoxStep(packet.text2);
}
You don't need a BackgroudWorker
for displaying the data anymore.
I tell more, BackgroudWorker
is obsolete and let it rest in peace. To run the Server you may spawn a simple Thread
.
public Form1()
{
InitializeComponent();
// but it's better to move this code to 'Form.Load' event handler
Thread thread = new Thread(RunServer) { IsBackground = true };
thread.Start();
}
private RunServer()
{
// former bgWorkerSocketListener_DoWork method's body
}
Another short thing: +=
is too slow operation for TextBox
because it rewrites the entire string concatenating old and appended values which cause redraw all the content.
tBoxStep.Clear();
tBoxStep.Text += value;
Can be replaced with
tBoxStep.Text = value;
Or if you want to append the text, you have 2 options
tBoxStep.AppendText(value);
tBoxStep.AppendLine(value); // appends Environment.NewLine automatically
These two are enough fast regardless of text amount that TextBox
contains already.
And finally, your Server can serve only one client at once. Consider to change the logic if you want to serve multiple clients at once. That's not so hard e.g. spawn new Thread
for each TcpClient
.
Further steps: learn about async/await
: Asynchronous Programming. Network or database interacting code is 100x times easier with it (with no Threads spawning).
-
1\$\begingroup\$ Thank you so much for taking your time to write this up, I'll take all of your suggestions on board. This is just a part of a bigger application (8k lines of code), so I can only imagine how many more monsters are hiding in there :D Thanks again, it's much appreciated. On passing data to methods as arguments, can you think of any page/blog that would help me understand it better? \$\endgroup\$W.Donald– W.Donald2021年03月03日 10:36:54 +00:00Commented Mar 3, 2021 at 10:36
-
\$\begingroup\$ @W.Donald if the answer was helpful, you may mark it as accepted.
On passing data to methods as arguments
- that's easy, look how I callDisplay
method with passing the data structure as argument. \$\endgroup\$aepot– aepot2021年03月03日 10:46:42 +00:00Commented Mar 3, 2021 at 10:46