-
Notifications
You must be signed in to change notification settings - Fork 581
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
Introduce a Serial Port class #1832
Comments
Can you explain the rationale behind this? What's the problem of having an event there? |
|
@pgrawehr I am afraid that totally removing the support to be "interrupt-driven" could preclude an easy migration from the current Serial Port or even from code that was initially designed in other languages/platforms. I would not go that far. Even if the API can be very different, we should consider an easy migration-path for many different use-cases. Most interestingly, I believe we need to precisely address the problem of the thread were the hard work should be done. Thread affinity as I said is very important and I think we should provide the pattern and the tools needed to do a clean work (maybe an external class for the EventLoop or a custom task scheduler). This is why I tried to involve Mr Toub. |
@krwq Ok, with the current API, that is really a problem. But most of this would be solved by having events that provide the data. So the As @raffaeler mentioned, I think it would be a good idea to separate these concerns. So we have a |
Correct but the issue you have with serial port is that you don't really know how much has arrived. Or how much you want to read. Let me have few examples as this should help finding the right way of calling the event:
Those 3 options regroup like 80%+, of the case we see in serial and if the data are sent there should be a way to setup this option. For the 20% remaining (nothing fixed, not really easy to predict), this is where the pattern proposed to have a delegate handling this can be very handy. |
Fully agree with having multiple layer of the API. Like a ring of services. Something super simple, like the existing one (can even be more simple). Then another for things which are protocol specific, then advance scenarios, maybe as extension or plugin. For the very low level (bit bang, etc), that should be part of the very simple class as this is something very close to the drivers. |
All of the options above can be solved with simple |
Of course they can be solved with Read/ReadAsync. But the suggestion here is to provide the helper class that converts the stream into something more "high level". Like a wrapper that fires an event for each line received. I agree that we have to be sure not to reinvent the wheel. We already have |
Exactly. I included the three option in my initial requirements not because they need to be addressed in the basic communication class, but because we have to provide all the needed code to make these use cases as simple as possible. We can also deliver support for them, but they can stay outside the communication class. |
Sure, it is definitely possible, but you may remember all the discussions when the Pipes where first introduced. The main example highlighting the power of the Pipes was showing how the data coming from the stream was hard and prone to error. We definitely have to provide a robust support to make those three major use-cases are easy to write for anyone. We can just limit the base class to stream, but in this case there is the need to provide another classes/helpers with an efficient solution to those cases.
Sure, but in most of the cases they are not enough to solve the three cases. |
That could be useful, I assume I could then use with RS485 CAN HAT for Raspberry Pi. |
@mguiness We have to see if the hw supports 9-bit words. If that is not possible, you can still use the RS485 with modbus. There are a few modbus library out there, I am using them and they work pretty well (both over the serial port and over the ethernet). |
before adding new protocols and apis, please try to resolve the two burning issues: dotnet/runtime#2379 and dotnet/runtime#55146. once done, that will bring true meaning to this statement:
current situation on unix is "weak". even mono implementation of SerialPortStream is 12 times better in cpu usage dotnet/runtime#2379 (comment), maybe use that as a baseline? |
@kasperk81 Apis are part of that. In one of those links the problem relates to the thread affinity (the thread where data is consumed), which is a very common issue often leading to consuming excessive resources.
There are no reasons to exclude mono implementation if that is better. The best thing would be to provide a repro code so that we can benchmark the new implementation in a wide range of use-cases. |
memory leak: here is @dseeg's repro https://github.com/dseeg/SerialPortMemoryLeak high cpu usage: we can use @jeroenwalter's library https://github.com/jeroenwalter/Arduino/tree/netcore30. delete the folder System.IO.Ports.Mono and ProjectReference:System.IO.Ports.Mono, then run integration tests. (unless @jeroenwalter has a simpler repro like @dseeg's) |
I wouldn't delete System.IO.Ports.Mono from my fork, as that was the whole point of the fork. The Microsoft serial port implementation is so borked, that I added the Mono implementation to it and added a Received event (I think, maybe it already had one). |
Thansk for the suggestions @jeroenwalter and @kasperk81 To be clear, I am not a Microsoft employee, just a MS MVP who is part of the triage team and trying to provide some help in this repository. I strongly believe that a robust and flexible serial port support is needed and the rest of the team agreed that. |
While we are in sharing pre Microsoft implementation for serial, I did created this one for .NET Core 2.0: https://github.com/Ellerbach/serialapp/tree/master/nuget Something about the stream is that it can be very handy, like in the UWP implementation to have only 2 streams: one to read and one to write which are really independent. |
sounds like you missed the point or the context of my comment. we want to "reproduce" the high cpu issue in System.IO.Ports, the very reason you switched to System.IO.Ports.Mono. this is to get the underlying issue fixed by someone with relevant hardware knowledge and know how to fix serial port implementation in dotnet runtime. |
@kasperk81 the thread should be sleeping most of the time if there is no work and seems from the description that's currently not happening, simple sleep usually brings down CPU usage to close to 0. Perhaps put a breakpoint somewhere here: https://github.com/dotnet/runtime/blob/main/src/libraries/System.IO.Ports/src/System/IO/Ports/SerialStream.Unix.cs#L853 and step to figure out why that's the case. It's a bit offtopic so let's not discuss this further in this issue. also if you have no events or pending reads/writes technically that loop should stop after few seconds and considering you got high CPU usage it's not the case |
Let me throw here a couple of ideas and thoughts from the perspective of using this class in MCUs, like we do in .NET nanoFramework.
|
@josesimoes I would rather prefer a base high-performance class that is open to be extended and also provide some default extensions (either derived classes or extension methods, TBD) for the most common scenarios (which I already listed at the beginning of this thread). |
Consider expanding the Write() with an overload to take a single byte as parameter. Often times one has to create a byte array with a single element just to send a wake-up byte to the UART, for example. (considering adding this to the SerialPort API in .NET nanoFramework) |
Indeed. BTW this is already possible in the current implementation by accessing the underlying stream. BTW I look forward to get general agreement on removing support (for the new SerialPort) to netstandard 2.0 so that I/we can optimize the code using the memory primitives (Span/Memory) and the newest language features. |
Most, if not all, parsing requirements can easily be solved by creating a Pipe from the BaseStream of the SerialPort. This also removes GC of all the data buffers since it uses MemoryPool. Pipes are really easy to use for all kinds of parsing, whether it is markers like newlines, STXs, header peaking without data copying, buffer data until full package received etc. |
Because quite often, one would use the serial port's control lines lines as GPIO outputs or inputs. For instance, many microcontrollers wire the RTS line of the USB-to-RS232 controller to the chip reset input. Sometimes, this is even used on the RX/TX line, to transmit 10-bit data or similarly strange data formats.
Our goal is to provide an operating-system independent API that can be used the same on all operating systems. We just want that users don't need to directly access the kernel APIs. |
Hi @maloo.
I am currently experimenting a request from @krwq to use the FileStream class to manage the async code towards the native code. As I do not work for MS, this will take some time out of my job. |
Any progress on the new low level API surface? Review/PR? I just had to do my own serial for Linux since not much was working in the dotnet one (mostly cancelation and timeouts). |
@maloo I am going on very slow because I am very busy with my job (I am just a contributor here). The Pipe API is one of the thing I tested and dropped because it is definitely slower than the standard Stream management. The current code in the branch is Windows only. If you have feedback there, please let me know. |
@raffaeler Are we talking about the same Pipe API? Linked API is used in ASP.NET Core and saturating 10Gb/s in benchmarks, why is that too slow for a device serial port? And the APIs in Pipe for peeking into data before consuming it makes it really great for serial communication where you want to make sure a full packet has arrived before parsing it. And I don't see why pushing bytes is bad, someone have to convert bytes to whatever the app needs, but it should definitely not be done by the lowest layer. |
@maloo Yes, that API had a fix that worsen down the perf because of a substantial increase of CPU usage. I did not say that pushing bytes is bad. I said that the Pipe API makes the transformation of bytes into other entities unpleasant. Peeking bytes is of course the reason why I initially tried to use the Pipes in the Serial port scenario. Beyond that, you will always able to add a PipeReader from the SerialPort if you want to do that. The goal in this new implementation is to provide the needed extensibility points so that you can consume the data in different ways. The branch is here: https://github.com/dotnet/iot/tree/feature-SerialPort |
Is it possible to add an ability to enable RS-485 support on Linux according to https://www.kernel.org/doc/Documentation/serial/serial-rs485.txt ? Thanks in advance. |
@slyshykO I totally agree. If you don't need the 9-bit support, you can already use the current |
@raffaeler |
@slyshykO Point noted, but consider that there are simpler solutions:
|
I think I would like to see a link to it and see if it is still open and if it is relevant for this task. In my mind an API can't have a race condition, an implementation can. Maybe Pipe or PipeReader/Writer? But if it is a problem, IDuplexPipe is the interesting part to expose, which requires PipeReader/Writer, but most in those can be overridden if needed. But hopefully, Pipe is good enough.
Not understanding the part about transforming bytes into other entities in unpleasant. Can you give an example? We can't get around the fact that all data from the kernel is bytes. So I can't see how the lowest layer (driver) can Not work with bytes. And if you use "struct messages", those can be "cast" directly to C# types and normal StreamReaders etc can be created from the Pipe handling text style protocols etc.
Yes, you can create pipe from stream, and stream from pipe. I just think IDuplexPipe makes the most sense for the lower level API.
Thanks, had a quick look. I don't like the mixing of async and sync in interfaces. Same issue as in many old .NET types, like stream. One of them will always be paying for the other. If impl use sync kernel calls, async will suffer, if kernel use async, sync calls will suffer. I was hoping for separation of async and sync (hopefully no sync at all). Again favoring IDuplexPipe. |
@maloo When we discussed the serial port in the triage, there was a consensus in not adding a verbose api, but just the bare minimum to send/receive data and what else may be needed to put further abstractions ontop of it, but nothing else.
When you pipe a stream of bytes, you may also want to analyze the data, recognize the variable-sized packet and transform it to a I agree the old code is quite complex, but the old serial port always worked very well and the difficulty of testing a different logic is my primary concern. In any case, should I rewrite it, I would definitely go with the completion ports, but I am not sure it is really worth the effort. Bitbanging the serial port pins is still needed. I cannot delegate the serial port pins to the GpioController as this has no knowledge of the serial port and never will. Those pins are important, for example, to drive RS485 as for the last comment on this thread. Event loops. I don't see the benefits right now of making a super-tested even loop management so that it can be reused, but this is something that can be added later on. As soon as I can find some time to work on this, any further proposal, discussion and fix is, of course, welcome. |
Hi everyone. I'm a bit beginner on this dotnet iot thing. But after reading through this issue, it means that System.IO.Ports would not work on Linux, or to be specific, won't work on Raspberry Pi devices? In fact, I have tried to do so but it does not connect to serial ports successfully. |
system.IO.SerialPort works on Linux and raspberry pi. There is only one catch. High cpu usage. But there are alternatives - serial ports ported from mono repository - about 5x lower cpu usage. You can find ones on nuget, or check my repo |
@nguyenlamlll I can confirm that it works just fine. I have a Raspberry Pi 4 with 5 serial ports open in parallel; no issues at all. This ticket is about a replacement implementation, but less because of technical problems but rather because of a semi-optimal interface. If you have problems getting it to work, please open a separate issue and post the code you're using. |
BTW @nguyenlamlll issues on |
The initial discussion started here.
Background and motivation
The
System.IO.Ports.SerialPort
class has been widely adopted in the .NET world to support serial port communication. Since it was first added in the .NET Framework, its implementation is quite old and does not support many of the new language and runtime improvements.As opposed to many other classes, it is very difficult validating the behavior of the serial port class because it strictly depends on the serial port driver and the connected hardware. For this reason it is preferable not to touch the existing implementation which could cause serious breaking changes. Instead, it is better providing a brand new implementation exposing a modern API and flexible API.
Anyway, the lower layer code (interacting with the serial hardware) is precious and already working cross-platform, therefore it is desirable to reuse it as much as possible.
This is not yet an API proposal that will follow right after collecting all the basic requirements that will shape the new implementation.
Requirements
This is a list of basic requirements that will guide shaping the final API. In the triage team we expect to see this list growing and discussions will follow to include or not them in the final list.
Modernization
BeginRead
,EndRead
,BeginWrite
,EndWrite
)Span<byte>
in place ofbyte[]
in the API parametersFeatures
EventLoop
class that could be wrapped to this purpose. It would be even more interesting to reuse an implementation for a cross-platform message-pump (if available) in the runtime (@stephentoub suggestions?)DisposeAsync
System.IO.Stream
, evaluate the possibility to expose Pipes or Channel (as done in ASP.NET Kestrel)DataReceived
event) as delegate insteadThese lists were on the top of my heads and they are certainly not exhaustive, therefore any feedback/suggestion from the members of the community is welcome.
/cc @joperezr @Ellerbach @krwq @pgrawehr
The text was updated successfully, but these errors were encountered: