Fix: Unable to read data on windows. Fixes #29 #94
Conversation
Change SerialPort to a struct, and move the implementation to a "sys" module which selects between different possible "real" implementations.
Add a public posix module containing a posix-specific SerialPortExt trait and the BreakDuration type.
Get the windows build working. Remove the windows-specific extensions module because windows doesn't have any methods not available on posix.
One of the CI toolchains uses an older version of rustc, which doesn't include as_deref, so we just do the same thing that as_deref does internally.
Implementing the extension in the posix extension mod ensures it will appear in documentation even if documentation is built on windows, since that mod is cfg(doc).
Current timeouts are based on [this comment from !78](https://gitlab.com/susurrus/serialport-rs/-/merge_requests/78#note_343695538), copying the windows timeout settings needed to get posix-like timeout behavior. That is, reads should return available data ASAP regardless of timeouts, but stop early when there is a timeout.
EventCache will hold the HANDLE for a read/write when not in use or will produce a new handle for each thread when multiple threads try to read/write at the same time. The cache holds a single value and will deallocate extra handles when there is already a handle stored. We have one cache for a read_handle and one cache for a write_handle. The expectation is that in the normal case, at most one thread will be reading/writing at a time, and so at most one handle will be created for each of read and write. But in abnormal cases, when multiple threads try to read/write at the same time, we auto-create and auto-close extra handles as needed. Fix the into_raw_handle/into_raw_fd to drop other fields that were previously leaked.
|
Fixed communication with Raspberry Pi Pico when using this branch. |
|
Could you rebase this to the current state of the main branch? |
|
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. |
|
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. |
This sounds reasonable to me. I have little experience on Windows but that what I would expect on a POSIX system. |
|
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. |
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?
This sounds strange indeed. What does your actual dependencies in |
|
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.
|
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. |
Happy to help if I can. //! 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.
In my cargo.lock I have this: 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. 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. |
|
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. |
|
I can confirm that it works for me and fixed the problem and now I can read. |
|
Hello! |
|
@Pythomancer wrote:
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. |
|
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. |
|
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
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. |
This PR is changing 4 things.
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.