minimal support for serial port on Linux. part 2.#30903
Conversation
| </ItemGroup> | ||
| <ItemGroup Condition="'$(TargetGroup)' != 'netfx' AND '$(TargetsWindows)' == 'true'"> | ||
| <Compile Include="System\IO\Ports\InternalResources.cs" /> | ||
| <Compile Include="System\IO\Ports\SerialStream.cs" /> |
There was a problem hiding this comment.
nit: renaming these files seems good, especially SerialStream.cs to something like SerialStream.Windows.cs
| { | ||
| // /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"); |
There was a problem hiding this comment.
Use the constant defined above
| // /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); |
There was a problem hiding this comment.
nit: per coding style we put the explicity type in this case: IEnumerable<FileSystemInfo>. Alternatively you could use Directory.GetFiles and avoid the DirectoryInfo allocation.
There was a problem hiding this comment.
That does not work. /sys is bunch of symbolic links and GetFiles() seems to ignore them.
(works on device files)
| } | ||
| else | ||
| { | ||
| // Fallback to scanning /dev. That may have more devices then needed as well as |
There was a problem hiding this comment.
nit: comment sentence is incomplete
| @@ -0,0 +1,39 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
There was a problem hiding this comment.
This file is not being referenced in the csproj. Are planning to include support to macOS? If yes the csproj need a few updates besides including this file.
There was a problem hiding this comment.
yes. This is part I tested so far so I was not comfortable enable full build.
That will probably come when Linux is stable.
|
|
||
| if (value != _handshake) | ||
| { | ||
| _handshake = value; |
There was a problem hiding this comment.
This should be done only in case of success.
There was a problem hiding this comment.
Or perhaps value not cached at all since this is hardware and someone might want to ensure that the operation is actually done.
|
|
||
| internal void SetBufferSizes(int readBufferSize, int writeBufferSize) | ||
| { | ||
| if (_handle == null) throw new IOException(); |
There was a problem hiding this comment.
nit: ObjectDisposedException like above?
| { | ||
| if (_handle == null) throw new ObjectDisposedException(SR.Port_not_open); | ||
| // This may or may not work depending on hardware. | ||
| Interop.Termios.TermiosDiscard(_handle, Interop.Termios.Queue.ReceiveQueue); |
There was a problem hiding this comment.
I see the comment but it is not clear to me if an error is possible here. If error is possible: is ok to ignore it? Similar below
There was a problem hiding this comment.
I'm not sure but since it all best effort, I think it would be better not to error here.
|
|
||
| internal unsafe int ReadByte(int timeout) | ||
| { | ||
| if (_handle == null) throw new IOException(); |
There was a problem hiding this comment.
nit: same as above: ObjectDisposedException for consistency?
|
|
||
| while (count > 0) | ||
| { | ||
| Interop.Sys.PollEvents events = Interop.Sys.PollEvents.POLLNONE; |
There was a problem hiding this comment.
nit: I rather see this in the scope that it is used.
nit: the whole poll handling, when timeout is greater than 0, is also repeated in a few places.
|
@dotnet-bot test this please |
|
@wfurt do you have cycles to clean-up the build error on Windows? |
|
yes, I'll take a look. |
|
@dotnet-bot test this please |
* minimal support for serial port on Linux * feedback from review * rename SerialStream -> SerialStream.Windows * some fixes and improvements Commit migrated from dotnet/corefx@ed4e3ea
3 participants
related to #18012 and #29033
This is initial and minimal implementation of support for serial port on Linux. (managed part)
many unit tests still fail but many do pass.
Some tests make specific assumptions about buffering and that may not work properly on Unix.
It needs more investigation.
Usb2Serial devices seems to behave different than old good RS-232 chips.
With this change, simple serial apps should be possible on Linux.
Other Unix like systems should be feasible. I just did not have time to complete testing.