IO.Ports UAP: use GetCommPorts #31539

pjanotti · 2018-08-01T18:05:03Z

Implements SerialPort.GetPortNames for UWP using GetCommPorts API. Fixes #20588, #23294.

Some cleanup for GetCommPorts fix direction of stackalloc

JeremyKuhne · 2018-08-01T18:10:19Z

src/Common/src/Interop/Windows/mincore/Interop.GetCommPorts.cs

+            Span<uint> portNumbers,
+            out uint portNumbersFound)
+        {
+            fixed (uint* portNumbersBuffer = &portNumbers[0])


The right thing to do with Span is use fixed (uint* portNumbersBuffer = &MemoryMarshal.GetReference(portNumbers)).

JeremyKuhne · 2018-08-01T18:13:41Z

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

+            var portNames = new string[portNumbersFound];
+            for (int i = 0; i < portNumbersFound; ++i)
+            {
+                portNames[i] = "COM" + portNumbers[i].ToString();


Nit: You'd be better off doing $"COM{portNumbers[i]}". It will save an unnecessary allocation for each loop and looks a little cleaner.

I am under the impression that the using interpolation will have a call to string.Format, boxing on the uint, and inside the string.Format a call to ToString() - plus likely a params array in some point of the stack. In the current format will be string.Concat and the ToString() so we saved the boxing and used string.Concat that is typically faster than string.Format. Is this some new optimization after 2.1 that I'm not aware? Not that it matters much in this case.

Interesting, interpolation with strings uses string.Concat but without the ToString() on the uint it goes to string.Format with boxing. On 2.1 here is what I get:

For portNames[i] = "COM" + portNumbers[i].ToString(); and portNames[i] = $"COM{portNumbers[i].ToString()}";:

IL_0094: ldstr "COM" IL_0099: ldloca.s V_0 IL_009b: ldloc.s V_6 IL_009d: call instance !0& valuetype [System.Runtime]System.Span`1<uint32>::get_Item(int32) IL_00a2: call instance string [System.Runtime]System.UInt32::ToString() IL_00a7: call string [System.Runtime]System.String::Concat(string, string) IL_00ac: stelem.ref

For portNames[i] = $"COM{portNumbers[i]}";:

IL_0094: ldstr "COM{0}" IL_0099: ldloca.s V_0 IL_009b: ldloc.s V_6 IL_009d: call instance !0& valuetype [System.Runtime]System.Span`1<uint32>::get_Item(int32) IL_00a2: ldind.u4 IL_00a3: box [System.Runtime]System.UInt32 IL_00a8: call string [System.Runtime]System.String::Format(string, object) IL_00ad: stelem.ref

OR maybe you could use string.Create with SpanAction. https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/shared/System/String.cs#L346

#30528

Hmm, forgot about the box. It would be interesting to benchmark differences. We have ISpanFormattable now that avoids string creation inside String::Format with int, uint, etc.

I don't care deeply here, but if we find they are roughly equivalent we may want to encourage the $ syntax in the future. I'm investigating improving formatting methods on StreamWriter etc. and the possibility of getting the compiler to generate calls directly to the format overloads (and avoiding everything but the boxing).

Yeah, it is really not that important since this API should be typically called only once from any app. Anyway I did measure and as usual benchmark/profile has some interesting info: the interpolation you suggested in the end has less allocations, however, it is the slowest by far. I quickly put together code using string.Create, I can't vouch it for correctness but should be on the ballpark of the required work, and it also surprises: great perf but not so great regarding allocation. I will keep the code as it is. Here are my results:

// * Summary * BenchmarkDotNet=v0.11.0, OS=Windows 10.0.17134.167 (1803/April2018Update/Redstone4) Intel Core i7-6700 CPU 3.40GHz (Skylake), 1 CPU, 8 logical and 4 physical cores .NET Core SDK=2.1.400-preview-009171 [Host] : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT DefaultJob : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT Method | Mean | Error | StdDev | Scaled | ScaledSD | Gen 0 | Allocated | ------------------------------------- |-----------:|----------:|----------:|-------:|---------:|-------:|----------:| AddOperator | 904.1 ns | 17.761 ns | 16.614 ns | 1.19 | 0.02 | 0.4005 | 1.65 KB | AddOperatorPlusToString | 758.6 ns | 7.396 ns | 6.556 ns | 1.00 | 0.00 | 0.3099 | 1.27 KB | Interpolation | 2,044.9 ns | 47.806 ns | 55.053 ns | 2.70 | 0.07 | 0.2785 | 1.15 KB | InterpolationPlusToString | 768.1 ns | 17.538 ns | 16.405 ns | 1.01 | 0.02 | 0.3099 | 1.27 KB | StringCreatePlusSpanAction | 820.4 ns | 15.409 ns | 16.487 ns | 1.08 | 0.02 | 0.4320 | 1.77 KB | StringCreatePlusSpanActionWithLambda | 754.0 ns | 14.552 ns | 13.612 ns | 0.99 | 0.02 | 0.5236 | 2.15 KB |

Yeah, it is really not that important since this API should be typically called only once from any app.

Right. You should be measuring code size and picking up the one that has smallest code.

JeremyKuhne · 2018-08-01T18:16:37Z

src/System.IO.Ports/tests/Support/PortHelper.cs

+
+        private static string[] GetCommPortsFromRegistry()
+        {
+            using (RegistryKey serialKey = Registry.LocalMachine.OpenSubKey(@"HARDWARE\DEVICEMAP\SERIALCOMM"))


Might be worth adding the link to https://msdn.microsoft.com/en-us/library/windows/hardware/ff546502.aspx.

JeremyKuhne

Few small things, otherwise looks good.

jkotas · 2018-08-01T23:11:19Z

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

+
+            if (error == Interop.Errors.ERROR_MORE_DATA)
+            {
+                portNumbers = portNumbersFound <= 64 ?


This is premature optimization - unnecessary code bloat. Just always allocate it on the heap.

safern · 2018-08-01T23:14:41Z

src/System.Runtime.Extensions/tests/System/EnvironmentTests.cs

-                Assert.NotEqual(Environment.GetFolderPath(Environment.SpecialFolder.System), Environment.SystemDirectory);
-                return;
-            }
+            //if (PlatformDetection.IsWindowsNanoServer)


Why not delete it instead of commenting it out?

Ops this was included by mistake is not supposed to be part of this PR.

vbaderks · 2018-08-02T21:38:15Z

@pjanotti Nice to see this being resolved. Interesting that the GetCommPorts API is also finally documented (except the return values, these are still unclear to me).

The GetCommPorts API docs at https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-getcommports
have a note that states: "If new entries are added to the registry key, the necessary size can changes between API calls.". Does it makes sense to use a while instead of a if for the size check?, aka:

while (error == Interop.Errors.ERROR_MORE_DATA)
{
       portNumbers = new uint[portNumbersFound];
       error = Interop.mincore.GetCommPorts(portNumbers, out portNumbersFound);
}

The change of this race condition happening is of course very theoretical (more then 16 ports to start with and a new port must be added between line 19 and 29).

pjanotti · 2018-08-03T15:16:03Z

@vbaderks I notified the docs team about the missing info about return values. It is a Win32 error code in case any the operations internally fail, but the "non-exceptional" values should be either ERROR_SUCCESS or ERROR_MORE_DATA.

You're correct about the possible race but I want to investigate https://github.com/dotnet/corefx/issues/31575 before doing anything in this regard.

stephentoub · 2018-09-24T14:42:01Z

src/Common/src/Interop/Windows/mincore/Interop.GetCommPorts.cs

+             uint uPortNumbersCount,
+             out uint puPortNumbersFound);
+
+        unsafe internal static int GetCommPorts(


Nit: per https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md, visibility should come first. We then generally put unsafe before the return type, e.g. internal static unsafe int.

rolfik · 2019-04-09T14:57:21Z

I would like to know where to get api-ms-win-core-comm-l1-1-2_dll this code depends on.
My machine has Windows 10 1809 build 17763.379.
According to documentation, this dll is part of Windows 10.0.17134.
So it should be on my machine, but there is only api-ms-win-core-comm-l1-1-0_dll one.

@pjanotti or anybody else, any help please?

We need it to support SerialPort in .NET Standard 2.0 library used in UWP (x64 / IoT ARM64) / .NET Core (Windows / Linux) / .NET Framework.

vbaderks · 2019-04-17T19:55:50Z

@rolfik api-ms-win-core-comm-l1-1-2_dll is not the name of an actual DLL, it is the name of a Windows API Set. See also https://docs.microsoft.com/en-us/windows/desktop/apiindex/windows-apisets
The LoadLibrary method will figure out which actual real DLL to load.

rolfik · 2019-04-17T20:34:54Z

@vbaderks
I use this nuget:
https://dotnet.myget.org/feed/dotnet-core/package/nuget/System.IO.Ports/4.6.0-preview4.19164.7

SerialPort.GetPortNames simply ends with PlatformNotSupportedException on UWP which seems to be result of:
try
{
error = Interop.mincore.GetCommPorts(portNumbers, out portNumbersFound);
}
catch (Exception e) when (e is EntryPointNotFoundException || e is DllNotFoundException)
{
throw new PlatformNotSupportedException(System.SR.PlatformNotSupported_SerialPort_GetPortNames);
}

Is there any usable version of SerialPort for UWP?

Actually my approach now is forced to use our custom ISerialPort interface implemented using SerialPort on .NET Standard 2 and SerialDevice on UWP in single nuget.

vbaderks · 2019-04-17T23:05:29Z

UWP .NET is based on a branch of .NET Core, see https://github.com/Microsoft/dotnet/tree/master/releases/UWP that doesn't have the improved implementation yet. I don't known when the UWP version of .NET core with this fix will be released. For the moment using an interface may be the best solution. You could copy the GetCommPorts source code and call it yourself until the functionality is present. I had problems in the past where SerialDevice would not return virtual com0com COM ports, while GetCommPorts was working ok.

IO.Ports UAP: use GetCommPorts Commit migrated from dotnet/corefx@0d4ade6

IO.Ports UAP: use GetCommPorts

a8a9eb3

Some cleanup for GetCommPorts fix direction of stackalloc

pjanotti added the area-System.IO label Aug 1, 2018

pjanotti self-assigned this Aug 1, 2018

pjanotti requested review from JeremyKuhne and safern August 1, 2018 18:05

JeremyKuhne reviewed Aug 1, 2018

View reviewed changes

JeremyKuhne suggested changes Aug 1, 2018

View reviewed changes

PR Feedback

d0b8c9d

JeremyKuhne approved these changes Aug 1, 2018

View reviewed changes

UAP needs System.Memory to use MemoryMarshal

ef6f59d

jkotas reviewed Aug 1, 2018

View reviewed changes

safern reviewed Aug 1, 2018

View reviewed changes

Remove early opt. and fix file changed by mistake

343ff71

pjanotti merged commit 0d4ade6 into dotnet:master Aug 2, 2018

pjanotti deleted the getcommports branch August 2, 2018 03:45

pjanotti mentioned this pull request Aug 3, 2018

Make UWP GetPortNames closer to Fx one #31591

Merged

karelz added this to the 3.0 milestone Aug 21, 2018

stephentoub reviewed Sep 24, 2018

View reviewed changes

YZahringer mentioned this pull request Feb 12, 2020

System.PlatformNotSupportedException raised on System.IO.Ports.SerialPort.GetPortNames() dotnet/runtime#31045

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

IO.Ports UAP: use GetCommPorts (dotnet/corefx#31539)

e706dd6

IO.Ports UAP: use GetCommPorts Commit migrated from dotnet/corefx@0d4ade6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IO.Ports UAP: use GetCommPorts #31539

IO.Ports UAP: use GetCommPorts #31539

pjanotti commented Aug 1, 2018

JeremyKuhne Aug 1, 2018

JeremyKuhne Aug 1, 2018

pjanotti Aug 1, 2018

pjanotti Aug 1, 2018 •

edited

safern Aug 1, 2018

JeremyKuhne Aug 1, 2018

pjanotti Aug 1, 2018

jkotas Aug 1, 2018

JeremyKuhne Aug 1, 2018

JeremyKuhne left a comment

jkotas Aug 1, 2018

safern Aug 1, 2018

pjanotti Aug 2, 2018

vbaderks commented Aug 2, 2018

pjanotti commented Aug 3, 2018 •

edited

stephentoub Sep 24, 2018

rolfik commented Apr 9, 2019 •

edited

vbaderks commented Apr 17, 2019

rolfik commented Apr 17, 2019

vbaderks commented Apr 17, 2019

IO.Ports UAP: use GetCommPorts #31539

IO.Ports UAP: use GetCommPorts #31539

Conversation

pjanotti commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjanotti Aug 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

JeremyKuhne left a 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

vbaderks commented Aug 2, 2018

pjanotti commented Aug 3, 2018 • edited

Choose a reason for hiding this comment

rolfik commented Apr 9, 2019 • edited

vbaderks commented Apr 17, 2019

rolfik commented Apr 17, 2019

vbaderks commented Apr 17, 2019

pjanotti Aug 1, 2018 •

edited

pjanotti commented Aug 3, 2018 •

edited

rolfik commented Apr 9, 2019 •

edited