Add automatic background braille display detection #7741
Conversation
| SetupDiEnumDeviceInfo.argtypes = (HDEVINFO, DWORD, PSP_DEVINFO_DATA) | ||
| SetupDiEnumDeviceInfo.restype = BOOL | ||
|
|
||
| CM_Get_Device_ID = ctypes.windll.cfgmgr32.CM_Get_Device_IDW |
There was a problem hiding this comment.
@jcsteh, it seems either you added this in your initial work, or it has been removed from core and I accidentally added it again. Anyway, it is not used within hwPortUtils. I just want to make sure what you intended to do with this function.
|
As far as I'm concerned it's good with USB. I have found these lines in NVDA log: |
|
what display is this?
|
|
It is a VarioUltra. |
|
Could you provide steps to reproduce this? E.g. is this when you have bluetooth enabled and both connected your Ultra to USB and enabled it using bluetooth? |
|
Hmm, looking into your log a little more, it seems like multiple background scanners are running at the same moment. This is really something I should investigate, since this should not happen. @Andre9642: Are you able to reproduce this issue multiple times, or was is just one incident? |
|
|
Now works perfectly! :) |
|
I found another case where it doesn't work fine.
Actual behaviour: NVDA tries to connect to the VarioUltra via Bluetooth permanently. Additionally, if I plug my VarioUltra at this point, auto detection in USB doesn't work. But, if I activate Bluetooth on the VarioUltra at this stage, auto detection in USB works. |
There was a problem hiding this comment.
A first review of this code, I haven't tried it yet in practice.
Most of this seems fine to me. However, I've a few questions about the Bluetooth detection:
- Scan interval for Bluetooth is 5 seconds, but what's the timeout for a BT COM port that's unavailable?
- Is there another reliable way to check if a BT device is in range before opening it's COM port?
- Do we have any data on the impact of constant BT scanning on laptop batteries?
source/bdDetect.py
Outdated
| KEY_SERIAL = "serial" | ||
| #: Key for devices with a manufacturer specific driver | ||
| KEY_CUSTOM = "custom" | ||
| #: Key foor bluetooth devices |
| try: | ||
| return _driverDevices[driver] | ||
| except KeyError: | ||
| ret = _driverDevices[driver] = defaultdict(set) |
There was a problem hiding this comment.
Why do you use an intermediate variable here?
There was a problem hiding this comment.
This is @jcsteh's code, but I kept it this way because it is not only an intermediate variable, but it also adds the driver to the drivers dictionary and sets the value as a DefaultDict(set). The latter is something I changed, as it was a regular dict before.
Note that _getDriver is only used while adding USB and Bluetooth info, which requires a driver specific dictionary to exist.
source/bdDetect.py
Outdated
|
|
||
| # brailliantB | ||
| addUsbDevices("brailliantB", KEY_HID, {"VID_1C71&PID_C006"}) | ||
| addUsbDevices("brailliantB", KEY_CUSTOM, {"VID_1C71&PID_C005"}) |
There was a problem hiding this comment.
It is serial, yes. and if we agree to use the generic serial implementation, that's fine with me. I need someone to test that with a Brailliant driver on Windows 7 if possible, though.
source/brailleDisplayDrivers/baum.py
Outdated
| @@ -122,72 +63,29 @@ class BrailleDisplayDriver(braille.BrailleDisplayDriver): | |||
|
|
|||
| @classmethod | |||
| def check(cls): | |||
There was a problem hiding this comment.
Wouldn't it be better to make this the default implementation in the BrailleDisplayDriver base class?
Eventually, we would like this to work for all or as many displays as possible, so I'd prefer if the default implementation does the right thing for autodetection. Besides, it won't break existing drivers which already override check.
There was a problem hiding this comment.
I'm reluctant to do this, as auto detection assumes thread safety of a driver, and a driver is not thread safe by default. I'm happy to do this as soon as NV Access agrees. cc @jcsteh and @michaelDCurran
There was a problem hiding this comment.
This is not really up to me now, but one idea is to consider having the base implementation do this stuff only for thread-safe drivers. if not thread-safe, just return False. Best of both worlds.
| @classmethod | ||
| def _getTryPorts(cls, port): | ||
| for match in super(BrailleDisplayDriver,cls)._getTryPorts(port): | ||
| if not match.type==bdDetect.KEY_CUSTOM: |
There was a problem hiding this comment.
Why is this custom and not serial?
There was a problem hiding this comment.
Due to bluetooth also using serial.
|
|
||
| @classmethod | ||
| def getManualPorts(cls): | ||
| return braille.getSerialPorts(filterFunc=lambda info: "bluetoothName" in info) |
There was a problem hiding this comment.
If the BT name doesn't match, I don't think there is a reason to list the port for selection. All BT devices that match should already been given by bdDetect, so I think it's better to remove this method all together, listing ports that can't have a display would only confuse users.
There was a problem hiding this comment.
Indeed, this method wasn't there initially. I added it because there might come newer Brailliant devices with another bt prefix in the future which communicate using the same protocol. IN that case, people can always manually connect until there's an NVDA update available.
|
|
||
| # Some older displays use a HID converter and an internal serial interface | ||
| # Some older Handy Tech displays use a HID converter and an internal serial interface. | ||
| # We need to keep these IDS around here to send additional data upon connection. | ||
| USB_IDS_HID_CONVERTER = { |
There was a problem hiding this comment.
Keeping this IDs on two places doesn't feel right with me. Can't we extend a match tuple with an extra "data" or "driverHints" field that can hold driver specific info? In this case, it might be {"hidSerial": True} or something like that.
There was a problem hiding this comment.
@michaelDCurran, @jcsteh, thoughts? May be include the device name in detection data as well? That would also help for #7458.
There was a problem hiding this comment.
I'm a little wary of putting driver specific stuff in bdDetect data. It kinda feels like the data is in the wrong place. The only reason we have the data in bdDetect at all is to enable detection without loading the driver. Putting driver specific data in there feels like we're associating too much stuff with "detection" and is a very slippery slope, IMO.
| @@ -54,6 +60,31 @@ def CreateFile(fileName,desiredAccess,shareMode,securityAttributes,creationDispo | |||
| raise ctypes.WinError() | |||
| return res | |||
|
|
|||
| def createEvent(eventAttributes=None, manualReset=False, initialState=False, name=None): | |||
There was a problem hiding this comment.
Please add a docstring to document the function and parameters and/or refer to the relevant Windows API calls documentation.
There was a problem hiding this comment.
Actually, all functions in winKernel lack a doc string. That's fine with me, as this is mostly a copy of the winKernel.h equivalent. @feerrenrut, what do you think about this?
There was a problem hiding this comment.
Where it's a straight call through I don't think it's necessary. However, if there is some manipulation of variables or several calls, then it's probably useful.
| raise ctypes.WinError() | ||
| return res | ||
|
|
||
| def createWaitableTimer(securityAttributes=None, manualReset=False, name=None): |
There was a problem hiding this comment.
Please add a docstring to document the function and parameters and/or refer to the relevant Windows API calls documentation.
user_docs/en/userGuide.t2t
Outdated
| No braille means that you are not using braille. | ||
|
|
||
| Please see the [Supported Braille Displays #SupportedBrailleDisplays] section for more information about supported braille displays. | ||
| Please see the [Supported Braille Displays #SupportedBrailleDisplays] section for more information about supported braille displays and which of these support automatic detection in the backgrounds. |
|
Hi Leonard!
…-----Mensagem Original-----
De: Leonard de Ruijter
Data: 11 de novembro de 2017 12:03
Para: nvaccess/nvda
Cc: Subscribed
Assunto: Re: [nvaccess/nvda] Add automatic background braille display
detection (#7741)
Here is a try build to test for this pr:
https://ci.appveyor.com/api/buildjobs/k7g9r71iva2hcjld/artifacts/output%2Fnvda_snapshot_try-i1271-14612%2C2585630f.exe
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@Andre9642 commented on 13 nov. 2017 17:45 CET:
This is actually expected behavior. Since NVDA does not know whether the device is in range, it will try to connect in the background until either it has been found or either auto detection or bluetooth is disabled.
Now this is interesting behavior. I will try to reproduce this. |
|
@ruifontes commented on 14 nov. 2017 01:42 CET:
I assume this wasn't all you had to say? ;) |
|
@bramd reviewed and asked:
That's currently the default, the connection attempt is made by the driver. I assume around five seconds, but this should be investigated.
@jcsteh: Did you do any research into this while working on this?
No. :) However, since we actually know when we are running on batteries, we can disable polling altogether. May be we'd make this optional. |
|
Replying to a bunch of questions. @leonardder commented on Nov 11, 2017, 7:58 PM GMT+10:
Thanks for finishing it! :) I actually don't have one of these displays. I had one on loan from HumanWare when I was working on the driver. @leonardder commented on Nov 14, 2017, 3:13 PM GMT+10:
There is no straightforward way I know of with com ports. You might be able to do something interesting with the Bluetooth APIs, but getting from a com port to some Bluetooth API is probably non-trivial. I haven't looked at your code, but I know my initial implementation tried opening the port and then closing it before trying to initialise the driver. I did this because the driver had to be initialised on the main thread. If you're still doing this, I don't think we need to do so any more; we can just try the driver, and if it fails, we know the display is unavailable.
From what I've been able to determine, iOS only polls when you unlock your device. That probably doesn't quite work for us, but maybe we could come up with a similar idea; e.g. poll on foreground changes or when opening the NVDA menu or something. Coming up with something intuitive is tricky, though. |
My mistake! In reflecting, it's obvious... |
|
@Andre9642 commented on 14 nov. 2017 09:09 CET:
It is not entirely clear to me what you're suggesting here. Could you give a use case scenario? I've actually successfully reproduced your issue. Simplified str:
Result: A rescan is triggered, but it seems it does not execute properly. |
|
14-11 try build: @Andre9642: This should also fixed the last issue you described. Things should be a bit more reliable now. |
LeonarddeR
force-pushed
the
i1271
branch
2 times, most recently
from
3d890b1
to
9cc8f08
Compare
|
I"m using this for quite a few days now and all seems quite stable. As soon as the hims driver (#7712) is in master, I will port that to BdDetect and then this is ready for another round of review. |
|
Actually, there seems to be an open point. @jcsteh commented on 14 nov. 2017 07:18 CET:
I actually like the idea to poll on foreground changes. I propose the following:
|
What about a extension point? |
|
@derekriemer commented on 5 dec. 2017 05:44 CET:
I'm a bit reluctant to add an extension point for foreground changes when there already is an event. It feels a bit like creating doubled functionality. |
|
true |
|
@michaelDCurran: Your review is welcome, but I think incubation into Next should wait until at least #7732 and #7489 are in master. |
|
Addressed two additional issues:
|
|
You could avoid this recursion by using a flag to detect re-entry and just refusing to run the detection if the flag is already set. I haven't followed a lot of the discussion here, but I don't quite understand the concerns with APCs. Sure, if you queue it, it will always run. However, you can just run a APC function which checks what work it has to do based on a variable protected by a lock. If the variable says there's nothing to do (because we already did it in a previous run), we do nothing. Having said that, there's probably no harm in bdDetect having its own thread. The whole point of the new model is that it's supposed to be thread safe, so initialising from that thread should be no problem. The more threads, the more resources, but this s only one extra thread. We should probably avoid respawning the thread too often, though. I am concerned about the fact that this is a daemon thread. We shouldn't be relying on this fact to kill off the thread for us. It's fine for safety, but we really should be terminating it gracefully if possible. Again, I haven't read the code, so sorry if you're already doing this. |
…e recursion does not occur when using an APC This reverts commit 5b97f39.
…canQueued within a lock
|
@jcsteh commented on 9 Jun 2018, 07:12 CEST:
That is true indeed and that also works. I was initially thinking that we should avoid this case of recursion altogether, but if you don't have a problem with such a check, I don't have one either. I prefer to set a lock when a scan is started and release it afterwards, since that can be easily done with a with statement, since a lock is a context manager. Then, to check recursion, evaluate the state of the lock inside the APC, and if it is locked, just return early. |
LeonarddeR
requested review from
michaelDCurran and
feerrenrut
and removed request for
feerrenrut
|
I have reviewed the code, but not to the extent that I understand the overall design. This concerns me somewhat. Considering Jamie, Leonard, and Davy are all familiar and happy with the design I don't think we should hold up the delivery of this work. Still, I would like to see the design / approach documented somewhere (probably on the wiki). Particularly, I would like documentation around the threads that are running and their responsibilities, and communication points between them. This would greatly aid my ability to review this code in an effective and time efficient manner, and would serve to help any future developers maintaining this code. |
|
Hello, spotted a spelling error in UG
regards, Jožef |
|
Hi, I’ll take a look at it as part of docs18.3 branch later in July. Thanks.
From: gregjozk <notifications@github.com>
Sent: Tuesday, July 3, 2018 9:15 AM
To: nvaccess/nvda <nvda@noreply.github.com>
Cc: Joseph Lee <joseph.lee22590@gmail.com>; Mention <mention@noreply.github.com>
Subject: Re: [nvaccess/nvda] Add automatic background braille display detection (#7741)
Hello,
spotted a spelling error in UG
BRLTYY is not involved in NVDA's automatic background braille display detection functionality.
regards, Jožef
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#7741 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkFQfOJa9F7IwoEcJqfYl7Hah1t70ks5uC5hsgaJpZM4QacCr> .
|
First of all, many thanks to @jcsteh for laying the foundation of this work. Although I concider this feature complete now, it is very important that this is tested thoroughly.
Link to issue number:
Closes #1271.
Fixes #8032.
Summary of the issue:
Currently, braille displays can't be automatically detected in the background, but have to selected manually from the braille display selection dialog. Furthermore, when a native driver display loses its connection, you have to reselect the braille display from the braille display selection dialog in order to re-initialize it.
Description of how this pull request fixes the issue:
Explicit changes since @jcsteh worked on this
Several things have changed, as @jcsteh's implementation was pre hwIo. Most notably:
hwIO.IoBase._recvEvt is no longer a Python event, since threading.Event.wait blocks the calling thread, including queued APCs.
hwIO.IoBase.waitForRead no longer waits for hwIO.IoBase._recvEvt to be set, but waits using the kernel32.WaitForSingleObjectEx method. This allows braille display initialization on the background thread. If this change wouldn't have been made, waitForRead would be run on the background thread but would also block all background read attempts, effectively causing no reading to occur at all. :)
Polling bluetooth displays is no longer based on a 5 seconds interval, but on application switches.
Braille display auto detection no longer uses its own thread but makes use of braille._BgThread. Display driver initialization, instead of being queued to the main thread, is now done entirely on this background thread. Since the background thread now knows about a succesful or failed display initialization, support for one USB identifier being defined for multiple drivers should theoretically be guaranteed.
In the old situation, a braille display could have USB and Bluetooth definitions. In the current situation, USB definitions are split up between USB HID, USB Serial and USB Custom. The latter will be used for native Hims (Native driver for Hims displays #7712) and Freedom Scientific drivers. It will also be fairly easy to add Bluetooth HID support to bluetooth probing.
The bdDetect module introduces a new concept called a device match, a namedtuple with several pieces of relevant information to make a connection to a device. In the old situation, we had two types of device matches:
In the current situation, we have only one device match named tuple: DeviceMatch(type","id, port, deviceInfo). Here, type is one of HID, Serial or Custom. Bluetooth device matches have type bluetooth, both Bluetooth Serial and Bluetooth HID are supported.
Freedom Scientific displays are excluded from auto detection for now, since we aren't sure whether they are thread safe. In contrast, the new Alva, Handy Tech, Hims and Eurobraille drivers have been added. The new native driver for Freedom Scientific displays will support bdDetect in the future.
The following methods have been added to the braille.BrailleDisplayDriver class:
Subclasses can of course extend this method.
hwPortUtils.listUsbDevices now yields dictionaries instead of just strings containing the USB ID. The yielded dictionaries contain the USB ID, Hardware ID and device path. The latter is useful for USB Bulk devices.
Dictionaries yielded by hwPortUtils.listComPorts now also contain a usbID entry for COM ports with USB VID/PID information in their hardware ID.
Testing performed:
Tested the following devices:
To be tested
Known issues
None I'm currently aware of, but it is likely that this has some issues, since it is huge and people make mistakes. :) I propose an extended incubation period for this, may be for at least 1.5 month.
Changelog entries
New features
Bug fixes
Changes for developers