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

Error in braille display driver when changing timeout of a serial connection #6035

Closed
dkager opened this issue Jun 2, 2016 · 21 comments
Closed
Assignees
Milestone

Comments

@dkager
Copy link
Collaborator

@dkager dkager commented Jun 2, 2016

This error usually occurs when using the HumanWare Brailliant BI, firmware v2.0.15 on USB (Serial), while the computer is under some load. Reproduced on Windows 7 and 10, and first reported in #5195.

ERROR - unhandled exception (16:00:36):
Traceback (most recent call last):
  File "_ctypes/callbacks.c", line 314, in 'calling callback function'
  File "hwIo.pyc", line 116, in _ioDone
  File "hwIo.pyc", line 183, in _notifyReceive
  File "serial\serialutil.pyc", line 377, in setTimeout
  File "serial\serialwin32.pyc", line 177, in _reconfigurePort
ValueError: Cannot configure port, some setting was wrong. Original message: [Error 31] A device attached to the system is not functioning.

It happens either when setting or clearing the timeout in hwIo.Serial._notifyReceive:

        # Set the timeout for onReceive in case it does a sync read.
        self._ser.timeout = self._origTimeout
        super(Serial, self)._notifyReceive(data)
        self._ser.timeout = None

So either on the second or the forth line of that code fragment. My initial guess is that either the display has more to send and doesn't like the reconfiguration, or the communication timeout is too short. I haven't investigated further. This may also apply to Bluetooth and/or to other displays.

@jcsteh
Copy link
Contributor

@jcsteh jcsteh commented Jun 2, 2016

@jcsteh jcsteh self-assigned this Jun 2, 2016
@jcsteh jcsteh added this to the 2016.3 milestone Jun 2, 2016
@dkager
Copy link
Collaborator Author

@dkager dkager commented Jun 3, 2016

Or maybe a way to only set the timeout could be added to pyserial? The current behavior has quite a bit of overhead if only the timeout changed.

@dkager
Copy link
Collaborator Author

@dkager dkager commented Jun 3, 2016

Hmm, seems a local tweak is much less fuss indeed. But while we're at it, would it be worth it to update to the latest pyserial? NVDA is a little behind.

@jcsteh
Copy link
Contributor

@jcsteh jcsteh commented Jun 3, 2016

@dkager
Copy link
Collaborator Author

@dkager dkager commented Jun 3, 2016

Local change sounds good, provided we have access to the port’s handle.

From: James Teh [mailto:notifications@github.com]
Sent: Friday, June 3, 2016 13:29
To: nvaccess/nvda nvda@noreply.github.com
Cc: Davy Kager mail@davykager.nl; Author author@noreply.github.com
Subject: Re: [nvaccess/nvda] Error in braille display driver when changing timeout of a serial connection (#6035)

I vaguely recall the newest pyserial was a lot heavier or was a big API
change or something, but I could also be dreaming. If it isn't a huge
change, we can certainly update. I don't really want to fork it for the
timeout thing, though; I'd rather just make the change locally.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #6035 (comment) , or mute the thread https://github.com/notifications/unsubscribe/AC9y8b4nxNycjwu5WLeDjoE-A3pgero0ks5qIA_sgaJpZM4Is06x . https://github.com/notifications/beacon/AC9y8Q5h9Zun5m27xm1YybGz1FU5ir12ks5qIA_sgaJpZM4Is06x.gif

@dkager
Copy link
Collaborator Author

@dkager dkager commented Jun 4, 2016

There is one API change that is relevant to hwIo.Serial in pyserial/pyserial@3ad62fb. This renames hComPort to _port_handle.

@dkager
Copy link
Collaborator Author

@dkager dkager commented Jun 6, 2016

Well, I've been running with pyserial 3.1 and the proposed fix for this issue for a while and it seems stable. I'll build and run this on the PC that gives me the most errors soon.

@jcsteh
Copy link
Contributor

@jcsteh jcsteh commented Jun 6, 2016

@dkager
Copy link
Collaborator Author

@dkager dkager commented Jun 7, 2016

I tried to work around the inWaiting change by adding a method that returns the property. However, there is no real reason to upgrade to pyserial 3.1. But looking at the changelog, maybe we can update to 2.7 which is the latest in the 2.x range, brings a number of bug fixes and should be backwards compatible. The changelog even mentions the error I’m getting, though in another context I think.

From: James Teh [mailto:notifications@github.com]
Sent: Tuesday, June 7, 2016 01:48
To: nvaccess/nvda nvda@noreply.github.com
Cc: Davy Kager mail@davykager.nl; Author author@noreply.github.com
Subject: Re: [nvaccess/nvda] Error in braille display driver when changing timeout of a serial connection (#6035)

Looking briefly, it seems pyserial 3.0 also changed inWaiting to a
property instead of a function, among other things. That's going to
break quite a few drivers. Because of that (and because we don't have
access to most of these displays), I think I'd rather drop updating
unless there's a really solid advantage to doing so.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #6035 (comment) , or mute the thread https://github.com/notifications/unsubscribe/AC9y8a6yKOb-sj2kevKo6mZDxOyHvxo4ks5qJLGtgaJpZM4Is06x . https://github.com/notifications/beacon/AC9y8QzCvymF9BA5whLgu9N0Sm7twR3vks5qJLGtgaJpZM4Is06x.gif

@jcsteh
Copy link
Contributor

@jcsteh jcsteh commented Jun 7, 2016

@dkager
Copy link
Collaborator Author

@dkager dkager commented Jun 7, 2016

Huh, I thought the old behavior already was for inWaiting to be a method. E.g. in the BrailleNote driver:

while self._serial is not None and self._serial.inWaiting():

The difference is that now it is an alias of a pyserial method, whereas with pyserial 3.0+ it would be an explicit method returning a property. But I agree upgrading to 3.x isn't worth it considering that there are no known issues. I feel good about upgrading to 2.7, though!

My clone of pyserial is just over 2MB, so a submodule should be fine. There are a few more Python files besides the ones NVDA already ships with, a matter of a few kilobytes. Should I go over to BitBucket and open an issue on nvda-misc-deps?

@jcsteh
Copy link
Contributor

@jcsteh jcsteh commented Jun 7, 2016

@dkager
Copy link
Collaborator Author

@dkager dkager commented Jun 8, 2016

Yeah, if they use serial.Serial directly that does ruin the nice abstraction we've got going on.
So let's stick with 2.7 and the few lines of extra code to set the timeout. Will do a PR after I run this code for a day or so.

@jcsteh
Copy link
Contributor

@jcsteh jcsteh commented Jun 8, 2016

@dkager
Copy link
Collaborator Author

@dkager dkager commented Jun 8, 2016

Yep yep. By the way, any chance misc-deps is making its way to GitHub? :)

@jcsteh
Copy link
Contributor

@jcsteh jcsteh commented Jun 8, 2016

dkager added a commit to dkager/nvda that referenced this issue Jun 14, 2016
michaelDCurran added a commit that referenced this issue Aug 4, 2016
…patible and HumanWare Brailliant B braille displays. (#6035)
@michaelDCurran
Copy link
Contributor

@michaelDCurran michaelDCurran commented Aug 4, 2016

Fixed in 9d78cdb

@dkager
Copy link
Collaborator Author

@dkager dkager commented Aug 10, 2016

For the Brailliant BI, this line in brailleDisplayDrivers\brailliantB.py now sometimes triggers:

log.debugWarning("Display at %r reports communication not allowed" % self._dev.port)

In turn this throws an error because there is no attribute port. Presumably because in hwIo.py, this attribute is only set if _isDebug() is true.

But more worrying is that this debug warning is triggered at all. I can't think of how this GitHub issue relates to it. Could the Windows 10 Anniversary Update be blamed? I upgraded a few days ago and can't recall this warning showing up before that. Nor did I get it when I ran this patch for a week before submitting it. And even now I can't reliably reproduce it.

@dkager
Copy link
Collaborator Author

@dkager dkager commented Aug 10, 2016

The actual traceback:

ERROR - hwIo.IoBase._notifyReceive (16:15:25):
Traceback (most recent call last):
  File "hwIo.pyc", line 127, in _notifyReceive
  File "brailleDisplayDrivers\brailliantB.pyc", line 177, in _serOnReceive
  File "brailleDisplayDrivers\brailliantB.pyc", line 183, in _serHandleResponse
AttributeError: 'Serial' object has no attribute 'port'
@jcsteh
Copy link
Contributor

@jcsteh jcsteh commented Aug 11, 2016

In turn this throws an error because there is no attribute port. Presumably because in hwIo.py, this attribute is only set if _isDebug() is true.

Actually, only a port local variable is set if debugging, not self.port. This is definitely a bug. I think a reasonable solution is to add a port attribute to hwIo.Serial.

But more worrying is that this debug warning is triggered at all.

The Brailliant sometimes refuses connections, particularly if you try to connect too quickly via Bluetooth. It is probably highly time sensitive. There's already a hack to try again if this gets thrown on the first attempt. Is this not working for you?

I can't think of how this GitHub issue relates to it.

It doesn't.

Could the Windows 10 Anniversary Update be blamed?

Maybe. It's possible it affected timing.

jcsteh added a commit that referenced this issue Aug 11, 2016
…braille display driver.

serial.Serial has a port attribute and a log message in the brailliantB driver relied on this. hwIo.Serial should have this attribute as well.
Originally reported here: #6035 (comment)
@dkager
Copy link
Collaborator Author

@dkager dkager commented Aug 11, 2016

Is this not working for you?

It is working, but I don't recall seeing the error before. The display is always activated even after this error, so the second try appears to work.

jcsteh added a commit that referenced this issue Aug 12, 2016
…braille display driver. (#6257)

serial.Serial has a port attribute and a log message in the brailliantB driver relied on this. hwIo.Serial should have this attribute as well.
Originally reported here: #6035 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants