Give NVDA the ability to automatically detect braille displays.

[nvaccessAuto]

Reported by jkenn337 on 2010-12-11 15:50 When a braille display is connected to the computer and brltty is installed, NVDA should scan for and automatically detect and start using the connected USB or bluetooth display. This way the user does not have to mess about with choosing their display and things. Orca and the mac already do this. This enhancement should be across all operating systems. Blocking #1555, #3648

[LeonarddeR]

@jcsteh commented on 30 aug. 2017 07:32 CEST:

2. Rather than bdDetect using its own thread to probe, it should use the same background thread that is used for background writing to thread safe displays (braille._BgThread). Note that APCs can be used to queue functions to run in that thread. See braille._bgThread.queueApc (and its callers). This means we can initialise displays in the background thread, rather than queuing this to the main thread.

Could it be that you overlooked the fact that the bg thread is also used during initialization? hwIo queues an async read on the bg thread during initialization. However, when display probing is active on the bg thread and it initializes a display from that same thread, the async read doesn't take place since it is queued after the currently running initialization, and the thread won't also take action on completed i/o. So, it will result in no processing of received data from the display, and eventually in a no display found runtime error.

[jcsteh]

Oh. Yes, i certainly overlooked this. :( I think it should be easy enough to fix, though. When a driver wants to wait on a response during init, it uses hwIo.IoBase.waitForRead. waitForRead should be changed to wait on a Windows kernel event (not a Python event) using WaitForSingleObjectEx with bAlertable set to true. That way, it will allow async io completion callbacks to execute while waiting.

You can create a Windows kernel event with something like this:

evt = winKernel.kernel32.CreateEventW(None, False, False, None)

Don't forget to close it with winKernel.closeHandle when cleaning up.

[LeonarddeR]

This would mean that waitForRead would return for any i/o, right? SO, also for writing?

[LeonarddeR]

@leonardder commented on 7 sep. 2017 07:28 CEST:

This would mean that waitForRead would return for any i/o, right? SO, also for writing?

Update: a, got it. waitForRead will now return True if waiting was ended by the event as previously. Returning False now means either a time out or waiting has ended due to an apc queued to the thread. The reason is appropriately logged.

[jcsteh]

Well, WaitForSingleObjectEx will return for a write or APC, but I don't think waitForRead should necessarily return in this case, since it's intentionally blocking until either the read is completed or the timeout is exceeded. In other words, I don't think it needs to return early for writes or APCs. That means you might need to re-trigger the wait in your waitForRead implementation.

[LeonarddeR]

@jcsteh commented on 7 sep. 2017 08:28 CEST:

Well, WaitForSingleObjectEx will return for a write or APC, but I don't think waitForRead should necessarily return in this case, since it's intentionally blocking until either the read is completed or the timeout is exceeded. In other words, I don't think it needs to return early for writes or APCs. That means you might need to re-trigger the wait in your waitForRead implementation.

Is there something better than subtracting time.time() before the wait call from time.time() after the wait call to calculate the remaining time for a next wait?

[LeonarddeR]

After some thinking, I'm not sure whether this fixes our issue.

When initializing a display from the background thread, that thread is currently busy with initializing, and in the initializing process, it calls waitForRead, which makes the background thread itself block. I assume that when queueing an overlapped completion routine when initializing, the initialization (which is the current apc) should have returned in order for the overlapped completion routine to be executed. However, we want to execute that routine during initialization, and not after it, as at that point, initialization has already failed.

Or, and probably I''ve just missed this, will the completion routine just interrupt the current running APC? I should study computer science one day.

[jcsteh]

@leonardder commented on 7 Sep 2017, 19:26 GMT+10:

Is there something better than subtracting time.time() before the wait call from time.time() after the wait call to calculate the remaining time for a next wait?

That's the only way I can think of.

@leonardder commented on 7 Sep 2017, 20:14 GMT+10:

When initializing a display from the background thread, that thread is currently busy with initializing, and in the initializing process, it calls waitForRead, which makes the background thread itself block. I assume that when queueing an overlapped completion routine when initializing, the initialization (which is the current apc) should have returned in order for the overlapped completion routine to be executed.

That's a good question. I actually had to go look this up myself. From the documentation for QueueUserAPC:

If you perform an alertable wait inside an APC, it will recursively dispatch the APCs. This can cause a stack overflow.

I think the stack overflow bit is only relevant if you have too many levels of this, but we should only ever go two APCs deep, so we should be fine. There's no reason we should ever run another init function within the first init function.

Btw, the documentation confirms that this also applies to IO completion functions:

Note that the ReadFileEx, SetWaitableTimer, and WriteFileEx functions are implemented using an APC as the completion notification callback mechanism.

I should study computer science one day.

This probably wouldn't be taught in computer science. :)

[LeonarddeR]

Thanks for elaborating!

I've successfully ported probing to the bg thread, only one nut left to crack:

DEBUGWARNING - WX Widgets (18:59:17.417):
..\..\src\common\timerimpl.cpp, line 60:
assert wxThread::IsMain(): timer can only be started from the main thread
Stack trace:
  File "C:\Python27\lib\threading.py", line 774, in __bootstrap
    self.__bootstrap_inner()
  File "C:\Python27\lib\threading.py", line 801, in __bootstrap_inner
    self.run()
  File "C:\Python27\lib\threading.py", line 754, in run
    self.__target(*self.__args, **self.__kwargs)
  File "braille.py", line 1923, in func
    winUser.MWMO_ALERTABLE,
  File "bdDetect.py", line 213, in _bgScan
    self._callLater.Start(POLL_INTERVAL, self._startBgScan, bluetooth=True)
  File "D:\NVDA_source\nvda\miscDeps\python\wx\_core.py", line 16816, in Start
    self.timer.Start(self.millis, wx.TIMER_ONE_SHOT)
  File "D:\NVDA_source\nvda\miscDeps\python\wx\_misc.py", line 1324, in Start
    return _misc_.Timer_Start(*args, **kwargs)
  File "core.py", line 233, in OnAssert
    log.debugWarning(message,codepath="WX Widgets",stack_info=True)

This is the wx.CallLater that executes a back ground scan every 10 seconds. Although it should only be started from the main thread, it seems to run just fine, but it shows this debug warning every 10 seconds, which is well, not what we want.

Do we want to have the timer run in the background thread? If not, which is the easiest method, we can just wrap the wx.CallLater in a wx.CallAfter in such a way that we can still stop the CallLater if desired. It executes _startBgScan which itself queues the actual probing apc in the background thread anyway. However, if we have a special background thread, why not have the timer run on that thread. I'm not sure how to do this though, probably using a waitable timer, which executes apc functions.

[jcsteh]

assert wxThread::IsMain(): timer can only be started from the main thread

Oo. Yes, this is bad and definitely needs to be fixed. While starting timers from background threads does seem to work, the fact that it causes wx assertions suggests it could have nasty side effects. We fixed several instances of this last year and suspect this may have led to unexplained crashes in the past.

This is the wx.CallLater that executes a back ground scan every 10 seconds. Although it should only be started from the main thread,

Not sure if you mean it's only ever started from the main thread or that wx requires that it only be started from the main thread. This part of the stack confirms it's definitely being started in the background thread:

  File "bdDetect.py", line 213, in _bgScan
    self._callLater.Start(POLL_INTERVAL, self._startBgScan, bluetooth=True)

As you suggest, I think having the timer in the background thread makes the most sense, since the thread is already there. I think using a waitable timer makes sense. Note that you'll need to set the timer in the background thread in all cases so that it gets executed there. Also, ensure it is a one shot timer (lPeriod 0) to avoid the timer accidentally running during a waitForRead, which would be... very bad.

[LeonarddeR]

I consider the code in the @babbagecom i1271 branch to be feature complete, but before I file a pr, here is an overview of the changes since @jcsteh worked on this. Several things have changed, as @jcsteh's implementation was pre hwIo. Most notably:

1. hwIO.IoBase._recvEvt is no longer a Python event, since threading.Event.wait blocks the calling thread, including queued APCs.

2. 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 wasn't 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. :)

3. Bluetooth polling is now every 5 seconds instead of 10. In practice, probing ports that represent devices out of range will cause an increase of this delay, so it will hardly be five seconds in reality.

4. 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. I've actually seen one case of this, namely Hims SyncBraille and Baum VarioPro. The latter is not yet supported by NVDA.

5. 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 (#7712) and Freedom Scientific drivers, both work in progress. It will also be fairly easy to add Bluetooth HID support to bluetooth probing.

6. 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:

   • UsbDeviceMatch(id)
   • BluetoothComPortMatch(address", name, port)

   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 currently have type Serial, but you can easily check whether it is a bluetooth port by checking "bluetoothName" in match.deviceInfo.

   Note that it has been discussed with @JCSTeh to subclass DeviceMatch, rather than adding a type slot to the named tuple. I have eventually made the decision not to do this, as per the following reasons.

   A. USB definitions are split up into three categories, and these three categories are saved in a dictionary using the same key constants that are used in the named tuples as the device type. So, whatever we do, unless I'm missing something, we need type constants. B. Due to the point above, using subclasses would require tieing subclasses to type constants as well as multiple isinstance checks, and thus would require several additional lines of code, which is unnessesary if we just use type constants within the named tuples.

Additional ideas are welcome, though.

7. Alva and Freedom Scientific displays are excluded from auto detection for now, since we aren't sure whether they are thread safe. In contrast, Handy Tech has been added. The new native drivers for Eurobraille, Hims and Freedom Scientific displays will support bdDetect in the future.

8. The following methods have been added to the braille.BrailleDisplayDriver class:

   • _newWithSupportedKwargs: Creates a new instance of a BrailleDisplayDriver, but only with the keyword args it supports. This, although officially unsupported, might ease downgrading to drivers without a port setting in the future. See https://github.com/nvaccess/nvda/pull/7590#issuecomment-334080120 and surrounding comments.
   • _getAutoPorts: Returns possible ports to connect to using L{bdDetect} automatic detection data. Either USB, Bluetooth or both.
   • _getTryPorts(port): Generic function that yields device matches based on the provided port. Port is either a deviceMatch or a string. If a string, should be one of "automatic", "usb", "bluetooth" or a specific Com port. This method yields device matches. To give an example of its functionality, see the below code snippet from the Baum driver.

   def __init__(self, port="auto"):
       super(BrailleDisplayDriver, self).__init__()
       self.numCells = 0
       self._deviceID = None
   
       for portType, portId, port, portInfo in self._getTryPorts(port):
           # At this point, a port bound to this display has been found.
           # Try talking to the display.
           self.isHid = portType == bdDetect.KEY_HID
           ......

   Subclasses can of course extend this method, which is currently the case for the brailliantB driver. It is very likely that the brailliantB override can be removed, though.

9. 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.

[LeonarddeR]

@michaeldcurran: I forgot to mention that your feedback on the above comment would also be appreciated. Also @qchristensen, @josephsl and others, it would be great if you could share your opinion on how you think braille display detection would fit into the current documentation (e.g. should we just list the supported displays in a separate paragraph, or should we add a note to every specific display paragraph)?

[zstanecic]

hi,

i think, that the separate paragraph " supported autodetection braille displays" will be a better option

W dniu 07.11.2017 o 10:58, Leonard de Ruijter pisze:

@michaelDCurran https://github.com/michaeldcurran: I forgot to mention that your feedback on the above comment would also be appreciated. Also @Qchristensen https://github.com/qchristensen, @josephsl https://github.com/josephsl and others, it would be great if you could share your opinion on how you think braille display detection would fit into the current documentation (e.g. should we just list the supported displays in a separate paragraph, or should we add a note to every specific display paragraph)?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nvaccess/nvda/issues/1271#issuecomment-342432655, or mute the thread https://github.com/notifications/unsubscribe-auth/AKohk7DPGG79Pxc1FzKucK6gLMe9xjkrks5s0CnXgaJpZM4JVEoT.

[jcsteh]

For what it's worth, I'd suggest:

1. Having a general paragraph noting that NVDA can auto detect many displays and that the display specific section will note if detection is not supported for that display; and
2. Having a sentence in a display section if auto detection is not supported.

Reasons:

1. This means a user doesn't need to read through a huge list to work out whether their display is supported for detection;
2. The eventual aim is for as many displays as possible to support detection, so a list is just going to get longer and longer; and
3. Users can just read the section about their display for display specific info.

Having said that, you should totally ignore my opinion here in favour of NV Access staff. :)

Also, I'd like to publicly thank Leonard for taking over this work, adopting my somewhat incomplete code. I'm really glad to see it live on and finally come to fruition. :)