wjwwood / serial

Cross-platform, Serial Port library written in C++
http://wjwwood.github.com/serial/
MIT License
2.13k stars 1.04k forks source link

default timeout of zero causes all writes to fail? #136

Closed rhd closed 7 years ago

rhd commented 8 years ago

Hi, If I don't explicitly set a timeout, all calls to write() fail. Is this expected? If I set the timeout to 5 ms like below, it works.

        serial::Serial serialPort;
...
        serialPort.setPort("/dev/ttyS0");
        serialPort.setBaudrate(115200);
//        auto t = serial::Timeout::simpleTimeout(5);
//        serialPort.setTimeout(t);

        serialPort.open();
...
        result = serialPort.write((uint8_t *)buffer, numBytes);
wjwwood commented 8 years ago

I think it depends on your OS (which OS are you using?) and hardware. Writing with the default timeout works in many cases, but I can't tell you exactly why it doesn't work in your case. You might look at the serial code responsible for writing the data -- it's not out of the question that there is a bug there.

rhd commented 8 years ago

I'm on Linux (Redhat 7.2)

14:08 $ uname -a
Linux 3.10.0-327.el7.x86_64 #1 SMP Thu Oct 29 17:29:29 EDT 2015 x86_64 x86_64 x86_64 GNU/Linux

What is the default timeout supposed to do? Block forever until the write finishes?

rhd commented 8 years ago

The hardware is a serial port PCIe card in a dell box.

06:00.0 Serial controller: Moxa Technologies Co Ltd CP-118EL-A (8-port RS-232/422/485 PCI Express Serial Board)
wjwwood commented 8 years ago

The default timeout is all 0, so non-blocking. I think it could be considered a bug that write does not work when the timeout is 0. I think I was thinking of read when I said it should work with the default timeout. The way the code is written for write, I don't see how it could ever try to write (even once) if the timeout is zero:

https://github.com/wjwwood/serial/blob/master/src/impl/unix.cc#L622-L625

rhd commented 8 years ago

Something like this does seem to fix the issue.

16:10 $ git diff
diff --git a/serial/src/impl/unix.cc b/serial/src/impl/unix.cc
index 0ff251e..0f6298c 100755
--- a/serial/src/impl/unix.cc
+++ b/serial/src/impl/unix.cc
@@ -617,12 +617,17 @@ Serial::SerialImpl::write (const uint8_t *data, size_t length)
   total_timeout_ms += timeout_.write_timeout_multiplier * static_cast<long> (length);
   MillisecondTimer total_timeout(total_timeout_ms);

+  bool first = true;
   while (bytes_written < length) {
     int64_t timeout_remaining_ms = total_timeout.remaining();
-    if (timeout_remaining_ms <= 0) {
+    if (!first && (timeout_remaining_ms <= 0)) {
       // Timed out
       break;
     }
+    if( first )
+    {
+        first = false;
+    }
     timespec timeout(timespec_from_ms(timeout_remaining_ms));

     FD_ZERO (&writefds);

which allows this testcase to work.

TEST_CASE( "Test Serial Port", "[SerialPort]" ) {

    serial::Serial outPort;
    serial::Serial inPort;

    outPort.setPort("/dev/ttyMUE5");
    outPort.setBaudrate(Rs485Port::Baud115200);
    auto t = serial::Timeout::simpleTimeout(250);
//    outPort.setTimeout(t);

    outPort.open();

    inPort.setPort("/dev/ttyMUE6");
    inPort.setBaudrate(Rs485Port::Baud115200);
//    inPort.setTimeout(t);

    inPort.open();

    std::string txt = "hello world";

    outPort.write((uint8_t*)txt.data(), txt.length());

    std::this_thread::sleep_for(std::chrono::milliseconds(500));

    std::string outtxt;
    auto size = inPort.read(outtxt, 128);
    REQUIRE(size > 0);

    REQUIRE(outtxt == txt);

}
wjwwood commented 8 years ago

Cool, would you consider opening a pull request to that effect? If you do, it would nice to mention this special case in the docstring of either setTimeout or write.

I'll eventually get to this if you don't make a pr, but with the pr it will get merged sooner and you'll get attribution 😄.