dotnet / iot

This repo includes .NET Core implementations for various IoT boards, chips, displays and PCBs.
MIT License
2.15k stars 581 forks source link

Introduce a Serial Port class #1832

Open raffaeler opened 2 years ago

raffaeler commented 2 years ago

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

Features

These 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

pgrawehr commented 2 years ago

Expose Interrupt-driven callbacks (e.g. old DataReceived event) as delegate instead

Can you explain the rationale behind this? What's the problem of having an event there?

krwq commented 2 years ago

@pgrawehr:

raffaeler commented 2 years ago

@pgrawehr The main issue is having multiple subscribers which does not seem (to me) very reasonable. Also, whereas an event is not strictly necessary, it also has an additional small cost that can be avoided (performance-wise). The other motivations listed by @krwq are indeed a potential issue, but this mostly depends on the scenario. On very high-speed USB serial communication, blocking can be a serious problem. In other simpler scenarios it certainly won't be.

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.

pgrawehr commented 2 years ago

@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 DataReceived event would deliver the actual data (either as byte[] or string) as argument. Then you don't have the problem of duplicate reads. And while rare, I did have use cases where multiple classes need to talk to the same serial port.

As @raffaeler mentioned, I think it would be a good idea to separate these concerns. So we have a SerialPort "low level" class and then provide some wrappers around it. These may provide protocol-level support (e.g for stringly-typed protocols) or provide different kinds of event callbacks or loops (e.g. async events, or synchronous callbacks).

Ellerbach commented 2 years ago

So the DataReceived event would deliver the actual data (either as byte[] or string) as argument. Then you don't have the problem of duplicate reads. And while rare, I did have use cases where multiple classes need to talk to the same serial port.

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.

Ellerbach commented 2 years ago

As @raffaeler mentioned, I think it would be a good idea to separate these concerns. So we have a SerialPort "low level" class and then provide some wrappers around it. These may provide protocol-level support (e.g for stringly-typed protocols) or provide different kinds of event callbacks or loops (e.g. async events, or synchronous callbacks).

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.

krwq commented 2 years ago

All of the options above can be solved with simple ReadAsync, there is no need for callback. I agree with having SerialPort low level but I think SerialStream without events would be completely fine implementation to represent that. The layer above would be StreamReader which already exists, if you need fixed buffer then simply use Read/ReadAsync on the stream

pgrawehr commented 2 years ago

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 StreamReader and TextReader classes.

raffaeler commented 2 years ago

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.

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.

raffaeler commented 2 years ago

@krwq

I agree with having SerialPort low level but I think SerialStream without events would be completely fine implementation to represent that.

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.

@pgrawehr

We already have StreamReader and TextReader classes.

Sure, but in most of the cases they are not enough to solve the three cases. For example (and this is a very common scenario), when you receive packets with a header, you may need to "peek" at the data until you get the header tha contains the length of the whole packet. Only after the minimal amount of bytes has been received you want to consume the exact number of bytes matching a single packet. All of this should be done with a single buffer and minimizing the number of additional allocations and copied memory. This is why it is not trivial.

mguinness commented 2 years ago

Verify whether it is possible to support 9 bit word communication typically used by RS485 in microcontrollers

That could be useful, I assume I could then use with RS485 CAN HAT for Raspberry Pi.

raffaeler commented 2 years ago

@mguiness Any transciever for RS485 would work. The transciever just translate voltage-based signals (0-3.3V in case of the RPi) to current-based signals where the bits depends on the direction of the current and not on the voltage levels. This allows more robust communication over longer wires and multiple receivers on the same wires.

We have to see if the hw supports 9-bit words. In this configuration the 9th bit is commonly used to "tag" the remaining 8 bits as an address rathern than data. Since the common topology is to have a single master and multiple slaves, the master send a packet tagging the address word with the 9th bit. This ensure that the slave knows which of the receiving bytes represents an address and if it matches with its own address, it processes the packet. For more details, you can download one of the many Microchip microcontrollers datasheets and see how they support the 9-bit word on RS232.

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).

kasperk81 commented 2 years ago

before adding new protocols and apis, please try to resolve the two burning issues: https://github.com/dotnet/runtime/issues/2379 and https://github.com/dotnet/runtime/issues/55146. once done, that will bring true meaning to this statement:

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.

current situation on unix is "weak". even mono implementation of SerialPortStream is 12 times better in cpu usage https://github.com/dotnet/runtime/issues/2379#issuecomment-643753651, maybe use that as a baseline?

raffaeler commented 2 years ago

@kasperk81 I certainly believe that the first goal should be to make the low level communication reliable everywhere, therefore the links you provided are valuable.

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.

even mono implementation of SerialPortStream is 12 times better in cpu usage

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.

kasperk81 commented 2 years ago

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)

jeroenwalter commented 2 years ago

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). That being said, I also don't recommend just using my code as is, as I haven't touched it in the last few years (and also don't plan to) and it is not in sync with the original SolidSoils Arduino library. If anything, only use the System.IO.Ports.Mono code, if you need raw serial port functionality.

raffaeler commented 2 years ago

Thansk for the suggestions @jeroenwalter and @kasperk81 Knowing in advance the current issues is important to start the right way. No decision has been made yet and there will be time to carefully evaluate the lower-level code to start from.

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. Said that, the input from any member of the community is very valuable and always taken into consideration.

Ellerbach commented 2 years ago

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 So far, it's archived. Worked really well. Now, it was inspired for the API by what was existing. So it's not about the API in this case.

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.

kasperk81 commented 2 years ago

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).

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.

krwq commented 2 years ago

@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

josesimoes commented 2 years ago

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.

  1. Events: I'm all for keeping an event (despite having delegates). That's much easier to deal with the existing "style" and API that we're using there. Mostly because generics are no available, nor is the async/await.

  2. The event args could be improved to include more relevant data to help the code in the event handler (or delegate) to deal with whatever is coming in a more efficient way.

  3. In nanoFramework we added a property WatchChar that causes the event to fire whenever that char shows in the incoming stream. That's very helpful when you're dealing with terminators at the end of packets. This could be extended to a string so that patterns could be detected instead of single chars. This has support on the event arg parameter that reports the type of event: data or watch char present. Coupled with this, it's a no brainier to then use ReadTo() to read a "packet" without any other guessing work.

  4. I'm all for adding native support for related protocol like RS485. We already have that too in nanoFramework.

raffaeler commented 2 years ago

@josesimoes I've used infinite times RS485 from 8 bit microcontrollers (in asm and C), sometimes with modbus, sometimes with a custom protocol (that's why I would love to support 9-bit communications for addressing). I agree the microcontroller perspective is different, but I would not "stretch" the API to try to fit all scenarios with a single API.

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).

josesimoes commented 2 years ago

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)

raffaeler commented 2 years ago

@josesimoes

Consider expanding the Write() with an overload to take a single byte as parameter.

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.

maloo commented 2 years ago

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. All the parsing stuff should be removed from SerialPort and built on top of stream/pipe. Usually I create an application specific parser on top of the serial port pipe exposing simple app Api like async enumerable of protocol parsed data. I hope the worker thread can be removed, never seen any other stream that have needed that. Why do we need bit banging in the port class? If you are running on Linux I assume you use the kernel API for serial (read/write, ioctl). If not, the implementation is a completly custom one. Are we going to support drivers? Seems a bit overly complex, can't bit banging drivers be separate implementations? For single and few bytes you can use stackalloc and Write(Span). Events/delegates could be put on top of parser, more flexible and probably more efficient than the current implementation anyway. Anlther advantage of using Pipe is that it is just a few lines of code to proxie data via a tcp socket and both streams can be wrapped in Pipe. Which makes testing/dev on PC with TCP remotely to Pi with serial port really RAD. It would also be nice if the port list contained the VID:PID of port (USB) which is more reliable than the device name (which is good for user selection though). Today you have to manually go and lookup the port device ID to select the right port.

pgrawehr commented 2 years ago

Why do we need bit banging in the port class?

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.

If you are running on Linux I assume you use the kernel API for serial (read/write, ioctl). If not, the implementation is a completly custom one.

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.

raffaeler commented 2 years ago

Hi @maloo. My personal view on your points:

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.

maloo commented 1 year ago

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). One thing I found was that the concept of event loop is missing in dotnet today, at least in public API surface. Having a common event loop where you can register outstanding requests would be very helpful. Now every serial read/write has to use its own event loop (or thread) implementation. Also, the new Pipe APIs from asp.net core is a very good match to OS read/writes.

raffaeler commented 1 year ago

@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. I tried in many ways to adopt the pipes in a project acquiring measures via USB (serial) from an A/D and the performance was so slow that I had to convert everything back to strings. When the Pipe were first launched, it was very fast, but later slowed down. Also they can only be used to push etherogenous data (typically bytes) and making if <T> with a variable T causes a lot of headaches.

The current code in the branch is Windows only. If you have feedback there, please let me know. Please consider that the API surface of the current branch is not the final one, I am still in the plumbing.

maloo commented 1 year ago

@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. Where is your current branch with Windows only support?

raffaeler commented 1 year ago

@maloo Yes, that API had a fix that worsen down the perf because of a substantial increase of CPU usage. The problem has been logged a long time ago as an issue. If I remember well the explanation was some kind of internal race condition. I can't find the link right now.

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. BTW I still am a fan of the Pipes which I presented in several conferences with some plain and edge use-cases, but now I am more careful and measure .

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 Since the System.IO.SerialPort uses properties to action changes in the serial port, I had to completely revise the code while preserving the interop. I still am tempted to try adopting completion ports but frankly I am not sure if there could be a tangible advantage. Also, it can be quite hard to test adequately.

slyshykO commented 1 year ago

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.

raffaeler commented 1 year ago

@slyshykO I totally agree. If you read my initial post, I specifically added the 9-bit support. This is almost exclusively used on RS485 communication. Microcontrollers usually transmit the slave address as a 9-bit word in order to disambiguify the data from the addresses. I already tested the algorithm on a microcontroller and my goal is to port this implementation in this class.

If you don't need the 9-bit support, you can already use the current System.IO.SerialPort (or any other serial port library) to communicate over RS-485. To do this, you just need a transciever like the MAX487 or any other among the many RS-485 transcievers available. You may want to get a one small board from AliExpress or Amazon in case you don't want to solder the components.

slyshykO commented 1 year ago

@raffaeler I am talking about not only 9-bit support but about the possibility of enabling a special mode for serial devices to control DE pin by hardware. This mode can be set by calling ioctl on the port file descriptor with a special struct serial_rs485 filled. Such special serial devices are present on many industrial SoC's. For example, on STM32MP1 series processor.

raffaeler commented 1 year ago

@slyshykO Point noted, but consider that there are simpler solutions:

  1. bitbanging the rs232 pins is very easy on any serial library and without any need to use ioctl on the serial driver.
  2. Some transcievers (but you can add this by yourself) manage the DE/RE automatically using consecutively 2 hex inverter buffers (74HC04)
maloo commented 1 year ago

@maloo Yes, that API had a fix that worsen down the perf because of a substantial increase of CPU usage. The problem has been logged a long time ago as an issue. If I remember well the explanation was some kind of internal race condition. I can't find the link right now.

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.

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. BTW I still am a fan of the Pipes which I presented in several conferences with some plain and edge use-cases, but now I am more careful and measure .

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.

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.

Yes, you can create pipe from stream, and stream from pipe. I just think IDuplexPipe makes the most sense for the lower level API.

The branch is here: https://github.com/dotnet/iot/tree/feature-SerialPort Since the System.IO.SerialPort uses properties to action changes in the serial port, I had to completely revise the code while preserving the interop. I still am tempted to try adopting completion ports but frankly I am not sure if there could be a tangible advantage. Also, it can be quite hard to test adequately.

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. I also think API complexity is high. Do we really need all the old complexity that made the old implementation so complex that it can't be fixed? Things like callback delegates for pin changes, can't that be done using gpio API if needed? Only things that Has to be coupled and put in one interface should go into serial port. And things like event loops should probably be extracted into its own reusable entity, because at least in IoT/embedded you often want to reuse that between ports, gpio, files, networking that support uring/epoll etc. for async (multithreading usually make devices more prone to unstability and bugs).

raffaeler commented 1 year ago

@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.

Not understanding the part about transforming bytes into other entities in unpleasant. Can you give an example?

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 <T> which can be consumed from the upper layers. I worked this way with the pipes. Weird API to do this, but initially worked great ... until some patch. In any case the debate is pointless, for the reasons I was explaining above.

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. Thanks

nguyenlamlll commented 2 months ago

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.

michaldobrodenka commented 2 months ago

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

pgrawehr commented 2 months ago

@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.

raffaeler commented 2 months ago

BTW @nguyenlamlll issues on System.IO.Ports.SerialPort should be opened in the https://github.com/dotnet/runtime/issues repository and not here.