Background A customer asked me to find out why their C# application (we'll call it XXX, delivered by a consultant who has fled the scene) is so flaky, and fix it. The application controls a measurement device over a serial connection. Sometimes the device delivers continuous readings (which are displayed on screen), and sometimes the app needs to stop continuous measurements and go into command-response mode. How NOT to do it For continuous measurements, XXX uses System.Timers.Timer for background processing of serial input. When the timer fires, C# runs the timer's ElapsedEventHandler using some thread from its pool. XXX's event handler uses a blocking commPort.ReadLine() with a several second timeout, then calls back to a delegate when a useful measurement arrives on the serial port. This portion works fine, however... When its time to stop realtime measurements and command the device to do something different, the application tries to suspend background processing from the GUI thread by setting the timer's Enabled = false . Of course, that just sets a flag preventing further events, and a background thread already waiting for serial input continues waiting. The GUI thread then sends a command to the device, and tries to read the reply – but the reply is received by the background thread. Now the background thread becomes confused as its not the expected measurement. The GUI thread meanwhile becomes confused as it didn't receive the command reply expected. Now we know why XXX is so flaky. Possible Method 1 In another similar application, I used a System.ComponentModel.BackgroundWorker thread for free-running measurements. To suspend background processing I did two things in the GUI thread: call the CancelAsync method on the thread, and call commPort.DiscardInBuffer() , which causes a pending (blocked, waiting) comport read in the background thread to throw a System.IO.IOException "The I/O operation has been aborted because of either a thread exit or an application request.\r\n" . In the background thread I catch this exception and clean up promptly, and all works as intended. Unfortunately DiscardInBuffer provoking the exception in another thread's blocking read is not documented behavior anywhere I can find, and I hate relying on undocumented behavior. It works because internally DiscardInBuffer calls the Win32 API PurgeComm, which interrupts the blocking read (documented behavior). Possible Method 2 Directly use the BaseClass Stream.ReadAsync method, with a monitor cancellation token, using a supported way of interrupting the background IO. Because the number of characters to be received is variable (terminated by a newline), and no ReadAsyncLine method exists in the framework, I don't know if this is possible. I could process each character individually but would take a performance hit (might not work on slow machines, unless of course the line-termination bit is already implemented in C# within the framework). Possible Method 3 Create a lock "I've got the serial port". Nobody reads, writes, or discards input from the port unless they have the lock (including repeating the blocking read in background thread). Chop the timeout values in the background thread to 1/4 second for acceptable GUI responsiveness without too much overhead. Question Does anybody have a proven solution to deal with this problem? How can one cleanly stop background processing of the serial port? I've googled and read dozens of articles bemoaning the C# SerialPort class, but haven't found a good solution. Thanks in advance!

Reputation: 413

C# - How to hand off which thread reads from serial port?

Background

A customer asked me to find out why their C# application (we'll call it XXX, delivered by a consultant who has fled the scene) is so flaky, and fix it. The application controls a measurement device over a serial connection. Sometimes the device delivers continuous readings (which are displayed on screen), and sometimes the app needs to stop continuous measurements and go into command-response mode.

How NOT to do it

For continuous measurements, XXX uses System.Timers.Timer for background processing of serial input. When the timer fires, C# runs the timer's ElapsedEventHandler using some thread from its pool. XXX's event handler uses a blocking commPort.ReadLine() with a several second timeout, then calls back to a delegate when a useful measurement arrives on the serial port. This portion works fine, however...

When its time to stop realtime measurements and command the device to do something different, the application tries to suspend background processing from the GUI thread by setting the timer's Enabled = false. Of course, that just sets a flag preventing further events, and a background thread already waiting for serial input continues waiting. The GUI thread then sends a command to the device, and tries to read the reply – but the reply is received by the background thread. Now the background thread becomes confused as its not the expected measurement. The GUI thread meanwhile becomes confused as it didn't receive the command reply expected. Now we know why XXX is so flaky.

Possible Method 1

In another similar application, I used a System.ComponentModel.BackgroundWorker thread for free-running measurements. To suspend background processing I did two things in the GUI thread:

call the CancelAsync method on the thread, and
call commPort.DiscardInBuffer(), which causes a pending (blocked, waiting) comport read in the background thread to throw a System.IO.IOException "The I/O operation has been aborted because of either a thread exit or an application request.\r\n".

In the background thread I catch this exception and clean up promptly, and all works as intended. Unfortunately DiscardInBuffer provoking the exception in another thread's blocking read is not documented behavior anywhere I can find, and I hate relying on undocumented behavior. It works because internally DiscardInBuffer calls the Win32 API PurgeComm, which interrupts the blocking read (documented behavior).

Possible Method 2

Directly use the BaseClass Stream.ReadAsync method, with a monitor cancellation token, using a supported way of interrupting the background IO.

Because the number of characters to be received is variable (terminated by a newline), and no ReadAsyncLine method exists in the framework, I don't know if this is possible. I could process each character individually but would take a performance hit (might not work on slow machines, unless of course the line-termination bit is already implemented in C# within the framework).

Possible Method 3

Create a lock "I've got the serial port". Nobody reads, writes, or discards input from the port unless they have the lock (including repeating the blocking read in background thread). Chop the timeout values in the background thread to 1/4 second for acceptable GUI responsiveness without too much overhead.

Question

Does anybody have a proven solution to deal with this problem? How can one cleanly stop background processing of the serial port? I've googled and read dozens of articles bemoaning the C# SerialPort class, but haven't found a good solution.

Thanks in advance!

Upvotes: 5

Answers (2)

Dave Nadler

Reputation: 413

OK, here's what I did... Comments would be appreciated as C# is still somewhat new to me!

Its crazy to have multiple threads trying to access the serial port concurrently (or any resource, especially an asynchronous resource). To fix up this application without a complete rewrite, I introduced a lock SerialPortLockObject to guarantee exclusive serial port access as follows:

The GUI thread holds SerialPortLockObject except when it has a background operation running.
The SerialPort class is wrapped so that any read or write by a thread not holding SerialPortLockObject throws an exception (helped find several contention bugs).
The timer class is wrapped (class SerialOperationTimer) so that the background worker function is called bracketed by acquiring SerialPortLockObject. SerialOperationTimer allows only one timer running at a time (helped find several bugs where the GUI forgot to stop background processing before starting up a different timer). This could be improved by using a specific thread for timer work, with that thread holding the lock for the entire time the timer is active (but would be still more work; as coded System.Timers.Timer runs worker function from thread pool).
When a SerialOperationTimer is stopped, it disables the underlying timer and flushes the serial port buffers (provoking an exception from any blocked serial port operation, as explained in possible method 1 above). Then SerialPortLockObject is reacquired by the GUI thread.

Here's the wrapper for SerialPort:

/// <summary> CheckedSerialPort class checks that read and write operations are only performed by the thread owning the lock on the serial port </summary>
// Just check reads and writes (not basic properties, opening/closing, or buffer discards). 
public class CheckedSerialPort : SafePort /* derived in turn from SerialPort */
{
    private void checkOwnership()
    {
        try
        {
            if (Monitor.IsEntered(XXX_Conn.SerialPortLockObject)) return; // the thread running this code has the lock; all set!
            // Ooops...
            throw new Exception("Serial IO attempted without lock ownership");
        }
        catch (Exception ex)
        {
            StringBuilder sb = new StringBuilder("");
            sb.AppendFormat("Message: {0}\n", ex.Message);
            sb.AppendFormat("Exception Type: {0}\n", ex.GetType().FullName);
            sb.AppendFormat("Source: {0}\n", ex.Source);
            sb.AppendFormat("StackTrace: {0}\n", ex.StackTrace);
            sb.AppendFormat("TargetSite: {0}", ex.TargetSite);
            Console.Write(sb.ToString());
            Debug.Assert(false); // lets have a look in the debugger NOW...
            throw;
        }
    }
    public new int ReadByte()                                       { checkOwnership(); return base.ReadByte(); }
    public new string ReadTo(string value)                          { checkOwnership(); return base.ReadTo(value); }
    public new string ReadExisting()                                { checkOwnership(); return base.ReadExisting(); }
    public new void Write(string text)                              { checkOwnership(); base.Write(text); }
    public new void WriteLine(string text)                          { checkOwnership(); base.WriteLine(text); }
    public new void Write(byte[] buffer, int offset, int count)     { checkOwnership(); base.Write(buffer, offset, count); }
    public new void Write(char[] buffer, int offset, int count)     { checkOwnership(); base.Write(buffer, offset, count); }
}

And here's the wrapper for System.Timers.Timer:

/// <summary> Wrap System.Timers.Timer class to provide safer exclusive access to serial port </summary>
class SerialOperationTimer
{
    private static SerialOperationTimer runningTimer = null; // there should only be one!
    private string name;  // for diagnostics
    // Delegate TYPE for user's callback function (user callback function to make async measurements)
    public delegate void SerialOperationTimerWorkerFunc_T(object source, System.Timers.ElapsedEventArgs e);
    private SerialOperationTimerWorkerFunc_T workerFunc; // application function to call for this timer
    private System.Timers.Timer timer;
    private object workerEnteredLock = new object();
    private bool workerAlreadyEntered = false;

    public SerialOperationTimer(string _name, int msecDelay, SerialOperationTimerWorkerFunc_T func)
    {
        name = _name;
        workerFunc = func;
        timer = new System.Timers.Timer(msecDelay);
        timer.Elapsed += new System.Timers.ElapsedEventHandler(SerialOperationTimer_Tick);
    }

    private void SerialOperationTimer_Tick(object source, System.Timers.ElapsedEventArgs eventArgs)
    {
        lock (workerEnteredLock)
        {
            if (workerAlreadyEntered) return; // don't launch multiple copies of worker if timer set too fast; just ignore this tick
            workerAlreadyEntered = true;
        }
        bool lockTaken = false;
        try
        {
            // Acquire the serial lock prior calling the worker
            Monitor.TryEnter(XXX_Conn.SerialPortLockObject, ref lockTaken);
            if (!lockTaken)
                throw new System.Exception("SerialOperationTimer " + name + ": Failed to get serial lock");
            // Debug.WriteLine("SerialOperationTimer " + name + ": Got serial lock");
            workerFunc(source, eventArgs);
        }
        finally
        {
            // release serial lock
            if (lockTaken)
            {
                Monitor.Exit(XXX_Conn.SerialPortLockObject);
                // Debug.WriteLine("SerialOperationTimer " + name + ": released serial lock");
            }
            workerAlreadyEntered = false;
        }
    }

    public void Start()
    {
        Debug.Assert(Form1.GUIthreadHashcode == Thread.CurrentThread.GetHashCode()); // should ONLY be called from GUI thread
        Debug.Assert(!timer.Enabled); // successive Start or Stop calls are BAD
        Debug.WriteLine("SerialOperationTimer " + name + ": Start");
        if (runningTimer != null)
        {
            Debug.Assert(false); // Lets have a look in the debugger NOW
            throw new System.Exception("SerialOperationTimer " + name + ": Attempted 'Start' while " + runningTimer.name + " is still running");
        }
        // Start background processing
        // Release GUI thread's lock on the serial port, so background thread can grab it
        Monitor.Exit(XXX_Conn.SerialPortLockObject);
        runningTimer = this;
        timer.Enabled = true;
    }

    public void Stop()
    {
        Debug.Assert(Form1.GUIthreadHashcode == Thread.CurrentThread.GetHashCode()); // should ONLY be called from GUI thread
        Debug.Assert(timer.Enabled); // successive Start or Stop calls are BAD
        Debug.WriteLine("SerialOperationTimer " + name + ": Stop");

        if (runningTimer != this)
        {
            Debug.Assert(false); // Lets have a look in the debugger NOW
            throw new System.Exception("SerialOperationTimer " + name + ": Attempted 'Stop' while not running");
        }
        // Stop further background processing from being initiated,
        timer.Enabled = false; // but, background processing may still be in progress from the last timer tick...
        runningTimer = null;
        // Purge serial input and output buffers. Clearing input buf causes any blocking read in progress in background thread to throw
        //   System.IO.IOException "The I/O operation has been aborted because of either a thread exit or an application request.\r\n"
        if(Form1.xxConnection.PortIsOpen) Form1.xxConnection.CiCommDiscardBothBuffers();
        bool lockTaken = false;
        // Now, GUI thread needs the lock back.
        // 3 sec REALLY should be enough time for background thread to cleanup and release the lock:
        Monitor.TryEnter(XXX_Conn.SerialPortLockObject, 3000, ref lockTaken);
        if (!lockTaken)
            throw new Exception("Serial port lock not yet released by background timer thread "+name);
        if (Form1.xxConnection.PortIsOpen)
        {
            // Its possible there's still stuff in transit from device (for example, background thread just completed
            // sending an ACQ command as it was stopped). So, sync up with the device...
            int r = Form1.xxConnection.CiSync();
            Debug.Assert(r == XXX_Conn.CI_OK);
            if (r != XXX_Conn.CI_OK)
                throw new Exception("Cannot re-sync with device after disabling timer thread " + name);
        }
    }

    /// <summary> SerialOperationTimer.StopAllBackgroundTimers() - Stop all background activity </summary>
    public static void StopAllBackgroundTimers()
    {
        if (runningTimer != null) runningTimer.Stop();
    }

    public double Interval
    {
        get { return timer.Interval; }
        set { timer.Interval = value; }
    }

} // class SerialOperationTimer

Upvotes: 0

VMAtm

Reputation: 28356

MSDN article for the SerialPort Class clearly states:

If a SerialPort object becomes blocked during a read operation, do not abort the thread. Instead, either close the base stream or dispose of the SerialPort object.

So the best approach, from my point of view, is second one, with async reading and step by step checking for the line-ending character. As you've stated, the check for each char is very big performance loss, I suggest you to investigate the ReadLine implementation for some ideas how to perform this faster. Note that they use NewLine property of SerialPort class.

I want also to note that there is no ReadLineAsync method by default as the MSDN states:

By default, the ReadLine method will block until a line is received. If this behavior is undesirable, set the ReadTimeout property to any non-zero value to force the ReadLine method to throw a TimeoutException if a line is not available on the port.

So, may be, in your wrapper you can implement similar logic, so your Task will cancel if there is no line end in some given time. Also, you should note this:

Because the SerialPort class buffers data, and the stream contained in the BaseStream property does not, the two might conflict about how many bytes are available to read. The BytesToRead property can indicate that there are bytes to read, but these bytes might not be accessible to the stream contained in the BaseStream property because they have been buffered to the SerialPort class.

So, again, I suggest you to implement some wrapper logic with asynchronous read and checking after each read, are there line-end or not, which should be blocking, and wrap it inside async method, which will cancel Task after some time.

Hope this helps.

Upvotes: 2