1
\$\begingroup\$

First of all: I'm completely new to C# and therefore I am not quite sure about the garbage collector. I try to implement the USS Protocol by Siemens. I tried it this way:

Ready file, byte per byte and pass it to the parsing function USS.RX()

USS.RX(byte ucByte): Message Start -> Should be Startbyte 0x02. If true create a new UssMsg object. Fill the UssMsg object with address, length, function, data and the BCC. If BCC is correct call a user specified event function with the created UssMsg object.

As I switched over from C my inner me says not to do that, as a new object is allocated and then sent over to an event function and then will possibly never been destroyed and creating a huge memory leak? Especially when I start to multitask this?

I would be very thankful for any constructive critic.

Example .USS file: 2016-04-30.rar

Form1.cs:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using System.IO;


namespace SGAtoUSS
{
    public partial class Form1 : Form
    {
        private UInt64 ui64count = 0;
        public Form1()
        {

            byte[] ucData = new byte[1];
            USS.USS_Res ussRes;
            USS oUSS = new USS();
            //Setup UI
            InitializeComponent();
            //Set up new Message Handler
            oUSS.OnNewMessage += new USS.MessageHandler(USSOnNewMsg);
            //Open File
            FileStream USSFile = new FileStream(@"w:\TEMPFEDA\2017-01-16.uss", FileMode.Open);
            //Read and parse file
            while (USSFile.Read(ucData, 0, 1) > 0)
                if ((ussRes = oUSS.RX(ucData)) != USS.USS_Res.OK)
                    Console.WriteLine("ErroR " + ussRes.ToString());


            Console.WriteLine("Done. {0} messages count ", ui64count);
        }
        void USSOnNewMsg(UssMsg ussMsg)
        {
            ui64count++;
            //Console.WriteLine("New Message: " + ussMsg.Function + " Len: " + ussMsg.Length + " BCC: " + ussMsg.BCC);
            //Console.WriteLine("Data: " + BitConverter.ToString(ussMsg.Data));
            //while (true) { }
        }
    }
}

USS.cs:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;



namespace SGAtoUSS
{
    public class UssMsg : EventArgs
    {
        public byte Length;
        public byte Address;
        public byte Function;
        public byte[] Data;
        public byte BCC;
        public UssMsg()
        {
        }
    }
    class USS
    {
        private const int USS_POS_START = 0;
        private const int USS_POS_Length = 1;
        private const int USS_POS_ADDRESS = 2;
        private const int USS_POS_FUNCTION = 3;
        private const int USS_POS_DATASTART = 4;
        private const int USS_STX = 0x02;

        private enum USS_State
        {
            Start = 1,
            GetLength,
            GetAddress,
            GetFunction,
            GetData
        };

        public enum USS_Res
        {
            OK = 0x00,
            ErrorStart,
            ErrorBCC,
            ErrorUnDef = 0xff
        }
        private byte posBuffRx = 0;


        USS_State ucRxState;
        UssMsg newMsg;

        public delegate void MessageHandler(UssMsg ussMsg);
        public event MessageHandler OnNewMessage;

        public USS()
        {
            posBuffRx = 0;
            ucRxState = USS_State.Start;
        }

        public USS_Res RX(byte ucRxByte)
        {
            USS_Res retVal = USS_Res.OK;
            //switch the states where the current message is right now
            switch (ucRxState)
            {
                case USS_State.Start:
                    //if recved byte is not start byte (0x02) throw exception
                    if (ucRxByte != USS_STX)
                        return USS_Res.ErrorStart;
                    //throw new System.InvalidCastException("Start byte wrong");

                    //otherwise start procedure
                    ucRxState = USS_State.GetLength;
                    posBuffRx = 0;
                    break;
                case USS_State.GetLength:
                    //Create a new UssMsg object
                    newMsg = new UssMsg();

                    newMsg.Length = ucRxByte;
                    //...with a new Data byte array with the length of the message minus the header and the BCC (footer)
                    newMsg.Data = new byte[newMsg.Length - 3];

                    ucRxState = USS_State.GetAddress;
                    break;
                case USS_State.GetAddress:
                    newMsg.Address = ucRxByte;
                    //Address
                    ucRxState = USS_State.GetFunction;
                    break;
                case USS_State.GetFunction:
                    newMsg.Function = ucRxByte;
                    //Function
                    ucRxState = USS_State.GetData;
                    break;
                case USS_State.GetData:
                    if (posBuffRx == newMsg.Length - 3)
                    { //message complete
                        newMsg.BCC = ucRxByte;

                        //TODO: checking BCC will come here
                        OnNewMessage(newMsg);
                        //retVal = USS_Res.NewMsg;
                        //else
                        //  retVal = USS_Res.ErrorBCC; //else return BCC Error

                        //start over next time
                        ucRxState = USS_State.Start;
                    }
                    else { newMsg.Data[posBuffRx++] = ucRxByte; }
                    break;
            }
            return retVal;
        }

        public USS_Res RX(byte[] ucRxBytes)
        {
            USS_Res retVal = USS_Res.OK;
            foreach (byte ucRxByte in ucRxBytes)
            {
                retVal = RX(ucRxByte);
            }
            return retVal;

        }

        private bool BCC(byte[] ucRxBytes)
        {
            return true;//TODO
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

Pass by value

C# works a bit differently as plain C when passing objects to a function.

In C, object types are passed by value by default. That means, a shallow copy will be performed on your objects. If you pass a pointer or a reference, you will be able to access and edit the values of the original object.

In C#, object types are also passed by value by default. However, if you pass a class as parameter, you will be able to edit values within that particular object (basically the same scenario, when you pass a pointer in C) since the object reference gets passed. If you want your object to be immutable, you will need to implement the ICloneable interface w/ constructor copying in order to clone it.

GC

In C, if you allocate a chunk of memory, no one will alert you later to delete it and you will leak memory. You will need to get rid of it at some point.

However, C# has Garbage Collector at its disposal. I will not go into details, there are countless excellent articles about GC. Fundamentally, GC tracks the valid references of your objects and if there are not any left, GC will free the allocated memory space. But, there is no guarantee that eg. your local variables will be immediately freed after exiting the scope (in C, local variables will be freed upon exiting the scope). When GC comes across, it will delete the unused objects and until then they exist as nothing has ever happened. You can force GC to get rid of your object by calling the Dispose() method or using the using statement that destroys the object after exiting its scope.

Your code

  • I would suggest you used a BinaryReader instead of a FileStream. It provides a more elegant style with its methods like ReadByte() or ReadInt32(). There is an example on MSDN how to use it with a file input. With this, you could get rid of your single-item byte array declared in the constructor.
  • All your processing methods in USS could be reworked with the BinaryReader to get rid of arrays.
  • You shouldn't put any logic into the constructor of your Form. Put it in a separate method and call it. You can also consider using the FormLoaded method to be a bit cleaner, but since your logic already goes after InitializeComponent, it shouldn't make a big difference.
  • Your const variables in the USS class are unused.
  • I might be wrong, but it would be a good idea to stop your code by breaking the while cycle whenever your RX function returns an error.
  • In your RX(byte[] ucRxBytes) function, the foreach loop could hide errors that occur. Let's review the following scenario: we have an array of 3 bytes. We process the first byte, it returns with OK and it gets put into the retVal variable. Then, we process the second byte and returns with an ERROR. It also gets put into the retVal variable. Finally, we process the third byte and it returns with OK. Guess what, the OK signal gets written to the retVal variable and will be returned to your main logic. The error has vanished..
  • It is a positive aspect that you use comments. Just the bare things that are mandatory to understand the logic, but nothing more.
  • You could consider making your USS public, but since the Form and USS are in the same namespace, it shouldn't matter.
  • If you wish to implement this solution in a multi-threaded environment, make sure that both the worker and GUI thread are synchronised. GUI thread is only accessible via the Invoke/BeginInvoke methods, otherwise an exception gets thrown.
  • If the world of C you may have gotten used to working with threads. It's okay to use them in C# too, but the Task async implementation is worth taking a look at. Task is really just "a promise of a result in the future" while Thread is a lower concept. I would recommend that you use the higher level abstraction wherever you can.

Keep on codin', C# is a wonderful language.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for this professional answer, I'll keep everything you mentioned in my mind! I'm very satisfied with C# so far. One question to your answer: you are right with the error checking, the Form function was quick and dirty. But as C# uses exceptions for error handling should I also throw an exception when I code routines by myself? Or should I use return values as like as the code above? \$\endgroup\$ – Max R. Apr 2 '17 at 11:47

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.