C# serial port driver wrapper class code and concept quality

Question

Hi folks, Would like to know from all you guys what do you think about my Serial Wrapper class. Had been a while I've been working with serial ports but never shared the code what somekind make me closed to my very own vision.

Would like to know if it's a good/bad approach, if the interface is enough and what more you see on it.

I know that Stackoverflow is for question but at the same time there's a lot of very good skilled people here and share code and opinion can also benefit everybody, it's why I decided to post it anyway.

thanks!

using System.Text;
using System.IO;
using System.IO.Ports;
using System;

namespace Driver
{
    class SerialSingleton
    {
        // The singleton instance reference
        private static SerialSingleton instance = null;

        // System's serial port interface
        private SerialPort serial;

        // Current com port identifier
        private string comPort = null;

        // Configuration parameters
        private int confBaudRate;
        private int confDataBits;
        private StopBits confStopBits;
        private Parity confParityControl;

        ASCIIEncoding encoding = new ASCIIEncoding();

// ==================================================================================
// Constructors

        public static SerialSingleton getInstance()
        {
            if (instance == null)
            {
                instance = new SerialSingleton();
            }
            return instance;
        }

        private SerialSingleton()
        {
            serial = new SerialPort();
        }

// ===================================================================================
// Setup Methods

        public string ComPort
        {
            get { return comPort; }
            set {
                if (value == null)
                {
                    throw new SerialException("Serial port name canot be null.");
                }

                if (nameIsComm(value))
                {
                    close();
                    comPort = value;
                }
                else
                {
                    throw new SerialException("Serial Port '" + value + "' is not a valid com port.");
                }
            }

        }

        public void setSerial(string baudRate, int dataBits, StopBits stopBits, Parity parityControl)
        {
            if (baudRate == null)
            {
                throw new SerialException("Baud rate cannot be null");
            }

            string[] baudRateRef = { "300", "600", "1200", "1800", "2400", "3600", "4800", "7200", "9600", "14400", "19200", "28800", "38400", "57600", "115200" };

            int confBaudRate;
            if (findString(baudRateRef, baudRate) != -1)
            {
                confBaudRate = System.Convert.ToInt32(baudRate);
            }
            else
            {
                throw new SerialException("Baurate parameter invalid.");
            }

            int confDataBits;
            switch (dataBits)
            {
                case 5:
                    confDataBits = 5;
                    break;
                case 6:
                    confDataBits = 6;
                    break;
                case 7:
                    confDataBits = 7;
                    break;
                case 8:
                    confDataBits = 8;
                    break;
                default:
                    throw new SerialException("Databits parameter invalid");
            }

            if (stopBits == StopBits.None)
            {
                throw new SerialException("StopBits parameter cannot be NONE");
            }

            this.confBaudRate = confBaudRate;
            this.confDataBits = confDataBits;
            this.confStopBits = stopBits;
            this.confParityControl = parityControl;
        }

// ==================================================================================

        public string[] PortList
        {
            get {
                return SerialPort.GetPortNames();
            }
        }

        public int PortCount
        {
            get { return SerialPort.GetPortNames().Length; }
        }

// ==================================================================================
// Open/Close Methods


        public void open()
        {
            open(comPort);
        }

        private void open(string comPort) 
        {
            if (isOpen())
            {
                throw new SerialException("Serial Port is Already open");
            }
            else
            {
                if (comPort == null)
                {
                    throw new SerialException("Serial Port not defined. Cannot open");
                }


                bool found = false;
                if (nameIsComm(comPort))
                {
                    string portId;
                    string[] portList = SerialPort.GetPortNames();
                    for (int i = 0; i < portList.Length; i++)
                    {
                        portId = (portList[i]);
                        if (portId.Equals(comPort))
                        {
                            found = true;
                            break;
                        }
                    }
                }
                else
                {
                    throw new SerialException("The com port identifier '" + comPort + "' is not a valid serial port identifier");
                }
                if (!found)
                {
                    throw new SerialException("Serial port '" + comPort + "' not found");
                }

                serial.PortName = comPort;
                try
                {
                    serial.Open();
                }
                catch (UnauthorizedAccessException uaex)
                {
                    throw new SerialException("Cannot open a serial port in use by another application", uaex);
                }

                try
                {
                    serial.BaudRate = confBaudRate;
                    serial.DataBits = confDataBits;
                    serial.Parity = confParityControl;
                    serial.StopBits = confStopBits;
                }
                catch (Exception e)
                {
                    throw new SerialException("Serial port parameter invalid for '" + comPort + "'.\n" + e.Message, e);
                }
            }
        }

        public void close()
        {
            if (serial.IsOpen)
            {
                serial.Close();
            }
        }

// ===================================================================================
// Auxiliary private Methods

        private int findString(string[] set, string search)
        {
            if (set != null)
            {
                for (int i = 0; i < set.Length; i++)
                {
                    if (set[i].Equals(search))
                    {
                        return i;
                    }
                }
            }
            return -1;
        }

        private bool nameIsComm(string name)
        {
            int comNumber;
            int.TryParse(name.Substring(3), out comNumber);
            if (name.Substring(0, 3).Equals("COM"))
            {
                if (comNumber > -1 && comNumber < 256)
                {
                    return true;
                }
            }
            return false;
        }

// =================================================================================
// Device state Methods

        public bool isOpen()
        {
            return serial.IsOpen;
        }

        public bool hasData()
        {
            int amount = serial.BytesToRead;
            if (amount > 0)
            {
                return true;
            }
            else
            {
                return false;
            }
        }

// ==================================================================================
// Input Methods

        public char getChar()
        {
            int data = serial.ReadByte();
            return (char)data;
        }

        public int getBytes(ref byte[] b)
        {
            int size = b.Length;
            char c;
            int counter = 0;
            for (counter = 0; counter < size; counter++)
            {
                if (tryGetChar(out c))
                {
                    b[counter] = (byte)c;
                }
                else
                {
                    break;
                }
            }
            return counter;
        }

        public string getStringUntil(char x)
        {
            char c;
            string response = "";
            while (tryGetChar(out c))
            {
                response = response + c;
                if (c == x)
                {
                    break;
                }
            }
            return response;
        }

        public bool tryGetChar(out char c)
        {
            c = (char)0x00;
            byte[] b = new byte[1];
            long to = 10;
            long ft = System.Environment.TickCount + to;
            while (System.Environment.TickCount < ft)
            {
                if (hasData())
                {
                    int data = serial.ReadByte();
                    c = (char)data;
                    return true;
                }
            }
            return false;
        }

// ================================================================================
// Output Methods

        public void sendString(string data)
        {
            byte[] bytes = encoding.GetBytes(data);
            serial.Write(bytes, 0, bytes.Length);
        }

        public void sendChar(char c)
        {
            char[] data = new char[1];
            data[0] = c;
            serial.Write(data, 0, 1);
        }


        public void sendBytes(byte[] data)
        {
            serial.Write(data, 0, data.Length);
        }

        public void clearBuffer()
        {
            if (serial.IsOpen)
            {
                serial.DiscardInBuffer();
                serial.DiscardOutBuffer();
            }
        }

    }

}

Answer 1

Here's some (non exhaustive) initial observations

• Don't be afraid of showing code (its good to ask for advice)
• Don't have comments stating the obvious (like // this singleton etc)
• You're throwing a SerialException on invalid arguments - there are already exceptions for this
• Comms is notoriously error prone. I supect you need to consider the reliability of the sends and receives.
• You have magic strings and numbers every where which may (!) be a localisation headache later
• Do you need singletons
• Consider IOC/DI patterns to make this testable.

Answer 2

A few things jump out:

• The class is a singleton, but a computer could have more than one serial port
• You take baudRate as a string and immediately convert it to a number. Why not just take an integer parameter.
• You're using switch(dataBits) to check if it is in a range, why not if (confDataBits < 5 || confDataBits > 8) //exception

In general, the extra functionality you added would not be too helpful for what I do with serial ports, but your scenarios are probably different then mine. The methods you added seem to ignore what I consider the harder part of using a SerialPort, the asynchronous sending and receiving. All of the input methods you have are synchronous, so calling them will cause the thread to block until enough data comes in.

Answer 3

I'm assuming that you have an app that needs to access a serial port the way other apps use stdin/stdout. If that's not the case, you should reconsider your use of a singleton for this.

• The setSerial method doesn't do anything useful if the serial port is already open. It should throw an exception, change the settings of the open port, or close the port and reopen it with new settings.

• getInstance, isOpen, and hasData should probably be read-only properties rather than methods.

• sendString, sendChar, and sendBytes could all be different overloads of a send method.

• tryGetChar has a busy loop. That's very bad. Either use ReadTimeout or have the reading thread wait on an event with a timeout and signal the event from a DataReceived handler.

• You should consider having a send timeout mechanism.