Fixes and improvements in Wire Protocol (#213)

nanoframework · Feb 6, 2020 · cee7f85 · cee7f85
1 parent ca52d79
commit cee7f85
Show file tree

Hide file tree

Showing 10 changed files with 67 additions and 30 deletions.
diff --git a/source/nanoFramework.Tools.DebugLibrary.Shared/DebuggerEventSource.cs b/source/nanoFramework.Tools.DebugLibrary.Shared/DebuggerEventSource.cs
@@ -20,7 +20,7 @@ internal class DebuggerEventSource : EventSource
         private static readonly Lazy<DebuggerEventSource> Log_ = new Lazy<DebuggerEventSource>(() => new DebuggerEventSource());
 
         [Flags]
-        enum PacketFlags
+        private enum PacketFlags
         {
             None = 0,
             NonCritical = 0x0001, // This doesn't need an acknowledge.
@@ -124,29 +124,34 @@ public static string GetCommandName(uint cmd)
         {
             string retVal;
             if (!CommandNameMap.TryGetValue(cmd, out retVal))
-                retVal = $"0x{cmd:X08}";
+                retVal = $"0x{cmd.ToString("X08")}";
 
             return retVal;
         }
 
         [Event(1, Opcode = EventOpcode.Send)]
         public void WireProtocolTxHeader(uint crcHeader, uint crcData, uint cmd, uint flags, ushort seq, ushort seqReply, uint length)
         {
-            Debug.WriteLine("TX: {0} flags=[{1}] hCRC: 0x{2:X08} pCRC: 0x{3:X08} seq: 0x{4:X04} replySeq: 0x{5:X04} len={6}"
-                                      , GetCommandName(cmd)
-                                      , (PacketFlags)flags
-                                      , crcHeader
-                                      , crcData
-                                      , seq
-                                      , seqReply
-                                      , length
-                                      );
+            Debug.WriteLine($"TX: " +
+                $"{GetCommandName(cmd)} " +
+                $"flags=[{(PacketFlags)flags}] " +
+                $"hCRC: 0x{crcHeader.ToString("X08")} " +
+                $"pCRC: 0x{crcData.ToString("X08")} " +
+                $"seq: 0x{seq.ToString("X04")} " +
+                $"replySeq: 0x{seqReply.ToString("X04")} " +
+                $"len={length}");
         }
 
         [Event(2, Opcode = EventOpcode.Receive)]
         public void WireProtocolRxHeader(uint crcHeader, uint crcData, uint cmd, uint flags, ushort seq, ushort seqReply, uint length)
         {
-            Debug.WriteLine($"RX: {GetCommandName(cmd)} flags=[{crcHeader}] hCRC: 0x{crcHeader.ToString("X08")} pCRC: 0x{crcData.ToString("X08")} seq: 0x{seq.ToString("X04")} replySeq: 0x{seqReply.ToString("X04")} len={length.ToString()}");
+            Debug.WriteLine($"RX: {GetCommandName(cmd)} " +
+                $"flags=[{(PacketFlags)flags}] " +
+                $"hCRC: 0x{crcHeader.ToString("X08")} " +
+                $"pCRC: 0x{crcData.ToString("X08")} " +
+                $"seq: 0x{seq.ToString("X04")} " +
+                $"replySeq: 0x{seqReply.ToString("X04")} " +
+                $"len={length.ToString()}");
         }
 
         [Event(3)]

diff --git a/source/nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/Controller.cs b/source/nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/Controller.cs
@@ -12,9 +12,10 @@ namespace nanoFramework.Tools.Debugger.WireProtocol
 {
     public class Controller : IControllerLocal
     {
-        private int lastOutboundMessage;
-        private Semaphore _sendSemaphore = new Semaphore(1, 1);
-        private int nextEndpointId;
+        private ushort lastOutboundMessage;
+        private readonly Semaphore _sendSemaphore = new Semaphore(1, 1);
+        private readonly int nextEndpointId;
+        private readonly object incrementLock = new object();
 
         public IControllerHostLocal App { get; internal set; }
 
@@ -26,7 +27,7 @@ public Controller(IControllerHostLocal app)
 
             Random random = new Random();
 
-            lastOutboundMessage = random.Next(65536);
+            lastOutboundMessage = ushort.MaxValue;
             nextEndpointId = random.Next(int.MaxValue);
 
             //default capabilities
@@ -119,7 +120,11 @@ public async Task<bool> SendAsync(MessageRaw raw, CancellationToken cancellation
 
         public ushort GetNextSequenceId()
         {
-            return (ushort)Interlocked.Increment(ref lastOutboundMessage);
+            lock (incrementLock)
+            {
+                lastOutboundMessage += 1;
+            }
+            return lastOutboundMessage;
         }
 
         public void StopProcessing()

diff --git a/source/nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/Engine.cs b/source/nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/Engine.cs
@@ -618,7 +618,7 @@ public bool ProcessMessage(IncomingMessage message, bool isReply)
                             cmdReply.m_source = Commands.Monitor_Ping.c_Ping_Source_Host;
                             cmdReply.m_dbg_flags = (StopDebuggerOnConnect ? Commands.Monitor_Ping.c_Ping_DbgFlag_Stop : 0);
 
-                            PerformRequestAsync(new OutgoingMessage(message, _controlller.CreateConverter(), Flags.c_NonCritical, cmdReply), _backgroundProcessorCancellation.Token).ConfigureAwait(false);
+                            PerformRequestAsync(new OutgoingMessage(_controlller.GetNextSequenceId(), message, _controlller.CreateConverter(), Flags.c_NonCritical, cmdReply), _backgroundProcessorCancellation.Token).ConfigureAwait(false);
 
                             return true;
                         }
@@ -911,7 +911,7 @@ private async Task RpcReceiveQuery(IncomingMessage message, Commands.Debugging_M
             reply.m_found = (eep != null) ? 1u : 0u;
             reply.m_addr = addr;
 
-            await PerformRequestAsync(new OutgoingMessage(message, CreateConverter(), Flags.c_NonCritical, reply), _cancellationTokenSource.Token);
+            await PerformRequestAsync(new OutgoingMessage(_controlller.GetNextSequenceId(), message, CreateConverter(), Flags.c_NonCritical, reply), _cancellationTokenSource.Token);
         }
 
         internal bool RpcCheck(Commands.Debugging_Messaging_Address addr)
@@ -1018,7 +1018,7 @@ private async Task RpcReceiveSendAsync(IncomingMessage msg, Commands.Debugging_M
             res.m_found = (eep != null) ? 1u : 0u;
             res.m_addr = addr;
 
-            await PerformRequestAsync(new OutgoingMessage(msg, CreateConverter(), Flags.c_NonCritical, res), _cancellationTokenSource.Token);
+            await PerformRequestAsync(new OutgoingMessage(_controlller.GetNextSequenceId(), msg, CreateConverter(), Flags.c_NonCritical, res), _cancellationTokenSource.Token);
 
             if (eep != null)
             {
@@ -1074,7 +1074,7 @@ private async Task RpcReceiveReplyAsync(IncomingMessage message, Commands.Debugg
             res.m_found = (eep != null) ? 1u : 0u;
             res.m_addr = addr;
 
-            await PerformRequestAsync(new OutgoingMessage(message, CreateConverter(), Flags.c_NonCritical, res), _cancellationTokenSource.Token);
+            await PerformRequestAsync(new OutgoingMessage(_controlller.GetNextSequenceId(), message, CreateConverter(), Flags.c_NonCritical, res), _cancellationTokenSource.Token);
 
             if (eep != null)
             {

diff --git a/source/nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/MessageRaw.cs b/source/nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/MessageRaw.cs
@@ -4,8 +4,11 @@
 // See LICENSE file in the project root for full license information.
 //
 
+using System;
+
 namespace nanoFramework.Tools.Debugger.WireProtocol
 {
+    [Serializable]
     public class MessageRaw
     {
         public byte[] Header;

diff --git a/source/nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/MessageReassembler.cs b/source/nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/MessageReassembler.cs
@@ -248,11 +248,13 @@ private bool VerifyHeader()
             uint crc = _messageBase.Header.CrcHeader;
             bool fRes;
 
-            _messageBase.Header.CrcHeader = 0;
 
             // verify CRC32 only if connected device has reported that it implements it
             if (_parent.App.IsCRC32EnabledForWireProtocol)
             {
+                // compute CRC32 with header CRC32 zeroed
+                _messageBase.Header.CrcHeader = 0;
+
                 fRes = CRC.ComputeCRC(_parent.CreateConverter().Serialize(_messageBase.Header), 0) == crc;
 
                 _messageBase.Header.CrcHeader = crc;

diff --git a/source/nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/OutgoingMessage.cs b/source/nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/OutgoingMessage.cs
@@ -8,7 +8,7 @@ namespace nanoFramework.Tools.Debugger.WireProtocol
 {
     public class OutgoingMessage
     {
-        ushort _sequenceId;
+        readonly ushort _sequenceId;
 
         public MessageRaw Raw { get; private set; }
         public MessageBase Base { get; private set; }
@@ -26,8 +26,10 @@ public OutgoingMessage(ushort sequenceId, Converter converter, uint command, uin
             UpdateCRC(converter);
         }
 
-        internal OutgoingMessage(IncomingMessage request, Converter converter, uint flags, object payload)
+        internal OutgoingMessage(ushort sequenceId, IncomingMessage request, Converter converter, uint flags, object payload)
         {
+            _sequenceId = sequenceId;
+
             InitializeForSend(converter, request.Header.Cmd, flags, payload);
 
             Base.Header.SeqReply = request.Header.Seq;

diff --git a/source/nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/Packet.cs b/source/nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/Packet.cs
@@ -9,13 +9,17 @@
 
 namespace nanoFramework.Tools.Debugger.WireProtocol
 {
+    [Serializable]
     public class Packet
     {
-        public static string MARKER_DEBUGGER_V1 = "NFDBGV1\0"; // Used to identify the debugger at boot time.
-        public static string MARKER_PACKET_V1 = "NFPKTV1\0"; // Used to identify the start of a packet.
+#pragma warning disable S1104 // Fields should not have public accessibility
+        // this is required because of the way the de-serialization works in Wire Protocol
+
+        public readonly static string MARKER_DEBUGGER_V1 = "NFDBGV1\0"; // Used to identify the debugger at boot time.
+        public readonly static string MARKER_PACKET_V1 = "NFPKTV1\0"; // Used to identify the start of a packet.
         public const int SIZE_OF_MARKER = 8;
 
-        public byte[] Marker;
+        public byte[] Marker = Encoding.UTF8.GetBytes(MARKER_PACKET_V1);
         public uint CrcHeader = 0;
         public uint CrcData = 0;
 
@@ -25,9 +29,11 @@ public class Packet
         public uint Flags = 0;
         public uint Size = 0;
 
+#pragma warning restore S1104 // Fields should not have public accessibility
+
         public Packet()
         {
-            Marker = Encoding.UTF8.GetBytes(MARKER_PACKET_V1);
+
         }
     }
 }
diff --git a/source/nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/WireProtocolRequestsStore.cs b/source/nanoFramework.Tools.DebugLibrary.Shared/WireProtocol/WireProtocolRequestsStore.cs
@@ -13,7 +13,16 @@ public void Add(WireProtocolRequest request)
         {
             lock (_requestsLock)
             {
-                _requests.Add(new Tuple<uint, ushort>(request.OutgoingMessage.Header.Cmd, request.OutgoingMessage.Header.Seq), request);
+                // it's wise to check if this key is already on the dictionary
+                // can't use TryAdd because that's only available on the UWP API
+                var newKey = new Tuple<uint, ushort>(request.OutgoingMessage.Header.Cmd, request.OutgoingMessage.Header.Seq);
+                if (_requests.ContainsKey(newKey))
+                {
+                    // remove the last one, before adding this
+                    _requests.Remove(newKey);
+                }
+
+                _requests.Add(newKey, request);
             }
         }
 

diff --git a/source/nanoFramework.Tools.Debugger.sln b/source/nanoFramework.Tools.Debugger.sln
@@ -19,6 +19,11 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Serial Test App WPF", "USB
 EndProject
 Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "nanoFramework.Tools.DebugLibrary.UWP", "nanoFramework.Tools.DebugLibrary.UWP\nanoFramework.Tools.DebugLibrary.UWP.csproj", "{21E5A6A5-F4F7-411A-80DC-F49E62D8A7C3}"
 EndProject
+Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution Items", "{C4E43092-51E8-46C3-8C7A-FD05E9A29FE2}"
+	ProjectSection(SolutionItems) = preProject
+		version.json = version.json
+	EndProjectSection
+EndProject
 Global
 	GlobalSection(SharedMSBuildProjectFiles) = preSolution
 		nanoFramework.Tools.DebugLibrary.Shared\nanoFramework.Tools.DebugLibrary.Net.projitems*{101d57ad-d22f-4905-a992-de15e723f164}*SharedItemsImports = 4

diff --git a/source/version.json b/source/version.json
@@ -1,6 +1,6 @@
 {
   "$schema": "https://raw.githubusercontent.com/AArnott/Nerdbank.GitVersioning/master/src/NerdBank.GitVersioning/version.schema.json",
-  "version": "1.5.0-preview.{height}",
+  "version": "1.6.0-preview.{height}",
   "buildNumberOffset": 10,
   "assemblyVersion": {
     "precision": "revision"