How to simplify or refactor this

Question

2.00/5 (2 votes)

See more:

I have this code for sending commands and values over serial, but I need to expand it for 100+ motors.
Is there a better way?
This is my code so far:

C#

private void sendBbutton_Click(object sender, EventArgs e)
        {
            if (!comport.IsOpen) OpenPort();
            comport.DiscardInBuffer();
            sendList = new List<byte[]>();
            index = 0;
            // Communication string send: <stx><command><group><device><value><etx>
            //                            <02> <FF>     <FF>   <FF>    <FFFF> <03>
            // Byte                       <1>  <2>      <3>    <4>     <5>    <6>
            // stx = start 
            // command move = 1, home=2
            // group 1 = motor 1 & 2, 2 = motor , all = FF
            // device 1 = first controler, 2 = second, all = FF;
            // etx end  
            if (!string.IsNullOrEmpty(motor1textBox.Text) && !string.IsNullOrEmpty(motor2textBox.Text)
                && !string.IsNullOrEmpty(motor3textBox.Text))
            {
                short motor1Value = Convert.ToInt16(motor1textBox.Text);
                short motor2Value = Convert.ToInt16(motor2textBox.Text);
                short motor3Value = Convert.ToInt16(motor3textBox.Text);
                sendList = new List<byte[]>();
                byte[] motor1Byte = new byte[7];
                byte[] motor2Byte = new byte[7];
                byte[] motor3Byte = new byte[7];
                motor1Byte[0] = stx;
                motor2Byte[0] = stx;
                motor3Byte[0] = stx;
                motor1Byte[1] = 1;
                motor2Byte[1] = 1;
                motor3Byte[1] = 1;
                motor1Byte[2] = 1;
                motor2Byte[2] = 2;
                motor3Byte[2] = 3;
                motor1Byte[3] = 1;
                motor2Byte[3] = 1;
                motor3Byte[3] = 2;
                motor1Byte[4] = ConvertIntToByteArray(motor1Value)[0];
                motor2Byte[4] = ConvertIntToByteArray(motor2Value)[0];
                motor3Byte[4] = ConvertIntToByteArray(motor3Value)[0];
                motor1Byte[5] = ConvertIntToByteArray(motor1Value)[1];
                motor2Byte[5] = ConvertIntToByteArray(motor2Value)[1];
                motor3Byte[5] = ConvertIntToByteArray(motor3Value)[1];
                motor1Byte[6] = etx;
                motor2Byte[6] = etx;
                motor3Byte[6] = etx;
                sendList.Add(motor1Byte);
                sendList.Add(motor2Byte);
                sendList.Add(motor3Byte);

                sendData(index);


            }
        }

Thanks,
Groover

Posted 7-Jun-13 8:59am

GrooverFromHolland

Add a Solution

2 solutions

Solution 2

Here is my solution, still have to test it with the receiving controler, but in debug it looks OK;

C#

private void sendBbutton_Click(object sender, EventArgs e)
        {
            if (!comport.IsOpen) OpenPort();
            comport.DiscardInBuffer();
            sendList = new List<byte[]>();
            index = 0;
            // Communication string send: <stx> <command> <group> <device> <value> <etx>
            //                            <02>  <ff>      <ff>    <ff>     <ffff>  <03>
            // Byte                       <[0]> <[1]>     <[2]>   <[3]>    <[4][5]><[6] >
            // stx = start 
            // command move = 1, home = 2, move 1 cw = 3, move 1 acw = 4, move many cw = 5,
            //         move many cw = 5, move many acw = 5,
            // group 1 = motor 1 & 2, 2 = motor , all = FF
            // device 1 = first controler, 2 = second, all = FF;
            // etx end  
            if (!string.IsNullOrEmpty(motor1textBox.Text) && !string.IsNullOrEmpty(motor2textBox.Text)
                && !string.IsNullOrEmpty(motor3textBox.Text))
            {
                motorValuesList[0] = Convert.ToInt16(motor1textBox.Text);
                motorValuesList[1] = Convert.ToInt16(motor2textBox.Text);
                motorValuesList[2] = Convert.ToInt16(motor3textBox.Text);
                for (int i = 3; i < motorValuesList.Count; i++)      // for testing only!
                {
                    motorValuesList[i] = motorValuesList[2];
                }
                sendList = new List<byte[]>();
                for (int motor = 0; motor < motorList.Count; motor++)
                {
                    motorList[motor][0] = stx;
                    motorList[motor][1] = 1;
                    motorList[motor][2] = (byte)motor;
                    motorList[motor][3] = 1;
                    motorList[motor][4] = ConvertIntToByteArray(motorValuesList[motor])[0];
                    motorList[motor][5] = ConvertIntToByteArray(motorValuesList[motor])[1];
                    motorList[motor][6] = etx;
                }
                for (int motorNr = 0; motorNr < motorList.Count; ++motorNr)
                {
                    sendList.Add(motorList[motorNr]);
                }

                sendData(index);
            }
        }
</ffff></ff></ff></ff></etx></value></device></group></command></stx>

Posted 7-Jun-13 22:07pm

GrooverFromHolland

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

Sergey Alexandrovich Kryukov · Accepted Answer · 2013-06-07T09:28:00

Solution 1

At least have an array or collection of motors, instead of using individual variables, which is really ugly. For example:

C#

byte[][] motorData;

//..

motorData = new byte[][motorCount];
motorData = new byte[motorCount][];
for (int motor = 0; motor < motorCount; ++motor)
    motorData[motor] = new byte[ /* ... */ ]; // this way, you could have individual sizes of data for different motors

If all motors have the same size of data, it's simpler:

C#

byte[,] motorData;

//...

motorData = new byte[motorCount, dataLength];
// ...

Very similar approach would be with collections, of course, generic. Please see http://msdn.microsoft.com/en-us/library/system.collections.generic.aspx[^].

—SA

Posted 7-Jun-13 9:28am

Sergey Alexandrovich Kryukov

Updated 7-Jun-13 10:41am

v2

Comments

GrooverFromHolland 7-Jun-13 15:43pm

Thank You Sergey,
I started with this for testing and it works, but for the real thing I am not using textboxes but values come from a database.
I will try with motor aray and valuearray.

Sergey Alexandrovich Kryukov 7-Jun-13 16:20pm

Very good. I did not mention any text boxes, by the way.
OK, will you accept the answer formally then (green button)?
—SA

GrooverFromHolland 8-Jun-13 3:59am

a +5 and solution accepted!
I'll post what I made of it and if You want please comment.

Thanks,

Groover.

Sergey Alexandrovich Kryukov 8-Jun-13 20:39pm

Sure. You are welcome.
—SA

Maciej Los 7-Jun-13 16:25pm

+5!

Sergey Alexandrovich Kryukov 7-Jun-13 16:28pm

Thank you, Maciej.
—SA