Binary protocol variability

Question

Please note: An updated solution is here

---

Let’s imagine that we designed more or less universal firmware for a microcontroller unit to be used across multiple types of devices. So we have something common in the binary data exchange protocol, and something different.

The most common way to define incoming command parsing is usually about switching over one huge enum with all the command discriminators (command selection codes), so system knows what command to read. The problem is enum + switch are difficult to maintain and almost impossible to distribute code between helper library, shared controller, and specific device related projects.

Proposed solution has been divided into three libraries where we can redefine protocol specifics.

namespace Company.Hardware

First of all, classes to map commands to discriminators:

public class Commands
{
 public static readonly Commands None = new Commands();
 protected Commands()
 {
 }
 public virtual Type this[int discriminator]
 {
 get { throw new NotSupportedException(); }
 }
 public Commands Include<TCommand>(int discriminator) =>
 new Commands<TCommand>(discriminator, this);
}
class Commands<TCommand> : Commands
{
 public Commands(int discriminator, Commands next)
 {
 Discriminator = discriminator;
 Next = next;
 }
 public override Type this[int discriminator] =>
 discriminator == Discriminator ? 
 typeof(TCommand) : 
 Next[discriminator];
 int Discriminator { get; }
 Commands Next { get; }
}

Mapping schema might be defined as:

Commands Commands = Commands.None
 .Include<VersionCommand>(10),
 .Include<ReadyCommand>(20);

We can get command Type back:

Type type = Commands[20];

Next class allows to read and log low level primitives of the protocol; command components like discriminator itself. We will inherit it later to define more specific versions of the protocol. It is basically a tokenizer.

public abstract class ProtocolReader : IDisposable
{
 protected ProtocolReader(BinaryReader reader, TextWriter log)
 {
 Reader = reader;
 Log = log ?? TextWriter.Null;
 }
 public void Dispose()
 {
 Reader.Dispose();
 Log.Dispose();
 }
 public Type ReadDiscriminator() =>
 Commands[
 Write("discriminator={0}", 
 Reader.ReadInt32())];
 protected T Write<T>(string format, T value)
 {
 Log.WriteLine(format, value);
 return value;
 }
 protected virtual Commands Commands => Commands.None;
 protected BinaryReader Reader { get; }
 protected TextWriter Log { get; } 
}

Now a façade to read commands:

public class CommandReader : IDisposable
{
 public CommandReader(ProtocolReader reader)
 {
 Reader = reader; 
 }
 public void Dispose() => Reader.Dispose();
 public dynamic Read()
 {
 try
 {
 var type = Reader.ReadDiscriminator();
 return Activator.CreateInstance(type, Reader);
 }
 catch (ObjectDisposedException)
 {
 return null;
 }
 catch(Exception)
 {
 throw;
 }
 }
 ProtocolReader Reader { get; }
}

namespace Company.Hardware.Controller

Let’s define more types of primitives and a command to read:

public abstract class ControllerReader : ProtocolReader
{
 public ControllerReader(BinaryReader reader, TextWriter log)
 : base(reader, log)
 {
 }
 protected override Commands Commands => base.Commands
 .Include<VersionCommand>(1);
 public Version ReadVersion() =>
 Write("version={0}",
 new Version(
 Reader.ReadInt32(),
 Reader.ReadInt32()));
}

The only responsibility of the command is to rehydrate itself properly:

public class VersionCommand
{
 public VersionCommand(ControllerReader reader)
 {
 Hardware = reader.ReadVersion();
 Firmware = reader.ReadVersion();
 }
 public Version Hardware { get; }
 public Version Firmware { get; }
}

namespace Company.Hardware.Controller.Device

Here the specifics come. Let’s define one more command.

public class ReadyCommand 
{
 public ReadyCommand(DeviceReader reader)
 {
 Status = reader.ReadStatus();
 }
 public string Status { get; }
}

And a final version of protocol reader:

public class DeviceReader : ControllerReader
{
 public DeviceReader(BinaryReader reader, TextWriter log)
 : base(reader, log)
 {
 }
 protected override Commands Commands => base.Commands
 .Include<ReadyCommand>(2);
 public string ReadStatus() =>
 Write("status={0}", 
 Reader.ReadString());
}

Demo

 static void Main(string[] args)
 {
 using (var inputStream = FakeInputDataStream())
 using (var binaryReader = new BinaryReader(inputStream))
 using (var protocolReader = new DeviceReader(binaryReader, Console.Out))
 using (var reader = new CommandReader(protocolReader))
 while(true)
 {
 var command = reader.Read();
 if (command != null)
 Handle(command);
 else
 break;
 }
 }
 static void Handle(VersionCommand command) =>
 Console.WriteLine("VERSION " + command.Hardware);
 static void Handle(ReadyCommand command) => 
 Console.WriteLine("READY " + command.Status);

• Vogel612: Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers .

Answer 1

public class CommandReader

A public class with a public constructor which is acting on a passed in argument (here ProtocolReader reader) should check if the used argument is null before any call to Dispose() of that object.

Answer 2

I dont like the method ProtocolReader.Write that logs a value and return them. IMHO that makes the code harder to read.

For instance compare:

public Version ReadVersion() =>
 Write("version={0}",
 new Version(
 Reader.ReadInt32(),
 Reader.ReadInt32()));
// ...
public Type ReadDiscriminator() =>
 Commands[
 Write("discriminator={0}", 
 Reader.ReadInt32())];

with

public Version ReadVersion()
{
 var major = Reader.ReadInt32();
 var minor = Reader.ReadInt32();
 var version = new Version(major, minor);
 Log("version={0}", version);
 return version;
}
// ...
public Type ReadDiscriminator()
{
 var discriminator = Reader.ReadInt32();
 Log("discriminator={0}", discriminator);
 return Commands[discriminator];
}

---

In general the code looks very abstract for me. I am not able to appraise if that is reasonable because I don't know the complexity of the use case...

For example
The ControllerReader knows how to read a Version and the VersionCommand gets the ControllerReader and calls the method to read the version. So, if a new command will be added, is it required to create a new command and add a method to the ControllerReader?

If that code is used like a framework I would expect, that extending new (or modifying existing) commands should be possible by adding (or modifing) just one class.

• Dmitry Nogin: 1) Could you please share any resources suggesting this approach? Microsoft recommends taking ownership; double disposing is guaranteed to be safe. 2) Thank you, agree. The best thing to be done is a decorator, but it means even more typing. 3) Many commands are sharing body parts, like rotatation-milidegree or depth-mm, so ProtocolReader descendants API will be reused to ensure consistent deserialization and logging.
• JanDotNet: 1) It was just my understanding... However, I didn't find any resources for that suggestion... Furthermore the StreamReader closes (and therfore diposes) the stream passed to it's constructor and MS suggests: "Allow a Dispose method to be called more than once without throwing an exception. The method should do nothing after the first call.". Therefore your implementation seems to be valid and the first part of my answer not. I'll remove it.
• Dmitry Nogin: Thanks! As for 3) I have just updated VersionCommand to look more closer to the end result. I think it makes more sense now.
• Dmitry Nogin: We could think about ProtocolReader as a tokenizer.

Answer 3

A couple of comments:

1. Your approach to "mapping" is rather unconventional IMHO. If you simply used a Dictionary<int, Type>, you would have much less lines of code, a more performant mapper (O(1) for dictionaries, vs O(N) for your linked list), and its functionality would be plain obvious to anyone. Or even a simple list containing commands, with a oneliner like Commands.FirstOrDefault(c => c.Discr == discr) would do. Right now, the Commands class basically implements a linked list in a convoluted way. It feels like LISP-like cdr in action, but the indexer gives a false assumption of a dictionary-like structure, or at least a random access collection. It's not idiomatic, especially with all the existing BCL structures at your disposal: Dictionary<T,K>, or anything implementing IEnumerable, or even a LinkedList.

2. I would prefer logging inside ProtocolReader to be done through an actual logging library (i.e. log4net). Passing a TextWriter limits numerous logging possibilities you get with a hierarchical logger like log4net. Even better, separate your parsers to share a generic interface so that you can log the output at a single place for all of them.

3. ProtocolReader.Write also doesn't feel right. If it was named DumpToLogAndReturn, then it would at least immediately hint at its purpose, but it's nevertheless at the wrong place. You can impose a "rule" to your implementors to always pass the value through Write, but this is something that should not be a responsibility for this class; its job should be to parse and be done with it (with any self-contained debug logging if needed).

4. The fact that ProtocolReader does not implement any interfaces, and is marked abstract but has no abstract members is also strange. Comparing it to other classes, you can notice that many of them follow a similar pattern, yet allow no generalization: you have a ReadDiscriminator, ReadStatus, ReadVersion, all of which are non-virtual, strongly named, differently named methods.

   They are all very tightly coupled in both directions:

   • A VersionCommand must specifically use ControllerReader.ReadVersion. At the same time, ControllerReader must specifically include a VersionCommand in its Commands list.

   • A ReadyCommand must specifically use DeviceReader.ReadStatus. At the same time, DeviceReader must specifically include a ReadyCommand in its Commands list.

5. Also, VersionCommand and ReadyCommand are responsible for parsing, while they should rather be plain POCOs. You cannot even instantiate any of them unless you provide a working reader. And since ReadStatus and ReadVersion in these readers, are really thin methods, which don't really do much, it seems like a more general way to parse a Command would simply be:

   // generic parser
   interface IParser<T> 
   {
    T Parse(BinaryReader writer);
   }
   // poco result 
   public class VersionCommand
   {
    public VersionCommand(Version hw, Version fw)
    { Hardware = hw; Firmware = fw; }
    public Version Hardware { get; }
    public Version Firmware { get; }
   }
   // actual implementation
   public VersionCommandParser : IParser<VersionCommand>
   {
    public VersionCommand Parse(BinaryReader reader)
    {
    var hw = new Version(reader.ReadInt32(), reader.ReadInt32());
    var fw = new Version(reader.ReadInt32(), reader.ReadInt32());
    return new VersionCommand(hw, fw);
    }
   }
   

   This is interesting for several reasons:

   a) parser interface is generic, meaning you can wrap easily wrap it generically. E.g. a LoggingParser<T> might be a wrapper around any IParser<T>, and dump the parsed value to the log, just like your Write method needs to do in every method now.

   b) serialization is now completely independent of the message and the message itself is a POCO.

   Additionally, I would suggest adding some error handling to the parsers, meaning the interface would be something like:

   // generic parser
   interface IParser<T> 
   {
    IResult<T> Parse(BinaryReader writer);
   }
   // actual implementation
   public VersionCommandParser : IParser<VersionCommand>
   {
    public IResult<VersionCommand> Parse(BinaryReader reader)
    {
    if (reader.Length < 4 * 4)
    return Result<VersionCommand>.NotEnoughData();
    var hw = new Version(reader.ReadInt32(), reader.ReadInt32());
    var fw = new Version(reader.ReadInt32(), reader.ReadInt32());
    var result = new VersionCommand(hw, fw);
    return Result<VersionCommand>.Success(result);
    }
   }
   

Your final goal should be to completely separate parsing, parsed values, and consumers of there values, into something like:

// pseudo code, but you should get the idea
// (generic parameter is redundant due to type inference of course)
Parsers.Register<VersionCommand>(10, new VersionCommandParser());
Parsers.Register<ReadyCommand>(20, new ReadyCommandParser());
Consumers.Add<VersionCommand>(new VersionCommandConsumer());
Consumers.Add<VersionCommand>(new SomeOtherVersionConsumer());
Consumers.Add<VersionCommand>(v => DoStuff(v));
Consumers.Add<ReadyCommand>(new ReadyCommandConsumer());

And then all you need to do is "wire" them:

using (var binaryReader = new BinaryReader(inputStream))
{
 var result = Parsers
 .Select(p => p.Parse(binaryReader))
 .FirstOrDefault(r => r.Type == ResultType.Success);
 if (result == null || result.Type != ResultType.Success)
 continue;
 Log.Info("Parsed: " + result);
 var consumer = Consumers
 .FindByType(result.Value)
 .Consume(result.Value);
}

• vgru: If you have time, check this answer also. It's not exactly the same type of parsiing, but it suggest some ideas which you might find useful and apply to your own problem.
• Dmitry Nogin: Thank you for detailed analysis! 1 I strongly prefer immutable structures over mutable; I would say that one day we will all consider mutability as a kind of technical debt (discovering Scala these days – started feeling good for humanity :) 2 Yes, I agree. Actually, I implemented IMyLog through Console.Out so many times, that just became lazy.
• Dmitry Nogin: 3 Totally agree. I need a LoggingReader decorator over IProtocolReader to follow SRP. Unfortunately, class API is going to be wide, so implementing + maintaining extra interface and decorator is about to be painful... OK, let’s purify here.
• Dmitry Nogin: 4 I do not see an issue in having abstract base classes without abstract or virtual methods where it is relevant. Why do you consider it as a strange thing? Another question – this name matching between Version/ReadVersion. Sorry for the wrongly chosen set of commands to include in the post. Usually they all reuse shared elements like ErrorCode, Timestamp, Cookie, Channel. There is no one to one matching here. I will add an answer with a better example below.
• Dmitry Nogin: 5 Oh, I am going to have 80+ of commands, so having the amount of classes being doubled (or tripled – introducing consumers) is probably not the most reasonable thing to do in this context. I actually have an extra ctor with simple state injection for unit testing on all commands. a It is important to log every byte read, not the command as a whole. b Allowing classes to rehydrate themselves is just a reasonable tradeoff because of high amount of classes. They just have no other code but for rehydration, so it is not so bad. There is no way to check Length on network stream.