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.

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

Implement usbser.sys workaround on Windows #154

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

haileys
Copy link

@haileys haileys commented Jan 18, 2024

I've run into a bug using espflash, where on Windows (and Windows alone), the reset/sync sequence doesn't work properly and I'm unable to flash a device over USB serial.

The core of the problem is that the usbser.sys driver on Windows doesn't seem to send changes to the DTR pin immediately.

There is a workaround for this problem in esptool, where upon RTS changes, it also sends a dummy DTR change with the last set value of the DTR pin:

https://github.com/espressif/esptool/blob/16e4faeeaa3f95c6b24dfdcc498ffc33924d5f5f/esptool/reset.py#L59-L62

There appears to be no ill effects to doing this, as esptool does this on every platform, not just Windows and not just when talking over USB serial. This pull request however only implements this behaviour on the Windows COMPort implementation.

I wasn't sure where the best place to implement this workaround was, in this crate, or downstream in the espflash crate, but I think this workaround might be useful for more than just espflash's purposes. But I also understand if the maintainers don't think this is an appropriate change to make in this crate!

@@ -122,6 +125,7 @@ impl COMPort {
handle: cloned_handle,
port_name: self.port_name.clone(),
timeout: self.timeout,
dtr: self.dtr,
Copy link
Author

Choose a reason for hiding this comment

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

This clones the dtr state when a COMPort is cloned, which might not always be the right behaviour.

Another option could be to remove the dtr field from this struct, and instead call DeviceIoControl with IOCTL_SERIAL_GET_DTRRTS in write_request_to_send to find the current state of the DTR pin, as described in this Stack Overflow answer. It's another potentially fallible syscall though, so I just went with this approach for now. Happy to make this change if desired however

@sirhcel
Copy link
Contributor

sirhcel commented Apr 29, 2024

Thank you very much for finding this workaround and filing the PR! If I remember correctly, I stumbled this issue some time ago as well, scratched my head and went for repeatedly setting the status for some time. The workaround in this PR looks better and from my perspective this is something, other users of the crate will benefit from too.

Is DTR always set to false/low when opening a serial port on Windows? In this case we would get away with assuming its state. I would still prefer to use a mutex or an AtomicBool to keep clones of the serial port in sync.

If not, I would prefer to take the overhead of the additional syscall to keep this signal in its original state. In case DeviceIoControl fails, the port could still refrain from touching DTR and failing silently. Do you see cases where calling DeviceIoControl might likely fail?

@RossSmyth
Copy link
Contributor

Is DTR always set to false/low when opening a serial port on Windows?

Since COMPort implements FromRawHandle we cannot assume that this library constructs the COM port. It seems that this PR just sets the dtr field to false, but that is probably not a correct assumption.

Probably the most sane way to do this while allowing clones is the following:

pub struct COMPort {
    handle: HANDLE,
    timeout: Duration,
    port_name: Option<String>,
    // keeps track of DTR state so we can re-send DTR on RTS change to work
    // around USB Serial driver misbehaviour
    // Put an AtomicBool in an Arc so that all clones will be coherent.
    dtr: Arc<AtomicBool>,
}

Then clone the Arc rather than copying a bool. Then:

fn write_data_terminal_ready(&mut self, level: bool) -> Result<()> {
        self.dtr.store(level, Ordering::Relaxed);

Since we aren't creating any happens-before edges, just a relaxed ordering is fine.

Issues:

  1. Increase the size of the COMPort struct more
  2. Adds an indirection on every RTS and DTR

If size is a problem I would say that using triomphe::Arc instead of std's Arc would be a good solution as there should not be any cycles. For the indirection, well serial comms isn't that performance sensitive so personally I would not worry about it.

@@ -152,6 +156,7 @@ impl COMPort {
handle: handle as HANDLE,
timeout: Duration::from_millis(100),
port_name: None,
dtr: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would do the call to DeviceIoControl here since we cannot assume the state. But if the call fails (which I would consider unlikely), the most sane option is to just panic as it means that the handle from FFI did something weird.

@sirhcel
Copy link
Contributor

sirhcel commented Jul 29, 2024

Since COMPort implements FromRawHandle we cannot assume that this library constructs the COM port. It seems that this PR just sets the dtr field to false, but that is probably not a correct assumption.

Thank you for your in-depth look @RossSmyth! The implementation of FromRawHandle makes a strong case for using DeviceIoControl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants