5
\$\begingroup\$

I have created a DLL library that controls an robotic arm. Could you tell me if my design is correct? The robot communicates over serial port. The code works but I would like to receive a feedback regarding the design as I don't have enough experience.

Robot (uArm) is moving cards from one bin to another. I have the following classes:

  • uArm
  • uArmCommands (and IuArmCommands)
  • SerialPortConnector
  • MyConfig

and a .config file.

uArm

/// <summary>
    /// Library to control the robotic arm made by evol.net with firmware made by HID
    /// </summary>
    public class uArm
    {
        #region Logging
        private static readonly log4net.ILog log = log4net.LogManager.GetLogger(typeof(uArm));
        #endregion

        private static bool hasCard = false;
        private MyConfig myConfig;
        private IuArmCommands myCommands;

        public uArm()
        {
            InitLogging();
            myConfig = new MyConfig();
            myCommands = new uArmCommands(myConfig.COMPort);
            log.Info("Creating a new uArm Robot");
        }

        /// <summary>
        /// Method checks if the provided bin is empty or not
        /// </summary>
        /// <param name="PositionID">PositionID of the bin to check</param>
        /// <returns>If the bin has at least 1 card returns true</returns>
        public Boolean BinNotEmpty(String PositionID)
        {
            return myConfig.BinNotEmpty(PositionID);
        }


        /// <summary>
        /// Method that moves the arm to the specified bin and takes one card.
        /// </summary>
        /// <param name="PositionID">Name of the bin from the dll config file</param>
        /// <returns>true if success</returns>
        public bool TakeCardFromBin(String PositionID)
        {

            bool result = false;
            // checking if the robot doesn't have any card already and if the bin is not empty
            try
            {
                if (!hasCard && myConfig.positionSection.Positions.getByPositionID(PositionID).Cards > 0)
                {
                    result = GoToPosition(myConfig.positionSection.Positions.getByPositionID(PositionID));
                    if (result)
                        result = myCommands.runPump();
                    if (result)
                    {
                        hasCard = true;
                        myConfig.positionSection.Positions.getByPositionID(PositionID).Cards--;
                        myConfig.SaveConfig();
                    }
                }
            }
            catch(Exception)
            {
                log.Error("Robot couldn't take a card from a bin");
                throw new Exception("Robot couldn't take a card from a bin");
            }

            //go down
            return result;
        }


        /// <summary>
        /// Method that moves the arm to the specified bin nad releases the card.
        /// </summary>
        /// <param name="PositionID">Name of the bin from the dll config file</param>
        /// <returns>true if success</returns>
        public bool ReturnCardToBin(String PositionID)
        {
            bool result = false;
            // checking if the robot really has a card.
            if (hasCard)
            {
                try
                {
                    result = GoToPosition(myConfig.positionSection.Positions.getByPositionID(PositionID));
                    if (result)
                        result = myCommands.offPump();
                    if (result)
                    {
                        hasCard = false;
                        myConfig.positionSection.Positions.getByPositionID(PositionID).Cards++;
                        myConfig.SaveConfig();
                    }
                }
                catch(Exception)
                {
                    log.Error("Robot couldn't return a card.");
                    throw new Exception("Robot couldn't return a card.");
                }
                //go down
            }
            else
            {
                log.Error("Impossible to return a card because the robot doesn't have any card.");
                throw new Exception("Impossible to return a card because the robot doesn't have any card.");
            }
            return result;
        }

        private bool GoToPosition(PositionElement position)
        {
            return myCommands.setPosition(position.Stretch, position.Rotation, position.Height);
        }

        ~uArm()
        {
            Dispose();
        }

        public void Dispose()
        {
            myCommands.offPump();
            myCommands.reset();
            GC.SuppressFinalize(this);
        }

    }

IuArmCommands

interface IuArmCommands
{
    bool getPos();
    bool offPump();
    bool pinch();
    bool releasePinch();
    bool reset();
    bool runBeeper();
    bool runPump();
    bool setHeight(int value);
    bool setPosition(int stretch, int rotation, int height);
    bool setRotation(int value);
    bool setStretch(int value);
    bool setVelocity(int value);
    bool setVelocityRot(int value);
    bool setVelocityWrist(int value);
    bool setWrist(int value);
    bool toggleDebug();
}

uArmCommands

public class uArmCommands : IuArmCommands
    {
        private SerialPortConnector mySerialPortConnector;
        public uArmCommands(String COMPort)
        {
            mySerialPortConnector  = new SerialPortConnector(COMPort);
        }

        /// <summary>
        /// Method that combines all the necessary movements to go to x,y,z position
        /// </summary>
        /// <param name="stretch"></param>
        /// <param name="rotation"></param>
        /// <param name="height"></param>
        /// <returns></returns>
        public bool setPosition(int stretch, int rotation, int height)
        {
            bool result = false;
            result = setStretch(stretch);
            if(result)
                setRotation(rotation);
            if(result)
                setHeight(height);
            return result;
        }

        /// <summary>
        /// Set Stretch Position
        /// </summary>
        /// <param name="value">0 to 210</param>
        /// <returns></returns>
        public bool setStretch(int value)
        {
            if (rangeCheck(0, 210, value))
                return mySerialPortConnector.Write("S" + value);
            else
                throw new ArgumentException("Value outside of range.");
        }

        /// <summary>
        /// Set Height Position
        /// </summary>
        /// <param name="value">150(highest) to -180(lowest)</param>
        /// <returns></returns>
        public bool setHeight(int value)
        {
            if (rangeCheck(-180, 150, value))
                return mySerialPortConnector.Write("H" + value);
            else
                throw new ArgumentException("Value outside of range.");
        }

        /// <summary>
        /// Set Rotation Position
        /// </summary>
        /// <param name="value">-90 to 90</param>
        /// <returns></returns>
        public bool setRotation(int value)
        {
            if (rangeCheck(-90, 90, value))
                return mySerialPortConnector.Write("R" + value);
            else
                throw new ArgumentException("Value outside of range.");
        }

        /// <summary>
        /// Set the wrist twist
        /// </summary>
        /// <param name="value">-90 to 90</param>
        /// <returns></returns>
        public bool setWrist(int value)
        {
            if (rangeCheck(-90, 90, value))
                return mySerialPortConnector.Write("W" + value);
            else
                throw new ArgumentException("Value outside of range.");
        }

        public bool reset()
        {
            return mySerialPortConnector.Write("!");
        }

        public bool getPos()
        {
            return mySerialPortConnector.Write("?");
        }

        public bool runPump()
        {
            return mySerialPortConnector.Write("P");
        }

        public bool offPump()
        {
            return mySerialPortConnector.Write("O");
        }

        /// <summary>
        /// Checks if the parameter is inside a specified range
        /// </summary>
        /// <param name="start"></param>
        /// <param name="stop"></param>
        /// <param name="value"></param>
        /// <returns></returns>
        private bool rangeCheck(int start, int stop, int value)
        {
            if (start <= value && stop >= value)
                return true;
            else
                return false;
        }
    }

SerialPortConnector

private static readonly log4net.ILog log = log4net.LogManager.GetLogger(typeof(SerialPortConnector));
        private String _COMPort;
        private SerialPort _serialPort1;
        static string serialBuffer = "";
        static string expectedEcho = null;
        static object expectedEchoLock = new object();
        static ManualResetEvent expectedEchoReceived = new ManualResetEvent(false);

        public SerialPortConnector(String ComPort)
        {
            _COMPort = ComPort;
            OpenPort(_COMPort);
        }

        ~SerialPortConnector()
        {
            ClosePort();
        }

        /// <summary>
        /// Setting connection parameters and opening serial port.
        /// </summary>
        /// <param name="COM"></param>
        public void OpenPort(String COM)
        {
            try
            {
                _serialPort1 = new SerialPort();
                // setting serial port connection parameters
                _serialPort1.WriteTimeout = 1000;
                _serialPort1.ReadTimeout = 1000;
                _serialPort1.NewLine = "\r\n";
                _serialPort1.PortName = COM;
                _serialPort1.BaudRate = 115200;
                _serialPort1.DataBits = 8;
                _serialPort1.Parity = (Parity)Enum.Parse(typeof(Parity), "None");
                _serialPort1.StopBits = (StopBits)Enum.Parse(typeof(StopBits), "1");
                _serialPort1.DataReceived += new SerialDataReceivedEventHandler(port_DataReceived);
                _serialPort1.Open();
            }
            catch (Exception)
            {
                log.Error("Unable to open serial port: " + _COMPort);
                throw new ApplicationException("Unable to open serial port.");
            }
        }

        /// <summary>
        /// Method that reads from serial port.
        /// </summary>
        /// <param name="sender"></param>
        /// <param name="e"></param>
        public void port_DataReceived(object sender, SerialDataReceivedEventArgs e)
        {
            while (_serialPort1.BytesToRead > 0)
            {
                byte[] buffer = new byte[_serialPort1.BytesToRead];
                int bytesRead = _serialPort1.Read(buffer, 0, buffer.Length);
                if (bytesRead <= 0) return;
                serialBuffer += Encoding.UTF8.GetString(buffer, 0, bytesRead);
                string[] lines = serialBuffer.Split('\r', '\n');
                // Don't process the last part, because it's not terminated yet
                for (int i = 0; i < (lines.Length - 1); i++)
                {
                    if (lines[i].Length > 0)
                        ProcessLine(lines[i]);
                }
                serialBuffer = lines[lines.Length - 1]; // keep last part
            }
        }

        /// <summary>
        /// Method to precess serial port message
        /// </summary>
        /// <param name="line"></param>
        public void ProcessLine(string line)
        {

            bool unsolicitedMessageReceived = false;
            lock (expectedEchoLock)
            {
                if (line == expectedEcho)
                {
                    expectedEchoReceived.Set();
                }
                else
                {
                    unsolicitedMessageReceived = true;
                }
            }
            if (unsolicitedMessageReceived)
            {
                // Process unsolicited/unexpected messages
            }
        }

        /// <summary>
        /// Writing to serial port.
        /// After sending a message a lock is made and another thread awaits for the reply.
        /// </summary>
        /// <param name="input"></param>
        /// <returns></returns>
        public bool Write(string input)
        {
            if (!_serialPort1.IsOpen)
            {
                // if the serial port was closed we try to open it again, 
                // if the open method fails it will throw an exception
                OpenPort(_COMPort);
            }

            string command = string.Empty;
            command = input;
            command = DelSpace(command);

            lock (expectedEchoLock)
            {
                expectedEchoReceived.Reset();
                expectedEcho = "K";
            }

            SendAsHex(command);
            return expectedEchoReceived.WaitOne(5000); // timeout after 5 seconds

        }

        private byte[] GetBytes(string str)
        {
            byte[] bytes = new byte[str.Length * sizeof(char)];
            System.Buffer.BlockCopy(str.ToCharArray(), 0, bytes, 0, bytes.Length);
            return bytes;
        }

        private void SendAsHex(string str)
        {
            _serialPort1.Write(GetBytes(str), 0, GetBytes(str).Length);
        }

        private string DelSpace(string str)
        {
            string TempStr = string.Empty;
            int Len = str.Length;
            for (int i = 0; i < Len; i++)
            {
                if (str[i] != ' ')
                    TempStr += str[i];
            }
            Len = TempStr.Length;
            if (Len % 2 != 0)
                TempStr = '0' + TempStr;
            return TempStr;
        }

        public void ClosePort()
        {
            _serialPort1.Close();
        }
    }

Config file

<?xml version="1.0" encoding="utf-8" ?>
<configuration>
  <configSections>
    <section name="MyConfigSection" type="Robots.uArm.MyConfigSection, Robots.uArm" requirePermission="false"/>
  </configSections>
  <appSettings>
    <add key ="COMPort" value="COM3"/>
  </appSettings>
  <MyConfigSection>
    <Positions>
      <Position PositionID="bin1" Type="bin" Stretch="100" Rotation="-50" Height="100" Twist="0" Cards="10" />
      <Position PositionID="bin2" Type="bin" Stretch="100" Rotation="50" Height="100" Twist="0" Cards="0"/>
    </Positions>
  </MyConfigSection>
</configuration>

Example of use

uArm myRobot = new uArm();
while (myRobot.BinNotEmpty("bin1"))
{
    myRobot.TakeCardFromBin("bin1");
    myRobot.ReturnCardToBin("bin2");
}
myRobot.Dispose();
\$\endgroup\$
0

1 Answer 1

1
\$\begingroup\$

Naming

Base on the naming guidelines methods should be named using PascalCase casing. That being said, the IuArmCommands interface should look like so

interface IuArmCommands
{
    bool GetPos();
    bool OffPump();
    bool Pinch();
    bool ReleasePinch();
    bool Reset();
    bool RunBeeper();
    bool RunPump();
    bool SetHeight(int value);
    bool SetPosition(int stretch, int rotation, int height);
    bool SetRotation(int value);
    bool SetStretch(int value);
    bool SetVelocity(int value);
    bool SetVelocityRot(int value);
    bool SetVelocityWrist(int value);
    bool SetWrist(int value);
    bool ToggleDebug();
}

One of the important aspects in naming is to be consistent with your naming style. You are mixing a lot of styles throughout your classes.

Some examples of class variables

private IuArmCommands myCommands;
private String _COMPort;
private SerialPort _serialPort1;

For class level variables you are using 3 different styles. You use camelCase casing, underscore + PascalCase casing and underscore + camelCase casing.

You really should stick to one style only.

The same is true for variables you use in your methods.

Some examples of method variables

bool unsolicitedMessageReceived = false;
string TempStr = string.Empty;  

again you are mixing PascalCase casing (which shouldn't be used for variables) and camelCase casing.

Based on the same naming guidelines method parameters should be named using camelCase casing, which your parameters do, but unfortunately not always.

Naming is a pretty hard field in our job, but doing it correctly will make the code easier to maintain and read. This doesn't mean that the C# naming guideline should be the bible for naming, but it helps a lot to have some kind of naming guidelines which then should be mandatory.

Also the names of methods and variables should be well choosen, so Bob the maintainer can see at first glance what they are about. This involves also that one shouldn't use abbreviations like GetPos() which would be better named like GetArmPosition() or GetPosition(). This is also true if you have a method named SetPosition() so Bob would expect a GetPosition() rather than a GetPos().

Speaking about GetPos() I think it is rather strange that this method returns a bool.

I also don't understand why you have prefixed all the classes with u like uArm. What does the u stand for. A more descriptive name could be RobotArm which again tells Bob what the class is about.


This

throw new ArgumentException("Value outside of range.");

should be changed to throw the correct exception. Do you know that in C# you can throw an ArgumentOutOfRangeException ?


A construct like

if (condition)
{
    return true;
}
else
{
    return false;
}  

can be simplified like

return condition;  

So your rangeCheck() method

private bool rangeCheck(int start, int stop, int value)
{
    if (start <= value && stop >= value)
        return true;
    else
        return false;
}

should look like so ( see also the PascalCase casing for the method name)

private bool RangeCheck(int start, int stop, int value)
{
    return start <= value && stop >= value;
} 

You have a lot of magic numbers in your code. These numbers should be extracted to meaningful constants.

A well named constant will tell you in a month or in a year, if you look at your code again, more than e.g the number itself. Also for Bob it isn't obvious what the numbers in e.g

public bool setStretch(int value)
{
    if (rangeCheck(0, 210, value))
        return mySerialPortConnector.Write("S" + value);
    else
        throw new ArgumentException("Value outside of range.");
}

mean.

Speaking about this method, I need to tell you that the xml documentation

    /// <summary>
    /// Set Stretch Position
    /// </summary>
    /// <param name="value">0 to 210</param>
    /// <returns></returns>  

tells one more than the method name, which isn't that good. Naming this method SetStretchPosition() would have been better.


I would like to encourage you to always use barces {} also if it is only a single statement for an if or else. Using braces will make your code less error prone.


\$\endgroup\$
2
  • \$\begingroup\$ I am using the u prefix (uArm) as it is the commercial name of the Robot but you are right that it is messy. I will apply all of your suggestions. Besides the naming, the object oriented design looks ok? Would it be better to make uArm to inherit from uArmCommands? I want all the methods from the interface IuArmCommands to be accessible from the library. \$\endgroup\$
    – Michal
    Aug 4, 2015 at 13:24
  • \$\begingroup\$ Basically its only a style review. About the library part you need to decide what your Arm should do. Right now the methods only use the IuArmCommands and I guess that is good if you want only to move the cards. You need to be aware about consequences if you want to alter the behaviour. I would like to encourage you to sit down with a pen and paper designing your library by hand first. Think about possibilities / extensions you may want to be included. Think about problems which could occur. \$\endgroup\$
    – Heslacher
    Aug 4, 2015 at 13:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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