1
\$\begingroup\$

I am building a console application which will be scheduled in Task Scheduler of Windows to run my code every day at a set hour. Summing up, this application will read and write through serial port. When I send something to the Arduino, I need to receive something from it to finish up what I've sent and execute the command.

In other words, I will send something so see if the door is opened, if it is the application will run a code to close it. If the door is already closed I will send a bunch of characters to be displayed into the Arduino Led Display.

So I've developed a code but I am not sure if it's totally correct, if possible help me improve it. There is any changes I could make?

static void Main(string[] args)
{
    SerialPort comport = new SerialPort("COM1", 9600, Parity.None, 8, StopBits.One);
    comport.Open();

    string start = "?";
    string carriageReturn = "\r";
    string text = string.Empty;

    string mensage = "@" + "r" + "\r";
    string mensage2 = "@" + "{" + texto + "\r";

        try
            {
                while (true)
                {
                    //Send to the Arduino
                    comport.Write(start + "*" + carriageReturn);

                    //If the serial port have bytes to read
                    if (comport.BytesToRead > 0)
                    { 
                        //Buffer with data
                        byte[] data = HexStringToByteArray(mensage);

                        //Handle data
                        comport.Read(data, 0, data.Length);

                        //Send again to execute the the command
                        comport.Write(start + "*" + carriageReturn);
                    }

                    if (comport.BytesToRead > 0)
                    { 
                        comport.Write(start + "*" + carriageReturn);

                        byte[] data2 = HexStringToByteArray(mensage2);

                        comport.Read(data2, 0, data2.Length);

                        comport.Write(text);
                    }
                    comport.Close();
                }
            }

        catch(Exception ex)
        {
        }
}

private static byte[] HexStringToByteArray(string s)
{
    s = s.Replace(" ", "");
    byte[] buffer = new byte[s.Length / 2];

    for (int i = 0; i < s.Length; i += 2)
        buffer[i / 2] = (byte)Convert.ToByte(s.Substring(i, 2), 16);
        return buffer;
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Your if (comport.BytesToRead > 0) and else if(comport.BytesToRead > 0) have the same condition. \$\endgroup\$ – Dmitry Nov 11 '16 at 12:21
  • \$\begingroup\$ So I have to do to different conditions? @Dmitry \$\endgroup\$ – Rekcs Nov 11 '16 at 12:24
  • \$\begingroup\$ I've updated the post @Dmitry \$\endgroup\$ – Rekcs Nov 11 '16 at 12:29
  • \$\begingroup\$ @Rekcs it doesn't make much sense now. You could put the code of both ifs into the same block and call them as two methods. \$\endgroup\$ – t3chb0t Nov 11 '16 at 12:30
  • \$\begingroup\$ @t3chb0t This is what I want my code to do: I will send "?*\r" to the Arduino, then I will wait for a answer. If the answer from Arduino is "@r\r" I will send the "?*\r" again. If the answer fro the Arduino is "@{/r" I will send a string to him. \$\endgroup\$ – Rekcs Nov 11 '16 at 12:32
6
\$\begingroup\$

There are a number of strange things going on here...

First, you have this:

//Buffer with data
byte[] data = HexStringToByteArray(mensage);

//Handle data
comport.Read(data, 0, data.Length);

The first line takes the mensage (message?) and turns it into a byte array, but then you immediately overwrite the data by reading into the same buffer. In essence, you are using HexStringToByteArray simply to create a new array of some length (3 bytes?).

Based on how you are using HexStringToByteArray, it certainly does not take a hex string and turn it into a byte array. mensage and mensage2 contain things other than valid hex characters. Maybe something more descriptive like StringToByteArray or StringToHexByteArray would be better. It does look like you are trying to convert a "hex" string input to byte data, but given the inputs I can't see how you can run this and not get an exception. The other problem with this method is that it assumes you have an input string length divisible by 2, and you are not checking that (nor are your values passed in adhering to that).

Since you are not doing anything with the data and data2 arrays, you are basically using them to flush some data out of the read buffer. I would have to assume that the code you posted is not complete, otherwise you could do something like SerialPort.Flush() instead.

Now, lets look at your variables:

string start = "?";
string carriageReturn = "\r";
string text = string.Empty;

string mensage = "@" + "r" + "\r";
string mensage2 = "@" + "{" + texto + "\r";

I would advise against using the carriageReturn and either call it something like messageDelimiter or just don't use it. It would be akin to defining something like:

int zero = 0;

Zero will always equal zero, so you shouldn't have variables for them. The name text doesn't really help here, you should give it a descriptive name like emptyMessage since it doesn't look like it ever changes. mensage and mensage2 are also not descriptive (or spelled correctly), maybe something like doorOpenQuery and doorCloseCommand. (And in addition, since texto isn't defined, I don't see this even compiling...)

The last problem I see is that you have an un-throttled while(true) loop in your program with no exit conditions other than something throwing an exception. How many times do you want to run this loop? You shouldn't have an unbounded while, you are going to have a not-responding program if you let it run for about 10 seconds.

From what I see I don't believe that this is functioning code. Have you run this through the debugger? If you have functionality problems this probably isn't the StackExchange site for that.

\$\endgroup\$
  • 2
    \$\begingroup\$ Nice answer, welcome to Code Review! \$\endgroup\$ – Adriano Repetti Nov 11 '16 at 13:47
  • \$\begingroup\$ The exception is clearly thrown after the first iteration of the Do loop thats the way the op is exiting the loop. \$\endgroup\$ – Heslacher Nov 11 '16 at 18:30
  • \$\begingroup\$ @Heslacher Only because the code doesn't work. If the OP means to only run the code once (which is most likely) then the loop serves no purpose what-so-ever. If the OP means to run it more than once, then the loop is fine, but should not be closing the port. The "exit condition" is only happening because the code doesn't work. Exceptions should never be used for control flow on purpose. \$\endgroup\$ – Ron Beyer Nov 11 '16 at 18:40
  • \$\begingroup\$ Thats just what I wanted to say only with fewer words ;-) \$\endgroup\$ – Heslacher Nov 11 '16 at 18:41

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.