Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for 1.5 stop bits when reading current port settings #195

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Added support for 1.5 stop bits when reading current port settings #195

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1aa1aab
Added support for 1.5 stop bits when reading current port settings
robdobsn Jun 6, 2024
a9f8de8
Added 1.5 stop bit case for posix
robdobsn Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions examples/hardware_check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ fn test_single_port(port: &mut dyn serialport::SerialPort, loopback: bool) {
// Test setting stop bits
println!("Testing stop bits...");
stop_bits_check!(port, StopBits::Two);
stop_bits_check!(port, StopBits::OnePointFive);
stop_bits_check!(port, StopBits::One);

// Test bytes to read and write
Expand Down
5 changes: 5 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,9 @@ pub enum StopBits {
/// One stop bit.
One,

/// One and a half stop bits.
OnePointFive,

/// Two stop bits.
Two,
}
Expand All @@ -233,6 +236,7 @@ impl fmt::Display for StopBits {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match *self {
StopBits::One => write!(f, "One"),
StopBits::OnePointFive => write!(f, "OnePointFive"),
StopBits::Two => write!(f, "Two"),
}
}
Expand All @@ -242,6 +246,7 @@ impl From<StopBits> for u8 {
fn from(value: StopBits) -> Self {
match value {
StopBits::One => 1,
StopBits::OnePointFive => 1,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
StopBits::OnePointFive => 1,
StopBits::OnePointFive |

StopBits::Two => 2,
}
}
Expand Down
1 change: 1 addition & 0 deletions src/posix/termios.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ pub(crate) fn set_data_bits(termios: &mut Termios, data_bits: DataBits) {
pub(crate) fn set_stop_bits(termios: &mut Termios, stop_bits: StopBits) {
match stop_bits {
StopBits::One => termios.c_cflag &= !libc::CSTOPB,
StopBits::OnePointFive => termios.c_cflag &= !libc::CSTOPB,
Copy link

@Skgland Skgland Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is ideal to implicitly treat 1.5 as 1 here.
This might work for reading as we get a longer stop bit than we require, but for writing we would send a too short stop bit, causing the symbol to not be properly recognized by the receiver.

Based on this stack overflow answer in some circumstances treating 1.5 as 2 might even be expected.

I don't think adding such a special case here is reasonably possible, so my suggestion would be to make this failable (i.e. return a Result) and return an Err for 1.5 on posix.

Copy link

@Skgland Skgland Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note on how this is treated elsewhere

  • pyserial treats 1.5 stop bits as 2 doc link
  • in some support forum it is suggested to use 2 as a replacement for 1.5 forum link

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the case I'm interested in I don't think it makes any difference what value is selected as the connection is actually implemented on the device as a USB CDC profile. There is no physical layer transmission of serial data in this situation. The issue is that the serialport library fails to connect to the device based on the state that it is in when the connection attempt is made. Further, I would have thought that the job of the library is to define the stop bits setting (along with other communications parameters) rather than declaring them. As I see it this is not a question of what stop bits setting to choose when the user requests 1.5 stop bits but of ensuring the library doesn't fail to connect to a device which is currently reporting that it is configured for 1.5 stop bits.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment, if you specify that you want to open a serial port with 2 stop bits (or 1 stop bit for that matter) and the port is currently set to have 1.5 stop bits then the open fails. That makes the library unusable on devices that default to 1.5 stop bits. All I am trying to do is to stop this scenario from failing. I am not trying to add support for 1.5 stop bits and I don't think that would be possible (or desirable) without significant changes that probably go beyond this library.

Copy link

@Skgland Skgland Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not arguing against adding 1.5 stop bits. I just wanted to argue that treating 1.5 as 2 rather than 1 appears to be more consistent with other sources.
As such I would propose to either change the code to treat 1.5 as 2.0 or reject 1.5 (i.e. panic) for set_stop_bits on unix and <u8 as From<StopBits>>::from in general. Either way I am not a maintainer of this project and the final decision lies in their not my purview.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't care what happens when you try to set 1.5 stop bits. It could reject or treat as 1 or 2. That doesn't matter to me. What does matter is what happens when the port is already set to 1.5 stop bits. In this case currently you cannot do anything with the port. When you try to open the port (with 1 or 2 stop bits) it fails. That is what I am trying to get fixed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
StopBits::OnePointFive => termios.c_cflag &= !libc::CSTOPB,
StopBits::OnePointFive |

StopBits::Two => termios.c_cflag |= libc::CSTOPB,
};
}
Expand Down
1 change: 1 addition & 0 deletions src/windows/com.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ impl SerialPort for COMPort {
let dcb = dcb::get_dcb(self.handle)?;
match dcb.StopBits {
TWOSTOPBITS => Ok(StopBits::Two),
ONE5STOPBITS => Ok(StopBits::OnePointFive),
ONESTOPBIT => Ok(StopBits::One),
_ => Err(Error::new(
ErrorKind::Unknown,
Expand Down
1 change: 1 addition & 0 deletions src/windows/dcb.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ pub(crate) fn set_parity(dcb: &mut DCB, parity: Parity) {
pub(crate) fn set_stop_bits(dcb: &mut DCB, stop_bits: StopBits) {
dcb.StopBits = match stop_bits {
StopBits::One => ONESTOPBIT,
StopBits::OnePointFive => ONE5STOPBITS,
StopBits::Two => TWOSTOPBITS,
};
}
Expand Down