C# - Message Based Communication Design

See more:

C#

patterns

message

Hi there,

This is a long long long post... I'm afraid that I cannot really make it shorter, so please be indulgent with me :)

Well, I'm trying to design an API.NET for communication with Testing Equipment and I cannot figure out a proper way for designing the architecture.

Basically, we have different (physical) products:

- A Controller to perform measurement on channel inputs (e.g. Current, Voltage, Frequency,
Digital, Relay, etc.) and generate some signals on its outputs (AWG, Voltage, Frequency, Digital, etc.) and some typical automotive buses: CAN, LIN and K-LINE.

- A Unit, able to deal with the power supply and to embed several controllers.

- An Extension to perform some temperature measurements and bring, it's connected to a Unit or alone.

The fact is the embedded team (who is charge of designing all the dirty stuff over the hardware) gave me the commands and acknowledgments are not really clear, neither user-friendly, and for some legacy reasons they're not gonna change that...

Mainly cause prior to that situation we used to have a software dealing directly with the commands and it was a huge huge huge mess, aka baked noodles. Since I decided to gather all the communication stuff into assembly dedicated to that matter. It allows btw to make instrument drivers (for NI LabVIEW) by wrapping the objects and methods of the assembly.

I'm pretty proud of that cause it allows our systems to be used in many many different Software platforms (e.g. Matlab) and in our own software platforms as well. I also manage myself to make our assembly Mono compliant.

The protocol as it has been defined it not that extensible (not necessarily there is a need to make it extensible but I feel sometimes that it could be better).

Unit Command: {@}{UnitId}{XX}{_}{UnitMessageKeyword}{=Parameters}(if any){;}
Unit Acknowledgment: {#}{UnitId}{XX}{_}{UnitMessageKeyword}{=Answer}(if any){;}

Controller Command: {@}{ParentUnitId}{ControllerId}{_}{ControllerMessageKeyword}{=Parameters}(if any){;}
Controller Acknowledgment: {#}{ParentUnitId}{ControllerId}{_}{ControllerMessageKeyword}{=Answers}(if any){;}

Extension Command: {@}{ExtensionId}{XX}{_}{ExtensionMessageKeyword}{=Parameters}(if any){;}
Extension Acknowledgment: {@}{ExtensionId}{XX}{_}{ExtensionMessageKeyword}{=Parameters}(if any){;}

Controller Instead to provide a dummy interface with the command to the end users, I decided to gather messages and acknowledgment into things what really the user needs through properties, it's more natural as it really represents the Unit:

- Property (Category) Power Supply => Enabled { get; set; } / CurrentMax { set; get; } / Current { get; } / Voltage { get; set; }
- Property (category) Information => SubCategory Identifiers { get; } / SubCategory OperatingTimes { get; } / etc.
- Method On() // Turn On...
- Method Off() // Turn Off...
- Method Lock() // Remote Control Only
- Method Unlock() // Allow Manual Mode
- ReadOnlyKeyedCollection Controllers { get; }
- Etc.

Lock() / Unlock() methods could be gathered as a Boolean Property Locked { get; set; }
The same for On() / Off() methods into Powered / On { get; set; } or whatsoever.

The fact is that we are making new products, some of them are based on the previous one: like a Controller which is out of the unit for the customers who do not need a big power supply.

For some reasons a Message Keyword does not necessarily match a Command Implementation in the assembly, sometimes the possibilities for one keywords are simply too many (and can lead to have a lot of Nullables as properties of the message), therefore when it's possible a keyword can be used in different command implementations when the concerns are properly separated.

Message => Object
- Acknowledgment (Token Reception: Start / Stop) => Message
- AcknowledgmentUnit => Acknowledgment
- AcknowledgmentController => Acknowledgment
- AcknowledgmentExtension => Acknowledgment

- Command (Token Transmission: Start / Stop) => Message
- CommandUnit => Command
- CommandController => Command
- CommandExtension => Command

And here is start my real problems, some might think that the context above is not really useful but I believe that knowing the context helps to have proper answers.

I have several problems but I'm not really sure that they can be solved.

- Overuse of generics, and some with "circular" definitions... [http://blogs.msdn.com/b/ericlippert/archive/2011/02/03/curiouser-and-curiouser.aspx][1]
[1]: http://blogs.msdn.com/b/ericlippert/archive/2011/02/03/curiouser-and-curiouser.aspx

- I have some virtual calls in base Constructors... yeah not good either...

- and some generics lead for some convenience stuff to generate automatically the properties give in generics via a private setter and reflection, then only the public getter is available with public modifier.

- A lack of interface use, mostly relying on abstracts...

Sometimes C# is not powerful enough to express all the constraints I would like to set and avoid any mistakes for further maintenance and improving this assembly.


I'm gonna paste only the parts presenting issues and introduce the design flaws:
Let's start with Message Class, with the virtual calls in the constructor:

public abstract class Message<TDeviceId, TDeviceMessageKeyword>
        where TDeviceId: struct, IComparable, IFormattable, IConvertible // Enum "Constraint"
        where TDeviceMessageKeyword: struct, IComparable, IFormattable, IConvertible // Enum "Constraint"
    {
        internal abstract String Header { get; }
        internal abstract String Content { get; }

        public abstract TDeviceId Id { get; }
        public abstract TDeviceMessageKeyword MessageKeyword { get; }

        private const String _separatorIdKeyword = "_";
        protected String SeparatorIdKeyword
        {
            get
            {
                return Message<TDeviceId, TDeviceMessageKeyword>._separatorIdKeyword;
            }
        }

        private const String _separatorHeaderContent = "=";
        protected String SeparatorHeaderContent
        {
            get
            {
                return Message<TDeviceId, TDeviceMessageKeyword>._separatorHeaderContent;
            }
        }

        private const String _flagStop = ";";
        protected virtual String FlagStop
        {
            get
            {
                return Message<TDeviceId, TDeviceMessageKeyword>._flagStop;
            }
        }

        protected abstract String FlagStart { get; }

        public abstract CommunicantPacket Packet { get; }
    }

I'm a little bit embarassed cause I could use interface to avoid with this kind of "virtual calls in the constructor" warnings but that's not really the purpose of an interface....

public abstract class Command<TDeviceId, TDeviceMessageKeyword> : Message<TDeviceId, TDeviceMessageKeyword>
        where TDeviceId: struct, IComparable, IFormattable, IConvertible
        where TDeviceMessageKeyword: struct, IComparable, IFormattable, IConvertible
    {
        protected Command(TDeviceId id)
        {
            var idByte = Convert.ToByte(id);
            var idByteStringHex = BitConverter.ToString (new [] { idByte });

            this._id = id;
            this._idByteStringHex = idByteStringHex;
        }

        private readonly String _idByteStringHex ;
        protected String IdByteStringHex
        {
            get
            {
                return this._idByteStringHex;
            }
        }

        private const String _flagStart = "@";
        protected override sealed String FlagStart
        {
            get
            {
                return Command<TDeviceId, TDeviceMessageKeyword>._flagStart;
            }
        }

        private readonly TDeviceId _id;
        public override TDeviceId Id
        {
            get
            {
                return this._id;
            }
        }

        private CommunicantPacket _packet;
        public sealed override CommunicantPacket Packet
        {
            get
            {
                if (this._packet == null)
                {
                    // Analysis disable once ConvertIfStatementToConditionalTernaryExpression
                    if (String.IsNullOrEmpty (this.Content))
                    {
                        this._packet = new CommunicantPacket (this.FlagStart + this.Header + this.FlagStop);
                    }
                    else
                    {
                        this._packet = new CommunicantPacket (this.FlagStart + this.Header + this.SeparatorHeaderContent + this.Content + this.FlagStop);
                    }
                }

                return this._packet;
            }
        }
    }

    public abstract class Command<TDeviceId, TDeviceKeyword, TAcknowledgment> : Command<TDeviceId, TDeviceKeyword>
        where TDeviceId: struct, IComparable, IFormattable, IConvertible
        where TDeviceKeyword: struct, IComparable, IFormattable, IConvertible
        where TAcknowledgment : Acknowledgment<TDeviceId, TDeviceKeyword>
    {
        protected Command(TDeviceId id)
            : base(id)
        {
        }

        // TODO: Optimization... Caching and stuff, using Expression Trees
        internal TAcknowledgment GetAcknowledgment (CommunicantPacket communicantPacket)
        {
            try
            {
                var typeAcknowledgment = typeof(TAcknowledgment);
                const BindingFlags bindingFlags = BindingFlags.Instance | BindingFlags.NonPublic;
                var arguments = new Object[] { communicantPacket, this };
                var acknowledgment = Activator.CreateInstance (typeAcknowledgment, bindingFlags, null, arguments, null) as TAcknowledgment;

                return acknowledgment;
            }
            catch (Exception exception)
            {
                if (exception.InnerException != null)
                {
                    throw exception.InnerException;
                }
                else
                {
                    throw;
                }
            }
        }
    }


public abstract class Acknowledgment<TDeviceId, TDeviceMessageKeyword> : Message<TDeviceId, TDeviceMessageKeyword>
        where TDeviceId: struct, IComparable, IFormattable, IConvertible
        where TDeviceMessageKeyword: struct, IComparable, IFormattable, IConvertible
    {
        protected Acknowledgment(CommunicantPacket acknowledgmentPacket)
            : base()
        {
            if (String.IsNullOrEmpty(acknowledgmentPacket.Data.String))
            {
                var message = @"The argument ""acknowledgmentPacket"" cannot be null or empty.";
                throw new ArgumentNullException ("acknowledgmentPacket", message);
            }
            else
            {
                const String pattern = @"^(?<FlagStart>{0})(?<Header>{1})((?<SeparatorHeaderContent>{2})(?<Content>{3}))?(?<FlagStop>{4})";
                var patternApplied = String.Format (pattern, this.FlagStart, RegexProxy.AnyOnceOrMoreButEqual, this.SeparatorHeaderContent, RegexProxy.AnyOnceOrMore, this.FlagStop);

                var regex = new Regex (patternApplied);

                var match = regex.Match (acknowledgmentPacket.Data.String);

                if (match.Success)
                {
                    this._header = match.Groups ["Header"].Value;
                    this._content = match.Groups ["Content"].Value;
                    this._packet = acknowledgmentPacket;
                }
                else
                {
                    throw new FormatException(acknowledgmentPacket.Data.String);
                }
            }
        }

        private readonly String _header;
        internal override String Header
        {
            get
            {
                return this._header;
            }
        }

        private readonly String _content;
        internal override String Content
        {
            get
            {
                return this._content;
            }
        }

        private const String _flagStart = "#";
        protected override sealed String FlagStart
        {
            get
            {
                return Acknowledgment<TDeviceId, TDeviceMessageKeyword>._flagStart;
            }
        }

        private readonly CommunicantPacket _packet;
        public sealed override CommunicantPacket Packet
        {
            get
            {
                return this._packet;
            }
        }
    }

    public abstract class Acknowledgment<TDeviceId, TDeviceKeyword, TCommand> : Acknowledgment<TDeviceId, TDeviceKeyword>
        where TDeviceId: struct, IComparable, IFormattable, IConvertible
        where TDeviceKeyword: struct, IComparable, IFormattable, IConvertible
        where TCommand : Command<TDeviceId, TDeviceKeyword>
    {
        private void VirtualCallProcessContent (TCommand relatedCommand, String content)
        {
            this.ProcessContent (relatedCommand, content);
        }

        protected Acknowledgment(CommunicantPacket acknowledgmentPacket, TCommand relatedCommand)
            : base(acknowledgmentPacket)
        {
            if (this.Header != relatedCommand.Header)
            {
                String message = String.Format (@"The Command and Acknowledgment Headers, respectively @""{0}"" and ""{1}"", are not matching.", this.Header, relatedCommand.Header);
                throw new FormatException (message);
            }
            else
            {
                this._id = relatedCommand.Id;
                this._keyword = relatedCommand.MessageKeyword;
                this._relatedCommand = relatedCommand;

                this.VirtualCallProcessContent (relatedCommand, this.Content);
            }
        }

        protected abstract void ProcessContent (TCommand relatedCommand, String content);

        private readonly TDeviceKeyword _keyword;
        public override TDeviceKeyword MessageKeyword
        {
            get
            {
                return this._keyword;
            }
        }

        private readonly TDeviceId _id;
        public override TDeviceId Id
        {
            get
            {
                return this._id;
            }
        }

        private readonly TCommand _relatedCommand;
        public TCommand RelatedCommand
        {
            get
            {
                return this._relatedCommand;
            }
        }
    }

Then we have the internal TAcknowledgment GetAcknowledgment (CommunicantPacket communicantPacket) done with Reflection to avoid the user to have to redefine it in every inherited implementions, apart that this constructor calls could be cached to run nearly as fast as compiled instructions (thanks to compiled expressions), I feel like something smells here.

Also an abstract method that is also called in the constructor... which btw does not help
but enforces the fact this method needs to be implemented for each command inherited.

This is mostly all about the message design flaws...

The other big design flaws are related to the classes representing the physical Devices.

public abstract class Device : Disposer, IDevice
{
    protected Device(Byte id)
    {
        this._id = id;
    }

    private readonly Byte _id;

    #region IDevice Implementation
    Byte IDevice.Id
    {
        get
        {
            return this._id;
        }
    }
    #endregion
}

// TODO: Check possible improvements using Laziness...
    public abstract class Device<TDeviceId, TDeviceInformation> : Device
        where TDeviceId : IComparable, IConvertible, IFormattable
        where TDeviceInformation : IDeviceInformation
{
    protected Device(TDeviceId id)
        : base(Convert.ToByte(id))
    {
        this._id = id;

        var typeDeviceInformation = typeof(TDeviceInformation);

        const BindingFlags bindingFlags = BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance;

        this._information = (TDeviceInformation)Activator.CreateInstance (typeDeviceInformation, bindingFlags, null, new Object[] { this }, null);
    }

    private readonly TDeviceId _id;
    public TDeviceId Id
    {
        get
        {
            return this._id;
        }
    }

    private readonly TDeviceInformation _information;
    public TDeviceInformation Information
    {
        get
        {
            return this._information;
        }
    }
}


public interface IDeviceCategory
{
}

public abstract class DeviceCategory<TDevice>
    where TDevice : class, IDevice
{
    internal DeviceCategory (TDevice parentDevice)
    {

    }

    private readonly TDevice _parentDevice;
    internal TDevice ParentDevice
    {
        get
        {
            return this._parentDevice;
        }
    }
}

public interface IDeviceInformation : IDeviceCategory
{
}

public abstract class DeviceInformation<TDevice> : DeviceCategory<TDevice>, IDeviceInformation
    where TDevice : class, IDevice
{
    internal DeviceInformation (TDevice parentDevice)
        : base (parentDevice)
    {

    }
}

public abstract class DeviceCategorySub<TParentDeviceCategory>
    where TParentDeviceCategory : class, IDeviceCategory
{
    internal DeviceCategorySub (TParentDeviceCategory parentDeviceCategory)
    {
        this._parentDeviceCategory = parentDeviceCategory;
    }

    private readonly TParentDeviceCategory _parentDeviceCategory;
    protected TParentDeviceCategory ParentDeviceCategory
    {
        get
        {
            return this._parentDeviceCategory;
        }
    }
}

Here I didnt find any proper way to enforce properly constraints so I have those useless interfaces instead... and still it does not enforce constraints enough...

I dunno if there something any other implementations that would make my code more elegant... and at the same time keeping its conveniency...