WIP - initial serial port support for Linux by wfurt · Pull Request #28415 · dotnet/corefx

wfurt · 2018-03-23T17:46:33Z

related to #18012
This still needs some work and testing but I would like to get early feedback.
I can do simple serial IO now on Linux.

This is first minimal set of changes.

PAL uses POSIX + few BSD extensions. It builds on Linux and OSX. FreeBSD should be probably OK.
I tested only simple test program on Ubuntu with RS-232 and USB-Serial IOgear adapter
on Raspberry ttyAMA0 should be detected but I don't currently have gear to test actual transfer.
no async event notifications

danmoseley · 2018-03-23T17:49:56Z

@kasper3 @JairoMarques @JohnRusk @dima117 @@daniel-mcassey-tmsuk @jamespettigrew in case folks want to try / give feedback.

ghost · 2018-03-23T19:32:28Z

Thank you! 👍

One question, in #25032, the effort was started to convert CPP to C, can the new code be C (to widen the compiler/platform options)? Just like bash vs. sh debate, sometimes we are not even using C++ features but yet the cpp extension deceives the compiler and cmake.. :)

wfurt · 2018-03-23T19:36:33Z

C would be easy if needed. Adding @janvorli for some thoughts.

danmoseley · 2018-03-23T20:44:32Z

    <ProjectGuid>{187503F4-BEF9-4369-A1B2-E3DC5D564E4E}</ProjectGuid>
    <IsPartialFacadeAssembly Condition="'$(TargetGroup)' == 'netfx'">true</IsPartialFacadeAssembly>
-    <GeneratePlatformNotSupportedAssemblyMessage Condition="'$(TargetGroup)' == 'netstandard' AND '$(TargetsWindows)' != 'true'">SR.PlatformNotSupported_IOPorts</GeneratePlatformNotSupportedAssemblyMessage>
+    <GeneratePlatformNotSupportedAssemblyMessage Condition="'$(TargetGroup)' == 'netstandard' AND '$(TargetsWindows)' != 'true' AND '$(TargetsLinux)' != 'true'">SR.PlatformNotSupported_IOPorts</GeneratePlatformNotSupportedAssemblyMessage>


You said it worked on BSD and OSX (or at least built) .. if so, no condition should be required on TargetsXXX .

I did build PAL (e.g. cpp on OSX). This was primarily to verify I can use certain termios functions.
I have done 0 testing so I was not comfortable enabling it in build. That will probably come in some next round.

@kasper3 the purpose of porting part of the stuff to C was to be able to share that piece with Mono. There is no plan to do such change globally. Even if it could be done for corefx, it is not possible for coreclr. We need exception handling, for example.

janvorli · 2018-03-23T22:04:35Z

+#include <stdio.h>
+
+/* This is dup of System/IO/Ports/NativeMethods.cs */
+enum {


A nit - the coding convention is to place the { on a separate line.

Another nit - it seems that it would be nice to name these "ParityXXXX" to match the style of the enum below.

nit: either rename as suggested by @janvorli or change NoneParity to NoParity

janvorli · 2018-03-23T22:07:55Z

+}
+
+static speed_t
+SystemNative_speed2termios(int speed)


Could you please keep the naming consistent - to start each part of the name with uppercase letter?

pjanotti · 2018-03-28T17:23:43Z

+
+using System.Runtime.InteropServices;
+using Microsoft.Win32.SafeHandles;
+using System.IO.Ports;


nit: put using System.IO.Ports as first using to match most other source files

pjanotti · 2018-03-28T17:27:07Z

+{
+    internal static partial class Sys
+    {
+        [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_Termios_Reset", SetLastError = true)]


nit: it seems that the convention is to use just one underscore after SystemNative and the remaining Pascal casing.

pjanotti · 2018-03-28T17:28:23Z

+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+


nit: remove one blank line

pjanotti · 2018-03-28T17:33:05Z

+    {
+        return -1;
+    }
+    return status;


nit: blank line after end of block

pjanotti · 2018-03-28T17:39:36Z

+    HandshakeBoth           // Software & Hardware Flow Control
+};
+
+static int  SystemNative_Termios_GetStatus(int fd)


nit: there is an extra space after int same pattern happens below

pjanotti · 2018-03-28T17:41:30Z

+
+static int  SystemNative_Termios_SetStatus(int fd, int status)
+{
+    if (ioctl(fd, TIOCMSET, &status) < 0)


In this case it seems that we don't need the if statement it can just be return ioctl(fd, TIOCMSET, &status); at least per http://man7.org/linux/man-pages/man4/tty_ioctl.4.html

pjanotti · 2018-03-28T17:57:30Z

+    }
+
+    cfsetspeed(&term, SystemNative_speed2termios(speed));
+//    cfsetospeed(&term, (uint32_t)speed);


nit: remove commented lines

pjanotti · 2018-03-28T17:58:32Z

+    term.c_iflag &= ~(static_cast<tcflag_t>(INPCK | ISTRIP));
+    switch (parity)
+    {
+    case OddParity:


nit: align inside switch, make like the switch following it

pjanotti · 2018-03-28T18:01:24Z

+    return 0;
+}
+
+extern "C" int32_t SystemNative_Termios_GetDcd(int fd)


nit: all these Get/Set methods could be consolidated in a single function and having the specific flag as parameter, it is small code but anyway avoids the repetition

pjanotti · 2018-03-28T18:03:51Z

+}
+
+extern "C" int32_t
+SystemNative_Termios_GetSpeed(int fd)


nit: consistency, keep on a single line like previous functions

pjanotti · 2018-03-28T18:10:04Z

+//    cfsetospeed(&term, (uint32_t)speed);
+//    cfsetispeed(&term, (uint32_t)speed);
+
+    if (tcsetattr (fd, TCSANOW, &term) < 0)


nit: spacing between function name and firt '('

The same happens in other places of the file.

pjanotti · 2018-03-28T18:29:24Z

+                        ports.Add("/dev/" + entry.Name);
+                    }
+                }
+                return ports.ToArray();


nit: blank line after block

pjanotti · 2018-03-28T18:39:44Z

+                // /sys is mounted. Let's explore tty class and pick active nodes.
+                List<string> ports = new List<string>();
+                DirectoryInfo di = new DirectoryInfo(@"/sys/class/tty");
+                var entries = di.EnumerateFileSystemInfos(@"*", SearchOption.TopDirectoryOnly);


I think that Directory.GetFiles will be better: you can avoid the allocations of FileInfo objects, set the capacity of ports at creation and use Path.GetFileName to get the name for the comparison below only when the one that uses the full path is not true.

@JeremyKuhne any better option?

GetFiles does not work as this is full os symbolic links.

pjanotti · 2018-03-28T18:42:09Z

+        {
+            List<string> ports = new List<string>();
+
+            foreach (string name in Directory.GetFiles("/dev", "tty.*"))


nit: since you are using Directory.GetFiles you have an upper-bound for ports, perform the two GetFiles and create ports using this upper-bound as capacity.

pjanotti · 2018-03-28T18:42:26Z

+                {
+                    ports.Add(name);
+                }
+            }


nit: blank line after block

pjanotti · 2018-03-28T18:49:58Z

+        private bool _inBreak = false;
+        private SafeFileHandle _handle = null;
+        private bool _rtsEnable = false;
+        ArrayList selectList;


naming, missing underscore, and why use ArrayList? Is there some usage that requires it to be an ArrayList?

pjanotti · 2018-03-28T18:51:19Z

+{
+    internal sealed partial class SerialStream : Stream
+    {
+        private const int infiniteTimeoutConst = -2;


nit: Pascal casing for constant

pjanotti · 2018-03-28T18:55:56Z

+  </ItemGroup>
+  <ItemGroup Condition="'$(TargetGroup)' != 'netfx' AND '$(TargetsWindows)' == 'true'">
+    <Compile Include="System\IO\Ports\InternalResources.cs" />
+    <Compile Include="System\IO\Ports\SerialStream.cs" />


Isn't enough common code between the serial stream to use the typical partial class pattern and have SerialStream.Windows.cs and so on?

pjanotti · 2018-03-28T18:59:21Z

+            foreach (string name in Directory.GetFiles("/dev", "tty.*"))
+            {
+                // GetFiles can return unexpected results because of 8.3 matching.
+                if (name.StartsWith("/dev/tty."))


I don't think you need this extra check here, nor the one in the other loop. If you don't need them you also don't need the ports list you can just create the array and put the entries.

I think I do. Without that check I was getting "/dev/tty" and that is not what I want.
Per the comment, it seems to be expected per MsDos compatibility ;(

pjanotti · 2018-03-28T19:40:11Z

+                {
+                    if (Interop.Sys.TerminalSetSpeed(_handle, value) != 0)
+                    {
+                        throw new IOException();


Since these functiontions set errno it will be better to have some wrapper that combines Interop.Sys.GetLastErrorInfo() w/ Interop.GetExceptionForIoErrno and possibly the name of the affected port.

pjanotti · 2018-03-28T19:54:25Z

+
+                if (value != _handshake)
+                {
+


Add a TODO for now? Same for the similar ones below.

pjanotti · 2018-03-28T19:56:13Z

+            set
+            {
+                Debug.Assert(!(value < StopBits.One || value > StopBits.OnePointFive), "An invalid value was passed to StopBits");
+


nit: remove blank line

pjanotti · 2018-03-28T19:57:43Z

+
+       internal void SetBufferSizes(int readBufferSize, int writeBufferSize)
+        {
+            if (_handle == null) throw new IOException();


That should be InvalidOperationException, or something else, not an actual IOException.

pjanotti · 2018-03-28T19:58:15Z

+        }
+
+        internal bool IsOpen => _handle != null;
+


nit: remove extra blank line

pjanotti · 2018-03-28T19:59:18Z

+        // Note: Serial driver's write buffer is *already* attempting to write it, so we can only wait until it finishes.
+        public override void Flush()
+        {
+            if (_handle == null) throw new ObjectDisposedException(SR.Port_not_open);


Is _handle null only after Dispose?

pjanotti · 2018-03-28T20:12:52Z

+            if (_handle == null)  throw new IOException();
+
+            selectList.Add(_handle);
+            Socket.Select(selectList, null, null, timeout);


Is this needed to wait for data a the serial port? Anyway selectList.Add is called every time ReadByte but it seems that if it is removed from the list then a timeout exception is throw but then the list keeps growing on each success call? What happens with one that is added if Socket.Select throws?

pjanotti · 2018-03-28T20:13:50Z

+            Interop.Sys.PollEvents events = Interop.Sys.PollEvents.POLLNONE;
+            if (timeout > 0)
+            {
+                Interop.Sys.Poll(_handle, Interop.Sys.PollEvents.POLLIN | Interop.Sys.PollEvents.POLLERR, timeout,out events);


nit: spacing of last parameter

pjanotti · 2018-03-28T20:14:56Z

+
+        internal unsafe void Write(byte[] array, int offset, int count, int timeout)
+        {
+


nit: remove blank line

pjanotti · 2018-03-28T20:17:51Z

+            int numBytes;
+            if (_isAsync)
+            {
+                throw new NotSupportedException(SR.PlatformNotSupported_IOPorts);


Same as above: use a specific message

pjanotti · 2018-03-28T20:18:39Z

+            {
+                throw new NotSupportedException(SR.PlatformNotSupported_IOPorts);
+            }
+            else


this doesn't need to be in a else block

pjanotti · 2018-03-28T20:19:10Z

+
+            if (numBytes == 0)
+                throw new TimeoutException(SR.Write_timed_out);
+


nit: remove extra blank line

pjanotti · 2018-03-28T20:25:08Z

+        internal SafeFileHandle OpenPort(string portName)
+        {
+            // TBD O_NOCTTY | O_NDELAY ???
+            SafeFileHandle handle = Interop.Sys.Open(portName, Interop.Sys.OpenFlags.O_RDWR | Interop.Sys.OpenFlags.O_CLOEXEC, 0);


Is it worth as a separate method?

pjanotti · 2018-03-28T20:26:23Z

+        internal SerialStream(string portName, int baudRate, Parity parity, int dataBits, StopBits stopBits, int readTimeout, int writeTimeout, Handshake handshake,
+            bool dtrEnable, bool rtsEnable, bool discardNull, byte parityReplace)
+        {
+


Typically we have the constructors at the top of the source after the fields.

nit: there is a extra blank line here too

pjanotti · 2018-03-28T20:31:15Z

+                }
+
+                DtrEnable = dtrEnable;
+                // query and cache the initial RtsEnable value


The comment does not match the actual code _rtsEnable is written to but not read anywhere.

pjanotti · 2018-03-28T20:32:30Z

+                _dataBits = dataBits;
+                _parity = parity;
+
+                selectList = new ArrayList();


nit: this seems just the normal initialization for selectList without a line separation from the if it gives the impression that it is related to next statement.

pjanotti · 2018-03-28T20:32:57Z

+
+                BaudRate = baudRate;
+
+                  // now set this.RtsEnable to the specified value.


nit: identation

pjanotti · 2018-03-28T20:34:09Z

+            }
+            catch
+            {
+                // if there are any exceptions after the call to CreateFile, we need to be sure to close the


nit: Windows related comment

pjanotti · 2018-03-28T20:34:36Z

+                // handle before we let them continue up.
+                tempHandle.Close();
+                _handle = null;
+                //_threadPoolBinding?.Dispose();


nit: delete commented out code

pjanotti · 2018-03-28T20:35:49Z

+                   _handle.Close();
+                    _handle = null;
+                    if (PinChanged != null || ErrorReceived != null || DataReceived != null)
+                    {


Anything to do in this case?

pjanotti · 2018-03-28T20:38:12Z

+            }
+
+            if (numBytes == 0)
+                throw new TimeoutException(SR.Write_timed_out);


nit: some of the if statements followed by a single statement have blocks others not, I don't mind either way but it should be consistent at least for a type.

pjanotti · 2018-03-28T20:38:59Z

+                throw new ArgumentOutOfRangeException(nameof(offset), SR.ArgumentOutOfRange_NeedPosNum);
+            if (count < 0)
+                throw new ArgumentOutOfRangeException(nameof(count), SR.ArgumentOutOfRange_NeedPosNum);
+            if (count == 0) return; // no need to expend overhead in creating asyncResult, etc.


nit: put the return on its own line (there are other occurrences of this).

pjanotti · 2018-03-28T20:40:20Z

@wfurt were you able to run any of the tests with actual ports in Linux?

wfurt · 2018-03-28T21:00:01Z

I did not attempt to run any test. I use my own app. I'd be happy to try. Are there any specific instructions?

jkotas · 2018-05-09T18:25:07Z

+/**
+ * Get states of serial line.
+ */
+extern "C" int32_t SystemNative_Terminal_GetDcd(int fd);


System.IO.Ports is standalone package, but this is adding dependency on System.Native. System.Native is internal implementation detail. We do not want things outside .NET Core distro to depend on it.

Should System.IO.Ports have its own supporting native library instead? Or depend on Mono.Posix?

pjanotti · 2018-06-25T16:43:07Z

I did not attempt to run any test. I use my own app. I'd be happy to try. Are there any specific instructions?

The first thing will be the serial ports and respective configuration identified in https://github.com/dotnet/corefx/blob/master/src/System.IO.Ports/tests/Support/TCSupport.cs from there tests are properly attributed to run/skip according to available hardware. That said I do expect a lot of them to fail, it is more about getting a very rough grasp of their shape with this PR.

wfurt · 2018-06-25T16:47:43Z

I run bunch of tests as part of #29033 @pjanotti This PR is old and I'll try to recover effort on getting #29033 (and second part) completed.

initial serial port support for Linux

98977fb

danmoseley reviewed Mar 23, 2018

View reviewed changes

janvorli reviewed Mar 23, 2018

View reviewed changes

karelz added this to the 2.1.0 milestone Mar 27, 2018

karelz added the area-System.IO label Mar 28, 2018

karelz assigned wfurt Mar 28, 2018

karelz removed this from the 2.1.0 milestone Mar 28, 2018

pjanotti reviewed Mar 28, 2018

View reviewed changes

karelz added this to the 2.2.0 milestone Apr 4, 2018

wfurt mentioned this pull request Apr 11, 2018

bottom part of serial port support on Unix #29033

Merged

jkotas reviewed May 9, 2018

View reviewed changes

wfurt closed this Jun 25, 2018

wfurt deleted the serial branch June 15, 2020 18:21


		internal unsafe void Write(byte[] array, int offset, int count, int timeout)
		{


		if (numBytes == 0)
		throw new TimeoutException(SR.Write_timed_out);


		BaudRate = baudRate;

		// now set this.RtsEnable to the specified value.

Conversation

wfurt commented Mar 23, 2018

Uh oh!

danmoseley commented Mar 23, 2018

Uh oh!

ghost commented Mar 23, 2018

Uh oh!

wfurt commented Mar 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pjanotti Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

pjanotti Mar 28, 2018 •

edited

Loading