Bugs in GadgeteerCore source (Serial42)

Archive
1

I found 2 bugs in the serial port code from the latest gadgeteer souce (gadgeteer-32422).
I was wondering what was the best way to report these.

  1. The Serial42/Serial.cs class has duplicate registrations to Interface.LineReceived
    https://gadgeteer.codeplex.com/SourceControl/latest#Main/GadgeteerCore/Libraries/Core/Serial42/Serial.cs
    I believe the registration of the Interface.LineReceived event was meant to be handled in the AutoReadLineEnabled method (using the value of _autoReadLineEnabled to track if it is currently registered).
    However, inside:
 public event LineReceivedEventHandler LineReceived

, it also registers the Serial class with Interface.LineReceived when it adds the first Event handler.
Bug- this code using the Serial class results in doubled events:


     serialPort.AutoReadLineEnabled = true;
     serialPort.LineReceived += serialPort_LineReceived;

the LineReceived Eventr’s add/remove blocks in Serial.cs should let AutoReadLineEnable handle the Interface.LineRecieved registration:

add
  {
    if (value == null)
      return;
    if (_lineReceivedHandler == null)
    {
        AutoReadLineEnabled = true;  // this method handles Interface.LineRecieved registration
    }
    _lineReceivedHandler += value;
  }
  1. Socket.SocketIntefaces.Serial.cs ReadLineThread is broken because of an apparent bug in the StringBuilder class. I tracked down some serial port issues and discovered that StringBuilder.ToString(0,i) is not returning the correct substring. I modified the class as follows:

 if (newLineFound)
 {
   string line = outcome.ToString(lastStart, i - lastStart);
   string lineFixed = outcome.ToString().Substring(lastStart,i-lastStart);
   DebugPrint("Error - Stringbuilder substring "+line+", not "+lineFixed);
   RaiseLineReceived(lineFixed);

   lastStart = i + this.newLine.Length;
   i = lastStart - 1;
 }

This works, and I get errors like: (these are reproducible, but random from I can tell)


Error - Stringbuilder substring #VER"NV-I8G FWv2 not #VER"NV-I8G FWv2.66 HWv0"
Error - Stringbuilder substring #SCFG1,ENABLE1,NAME"iMusic",GAIN not #SCFG1,ENABLE1,NAME"iMusic",GAIN0,NUVONET0,SHORTNAME"iM"
Error - Stringbuilder substring #ZCFG1,ENABLE1,NAME"North Master",SLAVETO0,GROUP0,SOURCES27,XRC0 not #ZCFG1,ENABLE1,NAME"North Master",SLAVETO0,GROUP0,SOURCES27,XRC0,IR2,DND0,LO#OK

A bug like this in StringBuilder is pretty serious. I need to double check my StringBuilder usage now.
It is picking up System.Text.StringBuilder from

#region Assembly mscorlib.dll, v4.2.0.0
// C:\Program Files (x86)\Microsoft .NET Micro Framework\v4.2\Assemblies\le\mscorlib.dll
 #endregion

Runtime version: v4.0.30319
C:\Program Files (x86)\Microsoft .NET Micro Framework\v4.2\Assemblies\le\mscorlib.dll
Created: ‎Monday, ‎October ‎01, ‎2012, ‏‎10:36:20 AM
Size: 83.5 KB (85,504 bytes)

I’m still tracking down why that last message was “#ZCFG1,ENABLE1,NAME"North Master”,SLAVETO0,GROUP0,SOURCES27,XRC0,IR2,DND0,LO#OK" when the device apparently sent #ZCFG1,ENABLE1,NAME"North Master",SLAVETO0,GROUP0,SOURCES27,XRC0,IR2,DND0,LOCKED0#OK … so I still have more issues.

2

@ eolson - The best way to report bugs in the .NET Gadgeteer Core is on the Issues tab of the Codeplex project:

http://gadgeteer.codeplex.com/workitem/list/basic

StringBuilder is part of the underlying .NET Micro Framework. Such issues should be reported to this Codeplex project:

http://netmf.codeplex.com/workitem/list/basic

Thanks for the diligent analysis and reporting of the bugs!

Cuno

3

stringbuilder.replace has a bug since 4.2 and somehow it snuck into in 4.3 as well…

Cheers…

4

@ eolson - It looks like a patch was submitted and accepted in 2011 for a similar StringBuilder.ToString(int,int) issue. See patch ID 10623 here: https://netmf.codeplex.com/SourceControl/list/patches?page=1

However, I can’t find the specific call described in the patch in the 4.2 or 4.3 code.

There are many possibilities:

  1. The code was changed after the patch was incorporated and a new bug was introduced
  2. The patch was accepted but never merged into the NETMF source tree
  3. The patch was accepted and merged into a branch of the NETMF source tree, but never made it back to the main branch

Thanks for logging a new issue on codeplex.

-Danny

5

It looks like it may be a similar root cause (grabbing data from the wrong chunk), bit the notes are pretty specific about fixing the replace function when the data grows. I just hope somebody takes a look, since it is a pretty fundamental function. Fortunately, there is a work-around that is not too bad (let the string class get the substring):
s.ToString(i,n) => s.ToString().Substring(i,n)

6

What’s interesting is that the implementation of StringBuilder.ToString(int, int) in NETMF is virtually identical to the implementation in .NET 4.5 except for the one line that copies the result into the output string. (and superficial differences such as pinning memory, but these are negligible to the algorithm)

It makes me wonder how heavily StringBuilder.ToString(int, int) is actually used and whether the memory savings (of not having to generate the entire string before extracting the substring) justifies the much more complex implementation.