C# multiple serial ports simultanious reading

Question

back again with another serial port question.

Over the past two weeks I have been refining my code to try and read off of two serial devices. Currently, the program gets almost all of the data (about 99% compared to the 50% I had before) but I am still missing data.

Here is my "Main" function:

private Program()
    {
        port.DataReceived += new SerialDataReceivedEventHandler(port_DataReceived);
        port2.DataReceived += new SerialDataReceivedEventHandler(port2_DataReceived);


        port.Open();
        port2.Open();


        Application.Run();
    }

Here is the code I am using for both serial ports:

public void port1IntoBuffer()
    {
        int messageLength = 96; 
        byte[] buffer = new byte[messageLength];
        port.BaseStream.ReadAsync(buffer, 0, messageLength);

        for (int i = 0; i < messageLength; i++)
        {
            if ((int)buffer[i] <= 48 && (int)buffer[i] > 0)
            {
                tickQ.Enqueue((new IdDate { Id = (int)buffer[i], Date = DateTime.Now.ToString("yyyy-MM-dd HH:mm:ss.fff") }));
            }
        }
        if (!Locked)
        {
            Locked = true;
            Thread readThread = new Thread(() => submitSQL());
            readThread.Start();
        }
    }

I have this code duplicated for port two as well. The messageLength is just an arbitrary number for testing. As for my input, I am expecting an integer value between 1-48. The tickQ variable is a ConcurrentQueue that I have both ports enqueued onto (it is later dequeued and sent to a SQL DB).

If anybody could give me some hints as to what I'm doing wrong I would appreciate it.

Thank you!

EDIT 1:

After reading BlueStrat's suggestion, I have changed my sql submit code to the following:

public void submitSQL()
    {
        lock (Locked)
        {
            int retryCount = 3;
            bool success = false;
            while (retryCount > 0 && !success)
            {
                try
                {



                    IdDate tempData;
                    using (SqlConnection con = new SqlConnection())
                    using (SqlCommand cmd = new SqlCommand())
                    {

                        con.ConnectionString = "Data Source=xxxxxx;" +
                                                "Initial Catalog=xxxxxx;" +
                                                "User id=xxxxxx; Password=xxxxxx; Connection Timeout=0";

                        cmd.CommandText = "INSERT INTO XXXXX (submitTime, machineID) VALUES (@submitTIME, @machineId)";

                        cmd.Connection = con;


                        con.Open();
                        while (tickQ.TryDequeue(out tempData))
                        {


                            cmd.Parameters.Clear();
                            cmd.Parameters.AddWithValue("@machineId", (tempData.Id));
                            cmd.Parameters.AddWithValue("@submitTIME", tempData.Date);

                            cmd.ExecuteNonQuery();
                        }
                        con.Close();



                    }
                }
                catch (SqlException e)
                {
                    Console.WriteLine(e.Message);
                    retryCount--;
                }
                finally
                {
                    success = true;
                }
            }
        }
    }

Now my problem is after 10 minutes, my RAM usage for this program explodes (grows from about 6MB to 150MB) and then the program crashes.

EDIT 2: With everybody's recommendations, the results have improved! I only missed about 8 out of ~15,000 transmission last night. I'm going to try increasing my serial read buffer to hopefully catch more transmissions.

Answer 1

There are a couple of issues I notice in your code, the first one I addressed as a comment. The 'ReadAsync()' function call is asynchronous, meaning it will return early, while the requested data is fetched in the background.

Normally you would handle this by issuing an await on the returned Task, signalling that you want the code to resume (without blocking the current thread) once the requested amount of bytes have been fully read.

await is only available since C# 5. If you are running on a lower version, then you typically schedule a continuation, which is a kind of delegate that will run AFTER the Task is complete. However, I think that simply switching to the Read() version would be fine, the thread will certainly block while waiting for the data, but at least you are certain that you have read the expected amount of bytes.

The other issue I notice is your locking code that is relying on a boolean flag, it is not thread-safe. The issue is that the code that checks the status of the Lock flag to conditionally enter the guarded block and the code that actually sets the status of the flag inside the block is not atomic. This means that a different thread could enter the code you are trying to protect while another thread has also entered the same code block, but still hasn't managed to switch the guard flag to true, in this case, both threads will run the same code and probably produce an unexpected result (not sure if this possibility has something to do with your missing data problem).

I can't really recommend how to rewrite your lock-guard code without viewing how else the Lock member is used and how it is released. If you post the rest of the code snippets interacting with the Lock member I may be able to offer some advice.

EDIT: (more information after OP's comment)

The purpose of that flag was to prevent every thread from posting to the database at once (I kept getting deadlock errors, but I don't think that would cause my data loss).

With that bit of information I think that you might have another problem that could certainly explain missing data. What happens if Flag is true? you simply skip launching the thread that submits data to your SQL server, you never retry, so the buffered data is lost once the method exits. Sorry, you won't lose data because you are enqueing it, what you might lose is the necessary amount of thread launches to actually drain the data reception queue. When you experience the data loss, check your tickQ queue, I bet that it will still have data to process, because the thread that should have dealt with it was never started for the same reasons.

Adding a lock() statement inside the submitSQL() function will protect you from that possibility.

Remove the Locked member completely from you code and try this:

// Add this member to your class;
object SqlLock = new object();



    void submitSQL() {
      // add this at the start of your submitSQL() method
       lock (SqlLock ) {
         ... the rest of your code
       }
    }

The difference with your current code is that if the SQL instance is in use by another thread, the calling thread will block AND WAIT for it to be available before continuing, instead of simply discarding the data that has just been read. not launching the thread that handles the en-queued data.

EDIT: Final piece of advice

I would encourage you to refrain from creating a new thread every time you need to drain your queue, instead you can rely on the thread pool (See Task.Run()). Using a thread from the pool removes the expensive overhead of manually creating threads.

Another possibility is to create a single thread that will be continuously dedicated to draining the data queue. You can use an AutoResetEvent in order to coordinate work between the producer thread (the thread that enqueues) and the consumer thread (the thread that dequeues) Your consumer thread enters a loop that calls AutoResetEvent.WaitOne(), where it will block; your producer thread enqueues received data and instead of spawning a new thread, it calls AutoResetEvent.Set() causing the consumer thread to wake-up and handle the enqueued data, once it is handled, it blocks again, waiting for the next data batch to arrive. If you do so, make sure you mark your thread as a BackgroundThread, so it doesn't prevent the application from shutting down if the thread is waiting for data.