jcurl / RJCP.DLL.SerialPortStream

SerialPortStream is an independent implementation of System.IO.Ports.SerialPort and SerialStream for better reliability and maintainability. Default branch is 2.x and now has support for Mono with help of a C library.
Microsoft Public License
631 stars 197 forks source link

Byte based operations? #118

Closed Hefaistos68 closed 2 years ago

Hefaistos68 commented 3 years ago

Just wondering why there are no byte array based (or even better Span<byte>) operations are implemented. Seems all based on string and char operations. Even the CircularBuffer used in the ReadToCache class uses char as type, while I would expect it to be byte based and provide conversions to strings. Any ReadExistingBytes() method or something like this would be pretty useful, but the whole SerialPortStream implementation is based on chars.

jcurl commented 3 years ago

The design was to keep APIs close to the SerialPort implementation by MS for .NET 4.0 and 4.5. Byte byte buffers are the internal classes, which is a fixed size array programmed in advance because of how GC allows large heap objects, and they're not garbage collected well.

The Span<byte> is something that is not planned at this time, as data is read into the internal circular queue which is only allocated once. It would likely need a completely new design, because of how Span handles memory (basically as references) and could end up with data corruption, only avoidable with copies, and thus a copy provides no benefit.

Perhaps if you describe your use case?

Hefaistos68 commented 3 years ago

I have a complex communication protocol thats based on byte values, currently basically mapping Spans to structs with casts. Having to convert from string back to byte arrays would be quite a waste.

jcurl commented 3 years ago

Would you like to provide a code snippet of what you're trying to do? It sounds like you're serializing/deserializing from bytes to structs/classes.

jcurl commented 3 years ago

If I think about it a bit more, you're specifically looking for an API that can take a Span<byte> to a Write function to send data via the serial port. Why don't you use the existing Write methods, which allow you to send a byte array with an offset and a count?

Hefaistos68 commented 3 years ago

Not really serializing, but the incoming data can be directly mapped onto fixed structs, almost all members are either byte or int types. I am using the following code to get a struct from a byte buffer:

    public static ref readonly T Struct<T>(this ReadOnlySpan<byte> span, int offset) where T : struct =>
        ref MemoryMarshal.Cast<byte, T>(span.Slice(offset))[0];

Using like this: (data is my Span which is my data buffer) MyDataStruct mds = data.Struct<MyDataStruct>(0);

MyDataStruct (simplified) in turn is:

    [StructLayout(LayoutKind.Sequential, Pack = 1)]
    public struct TS55_base
    {
        public byte Id;
        public byte DataSize;
    }

There are other structs with dozens of members, where the above struct is only the header. Writing is not much of a deal as most command are one or two byte commands, but reading is quite another deal.

jcurl commented 3 years ago

Interesting use case! an internal implementation to return a Span will still need to copy to a local byte array with the appropriate Read method. The copy needs to occur because the internal implementation writes to the CircularBuffer of it's own creation, not one that the user themselves create.

So with the current design, I don't expect any performance benefit. You could read the data yourself into your own buffer using a Read method, and then do the typecast from the array to a Span<byte>.

A new design would require that somehow data is read into your own buffer so you can do the Span, but then, you should probably consider using the Microsoft implementation which talks to the driver and writes to the buffer you provide.

Hefaistos68 commented 3 years ago

Copy will be needed only if the same location in the buffer is to be overwritten. But anyway, the current implementation using strings does a lot of copying around, so there will be no performance penalty. I am not sure, but it might be possible to parameterize the CircularBuffer type and the Read/Write methods in the stream buffer class. Will have a look, if time permits I make some additions. I have seen an(other) CircularBuffer implementation that uses Span´s directly to write and read its data.

jcurl commented 3 years ago

I'm not sure why you need character based API at all. Why don't you use the Stream.Read(byte[] buffer, int offset, int count), Stream.ReadAsync(byte[] buffer, int offset, int count) APIs which are byte based?

If I research MDSN, there's the API Read(Span<Byte>), which I still need to copy from the internal buffer then into this new buffer.

Any solution that's provided must still work with .NET 4.0/4.5.

It's not really the CircularBuffer that's the problem, it's the IO thread that writes to a buffer allocated by the CircularBuffer using fixed pointers with P/Invoke and Overlapped I/O. To avoid the copy operation, the P/Invoke may not write to the CircularBuffer, it must write to the buffer given by the Stream API. It's this change which is too big. It would mean removing the CircularBuffer completely, which is against one of the original design goals to read data in a thread independent of the application to avoid buffering in the driver itself, which under Windows some of those drivers have problems. Even though you can ask a driver to allocate buffer, some drivers do not (and could place an upper limit of 8kB which means internal buffering first).

jcurl commented 2 years ago

I'll be closing this issue when I have a full implementation of supporting Read(Span<byte>), ReadAsync(Memory<byte>), Write(ReadOnlySpan<byte>), WriteAsync(ReadOnlyMemory<byte.).

jcurl commented 2 years ago

This is now integrated and referenced. It will be in the next v3.0.0 release.