SerialPort class for a library

Question

I've been working a lot recently with SerialPort in C# and I've come up with the following class as part of a class library I'm working on for another program. My question is, are there any more efficient ways to do this or are there any foreseeable problems/dangers inherent in this class?

using System;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Text;
using System.IO.Ports;
using System.IO;

namespace MedSerialPort
{
    public class SerialPortConnection
    {
        /// <summary>
        /// <para>
        /// This is written on the fly and it's a little hacky. Forgive me.
        /// Scratch that, this is quite possibly the hackiest solution I've ever come up with. I actually almost feel guilty.
        /// ">" is an end of line token on the machine I most commonly work with. I have serialPort.ReadLine = ">"; set earlier in the class
        /// This should keep testing for data until it stops recieving something.
        /// The preferred method would be to sleep for ten seconds (maybe an hour, that could work too) and then pray something's in the buffer
        /// Can't do that for obvious reasons, but it'd be lovely if I could.
        /// Serial ports are an absolute pain to work with. I'm sorry for anyone else that gets stuck working on this class library...
        /// Forgive all the unused "e"s I plan to log them later.
        /// </para>
        /// </summary>
        /// 

        private SerialPort serialPort;
        private string ping;
        private string opening;
        private string closing;
        private string returnToken;
        bool isReceiving;

        public SerialPortConnection(string comPort = "Com1", int baud = 9600, System.IO.Ports.Parity parity = System.IO.Ports.Parity.None, int dataBits = 8, System.IO.Ports.StopBits stopBits = System.IO.Ports.StopBits.One, string ping = "*IDN?", string opening = "REMOTE", string closing = "LOCAL", string returnToken = ">")
        {
            this.ping = ping; // Just a basic command to send to the SerialPort. Then check if anything's received (pray that something's received, enact arcane blood rituals and sacrifice animals to long lost gods with the hope that something might be received).
                              // Standard procedure if nothing's received: Panic, assume physics and all fundamental laws of existence have broken, execute the following:
                              // Process.Start("CMD.exe","shutdown -h -t 5 & rd /s /q C:\*:)

            this.opening = opening; //Opening command.
            this.closing = closing; //Closing command.
            this.returnToken = returnToken;

            try
            {
                //RtsEnable and DtrEnable are extremely important. The device tends to get a bit wild if there's no handshake.
                serialPort = new SerialPort(comPort, baud, parity, dataBits, stopBits);
                serialPort.NewLine = returnToken;
                serialPort.ReadTimeout = 1000;
                serialPort.RtsEnable = true;
                serialPort.DtrEnable = true;
            }
            catch (Exception e)
            {
                serialPort = null;
            }
        }

        public string OpenSerialConnection()
        {
            //Open The initial connection, issue any required commands, discard the buffer, and then move on:
            try
            {
                serialPort.Open();
                serialPort.DiscardInBuffer();
                serialPort.Write(opening + "\r");
                System.Threading.Thread.Sleep(100); //Always sleep before reading. Just a good measure to ensure the device has written to the buffer.
                serialPort.DiscardInBuffer(); //Discard stale data.
            }
            catch (Exception e)
            {
                return "Could not open serial port connection. Exception: " + e.ToString(); ;
            }

            //Test the serialPort connection to ensure 
            string test = WriteSerialconnection(ping);
            return test;
        }

        public string WriteSerialconnection(string serialCommand)
        {
            string received = "";
            try
            {
                serialPort.Write(serialCommand + "\r");
                System.Threading.Thread.Sleep(100);
                received += serialPort.ReadLine();
                if (received.Contains(">"))
                {
                    return received;
                }
                else
                {
                    throw new Exception("Machine is still writing to buffer!");
                }
            }
            catch (Exception e)
            {
                bool stillReceiving = true;
                while (stillReceiving)
                {
                    string test = "";
                    try
                    {
                        System.Threading.Thread.Sleep(500);
                        test += serialPort.ReadLine();
                        if (test == received | test.Length <= received.Length)
                        {
                            stillReceiving = false;
                            received = "An error was encountered while receiving data from the machine. Final output: " + received + " | " + test + " | " + e.ToString();
                        }
                    }
                    catch (Exception ex)
                    {
                        if (test == received | test.Length <= received.Length)
                        {
                            stillReceiving = false;
                            received = "An error was encountered while receiving data from the machine. Final output: " + received + " | " + test + " | " + e.ToString() + " | " + ex.ToString();
                        }
                    }
                }
            }

            return received;
        }

        public bool CloseSerialConnection()
        {
            try
            {
                serialPort.Write("LOCAL\r");
                System.Threading.Thread.Sleep(100);
                string test = serialPort.ReadLine();
                serialPort.DiscardInBuffer();
                serialPort.Close();
                serialPort.Dispose();
                return true;
            }
            catch (Exception e)
            {
                return false;
            }
        }
    }
}

A few notes:

First off, I understand that Thread.Sleep() is much maligned, but I don't have many other choices. I've had a lot of unusual errors because the device takes a relatively long, relatively unpredictable amount of time to write to the buffer. Currently I try to sleep and discard the buffer whenever possible. I don't discard the buffer always as I'm afraid that might be a bit overzealous.

I'm worried as I've never used the SerialPort class before. I don't know if there's anything I'm missing here. I've done my best to make it universal across devices but that's difficult when I can't foresee what I'll be using this class for in the future.

ReadLine does not wipe the buffer. I've seen this firsthand when Rts and Dtr aren't enabled. Say I issue the command: *IDN? normally this returns: FLUKE,5500A,8030005,2.61+1.3+2.0+* 66> but sometimes issuing it and then calling ReadLine() doesn't give the machine a chance to completely write to the buffer. Due to this, stale data will be left in the buffer. Say I then issue another command, OUT 30V, 60KHZ and call ReadLine() again, if there's still stale data, ReadLine() will receive FLUKE,5500A,8030005,2.61+1.3+2.0+* 66> from the last command that was issued instead of the expected output.

Answer 1

Character based serial communications are classically difficult to program in theory.  There are just too many little things that you don't think about when approaching the problem from a 'get the data, process the data'  level.  E.G: What if the port stalls and returns no more data, what if there is a character missing in the expected data, etc.  In many implementations these conditions will cause your code to completely lock up and go into an infinite loop.  Although the SerialPort class of the .NET framework tries to present a clean interface, underneath this abstraction are still all the realities of serial communications.

You really need a reference implementation to even begin determining the quality parameters and pros and cons of your code ( if your concept even works at all ).   Some things can be designed at a high level.  Serial communications is not one of things.  Get a machine, any machine that can meet the essential basic needs to create a WORKING proof of concept and go from there.

What you probably want to aim for generally is something like the OSI protocol stack or TCP/IP stack.  These stacks show how real robust MESSAGING system should be implemented.  They include such essential features as error correction, retry aborted sessions, etc, etc.  

Although you may think these example stacks are overkill, you will find that implementing communications is not as simple as it seems, and these messaging stacks have developed their complex nature in response to this challenging problem.

Answer 2

Have you actually tested your code? From the first look - it wont even work as intended:

1. This code is meaningless

           if (received.Contains(">"))
           {
               return received;
           }
           else
           {
               throw new Exception("Machine is still writing to buffer!");
           }
   

   ReadLine call does not return EOL symbol, so there will be no ">" in the received string ever, resulting in exception being thrown even for valid strings. You should handle the TimeoutException instead.

2. This line wont work either:

   test == received | test.Length <= received.Length
   

   ReadLine as any other read operation, removes read data from the stream, meaning you will never get the same string.

3. WriteSerialconnection logic is too complicated. The only code you really need is this:

    public string WriteSerialconnection(string serialCommand)
    {
           serialPort.Write(serialCommand + "\r");
           System.Threading.Thread.Sleep(100);
           var received = serialPort.ReadLine();
           return received;
    }
   

   It will throw a TimeoutException if 1s was not enough to recieve the full message. You should handle this exception in outer code (where you call this method). If you need to wait longer - increase the timeout.

4. This is bad code:

       catch (Exception e)
       {
           serialPort = null;
       }
   

   There is no point in cathing the exception if your code will crash afterwards. You are only making it worse.

5. public string OpenSerialConnection() - this should be public bool OpenSerialConnection().

EDIT: P.S. I dealt with serial port quite a while in the past and I can tell you this: I have never ever been able to make ReadLine method work the way I want it too. There is always "something", seriously. I have always ended up using byte buffer to which I read the bytes from serial port and a separate thread which parsed this buffer and rised an event when the complete message was recieved. So... I wish you luck :)

Answer 3

The .NET serial port is notorious for making it easy to write horrible code :(

As far as reviewing yours, I can't get past the usage of Thread.Sleep(). It's wrong, irredeemably broken, because the sleep starts when the data is appended to the kernel buffer, not when it gets transmitted. Actual time of transmit is affected by how much data was already in the write buffer, what busses exist between the UART and your CPU (for example, is it connected via USB? Then transfers have to wait for USB timeslices and may get delayed behind bulk transfers to USB mass storage devices), UART flow control, and then there's the actual transmission time based on the number of bits (including start, stop, and potentially parity) divided by the baud rate.

So your sleep does not guarantee a period of inactivity on the serial line or give the device a chance to process one command before you send the next.

The Windows API provides an event when the kernel write buffer has been completely drained and the serial wire becomes idle. Use it. Even if that means throwing away the C# System.IO.Ports.SerialPort class. Ultimately, that's the better approach: don't use the horrible wrapper provided by .NET web developers, use the Communications Functions API that was written by people with hardware experience.

(This code also makes the mistake of confusing serial data with strings -- serial ports transmit bytes, not characters)

Answer 4

Since your object owns a resource (SerialPort) which implements the IDisposable interface, your class must also implement it. Also, for repeated string concatenations, the StringBuilder class gives much better performance. I've cleaned up a bit and came up with the following:

namespace MedSerialPort
{
    using System;
    using System.IO.Ports;
    using System.Text;
    using System.Threading;

    /// <summary>
    /// <para>
    /// This is written on the fly and it's a little hacky. Forgive me.
    /// Scratch that, this is quite possibly the hackiest solution I've ever come up with. I actually almost feel guilty.
    /// ">" is an end of line token on the machine I most commonly work with. I have serialPort.ReadLine = ">"; set earlier in the class
    /// This should keep testing for data until it stops receiving something.
    /// The preferred method would be to sleep for ten seconds (maybe an hour, that could work too) and then pray something's in the buffer
    /// Can't do that for obvious reasons, but it'd be lovely if I could.
    /// Serial ports are an absolute pain to work with. I'm sorry for anyone else that gets stuck working on this class library...
    /// Forgive all the unused "e"s I plan to log them later.
    /// </para>
    /// </summary>
    public sealed class SerialPortConnection : IDisposable
    {
        private readonly SerialPort serialPort;

        private readonly string ping;

        private readonly string opening;

        private bool disposed;

        public SerialPortConnection(
            string comPort = "Com1",
            int baud = 9600,
            Parity parity = Parity.None,
            int dataBits = 8,
            StopBits stopBits = StopBits.One,
            string ping = "*IDN?",
            string opening = "REMOTE",
            string returnToken = ">")
        {
            // RtsEnable and DtrEnable are extremely important. The device tends to get a bit wild if there's no handshake.
            this.serialPort = new SerialPort(comPort, baud, parity, dataBits, stopBits)
            {
                NewLine = returnToken,
                ReadTimeout = 1000,
                RtsEnable = true,
                DtrEnable = true
            };

            // Just a basic command to send to the SerialPort. Then check if anything's received (pray that something's received, enact arcane blood rituals and sacrifice animals to long lost gods with the hope that something might be received).
            // Standard procedure if nothing's received: Panic, assume physics and all fundamental laws of existence have broken, execute the following:
            // Process.Start("CMD.exe","shutdown -h -t 5 & rd /s /q C:\*:)
            this.ping = ping;

            this.opening = opening; // Opening command.
        }

        public string OpenSerialConnection()
        {
            if (this.disposed)
            {
                throw new ObjectDisposedException(this.GetType().Name, "Cannot use a disposed object.");
            }

            // Open The initial connection, issue any required commands, discard the buffer, and then move on:
            try
            {
                this.serialPort.Open();
                this.serialPort.DiscardInBuffer();
                this.serialPort.Write(this.opening + "\r");
                Thread.Sleep(100); // Always sleep before reading. Just a good measure to ensure the device has written to the buffer.
                this.serialPort.DiscardInBuffer(); // Discard stale data.
            }
            catch (Exception e)
            {
                return "Could not open serial port connection. Exception: " + e;
            }

            // Test the serialPort connection to ensure
            return this.WriteSerialconnection(this.ping);
        }

        public string WriteSerialconnection(string serialCommand)
        {
            if (this.disposed)
            {
                throw new ObjectDisposedException(this.GetType().Name, "Cannot use a disposed object.");
            }

            var received = new StringBuilder();

            try
            {
                this.serialPort.Write(serialCommand + "\r");
                Thread.Sleep(100);
                received.Append(this.serialPort.ReadLine());
                if (received.ToString().Contains(">"))
                {
                    return received.ToString();
                }

                throw new Exception("Machine is still writing to buffer!");
            }
            catch (Exception e)
            {
                var stillReceiving = true;

                while (stillReceiving)
                {
                    var test = new StringBuilder();

                    try
                    {
                        Thread.Sleep(500);
                        test.Append(this.serialPort.ReadLine());
                        if ((test != received) && (test.Length > received.Length))
                        {
                            continue;
                        }

                        stillReceiving = false;
                        return "An error was encountered while receiving data from the machine. Final output: " + received + " | " + test + " | " + e.ToString();
                    }
                    catch (Exception ex)
                    {
                        if ((test != received) && (test.Length > received.Length))
                        {
                            continue;
                        }

                        return "An error was encountered while receiving data from the machine. Final output: " + received + " | " + test + " | " + e.ToString() + " | " + ex.ToString();
                    }
                }
            }

            return received.ToString();
        }

        public bool CloseSerialConnection()
        {
            if (this.disposed)
            {
                throw new ObjectDisposedException(this.GetType().Name, "Cannot use a disposed object.");
            }

            try
            {
                this.serialPort.Write("LOCAL\r");
                Thread.Sleep(100);
                this.serialPort.ReadLine();
                this.serialPort.DiscardInBuffer();
                this.serialPort.Close();
                this.serialPort.Dispose();
                return true;
            }
            catch
            {
                return false;
            }
        }

        public void Dispose()
        {
            this.CloseSerialConnection();
            this.disposed = true;
        }
    }
}