Is this a good way to read serial?

NETMF & Gadgeteer
1

Hi Everyone,

Another G30 Serial Question. NetMF 4.3.

So I want to use a Circular Buffer. And I also want to heed advice I have received about keeping the SerialDataReceivedEventHandler as tight as possible.

I made a Circular Buffer class. It has a byte array as the Buffer like this:

public byte[] Buffer  { get; private set; }

The first thing I want to find out is that if class users Get the Buffer is it going to be a copy? Def don’t want that.

And the private set: I made a function CircularBuffer.Write with two signatures:

    public void Write(byte[] data)
    {
        for ( int i = 0; i < data.Length; i++)
        {
            Write(data[i]);
        }
    }

    public void Write(byte data)
    {
        Buffer[WriteIndex] = data;

        WriteIndex = (WriteIndex + 1) % Length;
    }

WriteIndex is an Interger belonging to an instance of this class. This is the only way it gets updated. Length is const, the size of the buffer.

Again am I making copies of byte arrays when I pass them into Write(byte[] data) ?

Finally, I’m stuck on what I should do in the event handler. Should I

    static void BufferIncomingBytes()
    {
        int NumBytesToRead = UART.BytesToRead;

        while (NumBytesToRead > 0)
        {
            CircularBufferInstance.Write((byte)UART.ReadByte());
            NumBytesToRead--;
        }
    }

or is it better to

    static byte[] tempBuffer = new byte[512]; // size of G30 RX Buffer

    static void BufferIncomingBytes()
    {
        int NumBytesToRead = UART.BytesToRead;

        UART.Read(tempBuffer, 0, NumToRead);

        CircularBufferInstance.Write(tempBuffer);
    }

Making this decision is paralyzing to me. Was hoping somebody with experience could shed some light on what the pros and cons are. And maybe if I’m doing anything foolish here.

Perhaps if I did the tempBuffer method I should turn the Write function into one or two calls to array.Copy? (one or two depending on whether it has to wrap around to the front of the buffer)

1 Like
2

Just a few general notes, all arrays are reference types so just passing them around won’t make copies. All uses will work on the same array (unless you explicitly make a new one and copy the data).

There are a number of ways you can do this. One such way is to wrap this all in a class. The actual byte array will be private to it so you don’t need to worry about anyone else interfering with it. In your SerialDataReceivedEventHandler, you just populate your internal circular buffer (keeping race conditions/torn updates in mind). Then you just expose whatever read functions you need. Those functions will interact with the circular buffer and return whatever data. Perhaps copy it to another array, return a parsed string, or return an integer. Writing just calls directly to the underlying UART, no buffering on your end is likely needed.

With Buffer being public, anyone can change the data inside it. They just can’t change the reference it points to since set is private.

Your second way or reading from the UART is better. Fewer lines of managed code to execute.

The below is just one possible implementation.

public class DataReader {
    private CircularBuffer buffer;

    public DataReader(int readBufferLength, SerialPort uart);

    public int ReadBytes(byte[] buffer, int offset, int length);
    public string ReadString(int length);
    public long ReadLong();

    public int WriteBytes(byte[] buffer, int offset, int length);
}

public class CircularBuffer {
    private byte[] buffer;

    public CircularBuffer(int length);

    public int BytesAvailable { get; }

    public bool Read(out byte data);
    public bool Write(byte data);
}
3 Likes
3

Oh wow everything makes so much more sense now. Thanks for this information.

So if I’m understanding, the first method I posted is going to be slower because of the nature of managed code and the number of method calls…

The second method I posted is going to be faster, however the tempBuffer is not really necessary. It would be triple buffering right? Because the hardware buffers are already drained into a software buffer implemented in the framework, and then my final buffer is my CircularBuffer, so the middle tempBuffer would be a third.

And it was misguided to try and make a public Buffer property so I can make it a private array and then I can call UART.Read() to read straight into that buffer. And If it is going to wrap around then I can split it into two calls to UART.Read()

Your example gives me the right idea of what I need to do… I think. I’m confused about DataReader being able to fill the buffer using the UART when the array is private in the CircularBuffer class.

4

Are you sure there’s a potential race here? Part of the reason I liked the circular buffer is that a writing thread (singular) deals with the WriteIndex and a reading thread (singular) deals with the ReadIndex. So at any given moment of preemption from the reading thread into the writing thread, the writing tread may quit earlier than it could have because its knowledge of the ReadIndex can either be correct or it could believe the value is smaller than it is supposed to be.

Same thing the other way around. If the Writing thread is incrementing the WriteIndex when it gets preempted, then the reading thread is going to read up until a stale value of WriteIndex. It won’t read one or more bytes that are actually available, but no big deal, we will be back shortly to try and read again.

Am I thinking about this correctly? The worst thing that can happen in my mind is that I could leave some stragglers in the CircularBuffer while reading, or I might leave some stragglers in the framework’s RX buffer. So I grab em on the next go, maybe leaving some new stragglers again.

Unless… Interlocked.Increment will actually postpone the preemption a couple microseconds and finish the operation so that I do better in terms of throughput? I predict negligible improvement unless netmf supports the atomic Add(int numToIncrementBy) operation, meaning we could be talking about large numbers of would-be stragglers being saved from straggleness in this current Time Slice.

ThreadSafeBuffer.png1156×902 13 KB

5

The DataReader would call the public methods on CircularBuffer to read and write data into the buffer. You may want to add overloads that accept an array and offset/length instead of just one byte at a time.

The potential for races/torn updates occurs when one thread does a Write, but within that function, after it has updated some fields but not all, its timeslice is up and it gets preempted. Then another thread gets control and goes to Read, it sees inconsistent data. This all depends on what fields you have to track state and can be managed by proper synchronization and locking at whatever level you deem appropriate.

6

I thought that my image in my previous post explained why I believe this is not an issue. Was it a clear example? In other words, did you follow my example and determine that I was wrong?

Yes the preempted Write will leave inconsistent data for a Read thread, but if the last thing done in the Write cycle is update WriteIndex then I don’t see how it would be a problem. I just wouldn’t get some of the information that is actually available. I can easily structure the program to make this okay as I can just get it next time. I think the main thing required is to make the Writing thread a singular entity and the Reading thread a singular entity. So that only one thread updates WriteIndex and only one thread updates ReadIndex. This is why I want to use a circular buffer, because in my mind it is inherently thread safe by design.[quote=“John_Brochue, post:5, topic:21031”]
The DataReader would call the public methods on CircularBuffer to read and write data into the buffer. You may want to add overloads that accept an array and offset/length instead of just one byte at a time.
[/quote]

Okay I understand. Thank you very much for this help.

7

High level your image looks correct, I just can’t really comment on specifics of your code outside of the general notes I gave.

1 Like