C# SerialPort implementation

Question

I'm writing a C# WPF project built with a MVVM pattern. This software is written to use a medical diagnostic device, so in our planning it will be certified IEC 62304. Since I've been mainly writing only C++ software for the last few years, I don't have much exeperience with C# programming. So I would like to receive a feedback on my code and understand if there are some weaknesses.

The software completely relies on a serial port communication with the device, so my serial port related code is very crucial. Since I read that there are a lot of problem when speakiing of the .NET serial port class, I would like to ask you if you think that my approach is right or not.

To costumize the serial port usage to my needs, I decided to implement a wrapper of the standard class, that hence inherits from SerialPort. Since I only need to communicate with one device, I decided to make my class static and call its public methods from my app. Is it a righ way to handle the communication? I also have some methods related to searching my serial port and check if my device (identified by USB VID and PID) is connected or not. I only need to receive data from the device when I ask for something, so I don't need to handle data indipendently sent from the device. I think I don't need a "DataReceived" event handler then.

In my MVVM pattern I call the serial port public methods from a Task launched from my ViewModel. Actually I have a second static class (called DeviceCommunication) with all the commands I have to send to my device, which calls my serial port methods like read and write. Do you see any problem in doing so? I'm posting my entire serial port class and some snippets from my ViewModel.

Any feedback is appreciated! Thanks!

/// <summary>
/// This class implements the Com Port Driver
/// </summary>
public static class ComPort
{
    /// <summary>
    /// Serial Port public handler
    /// </summary>
    static SerialPort devicePort = new SerialPort();

    /// <summary>
    /// Flag that tells if the device il connected
    /// </summary>
    public static bool DeviceConnected;

    /// <summary>
    /// COM port searching main method.
    /// </summary>
    public static void SearchCOM()
    {
        var QueryCmd = new byte[] { /* Command buffer content to query the device */ };
        var Reply = new byte[] { 0, 0, 0, 0 };
        var BytesRead = 0;

        if (DeviceConnected)
        {
            devicePort.Close();
            DeviceConnected = false;
        }

        while (!DeviceConnected)
        {
            var SerailPortsNames = ComPortNames("0483", "5740");
            if (SerailPortsNames.Count > 0)
            {
                foreach (string s in SerialPort.GetPortNames())
                {
                    if (SerailPortsNames.Contains(s))
                    {
                            devicePort.PortName = s;
                            devicePort.BaudRate = 115200;
                            devicePort.Parity = Parity.None;
                            devicePort.DataBits = 8;
                            devicePort.StopBits = StopBits.One;
                            devicePort.RtsEnable = false;
                            devicePort.DtrEnable = false;
                            devicePort.ReadTimeout = 10000;
                            devicePort.WriteTimeout = 1000;
                            devicePort.ReadBufferSize = 60000;
                            devicePort.Handshake = Handshake.None;
                            try
                            {
                                devicePort.Open();
                                if (devicePort.IsOpen)
                                {
                                    devicePort.Write(QueryCmd, 0, QueryCmd.Length);
                                    BytesRead = Read(Reply, 4);
                                    if (BytesRead != 4)
                                    {
                                        devicePort.Close();
                                    }
                                    else
                                    {
                                    DeviceConnected = true;
                                    }
                                }
                            }
                            catch(Exception e)
                            {
                                Console.WriteLine("{0} Exception caught.", e);
                            }
                      }
                }
            }
            System.Threading.Thread.Sleep(250);
        }

    }

    /// <summary>
    /// Public method to use for reading from COM port. It's an override of the already existing method from SerialPort class.
    /// </summary>
    /// <param name="buffer"> Buffer to use for storing data </param>
    /// <param name="lenght"> Number of bytes to read </param>
    /// <returns></returns>
    public static int Read(byte[] buffer, int lenght)
    {
        var i = 0;
        while (i < lenght)
        {
            i += devicePort.BaseStream.Read(buffer, i, (lenght - i));
        }
        return i;
    }

    /// <summary>
    /// Public method to use for writing to COM port. It's an override of the already existing method from SerialPort class.
    /// </summary>
    /// <param name="buffer"> Buffer to write </param>
    /// <param name="lenght"> Number of bytes to write </param>
    public static void Write(byte[] buffer, int lenght)
    {
        devicePort.BaseStream.Write(buffer, 0, lenght);
    }

    /// <summary>
    /// Public method to close the COM port.
    /// </summary>
    public static void Close()
    {
        devicePort.Close();
    }

    /// <summary>
    /// Static method that returns the list of connected COM ports with given VID and PID.
    /// </summary>
    /// <param name="vid"> Vendor ID </param>
    /// <param name="pid"> Product ID </param>
    /// <returns> List of connected COM ports with given VID and PID </returns>
    static List<string> ComPortNames(string vid, string pid)
    {
        int ID;
        var pattern = string.Format("^VID_{0}.PID_{1}", vid, pid);
        var _rx = new Regex(pattern, RegexOptions.IgnoreCase);
        var comports = new List<string>();
        var rk1 = Registry.LocalMachine;
        var rk2 = rk1.OpenSubKey("SYSTEM\\CurrentControlSet\\Enum");
        foreach (string s3 in rk2.GetSubKeyNames())
        {
            var rk3 = rk2.OpenSubKey(s3);
            foreach (string s in rk3.GetSubKeyNames())
            {
                if (_rx.Match(s).Success)
                {
                    var rk4 = rk3.OpenSubKey(s);
                    foreach (string s2 in rk4.GetSubKeyNames())
                    {
                        if (int.TryParse(s2, out ID))
                        {
                            var rk5 = rk4.OpenSubKey(s2);
                            var rk6 = rk5.OpenSubKey("Device Parameters");
                            if (IsDeviceConnected((string)rk6.GetValue("PortName")))
                            {
                                comports.Add((string)rk6.GetValue("PortName"));
                            }
                        }
                    }
                }
            }
        }
        return comports;
    }

    /// <summary>
    /// This method checks whether the device is connected or not.
    /// </summary>
    /// <param name="comName"> Name of the port </param>
    /// <returns> true if device is connected, fals if not. </returns>
    static bool IsDeviceConnected(string comName)
    {
        string GetComName;
        var Rk1 = Registry.LocalMachine;
        var Rk2 = Rk1.OpenSubKey("HARDWARE\\DEVICEMAP\\SERIALCOMM");
        foreach (string S1 in Rk2.GetValueNames())
        {
            GetComName = (string)Rk2.GetValue(S1);
            if (comName.Equals(GetComName))
            {
                return true;
            }
        }
        return false;
    }

}

In my ViewModel contructor I start two tasks:

var TaskSearchCOM = Task.Run(() => ComPort.SearchCOM());
var TaskStartup = Task.Run(() => Startup());


 ...

    /// <summary>
    /// This method asks the device for startup info.
    /// </summary>
    public async Task Startup()
    {
        while (!ComPort.DeviceConnected)
            await Task.Delay(250);

        Info = "Device found, asking for info...";
        while(taskState != TaskStates.Terminate)
        {

            switch(taskState)
            {
                case TaskStates.AskInfo:
                    {
                        var DeviceID = DeviceCommunication.GetDeviceSerialNumber();
                        ... break;
                    }
                case ...

Answer 1

The class ComPort is too general - especially for this implementation that is only designed to work with the specific device you're working with. You should rename it.

---

SerialPort is derived from Component which implements IDisposable. As you're holding a reference to an IDisposable you should implement IDisposable yourself and dispose the port in your own dispose method. That's the first indication that this doesn't belong as a static class.

---

Connections are much easier to handler when they're used as instances. E.g.

using (var connection = new YourDeviceConnection(vid, pid))
{

}

That also means you can use them in separate threads without worrying.

---

Method names should be a verb phrase. ComPortNames => GetComPortNames.

RegistryKey implements IDisposable so you should be clearing up after them.

String.StartsWith(string, StringComparison.OrdinalIgnoreCase) is easier than using a regular expression to ignore case.

---

Your documentation comments are really not good. There are numerous typos and not a single one of them tells me more than what the method name does (which isn't much).

---

Where possible you should return early rather than nesting:

var SerailPortsNames = ComPortNames("0483", "5740");
if (SerailPortsNames.Count > 0)
{
    // rest of code.

would be easier as:

var serialPortsNames = ComPortNames("0483", "5740");
if (!serialPortsNames.Any())
{
    // Could add a simple delay to not loop as quickly as possible...
    // Thread.Sleep(200);
    continue;
}
// rest of code.

Note that you had the 'i' and 'a' the wrong way round and given that it's a local variable it should begin with a lowercase letter.

---

As already mentioned, local variables should be camelCase.

Answer 2

About using static classes:

Maybe it is a matter of taste, but for my understanding static methods are functions that depend on input parameters only. If you have a single object that contains state, it is better to use a singleton or even better to use an DI framework.

So, the main disadvantage are IMHO that other developers may not expect a state behind a static class and it is not possible to mock that class (e.g. for unit testing). See also this discussion.

Code review

• The field DeviceConnected is not set from outside so it is better to create a property instead: DeviceConnected { get; private set; }.
• The class ComPort is not threadsafe but it is used from different threads. I can not see all usages, but that is generally not a good idea (even if it works, maybe somebody else has to change it some day)
• The methods ComPort.Read, ComPort.Write and ComPort.Close do not check if the device port is connected and will probably throw an meaningless exception. Maybe it makes sense to handle that or throw a descriptive exception.
• Not sure if that is desired, but if the device does not exist, the loop while (!DeviceConnected) { /* .. */ } will run endlessly. Consider to use a timer instead of the while loop.
• If method SearchCOM takes longer, it is better to start the task in view model with option TaskCreationOptions.LongRunning. see also