SerialPort - initial async implementation on Linux and bugfixes by krwq · Pull Request #33027 · dotnet/corefx

krwq · 2018-10-25T00:45:40Z

adds async implementation on Linux
- supports cancellation token
- multiple writes/reads at the same time will work correctly and be processed in order the writes were requested (reads and writes separately)
multiple random bug fixes
test fixes

Most interesting file is SerialStream.Unix.cs

There are still lots of failures, all of them are marked with custom [KnownFailure] - most of the tests won't run on CI because it doesn't have physical port.

Things known to still be broken on Linux:

Timeout 0-30ms is currently rounded up to 30ms since 0 is not realistic with current implementation (0-2ms is more realistic but we should not rely that much on timing precision) and we could as well always throw in such cases - I do not think this is worth fixing unless there is some reasonable use case
Parity replacement
DTREnable
BytesToWrite (looks ok through code inspection but seem to be misreporting, possibly data from the queue should be added there)
DataReceived event can fire more than once (SerialStream does it correctly but code on top of that in SerialPort is not) - this is likely also issue on Windows - I've added a comment why this happens how this can be fixed on Linux (likely something similar can be done on Windows)
Some tests related to StopBits are failing when measuring the transmission time - this might be a test issue but needs to be looked at
non standard BaudRates are currently not supported but I've seen some article explaining how to emulate those - there are also couple of test failures (or rather hangs) but likely those are test issues (except for when non-standard baud rate is used) - currently allowed baud rate ranges are hard coded on linux (couldn't find an easy way to get that info from OS - perhaps just trying to set it and seeing if OS allows it is a way to go)
software emulated RTS is not working (XonXoff)
flushing is not working
[test] HasHardwareFlowControl is not working correctly - this can be checked similarly as RTS related tests are doing it (this might have a broader meaning than just RTS/CTS so needs more look)

Things which might need improvement:

Dispose implementation (i.e. Shutdown call might need to be moved to a new SafeHandle class)

src/Native/Unix/System.IO.Ports.Native/pal_serial.c

stephentoub · 2018-10-25T02:26:17Z

src/System.IO.Ports/src/System/IO/Ports/SerialPort.cs

+                        //        I.e: Let _receivedBytesThreshold be 8 - when we get an event when 7 bytes are available
+                        //        BytesToRead can change while we run this event and thus
+                        //        we virtually can get 2 events when 8th byte arrives
+                        //        I.e. we might want to add total bytes available as internal field in the args event


Is there an issue number associated with this? Do we plan to fix it?

I'm currently tracking all of the issues in the PR description, once this will be stable enough I'll file them so that I don't do much churn with creating issues.

stephentoub · 2018-10-25T02:30:14Z

src/System.IO.Ports/src/System/IO/Ports/SerialStream.Unix.cs

        }

-        public override bool CanRead
+        static void CheckBaudRate(int baudRate)


Nit: private. Also, is this something that can be shared with Windows, and thus moved into the shared file?

Unfortunatelly this can't be shared as it depends on the driver - Windows currently supports more baud rates. I'm currently tracking expanding the range of baud rates on Linux by trying to emulate non supported ranges (I saw an article somewhere on how to do that)

the rates also depend on Driver and kernel version.

I think I'll explore this subject separately - I wanted to mimic the behavior as much as possible and throw only when I'm sure the baud rates are out of range.

src/System.IO.Ports/src/System/IO/Ports/SerialStream.Unix.cs

stephentoub · 2018-10-25T03:11:25Z

src/System.IO.Ports/tests/SerialPort/BaudRate.cs

        private enum ThrowAt { Set, Open };

        #region Test Cases
+        [KnownFailure] // either hanging or really slow


What is the KnownFailure attribute? How does that differ from ActiveIssue?

this doesn't disable anything by default, it's more like a temporary thing which helps to manage test cases while I still work on them and gives hints what can still be broken and what should already work - I'd think of it as adding a xunit trait at this point. I can technically remove it since I don't believe any of those test cases run in the CI but I think it is still useful as long as still issues are being polished up

note: those test cases were failing before (assuming you got null modem connected)

src/System.IO.Ports/tests/SerialPort/Handshake.cs

tarekgh · 2018-10-26T17:23:34Z

src/System.IO.Ports/src/System/IO/Ports/SerialStream.Unix.cs

-                    throw new IOException();
-                }
+                if ((_handshake == Handshake.RequestToSend || _handshake == Handshake.RequestToSendXOnXOff))
+                    throw new InvalidOperationException(SR.CantSetRtsWithHandshaking);


throw new InvalidOperationException(SR.CantSetRtsWithHandshaking); [](start = 20, length = 66)

why we throw in the getter?

Bad design from the past - matching Windows behavior :(

src/System.IO.Ports/src/System/IO/Ports/SerialStream.Unix.cs

tarekgh · 2018-10-26T17:36:36Z

src/System.IO.Ports/src/System/IO/Ports/SerialStream.Unix.cs

+        // returns number of bytes read/written
+        private static int DoIORequest(ConcurrentQueue<SerialStreamIORequest> q, RequestProcessor op)
+        {
+            // assumes dequeue-ing happens on a single thread


assumes dequeue-ing happens on a single threa [](start = 15, length = 45)

why this assumption?

This is only called in the IOLoop which is a single thread. If that wasn't true then TryDequeue could dequeue wrong item than we peeked

tarekgh · 2018-10-26T17:47:41Z

I added minor comments. modulo @stephentoub comments it LGTM

src/Native/Unix/System.IO.Ports.Native/pal_serial.c

stephentoub · 2018-10-26T21:52:32Z

src/System.IO.Ports/src/System/IO/Ports/SerialStream.Unix.cs

+            SerialStreamIORequest result = new SerialStreamIORequest(cancellationToken, buffer);
+            _writeQueue.Enqueue(result);
+            return result.Task;
        }


We should override the Read/WriteAsync methods that take a {ReadOnly}Memory<byte> as well.

@stephentoub this currently targets netstandard so those are not present there yet

src/System.IO.Ports/src/System/IO/Ports/SerialStream.Unix.cs

stephentoub · 2018-10-26T22:16:46Z

src/System.IO.Ports/src/System/IO/Ports/SerialStream.Unix.cs

            SafeFileHandle handle = Interop.Serial.SerialPortOpen(portName);
+            if (handle.IsInvalid)
+            {
+                throw new UnauthorizedAccessException(string.Format(SR.UnauthorizedAccess_IODenied_Port, portName));


The only reason this might fail is because of unauthorized access? Is that the only exception we might throw on Windows?

Good question. Windows does GetExceptionForLastWin32Error which in the most common case throws UnauthorizedAccessException. On Linux equivalent function (Interop.GetIOException(Interop.Sys.GetLastErrorInfo())) throws:
System.IO.IOException: Device or resource busy. The tests and users I think will expect UnauthorizedAccessException (even though IOException probably makes would make more sense if we rewrote it from scratch).

Is there any specific error you have in mind? I think always throwing UnauthorizedAccessException makes sense as it is the most common case but perhaps we could fix that in Interop.GetIOException.

thoughts?

src/System.IO.Ports/src/System/IO/Ports/SerialStream.Unix.cs

src/System.IO.Ports/src/System/IO/Ports/SerialPort.cs

src/System.IO.Ports/src/System/IO/Ports/SerialStream.Unix.cs

stephentoub · 2018-10-30T00:16:49Z

src/System.IO.Ports/src/System/IO/Ports/SerialStream.Unix.cs

+            }
+            remove
+            {
+                _dataReceived -= value;


Can you stop the loop if _dataReceived becomes null?

it does stop after 2s if there are no new read/write requests

src/System.IO.Ports/src/System/IO/Ports/SerialStream.Unix.cs

stephentoub · 2018-10-30T19:57:45Z

src/System.IO.Ports/src/System/IO/Ports/SerialStream.Unix.cs

+                bool hasPendingReads = !_readQueue.IsEmpty;
+                bool hasPendingWrites = !_writeQueue.IsEmpty;

                bool hasPendingIO = hasPendingReads || hasPendingWrites;


Is there a reason to store these into locals rather than just making this be:

bool hasPendingIO = !_readQueue.IsEmpty || !_writeQueue.IsEmpty;

? From my perspective, that makes the code easier to read, but there's also a (small) perf benefit, in that we don't have to check whether there are any writes if there are reads.

will create follow up PR with this change

actually no, I use them later when calling poll

stephentoub

LGTM with a squash.

…et/corefx#33027) * SerialPort - initial async implementation on Linux and bugfixes * Apply review feedback * Fix dequeueing on Release builds * Remove InternalResources.Windows.cs (use Win32Marshal.cs from Common folder) * Fix build errors on Windows * fix build errors on uap * Apply review feedback (+ IOLoop pausing when no activity) * Fix comment * apply feedback * Remove code duplication Commit migrated from dotnet/corefx@d69f113

SerialPort - initial async implementation on Linux and bugfixes

3f52741

krwq added the area-System.IO label Oct 25, 2018

krwq self-assigned this Oct 25, 2018

krwq requested review from JeremyKuhne, stephentoub, tarekgh and wfurt October 25, 2018 00:45

stephentoub reviewed Oct 25, 2018

View reviewed changes

src/Native/Unix/System.IO.Ports.Native/pal_serial.c Outdated Show resolved Hide resolved

stephentoub reviewed Oct 25, 2018

View reviewed changes

src/System.IO.Ports/src/System/IO/Ports/SerialStream.Unix.cs Outdated Show resolved Hide resolved