jcurl / RJCP.DLL.SerialPortStream

SerialPortStream is an independent implementation of System.IO.Ports.SerialPort and SerialStream for better reliability and maintainability. Default branch is 2.x and now has support for Mono with help of a C library.
Microsoft Public License
631 stars 197 forks source link

Wrong data gets send when sending more data than WriteBufferSize in chunks #104

Closed romerod closed 4 years ago

romerod commented 4 years ago

If I send data over the serial port in chunks of 250 bytes by specifying offset and count and the whole amount of data is larger then the default WriteBufferSize, wrong data is sent over the serial port.

Only the chunks which contain start (offset) before a multiple of the buffer size and the end (offset + count) after a multiple of the buffer size are wrong. In other words:

serialPort.Write(data, 0x01FFB8, 250) <= sends wrong the data

If only that part is send data gets sent correctly (not in a loop).

Workarounds:

I have this behavior with a USB to serial cable and I'm also able to reproduce it with com0com.

I've create a small linqpad script which reproduces it on com0com with on serialports COM25 and COM26.

com0com version: com0com-2.2.2.0-x64-fre-signed.zip from https://sourceforge.net/projects/com0com/files/com0com/2.2.2.0/ serialPortStreamTest.zip

jcurl commented 4 years ago

You're absolutely right. It occurs when the internal buffer boundaries are exceeded and the offset to write is non-zero. It occurs in CircularBuffer.cs in the method Append and the code needs to be:

if (count <= WriteLength) {
    ...
} else {
    count = Math.Min(Free, count);
    System.Array.Copy(array, offset, m_Array, End, WriteLength);
    System.Array.Copy(array, WriteLength + offset, m_Array, 0, count - WriteLength);
    Produce(count);
    return count;
}

the original line is:

    System.Array.Copy(array, WriteLength, m_Array, 0, count - WriteLength);

So when it realizes it's got to the end of the buffer, it writes the end part correctly, but then it copies the remaining data from the wrong position in the source array.

romerod commented 4 years ago

Thanks for looking into it that fast, are you going to publish a new version? I can test it if you want.

jcurl commented 4 years ago

I could look into it fast because you had a test case that I could rewrite using NUnit to investigate and single step. So Kudos for a very well written bug report.

I'll see what I can do, but my fulltime job has to take priority. I hope that I can have something in about two weeks, as I'd like to do some real world tests too, and will submit when I've got a couple unit test cases that checks this.

In the mean time, I've identified the problem, mentioned what needs to change, so you have a workaround until then.

romerod commented 4 years ago

Thank you, I'll test the change so you have already one real world test.

romerod commented 4 years ago

Successfully tested 2228614d36ee0542513dd87ab69f2485338d6b7a on a device

jcurl commented 4 years ago

Released in v2.2.1 on NuGet

romerod commented 4 years ago

The nuget package shows depencendencies for 4.6.2, should I open a new issue?

The empty group "<group targetFramework=".NETFramework4.6.2" />" is missing.

2.2.0 package:

<dependencies>
      <group targetFramework=".NETFramework4.6.2" />
      <group targetFramework=".NETStandard1.5">
        <dependency id="System.Threading.Overlapped" version="4.3.0" />
        <dependency id="System.Threading.Thread" version="4.3.0" />

2.2.1 package:

<dependencies>
      <group targetFramework=".NETStandard1.5">
        <dependency id="System.Threading.Overlapped" version="4.3.0" />
        <dependency id="System.Threading.Thread" version="4.3.0" />
jcurl commented 4 years ago

That's strange. I used NuGet 3.5.0 for packaging. The source .nuspec file never had the empty dependency. I look at the sources on the tag release/2.2.0.0 and it didn't have the empty target either. I don't remember what nuget.exe I used for the packaging back then.

What's the side effect of the missing 4.6.2? Does it pull in unexpected dependencies? If so, I should do a rerelease, in which case I'll open a new ticket, so I can reference it in the commit history.

Retested with NuGet 5.11 and it didn't add the empty 4.6.2 dependency either (but complained there was a missing net40 and net45 dependency).