Added support for 1.5 stop bits when reading current port settings #195
Conversation
This does not allow 1.5 stop bits to be set but avoids failure to open when the port is already set to 1.5 stop bits
| @@ -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, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -242,6 +246,7 @@ impl From<StopBits> for u8 { | |||
| fn from(value: StopBits) -> Self { | |||
| match value { | |||
| StopBits::One => 1, | |||
| StopBits::OnePointFive => 1, | |||
There was a problem hiding this comment.
| StopBits::OnePointFive => 1, | |
| StopBits::OnePointFive | |
| @@ -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, | |||
There was a problem hiding this comment.
| StopBits::OnePointFive => termios.c_cflag &= !libc::CSTOPB, | |
| StopBits::OnePointFive | |
|
Thank you for the PR @robdobsn! Thank you for the first review @Skgland! I've looked into how to deal with the API part of this change but I'm still indecisive whether to provide the fractional stop bits for all platforms or just the ones supporting it. Other serial port crates seem to ignore this issue. In every case rounding up fractional stop bits as suggested by @Skgland looks like the right thing to do to me. The cited examples are making a good case for it. As changing
|
Looking further into this issue, I can't find the path where opening a serial port (and starting to read some data) triggers interpreting the ports current stop bits configuration. Running It looks like opening the port always changes the ports stop bit setting and this likely breaks communication with the ESP32-S3. But I don't see where not supporting 1.5 stop bits makes opening the port fail (like in "with an error"). How are you attempting to open the port @robdobsn and which error are you getting in this case? Could you provide an example for reproducing this issue at best?
I'm asking because just adding support for interpreting the 1.5 stop bit case would not make an issue with opening the port go away. |
|
I did see this while reviewing other projects that does the opposite of moving to one stop bit But I think two makes more sense. |
On Windows 11 an Espressif ESP32-S3 module connected via USB defaults to 1.5 stop bits when in JTAG/CDC mode
Currently serialport-rs fails to open a port when it is in this state as the stop bits setting of 1.5 is not supported
This change does not allow 1.5 stop bits to be set but avoids failure to open when the port is already set to 1.5 stop bits