1
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
0

1 Answer 1

0
\$\begingroup\$

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 simple arrBool1.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).

\$\endgroup\$
2
  • 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
    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 call Display method with passing the data structure as argument. \$\endgroup\$
    – aepot
    Mar 3, 2021 at 10:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.