Avoid error when changing the read timeout on a serial port #6068
Conversation
…erything else too. re nvaccess#6035
|
It would be hugely appreciated if this could be perused for 2016.2 (don't we all say that, haha). I have recently started to run large'ish batches of unit tests and that constantly causes this error even when working in another window. Still no lemons on this branch. |
| # timeout=None to clear all timeouts, else a positive number of milliseconds. | ||
| timeouts = COMMTIMEOUTS() | ||
| if timeout is not None: | ||
| timeouts.ReadTotalTimeoutConstant = max(int(timeout * 1000), 1) |
There was a problem hiding this comment.
While this certainly sets the read timeout, unless I'm missing something, it also resets all other timeouts to 0 (including the write timeout). We can't assume the caller is only interested in the read timeout. Drivers do set the write timeout. In fact, no write timeout could be quite bad for real serial ports, since they may have no way of knowing the device was disconnected.
|
True. This patch is based on hwIo, which never sets the other timeouts. It doesn't help that drivers also talk to the serial port directly. This can probably be solved by first requesting the current timeouts, or maybe by grabbing these parameters from pyserial. Though I think those are private, so accessing them is ugly. |
|
hwIo.Serial doesn't set the timeouts, but it's just a thin wrapper
around pyserial and passes most constructor arguments, so they do get
set. It's true that they're private, but we're already accessing
hComPort (which kinda looks private even though it doesn't have a _
prefix), so I think we can live with this ugliness. Basically, we're
just using pyserial here to avoid reinventing the wheel.
|
|
In pyserial 3.x hComPort actually has an underscore. I am using that code to deal with timeouts, since it is backwards compatible but looks a bit nicer (using a struct instead of a tuple). Of course for NVDA we decided to stick with 2.7. I'll update my patch and test again with the Brailliant. |
|
Btw, does this change actually depend on pyserial 2.7? Still happy to upgrade, but I'd prefer to address this separately if possible so we can isolate these two changes. That way, it'll be easy to figure out where a regression (if any) lies.
|
|
No dependency on pyserial 2.7, the only thing used from pyserial (except for attributes) is the _COMMTIMEOUTS struct. That is present in 2.5 as well. |
|
Pushed a fix and will test on Monday. |
| # Adapted from pyserial 3.1.1. | ||
| timeouts = COMMTIMEOUTS() | ||
| if timeout is None: | ||
| pass # default of all zeros is OK |
There was a problem hiding this comment.
I'd prefer we didn't have empty blocks in an if like this. I'd change the else below to elif timeout is not None. Same with the later pass statements.
|
I agree, but thought it might be less clear where the code came from if edited. But I'll change it. :) |
|
Yeah, I was wondering whether that might be your reason. :)
|
|
Just pushed a fixup for both issues, and actually got to test it this time. |
|
Can someone please provide a suitable what's new entry on this pr? Then I can merge this to master. I don't quite understand its user-facing change. |
|
In Bug Fixes:
|
|
Actually, that is better written as: NVDA no longer occasionally stops rrecognising key presses on Baum compatible and HumanWare Bralliant B braille displays. |
Related issue: #6035
This explicitly uses SetCommTimeouts to change the read timeout of a serial port. In pyserial, the entire port is reconfigured, sometimes causing an error if the computer was busy. I've run this patch on the machine that was causing me the most problems for two days and didn't get the error.
Another request is to turn pyserial into a Git submodule and upgrade it to version 2.7 (tag release2_7).