4
\$\begingroup\$

This is an efficiency question to the following code, written in MS Visual in C#. If any explanation is missing, please let me know and I will add. What this part of the program does is:

  1. Button is pushed
  2. Motor starts moving (by sending 9-byte array with command)
  3. Waiting for Sensor to register a change in value
  4. Motor stops
  5. Motorposition is saved (command to get position is send and 9-byte array response is send by the motor), to have access to it later on and return there

Calls like rotate() and stop() are commands that write data into a 9-byte-array and write bytearray to serial port.

Now, my main issue is Step 5. I read on Stack Overflow that "if you have to use wait() and there is no point of using the DataReceived implementation". So should I really get rid of this or is there a nice way to handle this? If I do it without the wait() it saves the answer that is send after the stop command (every command also sends an answer).

And would it be more efficient to use DataReceived in step 3? In the beginning I save a value as new 0, and compare the current sensor readout here to this 0 value.

private void button13_Click(object sender, EventArgs e)
{
    //rotate right speed
    string Test = textBox4.Text;
    byte[] A = new byte[9];
    rotate(A, Test);
    //read sensor
    serialPort2.WriteLine("SR,00,002\r\n");
    string b = serialPort2.ReadLine();
    decimal caliber = decimal.Parse(Regex.Split(b, "SR,00,002,")[1]);
    decimal b1 = 0;
    do
    {
        serialPort2.WriteLine("SR,00,002\r\n");
        string z = serialPort2.ReadLine();
        b1 = decimal.Parse(Regex.Split(z, "SR,00,002,")[1]);
    }
    while (b1 <= caliber);
    // stop motor
    byte[] B = new byte[9];
    stop(B);
    //save position
    System.Threading.Thread.Sleep(5000);
    byte[] C = new byte[9];
    getPosi(C);
    System.Threading.Thread.Sleep(5000);
    M = buf;
    //move to 0
    byte[] D = new byte[9];
    MoveTo0(D);
}

In case it is needed for understanding the code above:

//global variables
byte[] buf = new byte[9];
byte[] M = new byte[9];
private void button1_Click(object sender, EventArgs e)      //Ports öffnen
{
    serialPort3 = new SerialPort();
    serialPort3.PortName = "COM6";
    serialPort3.Handshake = Handshake.RequestToSend;
    serialPort3.ReceivedBytesThreshold = 8;
    serialPort3.DataReceived += new SerialDataReceivedEventHandler(DataRecievedHandler);
    serialPort3.Open();
}
private void DataRecievedHandler(object sender, SerialDataReceivedEventArgs e)
{ 
    SerialPort sp = (SerialPort)sender;
    int bytes = serialPort3.BytesToRead;
    byte[] buffer = new byte[bytes];
    sp.Read(buffer, 0, bytes);
    string hex = BitConverter.ToString(buffer);
    hex.Replace("-", "");
    MessageBox.Show(hex);
    buf = buffer;         
}

Other methods:

private void rotate(byte[] A, string Test) //Rotate Motor Right
    {
        A = MB.TMCL_RMR(Test);
        serialPort3.Write(A, 0, A.Length);
    }

    private void stop(byte[] B) // Stop Motor
    {
        B = MB.TMCL_MST();
        serialPort3.Write(B, 0, B.Length);
    }

    private void getPosi(byte[] C) //Get Position
    {
        C = MB.GET_POSI();
        serialPort3.Write(C, 0, C.Length);
    }

    private void MoveTo0(byte[] D) //Move to 0
    {
        D = MB.MoveTo0();
        serialPort3.Write(D, 0, D.Length);
    }

And as example a method from my seperate motorcontrolclass on how the bytearray is created. Each hex-byte references a certain information, last byte is a checksum. If this array is send to the serial port it will request the motorposition as an answer.

public byte[] GET_POSI()
    {
        byte[] E = new byte[9];
        E[0] = 0x1;
        E[1] = 0x6;
        E[2] = 0x1;
        E[3] = 0x0;
        E[4] = 0x0;
        E[5] = 0x0;
        E[6] = 0x0;
        E[7] = 0x0;
        E[8] = 0x08;
        return E;
    }

The code is working as I want it to for several input tests. I just thought there might be a better way than the Thread.Sleep().

\$\endgroup\$
  • \$\begingroup\$ Add implementation of methods rotate(), stop(), getPosi(), MoveTo0(), so your code can be compiled and easily tested. \$\endgroup\$ – Tomáš Paul Dec 12 '16 at 22:27
3
\$\begingroup\$

I guess you thought about avoiding of Thread.Sleep() method, not Wait() you mentioned in title. I can't test your code because I don't have either COM port nor motor. I would like to give you some general tips how to improve your code. Let's implement some SOLID and DRY principles. I would start with class Motor which will represent your device. You can implement for example singleton class for this purpose. This helps you to separate your motor logic from other logic such is user interface. I can also imagine your port settings as a part of application configuration.

Motor.cs

public sealed class Motor : IDisposable
{
    public static readonly Motor Instance = new Motor();

    private Motor()
    {
        MotorPort = new SerialPort();
        MotorPort.PortName = "COM6";
        MotorPort.Handshake = Handshake.RequestToSend;
        MotorPort.ReceivedBytesThreshold = 8;
        MotorPort.DataReceived += new SerialDataReceivedEventHandler(dataRecievedHandler);
        MotorPort.Open();
    }

    public SerialPort MotorPort { get; private set; }
    public decimal Position { get; private set; }
    public void StopRotate()    { }
    public void StartRotate(string speed) { }
    public void MoveToPosition(int position) { }
    private void dataRecievedHandler(object sender, SerialDataReceivedEventArgs e)
    {
    }
    public void Dispose()
    {
        if (MotorPort.IsOpen)
        {
            MotorPort.Close();
        }
    }
}

Instead of using global fields (M, buf), encapsulate them into object and set the proper access modifier or better create properties. Use meaningful names your fields and properties. Use Capitalization Convention.

You can isolate your repeating strings into separate static class to make your string constant repository which would be more easily maintainable against direct using of particular string.

StringConstants.cs

static class StringConstants
{
    public const string Prefix = "SR,00,002,";
    public const string PrefixCrLf = "SR,00,002\r\n";
}

Extension methods can simplify your code and make it more readable.

Extensions.cs

static class Extensions
{
    public static decimal GetDecimalOutput(this SerialPort port)
    {
        return decimal.Parse(Regex.Split(port.ReadLine(), StringConstants.Prefix)[1]);
    }
    public static void SendCommand(this SerialPort port)
    {
        port.WriteLine(StringConstants.PrefixCrLf);
    }
}

If you have implemented Motor.cs methods, you can use it or extend it.

Form1.cs

private void btnTest_Click(object sender, EventArgs e)
{
    Motor.Instance.StartRotate(speed);
    Thread.Sleep(5000);
    Motor.Instance.StopRotate();
    Motor.Instance.MoveToPosition(0);

    //read sensor
    serialPort2.SendCommand();
    decimal caliber = serialPort2.GetDecimalOutput();

    do
    {
        serialPort2.SendCommand();
        actualValue = serialPort2.GetDecimalOutput();
    }
    while (actualValue <= caliber);
}

I am not experienced in serial port programming, but I am pretty sure that event SerialPort.DataReceived should be used to invoke subscribed method as you do with your serialPort3. I don't know exactly why you are using Thread.Sleep(), because I cannot see getPosi(C); method.

\$\endgroup\$
  • 1
    \$\begingroup\$ mhmm, if the Motor is a singleton then it doesn't make much sense if it's disposable. When the app gets closed the resources will be freed anyway Proper Disposal of Singleton IDisposable Object \$\endgroup\$ – t3chb0t Dec 13 '16 at 9:14
  • \$\begingroup\$ Thank you for this detailed reply. Every commandmethod will make the dataReceived Hanlder fire an answer stored in "buf". If I dont use the Thread.Spleep() after the stop(), the answer from getPosi() will be messed up and "M" won't be saved correctly (Answer is not completley registered when next SerialPort.Write() is already executed, hence next answer is fired). But seems like this can be fixed. I'll work through your answer and improve my coding regarding your tipps. \$\endgroup\$ – Chris Dec 13 '16 at 10:20
  • \$\begingroup\$ @t3chb0t, thanks for your comment and the link Proper Disposal. You are more experienced then me, let's assume you are right. I just wanted to be sure the port does not stay opened, so that I implemented IDisposable. \$\endgroup\$ – Tomáš Paul Dec 13 '16 at 11:28
3
\$\begingroup\$

Just a minor thing that jumped out at me:

private void rotate(byte[] A, string Test) //Rotate Motor Right
{
    A = MB.TMCL_RMR(Test);
    serialPort3.Write(A, 0, A.Length);
}

Why are you passing in a byte array to just replace it with something else on the first line?

private void SendRotateCommand(string speed)
{
    var commandBytes = MB.TMCL_RMR(speed);
    motorPort.Write(commandBytes, 0, commandBytes.Length); 
}

I've guessed that the Test parameter was actually the rotation speed from a comment in the method that calls it.

You need to improve your naming throughout. Your code is very simple but is very difficult to follow simply because the naming is so poor.


Another small thing:

public byte[] GET_POSI()
{
    byte[] E = new byte[9];
    E[0] = 0x1;
    E[1] = 0x6;
    E[2] = 0x1;
    E[3] = 0x0;
    E[4] = 0x0;
    E[5] = 0x0;
    E[6] = 0x0;
    E[7] = 0x0;
    E[8] = 0x08;
    return E;
}

Could be written more simply as:

public byte[] GET_POSI()
{
    return new byte[] { 1, 6, 1, 0, 0, 0, 0, 0, 8 };
}

I can't really understand what you're trying to save the position for. Or even how you're attempting to do it. You are sending messages to the motor and it's sending things back. You get a message, you figure out what it means and act on it.

You're also mixing DataReceived with SerialPort.ReadLine they both use the same underlying memory stream and you should be using one or the other - not both.

You're also freezing your UI for large chunks of time while you wait for operations to complete. It's fine to accept the command from the user and then update the UI as the action progresses. E.g. show the current position as the motor is rotating, when it gets to the right point, stop it.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the reply. I thought that i need to store the new array i get from the method calls in an array of the same sort. Good to know that there is a better way to do that. I'll fix that in my code now. The naming was mentioned in another reply as well. As i work on the programm everyday a bit I keep track of what is what, but i guess I'll take this tip to not fall into any bad habits. Will work on my naming as well. \$\endgroup\$ – Chris Dec 13 '16 at 10:05
  • \$\begingroup\$ @Chris I added a bit more detail in case it's helpful. \$\endgroup\$ – RobH Dec 13 '16 at 11:58
  • 1
    \$\begingroup\$ thanks again =) The byte method looks cleaner this way, does it also work, if I want to write things like 0xe8 as {e8} or 0xff as {ff} ? To be more clear, each byte has a meaning (board number, motor number, command number, 4 bytes that contain value for the command in hex, like speed or position and last byte is checksum). I want to measure distance betwenn point A (Saved Value, Sensor at 0) and Point B (The amount the sensor is pushed in when the motor returns to saved value). I also will keep the ReadLine thing in mind and only use one of them. \$\endgroup\$ – Chris Dec 13 '16 at 12:26
  • \$\begingroup\$ @Chris I changed the hex literals to int literals because the numbers were so small. You could still do new byte[] { 0xFF } if you wanted. \$\endgroup\$ – RobH Dec 13 '16 at 13:06

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.