Skip to main content
edited body
Source Link
Nikita B
  • 13.1k
  • 1
  • 26
  • 57

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.

  1. 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.

  2. 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.

  1. 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.

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

EDIT: P.S. I dealt with serial port quite a while iin the past and iI can tell you this: iI have never ever been able to make ReadLine method work the way iI want it too. THereThere is always "something", seriously. I have always ended up using byte buffer to which iI 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... iI wish you luck :)

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.

  1. 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.

  2. 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.

  1. 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.

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

EDIT: P.S. I dealt with serial port quite a while i 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 :)

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.

  1. 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.

  2. 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.

  1. 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.

  1. 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 :)

added 392 characters in body
Source Link
Nikita B
  • 13.1k
  • 1
  • 26
  • 57

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.

  1. 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.

  2. 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.

  1. 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.

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

I'll addEDIT: P.S. I dealt with serial port quite a postscriptum later onwhile i 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 :)

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.

  1. 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.

  2. 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.

  1. 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.

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

I'll add a postscriptum later on :)

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.

  1. 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.

  2. 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.

  1. 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.

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

EDIT: P.S. I dealt with serial port quite a while i 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 :)

Source Link
Nikita B
  • 13.1k
  • 1
  • 26
  • 57

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.

  1. 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.

  2. 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.

  1. 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.

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

I'll add a postscriptum later on :)