C #: reading serial port - BytesToRead

Question

I am modifying a C # based interface that interacts with a small PIC microcontroller tester device.

The user interface consists of a pair of buttons that initiate the test by sending a “command” to the microcontroller via the serial port. Every 250 milliseconds, the serial interface polling user interface searches for a short message consisting of the test results from the PIC. The message is displayed in the text box.

The code I inherited is as follows:

try { btr = serialPort1.BytesToRead; if (btr > 0) Thread.Sleep(300); btr = serialPort1.BytesToRead; if (btr > 0) { Thread.Sleep(300); btr = serialPort1.BytesToRead; numbytes = serialPort1.Read(stuffchar, 0, btr); for (x = 0; x < (numbytes); x++) { cc = (stuffchar[x]); stuff += Convert.ToString(Convert.ToChar((stuffchar[x]))); } 

What would be the rationale for the first few lines consisting of three BytesToRead calls and two 300-millisecond sleep calls before the serial port is finally read? If I misinterpret the code, any successful reading from the serial port will take more than 600 milliseconds, which seems strange to me.

Answer 1

This is a terrible hack to the behavior of SerialPort.Read (). Which returns only the number of bytes actually received. Usually only 1 or 2, serial ports are slow, and modern PCs are very fast. Therefore, by calling Thread.Sleep (), the code delays the user interface thread long enough to invoke the Read () call to return more bytes. Hopefully all of them, no matter what the protocol looks like. Usually works, not always. And in the published code, this did not work, and the programmer simply arbitrarily delayed twice as long. Ugh.

The great misfortune, of course, is that the UI thread is quite catatonic when it is forced to sleep. Quite noticeably, it draws very slowly and responds to user input.

This must be repaired by first paying attention to the protocol. The PIC should either send a fixed number of bytes in its response, so you can simply count them or give the PC a way to detect that a complete answer has been received. This is usually done by sending a unique byte as the last byte of the response (SerialPort.NewLine) or by including the length of the response as a byte value at the beginning of the message. Specific advice is difficult to give; you have not described the protocol at all.

You can save the hacked code and move it to the workflow so that it does not affect the interface so much. You get it for free from the SerialPort.DataReceived event. But this, as a rule, creates two problems instead of solving the main problem.

Answer 2

If this code was originally in a loop, perhaps it was a way to wait for the PIC to collect data.

If you have real testing equipment, I would say that you remove both Sleeps.

Answer 3

@TomWr you are right, from what I read, it is. Below is your snippet with my comments:

 try { // Let check how many bytes are available on the Serial Port btr = serialPort1.BytesToRead; // Something? Alright then, let wait 300 ms. if (btr > 0) Thread.Sleep(300); // Let check again that there are some bytes available the Serial Port btr = serialPort1.BytesToRead; // ... and if so wait (maybe again) for 300 ms // Please note that, at that point can be all cumulated about 600ms // (if we actually already waited previously) if (btr > 0) { Thread.Sleep(300); btr = serialPort1.BytesToRead; numbytes = serialPort1.Read(stuffchar, 0, btr); for (x = 0; x < (numbytes); x++) { // Seems like a useless overhead could directly use // an Encoding and ReadExisting method() of the SerialPort. cc = (stuffchar[x]); stuff += Convert.ToString(Convert.ToChar((stuffchar[x]))); } 

My guess is the same as idstam mentioned above, basically, probably to check if the data sent by your device is and select it

You can easily reorganize this code using the appropriate SerialPort methods, because there are actually much better and more concise ways to check if there is data in the Serial Port or not.

Instead of "I check how many bytes are on the port, if there is something, then I wait 300 ms, and then again and again." it ends miserably with "So, yep 2 times 300 ms = 600 ms, or only once (depending on whether it was the first time) or nothing at all (depending on the device you are communicating with through this interface , which can be very weak since Thread. Sleep blocks the user interface ...).

First, let's consider for a while that you are trying to save as much of the same code base as possible, why not just wait for 600 ms?

Or why not just use the ReadTimeout property and catch the timeout exception rather than the clean one, but at least it is better in terms of readability, and you also get your line directly, rather than using some Convert.ToChar () calls. ..

I feel that the code has been ported from C or C ++ (or at least for justification) from someone who most likely has a built-in software background.

In any case, back to the number of byte checks available, I mean, if the Serial Port data is not cleared in another Thread / BackgroundWorker / Task handler, I see no reason for checking it twice, especially in how it is encoded.
Do it faster? Not really, because there is an additional delay if there is data on the Serial Port. It doesn’t make that sense to me.

Another way to make your snippet a little better is to poll with ReadExisting ().

Otherwise, you can also consider asynchronous methods using SerialPort BaseStream.

In general, it’s quite difficult to say that without access to the rest of your code base, aka, context.

If you have additional information about what goals / protocol can give some clues about what to do. Otherwise, I can just say that this seems to be poorly encoded, again, out of context.

I doubled and even tripled the fact that Hans mentioned the user interface response in the sense that I really hope that your snippet works in a thread that is not a user interface (although you mentioned in the post that I polled the user interface still hope a snippet for another worker).

If this is really a UI thread, it will be blocked every time there is a Thread.Sleep call, which makes the user interface not very sensitive to user interactions and may give some end-user frustration.

It might be worthwhile to subscribe to the DataReceived event and execute what you want / need with the handler (for example, using a buffer and a comparison value, etc.).

Note that mono still does not implement the trigger for this event, but if you are working against a simple MS.NET implementation, that’s fine, without multithreading issues.

In short:

• Check which threads (s) (s) care about your fragment and are able to respond to the user interface
  • If it is a user interface, use another thread through Thread, BackgroundWorker (Threadpool) or Task.
  • Asynchronous stream methods to avoid user interface thread synchronization problems.
• Try to find out if the goals really deserve a double call with a 300 ms call to Sleep.
• You can directly retrieve String instead of collecting if the latest checks are used to perform operations, rather than collecting bytes yourself (if the selected encoding can satisfy your needs).