Fix: Unable to read data on windows. Fixes #29

[LukaOber]

This PR is changing 4 things.

1. Set fDtrControl to true
2. Set fRtsControl to true
3. Change the ReadEventCache
4. Clears the Input and Output Buffer after initializing the port

Changes 1, 2 and 3 should fix the issue of not being able to read data on Windows system. I tested this code with three different types of microcontrollers (Teensy, Arduino, ESP-32) and they all worked. The 4th change is not needed for reading, but I think clearing the buffers after initialization is the correct behavior.

All the changes I made are based on the default values for the port initialization in pyserial.

As far as I can tell, these changes bring no downsides, but I am not too familiar with Windows systems or this crate.

This PR is based on zstewar1 5.0 proposal as it is the branch I am personally working with.

[Wouter-Badenhorst]

Fixed communication with Raspberry Pi Pico when using this branch.

[eldruin]

Could you rebase this to the current state of the main branch?

[LukaOber]

Sure. But I will probably not get to it until June, as I am currently travelling for work and don't have access to a Windows Computer.

[kinire98]

I tested with my program with this PR rather than with the crates.io version and it solved my problem of timeouts and now it works perfectly. Only thing it seems to read nothing from the buffer, don't know if it's my fault.

[sirhcel]

The 4th change is not needed for reading, but I think clearing the buffers after initialization is the correct behavior.

This sounds reasonable to me. I have little experience on Windows but that what I would expect on a POSIX system.

[Krahos]

It doesn't solve the timeouts problems described on #29 for me. Weirdly enough, the 4.2 version was working fine up until yesterday. The microcontroller I have is STM32L476QEI6.

[sirhcel]

It doesn't solve the timeouts problems described on #29 for me.

We seem to be on the short-handed side of Windows developers here. Could you help us to look into this by explaining your communication scenario and sharing the code you are experiencing this issue with with us?

Weirdly enough, the 4.2 version was working fine up until yesterday. The microcontroller I have is STM32L476QEI6.

This sounds strange indeed. What does your actual dependencies in Cargo.lock with respect to serialport-rs look like? We released 4.2.1 last week and maybe this causes an issue. Could you give us an example of the communication taking place in this case?

[mlsvrts]

So, I'm not sure I agree with the changes proposed by this MR:

1/2) I think it's correct to leave DTR/RTS disabled by default -- not all hardware expects and uses these signals. I haven't looked too closely, but if we don't have support for setting these flow control signals, I would be in favor of an MR to add a common API to allow it. I do think that could be part of a 5.0 milestone. However, simply changing how they are initialized for windows does not seem like the correct solution.

3. I haven't looked into this at all :)

4. I'm not sure that it's always correct to clear the input/output buffers on init. Consider a scenario where I'm polling for a USB-Serial bootloader to enumerate, then attaching and reading the log -- clearing input buffers could cause me to miss the start of the log. I think users calling an API to clear buffers/providing a port builder option for it is a more robust solution.

[mlsvrts]

It doesn't solve the timeouts problems described on #29 for me. Weirdly enough, the 4.2 version was working fine up until yesterday. The microcontroller I have is STM32L476QEI6.

I think the changes in this MR will only help for hardware that is blocking data transmission on RTS/DTR. If you are able to read some data, but your reads always timeout, it's more likely that you're fighting with the windows COMTIMEOUTS configuration.

[Krahos]

We seem to be on the short-handed side of Windows developers here. Could you help us to look into this by explaining your communication scenario and sharing the code you are experiencing this issue with with us?

Happy to help if I can. I have a USB device developed in house, which exposes 2 serial ports. One is used to send commands to the device, the other one is used to read data. I'm posting here the whole interface with the device, the rest of the software is just an abstraction over this, so I don't think it's relevant:

//! This module provide raw access to the mela sensor through the Read and Write traits.

use std::{
    io::{Read, Write},
    time::Duration,
};

use anyhow::Context;
use serialport::{SerialPort, SerialPortType};

/// This trait is used as an abstraction over the `RawMela` type.
pub trait MelaSensor: Read + Write + Sized {
    /// Used as initialization for the handler.
    fn new() -> Result<Self, SensorError>;
}

/// This object represents the mela network. All interactions with the network use this.
pub struct RawMela {
    command_port: CommandPort,
    data_port: DataPort,
}

impl MelaSensor for RawMela {
    /// Looks for and opens a mela sensor connected to the machine.
    fn new() -> Result<Self, SensorError> {
        let port_names = Self::enumerate().context("Serial port enumeration failed")?;
        if port_names.len() < 2 {
            return Err(SensorError::Ports);
        }
        let (command_port, data_port) =
            Self::open(&port_names[0], &port_names[1]).context("Opening serial ports failed")?;

        Ok(RawMela {
            command_port,
            data_port,
        })
    }
}

impl Read for RawMela {
    fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> {
        self.data_port.as_mut().read_exact(buf)?;
        Ok(buf.len())
    }
}

impl Write for RawMela {
    fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
        self.command_port.as_mut().write_all(buf)?;
        Ok(buf.len())
    }

    fn flush(&mut self) -> std::io::Result<()> {
        Ok(())
    }
}

impl RawMela {
    /// Scans the USB ports of the system and returns their name, if at least one mela sensor is connected to them.
    fn enumerate() -> anyhow::Result<Vec<String>> {
        let ports = serialport::available_ports()
            .context("Serial port crate failed to return available ports")?;

        let mut mela_ports = Vec::new();

        for port in ports {
            if let SerialPortType::UsbPort(info) = port.port_type {
                if info.vid == 291 && info.pid == 1 {
                    mela_ports.push(port.port_name)
                }
            }
        }

        Ok(mela_ports)
    }

    /// Opens the USB ports of a mela sensor.
    fn open(command_port: &str, data_port: &str) -> anyhow::Result<(CommandPort, DataPort)> {
        let command = serialport::new(command_port, 115200)
            .timeout(Duration::from_secs(15))
            .open()
            .context("Failed to open command port")?;

        let data = serialport::new(data_port, 115200)
            .timeout(Duration::from_secs(15))
            .open()
            .context("Failed to open data port")?;

        Ok((CommandPort(command), DataPort(data)))
    }
}

/// The command port used to send commands to the sensor.
pub struct CommandPort(Box<dyn SerialPort>);

impl AsMut<Box<dyn SerialPort>> for CommandPort {
    fn as_mut(&mut self) -> &mut Box<dyn SerialPort> {
        &mut self.0
    }
}

/// The data port used to read data from the sensor.
pub struct DataPort(Box<dyn SerialPort>);

impl AsMut<Box<dyn SerialPort>> for DataPort {
    fn as_mut(&mut self) -> &mut Box<dyn SerialPort> {
        &mut self.0
    }
}

/// The possible errors that can occur when using the raw mela handler.
#[derive(Debug, thiserror::Error)]
pub enum SensorError {
    /// This variant represents the case where the data port or the command port or both weren't found.
    #[error("not enough ports found, mela sensor might not be connected")]
    Ports,
    /// Error opening or using the serial port.
    #[error("there was an error opening the serial ports")]
    SerialPort(#[from] anyhow::Error),
}

This is the code using the 4.2 version, the timeout occurs the first time I attempt to call the read. I also tried various combinations of write data terminal ready, write request to send and flow control, but same result.

This sounds strange indeed. What does your actual dependencies in Cargo.lock with respect to serialport-rs look like? We released 4.2.1 last week and maybe this causes an issue. Could you give us an example of the communication taking place in this case?

In my cargo.lock I have this:

[[package]]
name = "serialport"
version = "4.2.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "aab92efb5cf60ad310548bc3f16fa6b0d950019cb7ed8ff41968c3d03721cf12"
dependencies = [
 "CoreFoundation-sys",
 "IOKit-sys",
 "bitflags",
 "cfg-if",
 "libudev",
 "mach 0.3.2",
 "nix",
 "regex",
 "winapi",
]

It's the same for the branch using this pull request, with the only difference that the version and source are different of course.

To add more context, The timeout issue used to happen only on a few of these USB devices, so I couldn't figure out the steps to reproduce, now it seems to happen to all the 4 devices I have here. Usually I specify the dependency as serialport=4.2, but just tried with version 4.0, 4.1 and to pin point to 4.2.1 and the issue still persists, so I don't think it's a regression of the last patch.

The device works properly if operated through PuTTY on Windows and through the software on -nix, so the issue has to be somewhere in Windows implementation, unless I'm missing something.

Let me know what else I can do to help.

edit: @mlsvrts maybe I should move this comment to the issue discussion instead? Since this pull request might not be relevant.

[jerrywrice]

I have the same concern as mlsvrts. When used the DTR/RTS control lines are often specific to a given (hardware) device. Typically computer to computer serial communications, especially with USB-serial cables, do not expect these lines to be active.

I'm new to this forum, but I assume there are discussions concerning (potentially) exposing more of these types of configuration settings in a future version. It's a problem when one must rely upon default settings which are not exposed from the public module interface.

[thewh1teagle]

I can confirm that it works for me and fixed the problem and now I can read. does it gonna merged?

[Pythomancer]

Hello! I had the same issue. It appears that the read fails with the Timed Out message when the the buffer (Vec::new()) is empty. I used the following: let mut data = vec![0; 200]; That resolved the issue for me, ie. non-empty intake buffer, 200-long u8 buffer in my case. It seems to me that the Timed Out message is not very helpful for this particular failure case. Best of luck.

[akauppi]

@Pythomancer wrote:

read fails with the Timed Out message when the the buffer (Vec::new()) is empty

I, too, was bit by this. Had code like:

let mut buf: Vec<u8> = Vec::with_capacity(15);   // does not work!
let n = port.read(&mut buf).unwrap_or_else( ...

It would be nice to get at least the buffer fix (from this PR?) merged. As @Pythomancer wrote "the Timed Out message is not very helpful for this particular failure case." I agree.

[eleroy]

Hello everyone,

I've been using this branch for a while now, is there any reason why it is not merged yet ?

Thank you

[eleroy]

Hello everyone,

I've been using this branch for a while now, is there any reason why it is not merged yet ?

Thank you

My bad, I just made some changes in my code:

serial.write_data_terminal_ready(true)
serial.write_request_to_send(true)

It made everything worked on my usb cdc link with the current version.

[RossSmyth]

This PR is very large and should be split into many different ones. It is also unsound wrt to provenence as currently implemented.

This PR is doing too many things at once and make it hard to review. I would split this in to

1. Making fDtrControl & fRtsControl true
2. Adding the new EventCache (which is currently unsound atm)
3. Clearing the buffer
4. Making the objects thread safe
5. Moving the files around to a sys directory
6. Changing the API and v5 bump

Or something like that. Many of the changes are not mentioned in the PR description. I also do not agree with some of the change as mlsvrts & jerrywrice have expressed.