bottom part of serial port support on Unix #29033

wfurt · 2018-04-11T17:51:38Z

This is part of original #28415 after addressing feedback. We publish System.IO.Ports as separate package so we can iterate on upper parts later.

I'm at the point where I can run Serial port tests and I still get 20-30 failures (out of few hundred) depending in what HW I uses. The upper part still may need some polishing so as the test suite.

In general issues seems to be in two areas:

discarding sent data when using USB/Serial adapter does not work well. That seems to be know limitation with Linux drivers. (or possibly USB)
manipulating buffer sizes does not work as on Windows. That breaks tests.

joshfree · 2018-04-11T18:48:05Z

tagging this 2.2.0 per #29033

stephentoub · 2018-04-17T13:23:33Z

src/Common/src/Interop/Unix/System.Native/Interop.Terminal.cs

+    internal static partial class Sys
+    {
+        [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_TermiosReset", SetLastError = true)]
+        internal static extern int TerminalReset(SafeFileHandle fd, int speed, int data, StopBits stop, Parity parity, Handshake flow);


These methods should have the same name as the native counterpart, minus the "SystemNative_" prefix, so e.g. TermiosReset instead of TerminalReset.

stephentoub · 2018-04-17T13:26:17Z

src/Native/Unix/System.Native/pal_termios.cpp

@@ -0,0 +1,360 @@
+// Licensed to the .NET Foundation under one or more agreements.


We're now trying to use C rather than C++ moving forward, so that more and more can be shared with Mono. It'd be good to not add to that debt. See #25032 for more background.
cc: @luhenry

stephentoub · 2018-04-17T13:27:50Z

src/Native/Unix/System.Native/pal_termios.cpp

+extern "C" int32_t SystemNative_TermiosGetDtr(int fd)
+{
+    return SystemNative_TermiosGetSignal(fd, TIOCM_DTR);
+}


Do we need to expose so many externs? Or could we just expose TermiosGetSignal as well as a PAL'd set of symbols for the bit argument?

stephentoub · 2018-04-17T13:30:21Z

src/Native/Unix/System.Native/pal_termios.cpp

+    case B75:
+        return 75;
+    case B50:
+        return 50;


Do these constant B values not always have the same numerical value as what their name suggests?

They don't, they have completely different values. They are enum-like, with values going up by one upto B38400. Above that, there is a large jump in the numbering and then they go again by one.

luhenry · 2018-04-17T15:55:02Z

src/Native/Unix/System.Native/pal_termios.h

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


You can use BEGIN_EXTERN_C and END_EXTERN_C before and after the functions declarations, that will avoid you to repeat extern "C".

Or we can simply rename pal_termios.cpp to pal_termios.c, as there is no real dependency on libstdc++ in there, and we already have some C files in the codebase.

I know it was discussed previously, but I really see no harm either way. It's just a matter of picking dependencies sparingly for each build unit separately (just like I wouldn't add reference to a package in managed code unless I need it), otherwise one can always argue "we already depend on libstdc++ for 10 other reasons".

You still need to be able to be included in C++ files, thus the need for extern "C". My comment is only to help you reduce repetition, it's not a mandated or required change.

janvorli · 2018-04-21T23:27:46Z

src/Common/src/Interop/Unix/System.Native/Interop.Terminal.cs

+        internal static extern int TerminalReset(SafeFileHandle fd, int speed, int data, StopBits stop, Parity parity, Handshake flow);
+
+        [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_TermiosGetDcd", SetLastError = true)]
+        internal static extern int TerminalGetCd(SafeFileHandle fd);


You are missing "D" in the function name - the function should end "GetDcd" instead of "GetCd".

janvorli · 2018-04-21T23:32:05Z

src/Native/Unix/System.Native/pal_termios.cpp

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


It seems that the "r" in the "rate" in the function name should be upper case

janvorli · 2018-04-21T23:32:33Z

src/Native/Unix/System.Native/pal_termios.cpp

+}
+
+static int
+SystemNative_TermiosRate2speed(speed_t brate)


It seems that the "s" in the "speed" in the function name should be upper case

janvorli · 2018-04-21T23:37:38Z

src/Native/Unix/System.Native/pal_termios.cpp

+    case B75:
+        return 75;
+    case B50:
+        return 50;


They don't, they have completely different values. They are enum-like, with values going up by one upto B38400. Above that, there is a large jump in the numbering and then they go again by one.

janvorli · 2018-04-21T23:48:24Z

src/Native/Unix/System.Native/pal_termios.cpp

+        term.c_cflag &= ~(static_cast<tcflag_t>(PARODD));
+        break;
+    default:
+        term.c_cflag &= ~(static_cast<tcflag_t>(PARENB | PARODD));


A nit - here you clear the parity bits in the default and in the code below, you pre-clear the xon/xoff and rts/cts bits. It would be nice to be consistent.

wfurt · 2018-04-25T01:13:36Z

Thanks for all suggestions. I’m out without computer access until 5/8. I’ll deal with feedback when I get back.

jkotas · 2018-05-09T18:27:41Z

Copying comment from #28415 (comment) here as well:

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?

tedvanderveen · 2018-05-16T09:19:03Z

Hi @wfurt @janvorli what's the expected timeframe for this PR to be merged?

janvorli · 2018-05-21T08:37:24Z

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

+    case 50:
+        return B50;
+    }
+    return ((speed_t)(speed));


We should return some indication of failure when we don't match any of the supported speeds instead of casting just the passed in value to the speed_t in case we don't find any match.

I think Linux supports custom speeds but I did not get to testing it.
I decided to return 0 if speed does not match one of the supported values and treat it as error.
On Unix B0 has special meaning but I don't think it is on Windows. So I think it is ok to use it. speed_t is unsigned long so we can possibly use other value.

janvorli · 2018-05-21T08:38:16Z

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

+static int TermiosGetStatus(int32_t fd)
+{
+    int status = 0;
+    if(ioctl(fd, TIOCMGET, &status) < 0)


A nit - please add space between the if and the opening ( at multiple places.

janvorli · 2018-05-21T08:40:08Z

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

+        return -1;
+    }
+
+    status  = TermiosGetStatus(fd);


A nit - please remove one extra space between the status and =

janvorli · 2018-05-21T08:44:40Z

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

+{
+    switch (speed)
+    {
+    case 230400:


I can see that there are more speeds (upto B4000000) on my Ubuntu 16.04. Should we support all of them here?

The high speeds are not portable. OSX does not have them and FreeBSD has some.
So I added them conditionally and we should be able to support them on platforms where available.

janvorli · 2018-05-21T08:47:50Z

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

+int32_t
+TermiosGetSpeed(int32_t fd)
+{
+    struct termios       term;


A nit - can you please remove the extra spaces between termios and term here and in the TermiosSetSpeed and TermiosReset functions below?

janvorli · 2018-05-21T08:49:22Z

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

+TermiosAvailableBytes(int32_t fd, int32_t readBuffer)
+{
+    int32_t bytes;
+    if (ioctl (fd, readBuffer ? FIONREAD : TIOCOUTQ, &bytes) == -1) {


A nit - please put the { on a separate line.

janvorli · 2018-05-21T08:50:52Z

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

+int32_t
+TermiosDiscard(int32_t fd, int32_t queue)
+{
+    return tcflush(fd, queue == 0 ? TCIOFLUSH : queue == 1 ? TCIFLUSH : TCOFLUSH);


A nit - would you mind adding parentheses around each of the conditions here to make it easier to read?

janvorli · 2018-05-21T09:01:08Z

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

+
+    cfmakeraw(&term);
+    term.c_cflag |=  (CLOCAL | CREAD);
+    term.c_lflag &= ~((tcflag_t)(ICANON | ECHO | ECHOE | ECHOK | ECHONL | ISIG | IEXTEN ));


I wonder - why don't we set all the flags ourselves and depend on the preinitialization by the cfmakeraw instead?

we possibly could. In some ways I like it explicit as I see what this exactly does without looking at man pages (with cfmakeraw as base line)
Even if not supported in first round, I assume serial port will be supported on all Unix like systems so I would need to also verify that cfmakeraw() works same on all OSes.

janvorli · 2018-05-21T09:05:27Z

src/System.IO.Ports/src/Interop/Unix/Interop.Termios.cs

+            AllQueues = 3,
+        };
+
+        [DllImport(Libraries.IOPortsNative, EntryPoint = "TermiosReset", SetLastError = true)]


Looking at other mini-PALs we have, it looks like the native functions should be named with underscore between the Termios and the rest of the function name to match the convention used elsewhere. So e.g. Termios_Reset.

jkotas · 2018-05-22T18:15:27Z

src/System.IO.Ports/src/Interop/Unix/Interop.Termios.cs

+        internal static extern int TermiosGetSignal(SafeFileHandle fd, Signals signal, int set);
+
+        [DllImport(Libraries.IOPortsNative, EntryPoint = "Termios_SetSpeed", SetLastError = true)]
+        internal static extern int TermiosSetSpeed(SafeFileHandle fd, int speed);


The speed argument of the unmanaged method is unsigned.

jkotas · 2018-05-22T18:18:39Z

src/Native/Unix/System.IO.Ports.Native/pal_termios.h

+
+int32_t Termios_AvailableBytes(int32_t fd, int32_t readBuffer);
+
+int32_t Termios_Reset(int32_t fd, int speed, int dataBits, int stopBits, int parity, int handshake);


We prefer explicitly sized types. From https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/interop-guidelines.md#unix-shims:

Use int32_t, int64_t, etc. from stdint.h and not int, long, etc.

wfurt · 2018-05-24T16:52:38Z

Windows Nano test failures are unrelated to this.

janvorli · 2018-05-31T23:16:08Z

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

+};
+
+/* Interop/Unix/Interop.Termios.cs */
+enum {


A nit - can you please put the curly brace on a separate line like in the previous enums?

janvorli · 2018-05-31T23:16:15Z

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

+    SignalDcd = 5,
+};
+
+enum {


A nit - can you please put the curly brace on a separate line like in the previous enums?

janvorli · 2018-05-31T23:26:14Z

src/System.IO.Ports/src/Interop/Unix/Interop.Termios.cs

+    {
+        internal enum Signals
+        {
+            SignalDtr = 1,


@stephentoub, what is the convention for enums shared between native and managed code - should the names of the members be prefixed by the "Signal" even on the managed side?
And, should the name of the enum type really be plural?

should the names of the members be prefixed by the "Signal" even on the managed side?

In most cases we name the enum value the same as the native symbol and we name the enum something meaningful / descriptive, e.g.

[Flags] internal enum MemoryMappedProtections { PROT_NONE = 0x0, PROT_READ = 0x1, PROT_WRITE = 0x2, PROT_EXEC = 0x4 }

janvorli · 2018-05-31T23:27:41Z

src/System.IO.Ports/src/Interop/Unix/Interop.Serial.cs

+{
+    internal static partial class Serial
+    {
+        [DllImport(Libraries.IOPortsNative, EntryPoint = "SerialPortOpen", SetLastError = true)]


It seems that even for this function, we should follow the convention of other similar ones and name it with Serial_ prefix. Maybe Serial_OpenPort ?

Yes, we should prefix all of the exports with a name related to the library, e.g. everything in System.Native.so is prefixed with SystemNative_.

stephentoub · 2018-07-01T14:04:17Z

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

+
+BEGIN_EXTERN_C
+
+int SerialPortOpen(const char * name);


We've generally prefixed all exports with the name of the library, e.g. SystemIoPortsNative_SerialPortOpen.

stephentoub · 2018-07-01T14:06:14Z

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

+    SendQueue = 2,
+};
+
+static int TermiosGetStatus(int32_t fd)


In other libraries, we've used intptr_t for file descriptors at the interop layer. Then we can use SafeHandle in the P/Invokes. We should do the same here, e.g.

corefx/src/Native/Unix/System.Native/pal_networking.c

Lines 986 to 993 in 95b2163

int32_t SystemNative_GetIPv6MulticastOption(intptr_t socket, int32_t multicastOption, IPv6MulticastOption* option)

{

if (option == NULL)

{

return Error_EFAULT;

}

int fd = ToFileDescriptor(socket);

stephentoub · 2018-07-01T14:07:39Z

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

+
+int32_t Termios_SetSignal(int32_t fd, int32_t signal, int32_t set)
+{
+    int status;


Can this be moved down to where it's actually initialized?

stephentoub · 2018-07-01T14:08:08Z

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

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


Nit: why is this on its own line?

stephentoub · 2018-07-01T14:08:44Z

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

+}
+
+int32_t
+Termios_GetSpeed(int32_t fd)


Same question for all of these... why have a newline between the return type and the name?

stephentoub · 2018-07-01T14:10:57Z

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

+
+
+/* Open device file in non-blocking mode and without controlling terminal */
+int SerialPortOpen(const char * name)


As noted in another comment, we should represent file descriptors at the interop layer as intptr_t. As it's currently written, on a 64-bit platform there's potentially going to be corruption here, as this is going to return a 32-bit value and the managed code is expected a 64-bit value.

ghost · 2018-07-02T06:15:44Z

src/Native/Unix/System.IO.Ports.Native/CMakeLists.txt

@@ -0,0 +1,22 @@
+project(System.IO.Ports.Native)


Please add C

project(System.IO.Ports.Native C)

ghost · 2018-07-02T06:17:02Z

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

+#include "pal_types.h"
+#include "pal_compiler.h"
+
+BEGIN_EXTERN_C


This macro and its END_ counterpart are not defined anymore and can be removed. Now we are pure C.

ghost · 2018-07-02T06:18:10Z

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

+
+BEGIN_EXTERN_C
+
+intptr_t SystemIoPortsNative_SerialPortOpen(const char * name);


Prefix misssing:
DLLEXPORT intptr_t

same for add SystemIOPortNative_* in .h files only.

ghost · 2018-07-02T06:20:17Z

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

+    return status;
+}
+
+int32_t SystemIoPortsNative_TermiosGetSignal(intptr_t handle, int32_t signal)


Casing of IO, should be: SystemIOPortsNative_ ?

I don't know. I used prefix @stephentoub suggested.

ghost · 2018-07-02T06:21:30Z

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

+    while ((result = open(name, O_RDWR | O_NOCTTY | O_CLOEXEC | O_NONBLOCK)) < 0 && errno == EINTR);
+    return result;
+}
+


nit: there are two extra blank lines at the end of this file, reduce it to one blank line?

ghost · 2018-07-02T06:21:49Z

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

+#include <errno.h>
+#include <pal_serial.h>
+
+


nit: we have one blank line elsewhere

stephentoub · 2018-07-04T03:39:42Z

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

+    return -1;
+}
+
+static speed_t SystemIoPortsNative_TermiosSpeed2Rate(int speed)


Nit: you don't need the prefix for non-exported functions, though it's fine to have it

yes, I previously did not have them for internal static functions.
But I decided it may be better to just keep everything same.
Thanks for all your feedback.

stephentoub · 2018-07-05T00:08:56Z

@wfurt, looks like this needs to be rebased.

stephentoub · 2018-07-05T20:29:35Z

looks like this needs to be rebased.

Actually, I'm wrong, it's fine for a squash and merge. It would only have issues with a rebase and merge because some of the earlier commits have conflicts.

* bottom part of serial port support * small cleanup * feedback from reviews * remove previous changes to System.Native * feedback from review * use consistently int32_t * update names and types based on feedback * feedback from kasper3 Commit migrated from dotnet/corefx@83753bc

Tomas Weinfurt added 2 commits April 11, 2018 10:12

bottom part of serial port support

bea5e41

small cleanup

cbb5838

wfurt added the os-linux Linux OS (any supported distro) label Apr 11, 2018

wfurt self-assigned this Apr 11, 2018

wfurt requested review from pjanotti, danmoseley and janvorli April 11, 2018 17:51

joshfree added this to the 2.2.0 milestone Apr 11, 2018

karelz added the area-System.IO label Apr 14, 2018

stephentoub reviewed Apr 17, 2018

View reviewed changes

luhenry reviewed Apr 17, 2018

View reviewed changes

danmoseley mentioned this pull request Apr 19, 2018

SerialPort on Linux dotnet/core#740

Closed

janvorli reviewed Apr 21, 2018

View reviewed changes

Merge branch 'master' of https://github.com/dotnet/corefx into serial2

03c57c1

Merge branch 'master' of https://github.com/dotnet/corefx into serial2

5ec3a0f

Tomas Weinfurt added 3 commits May 16, 2018 17:01

Merge branch 'master' of https://github.com/dotnet/corefx into serial2

04e0b1f

feedback from reviews

8abfee1

remove previous changes to System.Native

d8565ec

janvorli reviewed May 21, 2018

View reviewed changes

feedback from review

176d786

jkotas reviewed May 22, 2018

View reviewed changes

use consistently int32_t

f3fa5b9

janvorli reviewed May 31, 2018

View reviewed changes

wfurt mentioned this pull request Jun 25, 2018

WIP - initial serial port support for Linux #28415

Closed

stephentoub reviewed Jul 1, 2018

View reviewed changes

update names and types based on feedback

36b4a41

ghost reviewed Jul 2, 2018

View reviewed changes

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

#include <errno.h>

#include <pal_serial.h>

Copy link

ghost Jul 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we have one blank line elsewhere

feedback from kasper3

c04b26e

stephentoub reviewed Jul 4, 2018

View reviewed changes

stephentoub approved these changes Jul 4, 2018

View reviewed changes

wfurt merged commit 83753bc into dotnet:master Jul 5, 2018

wfurt mentioned this pull request Jul 9, 2018

minimal support for serial port on Linux. part 2. #30903

Merged

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

		@@ -0,0 +1,360 @@
		// Licensed to the .NET Foundation under one or more agreements.


		int32_t Termios_AvailableBytes(int32_t fd, int32_t readBuffer);

		int32_t Termios_Reset(int32_t fd, int speed, int dataBits, int stopBits, int parity, int handshake);

	int32_t SystemNative_GetIPv6MulticastOption(intptr_t socket, int32_t multicastOption, IPv6MulticastOption* option)
	{
	if (option == NULL)
	{
	return Error_EFAULT;
	}

	int fd = ToFileDescriptor(socket);



		/* Open device file in non-blocking mode and without controlling terminal */
		int SerialPortOpen(const char * name)


		BEGIN_EXTERN_C

		intptr_t SystemIoPortsNative_SerialPortOpen(const char * name);

bottom part of serial port support on Unix #29033

bottom part of serial port support on Unix #29033

Conversation

wfurt commented Apr 11, 2018

joshfree commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wfurt commented Apr 25, 2018

jkotas commented May 9, 2018

tedvanderveen commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wfurt commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Jul 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Jul 5, 2018

stephentoub commented Jul 5, 2018

stephentoub Jul 1, 2018 •

edited