2
\$\begingroup\$

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 ...
\$\endgroup\$
  • \$\begingroup\$ Because ComPort has a state, I would not use a static class for that for that. \$\endgroup\$ – JanDotNet May 20 '16 at 10:18
  • \$\begingroup\$ @JNS Hi! Could you please expan on that? Since I would only have one instance of that non static class, what difference would it make? \$\endgroup\$ – Lori May 20 '16 at 11:05
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the good suggestions! I'm quite convinced to swtich from a static class to maybe a singleton and implement a Dispose() method. I'm quite confused though about the using pattern. I want my connection to open as the first action in my software and close at the end. In the middle I have to do multiple write and read actions, so how could I handle my whole software within a single using(...) {...} directive? I think I followed all your other suggestions. I know my comments are bad, but I only wanted to start taking care of then, I'll pay more attention to them later on :) \$\endgroup\$ – Lori May 22 '16 at 9:57
  • \$\begingroup\$ @Lori - If you need the connection open for the lifetime of your application then store the instance and remember to dispose before you close the app. I was suggesting that you may have been able to make the connection lightweight and so been able to create multiple connections with smaller lifetimes. \$\endgroup\$ – RobH May 22 '16 at 11:23
  • \$\begingroup\$ I'm not sure if I'm getting it right, are you suggesting that I should create an instance, open and then close the port every time I have to write or read, instead of keeping the connection open during all the time? Are there any advantages in doing so vs the way I do that? \$\endgroup\$ – Lori May 23 '16 at 6:43
1
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ @ JNS thanks for the info and the good links! I'll deeply dive into them! Regarding the "static" issue. What do you mean by state? Are you reffering to the SerialPort devicePort's properties or do you consider my DeviceConnected as a state too? Anyway I think I'm getting it and I'll have a try to convert my class. \$\endgroup\$ – Lori May 20 '16 at 14:43
  • \$\begingroup\$ Regarding the review: - Done, thanks - How could I make it thread safe? Anyway this is not a problem since I only call a Write() or Read() method from one Task and only if the previous call has returned. - I think I'll add a some checks, Thanks! - That's the pourpose indeed! Staying in the loop until I switch my device on. It's a matter of a few seconds anyway! \$\endgroup\$ – Lori May 20 '16 at 14:43
  • \$\begingroup\$ @Lori: "What do you mean by state?" Everything that affects the result of a method. So, in you case both: devicePort and DeviceConnected. \$\endgroup\$ – JanDotNet May 20 '16 at 14:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.