Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider moving System.IO.Ports package to dotnet/iot repo #1797

Closed
danmoseley opened this issue Feb 17, 2022 · 17 comments
Closed

Consider moving System.IO.Ports package to dotnet/iot repo #1797

danmoseley opened this issue Feb 17, 2022 · 17 comments

Comments

@danmoseley
Copy link
Member

danmoseley commented Feb 17, 2022

This package does not get a lot of attention in dotnet/runtime, partly because one typically needs particular - sometimes very specific - hardware to test it. Also because of that reason, it gets little value from automated test coverage in that repo: most tests are skipped. The code is not much changed from .NET Framework except for adding Unix support.

Serial port seems to have stronger affinity to IOT scenarios, and to the community in this repo, who in many cases may be set up to test it out locally. It would be easier to keep it up to date with the latest hardware and scenarios.

We should consider migrating the package here. Either shipping future versions from here (preferably) or dead ending the package in dotnet/runtime, and shipping from here with a new identity (the System.Data.SqlClient/Microsoft.SqlClient model). It would bring over about 20 or so open issues/requests on it with it.

@richlander shared that he has no objection in principle. I believe @ericstj indicated that it would be technically possible, but we didn't work through the details. Packages from this repo are a little different.

Opening this issue to trigger discussion.

cc @krwq @dotnet/area-system.io @terrajobst

@Ellerbach
Copy link
Member

@danmoseley you are right, as networking is widely used to communicate across machines, Serial is mainly used in IoT scenarios or with older hardware. So, makes sense to have it with .NET IoT .
And you are right for the tests as well. We can run some in the pipelines we have currently with .NET IoT. It will require to connect few pins from the Raspberry Pi to have them running properly on Linux and Windows.
Note: I have a long time due PR I need to do that will improve the port detection in Windows, Linux and MacOS.

@pgrawehr
Copy link
Contributor

Sounds reasonable. dotnet/runtime is getting so much attention that these smaller libraries quickly get out of focus and issues land on the huge pile of open tasks. I have moved parts from one repository to another (including history) before, so that is possible.

@joperezr I believe this change would also require changing the signing key for System.IO.Ports, too. But since this one used to be a core library, do you see any issue here?

@raffaeler
Copy link
Contributor

The SerialPort class is just a very small piece of what is really needed when talking to a device.
Real use-cases examples of communication styles are:

  • pure text (terminal-like)
  • fixed and variable sized packages (with or w/o headers)
  • streaming data

Above them you can then write application-specific protocols.

The problem with moving the SerialPort class here is that we should avoid any breaking changes. But precludes a golden occasion to create here a better library using a modern architecture. In other words, the current code does not provide the best "base support" for the above scenarios.

I am not currently happy with the SerialPort class structure. I would rather opt for leaving the SerialPort in System.IO and creating a better library here supporting all of the above things.

  • Adopting Span<T> whenever possible
  • Adopting System.IO.Pipelines to provide a high-perf and memory efficient management of those communication strategies.

@krwq
Copy link
Member

krwq commented Feb 24, 2022

Seems I've put RX and TX pins on the test board so we should be able to even finally run automated tests for that library (see https://github.com/dotnet/iot/tree/main/src/System.Device.Gpio.Tests for reference) - I can't remember if I've soldered that header though so it might be a bit of work for @joperezr or someone else in the campus 😄 (or me whenever I visit) looking at the picture I made it's most likely not soldered

@joperezr
Copy link
Member

IIRC S.IO.Ports has a native component on it, where the rest of the IoT repo is managed only so that is one potential concern. I'm not opposed to the move, but as part of the move we could consider the possibility of re-writing the native part to be managed as well, which should make it a much better story for CI, build pipelines, packaging, and toolset dependencies in order to work on the repo. @krwq how possible would it be to make S.IO.Ports managed-only?

@krwq
Copy link
Member

krwq commented Feb 24, 2022

Should be possible but I think there were couple of constants which were problematic and I'm not 100% sure they are the same between OS-es. @wfurt might have more details on that

@wfurt
Copy link
Member

wfurt commented Feb 24, 2022

The constants are definitely platform specific. For example O_NOCTTY we use is 00000400 on Linux but 0x00020000 on macOS. Getting the values from OS headers was the easiest path.

Termios part is another story. that is part of "libc" and that on Linux can be either glibc or it can be musl (e.g. Alpine)
I don't know if all the constants we use are at least identical between libc and musl. But they are different between OSes.
TIOCM_CTS is 0x020 on Linux and 0040 on macOS.

Managed PAL is still probably possible but it going to tedious - at least initially. The good part is that the serial ports did not changed that much in last few decades.

If we move the code to IOT we can possibly consider better API. While the Stream is convenient .NET abstraction I don't think that necessarily maps well to the HW. We can keep the current API for compatibility but we can also expose something better and share core parts.

@raffaeler
Copy link
Contributor

@wfurt Just to be clear, as I wrote above in this thread, I am favorable to a brand new API in this repo.
But during the last triage I stressed to leave the current SerialPort class untouched because it is extremely difficult to test, and subtle problems may occur in a production environment.
The problem is not recompiling or referencing a different nuget package, not even refactoring. The real problem is ensuring that the behavior is exactly the same because the customer may not have the possibility to test it with a specific device. And often the device to test is not available in a dev environment because it is extremely expensive.

This is why I stressed about:

  • keeping the current code as-is (eventually in a different nuget package, this is not really important)
  • creating a brand new implementation in this repository, possibly reusing as much as possible of the SerialPort class
  • shaping the new API so that it can be specialized to support the scenarios I posted in my other comment.

@Ellerbach
Copy link
Member

I guess the platform specific will have to stay. Now, we may not need to have a specific native pet for each platform. If we do it will be for higher performance reasons. But looking at a serial port our days, even open at 1M, any platform will support this without issue.
I did a .NET core implementation long time ago based on libc and using Termios, works really nicely on various Linux. It was far before the availability of the Microsoft version and I had quite some users for it on Linux and MacOS. I am not afraid about the work which needs to be done on all this.

And I agree with @raffaeler, it is a great opportunity to modernize the API by adding what is missing. I would still keep the current ones and add new once. I dislike the idea of having 2 packages doing the same thing with different API.
On the API side there were good ideas on the Windows.Devices.SerialCommunication side like separating the stream for sending and receiving. Now it was, at least for me, far to be a great one. But some were good ideas.
Also a perfect occasion to add some lovely Span and Span there.
Definitely some work. A lot of work. Not all need to be done right away. A good rodmap for the API and the work to be done needs to be clearly agreed.

@danmoseley
Copy link
Member Author

Cc @dotnet/area-system-io and @stephentoub in case either have thoughts here

@stephentoub
Copy link
Member

@stephentoub in case either have thoughts here

I read the issue description and the comment, but I'm not seeing what significant value moving it would bring. Seems like it'd just be work for little reward? I'm skeptical just moving it to a different repo would somehow significantly increase investment in the library.

@danmoseley
Copy link
Member Author

I'm skeptical just moving it to a different repo would somehow significantly increase investment in the library.

My assumption is that in this repo there is an active community that is passionate about serial ports. But, it would still have to have the same compat bar; so the idea of creating a new library that could move faster may make more sense.

@joperezr
Copy link
Member

joperezr commented Mar 3, 2022

We spoke about this topic in our last 2 IoT community standups. In general, here are some of the conclusions we came up with in consensus:

  • It would be great to have a SerialPort type over here in dotnet/iot.
  • We don't see a lot of value of bringing in the library System.IO.Ports as-is in dotnet/runtime and just maintain it here. Instead, we think that we should start building a new API that fits well both with the rest of the communication protocols we have here (GPIO, SPI, I2C, PWM) and is also modernized to use things like Span.
  • While the API design would be all new, we should take inspiration from the existing type in System.IO.Ports as well as take implementation details where relevant.
  • By being a brand new API, there shouldn't be any expectation from consumers for it to be an in-place replacement for System.IO.Ports, but instead a modernize version of it that aligns better with the rest of the APIs in dotnet/iot.

@terrajobst
Copy link
Member

  • We don't see a lot of value of bringing in the library System.IO.Ports as-is in dotnet/runtime and just maintain it here. Instead, we think that we should start building a new API that fits well both with the rest of the communication protocols we have here (GPIO, SPI, I2C, PWM) and is also modernized to use things like Span.

💯👍

@danmoseley
Copy link
Member Author

Feel free to re-title or replace this issue if the working proposal is now to create a new package. (I have no opinion -- was just looking for options to revitalize our .NET API for serial ports. )

@danmoseley
Copy link
Member Author

I think #1832 and discussion above now makes this issue redundant so I will close it.

I notice @raffaeler your comment above that more is required than serial port support. I wonder whether more issues are warranted for those. Anyway, I"ll close this. Thanks for kicking it off!

@raffaeler
Copy link
Contributor

raffaeler commented Mar 11, 2022

@danmoseley

I wonder whether more issues are warranted for those

I opened the issue after our weekly triage discussion. We all agreed that a new API was more than desirable.

Also consider that the IoT world is dominated by python. If you want to attract people from that side, we have to provide a first-class support for the serial port which is commonly used to talk with Arduinos and many other boards. And I strongly believe that we can do a better job.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants