From b50bbc38c79b6e4172736093c0f18befc9b2fced Mon Sep 17 00:00:00 2001 From: Christian Meusel Date: Sun, 4 Aug 2024 13:08:04 +0200 Subject: [PATCH 1/3] Don't ingnore errors when setting baud rate "Don't you think we should tell them that this baud rate is not supported? Nah. Just smile and wave boys smile, smile and wave." While this makes great movies, this does not make setting a knowingly unsupported baud rate a great experience. At least when in comes to debugging. Play nice and report this and other errors related to baud rates back. --- src/posix/termios.rs | 19 ++++++++++++------- src/posix/tty.rs | 4 ++-- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/posix/termios.rs b/src/posix/termios.rs index f53af80..cc5671e 100644 --- a/src/posix/termios.rs +++ b/src/posix/termios.rs @@ -213,11 +213,12 @@ pub(crate) fn set_stop_bits(termios: &mut Termios, stop_bits: StopBits) { )) ) ))] -pub(crate) fn set_baud_rate(termios: &mut Termios, baud_rate: u32) { +pub(crate) fn set_baud_rate(termios: &mut Termios, baud_rate: u32) -> Result<()> { termios.c_cflag &= !nix::libc::CBAUD; termios.c_cflag |= nix::libc::BOTHER; termios.c_ispeed = baud_rate; termios.c_ospeed = baud_rate; + Ok(()) } // BSDs use the baud rate as the constant value so there's no translation necessary @@ -227,9 +228,10 @@ pub(crate) fn set_baud_rate(termios: &mut Termios, baud_rate: u32) { target_os = "netbsd", target_os = "openbsd" ))] -pub(crate) fn set_baud_rate(termios: &mut Termios, baud_rate: u32) { - // Ignore the return value because this should never fail - unsafe { libc::cfsetspeed(termios, baud_rate.into()) }; +pub(crate) fn set_baud_rate(termios: &mut Termios, baud_rate: u32) -> Result<()> { + let res = unsafe { libc::cfsetspeed(termios, baud_rate.into()) }; + nix::errno::Errno::result(res)?; + Ok(()) } #[cfg(all( @@ -240,7 +242,9 @@ pub(crate) fn set_baud_rate(termios: &mut Termios, baud_rate: u32) { target_arch = "powerpc64" ) ))] -pub(crate) fn set_baud_rate(termios: &mut Termios, baud_rate: u32) { +pub(crate) fn set_baud_rate(termios: &mut Termios, baud_rate: u32) -> Result<()> { + use crate::{Error, ErrorKind}; + use self::libc::{ B1000000, B1152000, B1500000, B2000000, B2500000, B3000000, B3500000, B4000000, B460800, B500000, B576000, B921600, @@ -281,8 +285,9 @@ pub(crate) fn set_baud_rate(termios: &mut Termios, baud_rate: u32) { 3_000_000 => B3000000, 3_500_000 => B3500000, 4_000_000 => B4000000, - _ => return, + _ => return Err(Error::new(ErrorKind::InvalidInput, "Unsupported baud rate")), }; let res = unsafe { libc::cfsetspeed(termios, baud_rate) }; - nix::errno::Errno::result(res).expect("cfsetspeed failed"); + nix::errno::Errno::result(res)?; + Ok(()) } diff --git a/src/posix/tty.rs b/src/posix/tty.rs index b1d68cf..fe7e849 100644 --- a/src/posix/tty.rs +++ b/src/posix/tty.rs @@ -176,7 +176,7 @@ impl TTYPort { termios::set_data_bits(&mut termios, builder.data_bits); termios::set_stop_bits(&mut termios, builder.stop_bits); #[cfg(not(any(target_os = "ios", target_os = "macos")))] - termios::set_baud_rate(&mut termios, builder.baud_rate); + termios::set_baud_rate(&mut termios, builder.baud_rate)?; #[cfg(any(target_os = "ios", target_os = "macos"))] termios::set_termios(fd.0, &termios, builder.baud_rate)?; #[cfg(not(any(target_os = "ios", target_os = "macos")))] @@ -630,7 +630,7 @@ impl SerialPort for TTYPort { ))] fn set_baud_rate(&mut self, baud_rate: u32) -> Result<()> { let mut termios = termios::get_termios(self.fd)?; - termios::set_baud_rate(&mut termios, baud_rate); + termios::set_baud_rate(&mut termios, baud_rate)?; termios::set_termios(self.fd, &termios) } From ee4bb5e7a35478074e9c8cd8c614c23db13480b4 Mon Sep 17 00:00:00 2001 From: Christian Meusel Date: Fri, 30 Aug 2024 21:17:15 +0200 Subject: [PATCH 2/3] Add hardware tests for setting baud rates --- Cargo.toml | 1 + tests/test_baudrate.rs | 191 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 192 insertions(+) create mode 100644 tests/test_baudrate.rs diff --git a/Cargo.toml b/Cargo.toml index 5957033..9b22250 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -51,6 +51,7 @@ os_str_bytes = ">=6.0, <6.6.0" quickcheck = "1.0.3" quickcheck_macros = "1.0.0" rstest = { version = "0.12.0", default-features = false } +rstest_reuse = "0.6.0" rustversion = "1.0.16" [features] diff --git a/tests/test_baudrate.rs b/tests/test_baudrate.rs new file mode 100644 index 0000000..4e33b25 --- /dev/null +++ b/tests/test_baudrate.rs @@ -0,0 +1,191 @@ +mod config; + +use config::{hw_config, HardwareConfig}; +use rstest::rstest; +use rstest_reuse::{self, apply, template}; +use serialport::SerialPort; +use std::ops::Range; + +const RESET_BAUD_RATE: u32 = 300; + +/// Returs an acceptance interval for the actual baud rate returned from the device after setting +/// the supplied value. For example, the CP2102 driver on Linux returns the baud rate actually +/// configured a the device rather than the the value set. +fn accepted_actual_baud_for(baud: u32) -> Range { + let delta = baud / 200; + baud.checked_sub(delta).unwrap()..baud.checked_add(delta).unwrap() +} + +fn check_baud_rate(port: &dyn SerialPort, baud: u32) { + let accepted = accepted_actual_baud_for(baud); + let actual = port.baud_rate().unwrap(); + assert!(accepted.contains(&actual)); +} + +#[template] +#[rstest] +#[case(9600)] +#[case(57600)] +#[case(115200)] +fn standard_baud_rates(#[case] baud: u32) {} + +#[template] +#[rstest] +#[case(1000)] +#[case(42000)] +#[case(100000)] +fn non_standard_baud_rates(#[case] baud: u32) {} + +/// Test cases for setting the baud rate via [`SerialPortBuilder`]. +mod builder { + use super::*; + + #[apply(standard_baud_rates)] + #[cfg_attr(feature = "ignore-hardware-tests", ignore)] + fn test_standard_baud_rate(hw_config: HardwareConfig, #[case] baud: u32) { + let port = serialport::new(hw_config.port_1, RESET_BAUD_RATE) + .baud_rate(baud) + .open() + .unwrap(); + check_baud_rate(port.as_ref(), baud); + } + + #[apply(non_standard_baud_rates)] + #[cfg_attr( + any( + feature = "ignore-hardware-tests", + not(all(target_os = "linux", target_env = "musl")), + ), + ignore + )] + fn test_non_standard_baud_rate_fails_where_not_supported( + hw_config: HardwareConfig, + #[case] baud: u32, + ) { + let res = serialport::new(hw_config.port_1, RESET_BAUD_RATE) + .baud_rate(baud) + .open(); + assert!(res.is_err()); + } + + #[apply(non_standard_baud_rates)] + #[cfg_attr( + any( + feature = "ignore-hardware-tests", + all(target_os = "linux", target_env = "musl"), + ), + ignore + )] + fn test_non_standard_baud_rate_succeeds_where_supported( + hw_config: HardwareConfig, + #[case] baud: u32, + ) { + let port = serialport::new(hw_config.port_1, RESET_BAUD_RATE) + .baud_rate(baud) + .open() + .unwrap(); + check_baud_rate(port.as_ref(), baud); + } +} + +/// Test cases for setting the baud rate via [`serialport::new`]. +mod new { + use super::*; + + #[apply(standard_baud_rates)] + #[cfg_attr(feature = "ignore-hardware-tests", ignore)] + fn test_standard_baud_rate(hw_config: HardwareConfig, #[case] baud: u32) { + let port = serialport::new(hw_config.port_1, baud).open().unwrap(); + check_baud_rate(port.as_ref(), baud); + } + + #[apply(non_standard_baud_rates)] + #[cfg_attr( + any( + feature = "ignore-hardware-tests", + not(all(target_os = "linux", target_env = "musl")), + ), + ignore + )] + fn test_non_standard_baud_rate_fails_where_not_supported( + hw_config: HardwareConfig, + #[case] baud: u32, + ) { + assert!(serialport::new(hw_config.port_1, baud).open().is_err()); + } + + #[apply(non_standard_baud_rates)] + #[cfg_attr( + any( + feature = "ignore-hardware-tests", + all(target_os = "linux", target_env = "musl"), + ), + ignore + )] + fn test_non_standard_baud_rate_succeeds_where_supported( + hw_config: HardwareConfig, + #[case] baud: u32, + ) { + let port = serialport::new(hw_config.port_1, baud).open().unwrap(); + check_baud_rate(port.as_ref(), baud); + } +} + +/// Test cases for setting the baud rate via [`SerialPort::set_baud`]. +mod set_baud { + use super::*; + + #[apply(standard_baud_rates)] + #[cfg_attr(feature = "ignore-hardware-tests", ignore)] + fn test_standard_baud_rate(hw_config: HardwareConfig, #[case] baud: u32) { + let mut port = serialport::new(hw_config.port_1, RESET_BAUD_RATE) + .open() + .unwrap(); + check_baud_rate(port.as_ref(), RESET_BAUD_RATE); + + port.set_baud_rate(baud).unwrap(); + check_baud_rate(port.as_ref(), baud); + } + + #[apply(non_standard_baud_rates)] + #[cfg_attr( + any( + feature = "ignore-hardware-tests", + not(all(target_os = "linux", target_env = "musl")), + ), + ignore + )] + fn test_non_standard_baud_rate_fails_where_not_supported( + hw_config: HardwareConfig, + #[case] baud: u32, + ) { + let mut port = serialport::new(hw_config.port_1, RESET_BAUD_RATE) + .open() + .unwrap(); + check_baud_rate(port.as_ref(), RESET_BAUD_RATE); + + assert!(port.set_baud_rate(baud).is_err()); + check_baud_rate(port.as_ref(), RESET_BAUD_RATE); + } + + #[apply(non_standard_baud_rates)] + #[cfg_attr( + any( + feature = "ignore-hardware-tests", + all(target_os = "linux", target_env = "musl"), + ), + ignore + )] + fn test_non_standard_baud_rate_succeeds_where_supported( + hw_config: HardwareConfig, + #[case] baud: u32, + ) { + let mut port = serialport::new(hw_config.port_1, RESET_BAUD_RATE) + .open() + .unwrap(); + check_baud_rate(port.as_ref(), RESET_BAUD_RATE); + + port.set_baud_rate(baud).unwrap(); + check_baud_rate(port.as_ref(), baud); + } +} From 1ad82860448d11888efe851602a8dd7919518eac Mon Sep 17 00:00:00 2001 From: Christian Meusel Date: Sat, 31 Aug 2024 00:18:51 +0200 Subject: [PATCH 3/3] Add changelog for fixing ignoring errors from setting baud rates --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f19ec7..bbbf72c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,9 @@ project adheres to [Semantic Versioning](https://semver.org/). ### Added ### Changed ### Fixed +* Fix ignoring errors from setting baud rate and ignoring unsupported baud + rates. + [#213](https://github.com/serialport/serialport-rs/pull/213) ### Removed