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

Add automatic background braille display detection #7741

Merged
merged 14 commits into from Jul 17, 2018

Conversation

@leonardder
Collaborator

leonardder commented Nov 11, 2017

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:

  1. This adds a new bdDetect module that, among various detection functions and a detector class, is the new home for declaring usb and bluetooth identifiers for drivers that support automatic detection using this new system. USB identification is based on usb VID and PID. For bluetooth identification, drivers must provide a callable that takes a device match (see below) as its argument. When the function returns True, a Bluetooth device is concidered to be a device for that particular driver.
  2. A new "automatic" option is added to the list of braille displays, which will enable automatic display detection in the background.
  3. When the automatic display is selected, NVDA does a one time check of USB and Bluetooth devices. After this initial check, NVDA will do a recheck when a new device is connected to the system. When Bluetooth is on and Bluetooth com ports are available, NVDA will try to connect to these com ports every time the system is switched to another application (e.g. with alt+tab). For notification of app switches, a new appModuleHandler.post_appSwitch extension point had been created.
  4. Custom braille display drivers will have to ship with a global plugin that adds the nescessary information (such as USB identifiers) to bdDetect in order to provide automatic detection.
  5. braille.BrailleDisplayDriver.getPossiblePorts is no longer NotImplemented, but provides a generic implementation to list possible ports for a driver. This generic implementation includes the possibility to explicitly choose automatic detection of USB or bluetooth devices.

Explicit 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 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. :)

  3. Polling bluetooth displays is no longer based on a 5 seconds interval, but on application switches.

  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.

  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. 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 have type bluetooth, both Bluetooth Serial and Bluetooth HID are supported.

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

  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 #7590 (comment) 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.

  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.

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

  • Baum Vario Ultra USB)
  • Handy Tech Active Braille (USB and Bluetooth)
  • Handy Tech Modular Evolution 88
  • Baum Pronto 18 (USB)
  • Handy Tech Basic Braille 40
  • Eurobraille Esys 12
  • Hims Smart Beetle
  • Hims Braille Edge

To be tested

  • brailliantB: @dkager, @bramd, @jcsteh, your help would be appreciated
  • brailleNote: @josephsl: Your help would be appreciated
  • superBrl: @michaelDCurran, do you still have such a device for testing? If so, testing would be appreciated

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

    • Support has been added to automatically detect braille displays in the background. (#1271)
      • ALVA, Baum/HumanWare/APH/Orbit, Eurobraille, Handy Tech, Hims, SuperBraille and HumanWare BrailleNote and Brailliant BI/B displays are currently supported.
      • You can enable this feature by selecting the automatic option from the list of braille displays in NVDA's braille display selection dialog.
      • Please consult the documentation for additional details.
  • Bug fixes

    • Switching braille context presentation when in browse mode no longer causes braille output to stop following. (#7741)
  • Changes for developers

    • Some changes have been made to the hwPortUtils module: (#1271)
      • listUsbDevices now yields dictionaries with device information including hardwareID and devicePath.
      • Dictionaries yielded by listComPorts now also contain a usbID entry for COM ports with USB VID/PID information in their hardware ID.
SetupDiEnumDeviceInfo.argtypes = (HDEVINFO, DWORD, PSP_DEVINFO_DATA)
SetupDiEnumDeviceInfo.restype = BOOL
CM_Get_Device_ID = ctypes.windll.cfgmgr32.CM_Get_Device_IDW

This comment has been minimized.

@leonardder

leonardder Nov 11, 2017

Collaborator

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

@leonardder

leonardder Nov 11, 2017

Collaborator

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

@Andre9642

This comment has been minimized.

Show comment
Hide comment
@Andre9642

Andre9642 Nov 11, 2017

Contributor

As far as I'm concerned it's good with USB.
I have a strange problem with Bluetooth, connection is constantly reset. Besides, sometimes USB connection switches to Bluetooth connection.

I have found these lines in NVDA log:

DEBUG - braille.BrailleHandler.setDisplayByName (14:33:44.145):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
DEBUG - braille.BrailleHandler.setDisplayByName (14:33:44.151):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
DEBUGWARNING - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (14:33:44.151):
Traceback (most recent call last):
  File "brailleDisplayDrivers\baum.pyc", line 84, in __init__
  File "hwIo.pyc", line 255, in __init__
WindowsError: [Error 32] Le processus ne peut pas accéder au fichier car ce fichier est utilisé par un autre processus.
DEBUGWARNING - braille.BrailleHandler.setDisplayByName (14:33:44.151):
Couldn't initialize display driver for kwargs {'port': DeviceMatch(type='hid', id=u'VID_0904&PID_6102', port=u'\\\\?\\hid#vid_0904&pid_6102&mi_00#7&40b644d&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}', deviceInfo={'usbID': u'VID_0904&PID_6102', 'devicePath': u'\\\\?\\hid#vid_0904&pid_6102&mi_00#7&40b644d&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}', 'hardwareID': u'HID\\VID_0904&PID_6102&REV_0000&MI_00'})}
Traceback (most recent call last):
  File "braille.pyc", line 1582, in setDisplayByName
  File "braille.pyc", line 2044, in _newWithSupportedKwargs
  File "extensionPoints.pyc", line 135, in callWithSupportedKwargs
  File "brailleDisplayDrivers\baum.pyc", line 121, in __init__
RuntimeError: No Baum display found
DEBUG - braille.BrailleHandler.setDisplayByName (14:33:44.151):
Reinitializing noBraille braille display
INFO - braille.BrailleHandler.setDisplayByName (14:33:44.151):
Loaded braille display driver noBraille, current display has 0 cells.
DEBUG - braille.BrailleHandler.setDisplayByName (14:33:44.163):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
[...]
INFO - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (14:33:45.183):
Found VarioUltra40 connected via serial (COM5)
DEBUG - braille.BrailleHandler.setDisplayByName (14:33:45.183):
Switching braille display from noBraille to baum
INFO - braille.BrailleHandler.setDisplayByName (14:33:45.183):
Loaded braille display driver baum, current display has 40 cells.
DEBUG - NVDAObjects.NVDAObject._get_placeholder (14:33:45.184):
Potential unimplemented child class: <NVDAObjects.IAccessible.Taskbar object at 0x07438810>
INFO - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (14:33:45.188):
Found VarioUltra40 connected via hid (\\?\hid#vid_0904&pid_6102&mi_00#7&40b644d&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030})
DEBUG - braille.BrailleHandler.setDisplayByName (14:33:45.188):
Switching braille display from baum to baum
INFO - braille.BrailleHandler.setDisplayByName (14:33:45.190):
Loaded braille display driver baum, current display has 40 cells.
Contributor

Andre9642 commented Nov 11, 2017

As far as I'm concerned it's good with USB.
I have a strange problem with Bluetooth, connection is constantly reset. Besides, sometimes USB connection switches to Bluetooth connection.

I have found these lines in NVDA log:

DEBUG - braille.BrailleHandler.setDisplayByName (14:33:44.145):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
DEBUG - braille.BrailleHandler.setDisplayByName (14:33:44.151):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
DEBUGWARNING - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (14:33:44.151):
Traceback (most recent call last):
  File "brailleDisplayDrivers\baum.pyc", line 84, in __init__
  File "hwIo.pyc", line 255, in __init__
WindowsError: [Error 32] Le processus ne peut pas accéder au fichier car ce fichier est utilisé par un autre processus.
DEBUGWARNING - braille.BrailleHandler.setDisplayByName (14:33:44.151):
Couldn't initialize display driver for kwargs {'port': DeviceMatch(type='hid', id=u'VID_0904&PID_6102', port=u'\\\\?\\hid#vid_0904&pid_6102&mi_00#7&40b644d&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}', deviceInfo={'usbID': u'VID_0904&PID_6102', 'devicePath': u'\\\\?\\hid#vid_0904&pid_6102&mi_00#7&40b644d&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}', 'hardwareID': u'HID\\VID_0904&PID_6102&REV_0000&MI_00'})}
Traceback (most recent call last):
  File "braille.pyc", line 1582, in setDisplayByName
  File "braille.pyc", line 2044, in _newWithSupportedKwargs
  File "extensionPoints.pyc", line 135, in callWithSupportedKwargs
  File "brailleDisplayDrivers\baum.pyc", line 121, in __init__
RuntimeError: No Baum display found
DEBUG - braille.BrailleHandler.setDisplayByName (14:33:44.151):
Reinitializing noBraille braille display
INFO - braille.BrailleHandler.setDisplayByName (14:33:44.151):
Loaded braille display driver noBraille, current display has 0 cells.
DEBUG - braille.BrailleHandler.setDisplayByName (14:33:44.163):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
[...]
INFO - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (14:33:45.183):
Found VarioUltra40 connected via serial (COM5)
DEBUG - braille.BrailleHandler.setDisplayByName (14:33:45.183):
Switching braille display from noBraille to baum
INFO - braille.BrailleHandler.setDisplayByName (14:33:45.183):
Loaded braille display driver baum, current display has 40 cells.
DEBUG - NVDAObjects.NVDAObject._get_placeholder (14:33:45.184):
Potential unimplemented child class: <NVDAObjects.IAccessible.Taskbar object at 0x07438810>
INFO - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (14:33:45.188):
Found VarioUltra40 connected via hid (\\?\hid#vid_0904&pid_6102&mi_00#7&40b644d&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030})
DEBUG - braille.BrailleHandler.setDisplayByName (14:33:45.188):
Switching braille display from baum to baum
INFO - braille.BrailleHandler.setDisplayByName (14:33:45.190):
Loaded braille display driver baum, current display has 40 cells.
@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Nov 11, 2017

Collaborator
Collaborator

leonardder commented Nov 11, 2017

@Andre9642

This comment has been minimized.

Show comment
Hide comment
@Andre9642

Andre9642 Nov 11, 2017

Contributor

It is a VarioUltra.

Contributor

Andre9642 commented Nov 11, 2017

It is a VarioUltra.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Nov 11, 2017

Collaborator

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?
It seems like NVDA is trying to connect with the Ultra in the background but a previous connection has not been destroyed properly.
I will be able to test with an ultra again next monday.

Collaborator

leonardder commented Nov 11, 2017

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?
It seems like NVDA is trying to connect with the Ultra in the background but a previous connection has not been destroyed properly.
I will be able to test with an ultra again next monday.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Nov 13, 2017

Collaborator

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?

Collaborator

leonardder commented Nov 13, 2017

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?

@Andre9642

This comment has been minimized.

Show comment
Hide comment
@Andre9642

Andre9642 Nov 13, 2017

Contributor

Are you able to reproduce this issue multiple times, or was is just one incident?
Yes, easily. I just select "auto" as braille display and I just activate the Bluetooth connection (no USB cable plugged in).

DEBUG - braille.BrailleHandler.setDisplayByName (07:29:51.267):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
DEBUGWARNING - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (07:29:51.269):
Traceback (most recent call last):
  File "brailleDisplayDrivers\baum.pyc", line 86, in __init__
  File "hwIo.pyc", line 161, in __init__
  File "serial\serialwin32.pyc", line 38, in __init__
  File "serial\serialutil.pyc", line 282, in __init__
  File "serial\serialwin32.pyc", line 66, in open
SerialException: could not open port u'COM5': WindowsError(5, 'Acc\xe8s refus\xe9.')
DEBUGWARNING - braille.BrailleHandler.setDisplayByName (07:29:51.269):
Couldn't initialize display driver for kwargs {'port': DeviceMatch(type='serial', id=u'VarioUltra40 FB2060050000\r\n', port=u'COM5', deviceInfo={'friendlyName': u'Lien s\xe9rie sur Bluetooth standard (COM5)', 'bluetoothName': u'VarioUltra40 FB2060050000\r\n', 'hardwareID': u'BTHENUM\\{00001101-0000-1000-8000-00805f9b34fb}_LOCALMFG&0002', 'bluetoothAddress': 90185979, 'port': u'COM5'})}
Traceback (most recent call last):
  File "braille.pyc", line 1582, in setDisplayByName
  File "braille.pyc", line 2044, in _newWithSupportedKwargs
  File "extensionPoints.pyc", line 135, in callWithSupportedKwargs
  File "brailleDisplayDrivers\baum.pyc", line 121, in __init__
RuntimeError: No Baum display found
DEBUG - braille.BrailleHandler.setDisplayByName (07:29:51.269):
Reinitializing noBraille braille display
INFO - braille.BrailleHandler.setDisplayByName (07:29:51.269):
Loaded braille display driver noBraille, current display has 0 cells.
DEBUG - braille.BrailleHandler.setDisplayByName (07:29:51.269):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
[...]
INFO - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (07:29:51.673):
Found VarioUltra40 connected via serial (COM10)
DEBUG - braille.BrailleHandler.setDisplayByName (07:29:51.674):
Switching braille display from noBraille to baum
INFO - braille.BrailleHandler.setDisplayByName (07:29:51.674):
Loaded braille display driver baum, current display has 40 cells.
[...]
DEBUGWARNING - braille.BrailleHandler.setDisplayByName (07:29:51.752):
Couldn't initialize display driver for kwargs {'port': DeviceMatch(type='serial', id=u'VarioUltra40 FB2060050000\r\n', port=u'COM10', deviceInfo={'friendlyName': u'Lien s\xe9rie sur Bluetooth standard (COM10)', 'bluetoothName': u'VarioUltra40 FB2060050000\r\n', 'hardwareID': u'BTHENUM\\{00001101-0000-1000-8000-00805f9b34fb}_LOCALMFG&0002', 'bluetoothAddress': 90185979, 'port': u'COM10'})}
[...]
DEBUG - braille.BrailleHandler.setDisplayByName (07:29:51.752):
Switching braille display from baum to noBraille
INFO - braille.BrailleHandler.setDisplayByName (07:29:51.753):
Loaded braille display driver noBraille, current display has 0 cells.
INFO - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (07:29:51.760):
Found VarioUltra40 connected via serial (COM5)
DEBUG - braille.BrailleHandler.setDisplayByName (07:29:51.760):
Switching braille display from noBraille to baum
INFO - braille.BrailleHandler.setDisplayByName (07:29:51.762):
Loaded braille display driver baum, current display has 40 cells.
[...]
DEBUG - braille.BrailleHandler.setDisplayByName (07:29:56.823):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
DEBUG - braille.BrailleHandler.setDisplayByName (07:29:56.823):
Reinitializing baum braille display
INFO - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (07:29:57.507):
Found VarioUltra40 connected via serial (COM5)
INFO - braille.BrailleHandler.setDisplayByName (07:29:57.507):
Loaded braille display driver baum, current display has 40 cells.
[...]
DEBUG - braille.BrailleHandler.setDisplayByName (07:30:02.520):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
DEBUG - braille.BrailleHandler.setDisplayByName (07:30:02.522):
Reinitializing baum braille display
INFO - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (07:30:03.134):
Found VarioUltra40 connected via serial (COM5)
INFO - braille.BrailleHandler.setDisplayByName (07:30:03.134):
Loaded braille display driver baum, current display has 40 cells.
[....]
Contributor

Andre9642 commented Nov 13, 2017

Are you able to reproduce this issue multiple times, or was is just one incident?
Yes, easily. I just select "auto" as braille display and I just activate the Bluetooth connection (no USB cable plugged in).

DEBUG - braille.BrailleHandler.setDisplayByName (07:29:51.267):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
DEBUGWARNING - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (07:29:51.269):
Traceback (most recent call last):
  File "brailleDisplayDrivers\baum.pyc", line 86, in __init__
  File "hwIo.pyc", line 161, in __init__
  File "serial\serialwin32.pyc", line 38, in __init__
  File "serial\serialutil.pyc", line 282, in __init__
  File "serial\serialwin32.pyc", line 66, in open
SerialException: could not open port u'COM5': WindowsError(5, 'Acc\xe8s refus\xe9.')
DEBUGWARNING - braille.BrailleHandler.setDisplayByName (07:29:51.269):
Couldn't initialize display driver for kwargs {'port': DeviceMatch(type='serial', id=u'VarioUltra40 FB2060050000\r\n', port=u'COM5', deviceInfo={'friendlyName': u'Lien s\xe9rie sur Bluetooth standard (COM5)', 'bluetoothName': u'VarioUltra40 FB2060050000\r\n', 'hardwareID': u'BTHENUM\\{00001101-0000-1000-8000-00805f9b34fb}_LOCALMFG&0002', 'bluetoothAddress': 90185979, 'port': u'COM5'})}
Traceback (most recent call last):
  File "braille.pyc", line 1582, in setDisplayByName
  File "braille.pyc", line 2044, in _newWithSupportedKwargs
  File "extensionPoints.pyc", line 135, in callWithSupportedKwargs
  File "brailleDisplayDrivers\baum.pyc", line 121, in __init__
RuntimeError: No Baum display found
DEBUG - braille.BrailleHandler.setDisplayByName (07:29:51.269):
Reinitializing noBraille braille display
INFO - braille.BrailleHandler.setDisplayByName (07:29:51.269):
Loaded braille display driver noBraille, current display has 0 cells.
DEBUG - braille.BrailleHandler.setDisplayByName (07:29:51.269):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
[...]
INFO - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (07:29:51.673):
Found VarioUltra40 connected via serial (COM10)
DEBUG - braille.BrailleHandler.setDisplayByName (07:29:51.674):
Switching braille display from noBraille to baum
INFO - braille.BrailleHandler.setDisplayByName (07:29:51.674):
Loaded braille display driver baum, current display has 40 cells.
[...]
DEBUGWARNING - braille.BrailleHandler.setDisplayByName (07:29:51.752):
Couldn't initialize display driver for kwargs {'port': DeviceMatch(type='serial', id=u'VarioUltra40 FB2060050000\r\n', port=u'COM10', deviceInfo={'friendlyName': u'Lien s\xe9rie sur Bluetooth standard (COM10)', 'bluetoothName': u'VarioUltra40 FB2060050000\r\n', 'hardwareID': u'BTHENUM\\{00001101-0000-1000-8000-00805f9b34fb}_LOCALMFG&0002', 'bluetoothAddress': 90185979, 'port': u'COM10'})}
[...]
DEBUG - braille.BrailleHandler.setDisplayByName (07:29:51.752):
Switching braille display from baum to noBraille
INFO - braille.BrailleHandler.setDisplayByName (07:29:51.753):
Loaded braille display driver noBraille, current display has 0 cells.
INFO - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (07:29:51.760):
Found VarioUltra40 connected via serial (COM5)
DEBUG - braille.BrailleHandler.setDisplayByName (07:29:51.760):
Switching braille display from noBraille to baum
INFO - braille.BrailleHandler.setDisplayByName (07:29:51.762):
Loaded braille display driver baum, current display has 40 cells.
[...]
DEBUG - braille.BrailleHandler.setDisplayByName (07:29:56.823):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
DEBUG - braille.BrailleHandler.setDisplayByName (07:29:56.823):
Reinitializing baum braille display
INFO - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (07:29:57.507):
Found VarioUltra40 connected via serial (COM5)
INFO - braille.BrailleHandler.setDisplayByName (07:29:57.507):
Loaded braille display driver baum, current display has 40 cells.
[...]
DEBUG - braille.BrailleHandler.setDisplayByName (07:30:02.520):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
DEBUG - braille.BrailleHandler.setDisplayByName (07:30:02.522):
Reinitializing baum braille display
INFO - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (07:30:03.134):
Found VarioUltra40 connected via serial (COM5)
INFO - braille.BrailleHandler.setDisplayByName (07:30:03.134):
Loaded braille display driver baum, current display has 40 cells.
[....]
@Andre9642

This comment has been minimized.

Show comment
Hide comment
@Andre9642

Andre9642 Nov 13, 2017

Contributor

Now works perfectly! :)

Contributor

Andre9642 commented Nov 13, 2017

Now works perfectly! :)

@Andre9642

This comment has been minimized.

Show comment
Hide comment
@Andre9642

Andre9642 Nov 13, 2017

Contributor

I found another case where it doesn't work fine.

  1. Select "automatic" braille display (of course!).
  2. Disable Bluetooth on VarioUltra and don't connect the device to USB.
  3. Restart NVDA.

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.

DEBUGWARNING - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (17:29:27.233):
Traceback (most recent call last):
  File "brailleDisplayDrivers\baum.pyc", line 86, in __init__
  File "hwIo.pyc", line 160, in __init__
  File "serial\serialwin32.pyc", line 38, in __init__
  File "serial\serialutil.pyc", line 282, in __init__
  File "serial\serialwin32.pyc", line 66, in open
SerialException: could not open port u'COM5': WindowsError(121, 'Le d\xe9lai de temporisation de s\xe9maphore a expir\xe9.')
DEBUGWARNING - braille.BrailleHandler.setDisplayByName (17:29:27.233):
Couldn't initialize display driver for kwargs {'port': DeviceMatch(type='serial', id=u'VarioUltra40 FB2060050000\r\n', port=u'COM5', deviceInfo={'friendlyName': u'Lien s\xe9rie sur Bluetooth standard (COM5)', 'bluetoothName': u'VarioUltra40 FB2060050000\r\n', 'hardwareID': u'BTHENUM\\{00001101-0000-1000-8000-00805f9b34fb}_LOCALMFG&0002', 'bluetoothAddress': 90185979, 'port': u'COM5'})}
Traceback (most recent call last):
  File "braille.pyc", line 1582, in setDisplayByName
  File "braille.pyc", line 2044, in _newWithSupportedKwargs
  File "extensionPoints.pyc", line 135, in callWithSupportedKwargs
  File "brailleDisplayDrivers\baum.pyc", line 121, in __init__
RuntimeError: No Baum display found
DEBUG - braille.BrailleHandler.setDisplayByName (17:29:27.234):
Reinitializing noBraille braille display
INFO - braille.BrailleHandler.setDisplayByName (17:29:27.234):
Loaded braille display driver noBraille, current display has 0 cells.
DEBUG - braille.BrailleHandler.setDisplayByName (17:29:27.234):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
DEBUGWARNING - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (17:29:32.361):
Traceback (most recent call last):
  File "brailleDisplayDrivers\baum.pyc", line 86, in __init__
  File "hwIo.pyc", line 160, in __init__
  File "serial\serialwin32.pyc", line 38, in __init__
  File "serial\serialutil.pyc", line 282, in __init__
  File "serial\serialwin32.pyc", line 66, in open
SerialException: could not open port u'COM10': WindowsError(121, 'Le d\xe9lai de temporisation de s\xe9maphore a expir\xe9.')
DEBUGWARNING - braille.BrailleHandler.setDisplayByName (17:29:32.361):
Couldn't initialize display driver for kwargs {'port': DeviceMatch(type='serial', id=u'VarioUltra40 FB2060050000\r\n', port=u'COM10', deviceInfo={'friendlyName': u'Lien s\xe9rie sur Bluetooth standard (COM10)', 'bluetoothName': u'VarioUltra40 FB2060050000\r\n', 'hardwareID': u'BTHENUM\\{00001101-0000-1000-8000-00805f9b34fb}_LOCALMFG&0002', 'bluetoothAddress': 90185979, 'port': u'COM10'})}
Traceback (most recent call last):
  File "braille.pyc", line 1582, in setDisplayByName
  File "braille.pyc", line 2044, in _newWithSupportedKwargs
  File "extensionPoints.pyc", line 135, in callWithSupportedKwargs
  File "brailleDisplayDrivers\baum.pyc", line 121, in __init__
RuntimeError: No Baum display found
DEBUG - braille.BrailleHandler.setDisplayByName (17:29:32.361):
Reinitializing noBraille braille display
INFO - braille.BrailleHandler.setDisplayByName (17:29:32.361):
Loaded braille display driver noBraille, current display has 0 cells.
DEBUG - braille.BrailleHandler.setDisplayByName (17:29:32.362):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
DEBUGWARNING - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (17:29:37.486):
Traceback (most recent call last):
  File "brailleDisplayDrivers\baum.pyc", line 86, in __init__
  File "hwIo.pyc", line 160, in __init__
  File "serial\serialwin32.pyc", line 38, in __init__
  File "serial\serialutil.pyc", line 282, in __init__
  File "serial\serialwin32.pyc", line 66, in open
SerialException: could not open port u'COM4': WindowsError(121, 'Le d\xe9lai de temporisation de s\xe9maphore a expir\xe9.')
DEBUGWARNING - braille.BrailleHandler.setDisplayByName (17:29:37.487):
Couldn't initialize display driver for kwargs {'port': DeviceMatch(type='serial', id=u'VarioUltra40 FB2060050000\r\n', port=u'COM4', deviceInfo={'friendlyName': u'Lien s\xe9rie sur Bluetooth standard (COM4)', 'bluetoothName': u'VarioUltra40 FB2060050000\r\n', 'hardwareID': u'BTHENUM\\{00001101-0000-1000-8000-00805f9b34fb}_LOCALMFG&0002', 'bluetoothAddress': 90185979, 'port': u'COM4'})}
Traceback (most recent call last):
  File "braille.pyc", line 1582, in setDisplayByName
  File "braille.pyc", line 2044, in _newWithSupportedKwargs
  File "extensionPoints.pyc", line 135, in callWithSupportedKwargs
  File "brailleDisplayDrivers\baum.pyc", line 121, in __init__
RuntimeError: No Baum display found
DEBUG - braille.BrailleHandler.setDisplayByName (17:29:37.487):
Reinitializing noBraille braille display
INFO - braille.BrailleHandler.setDisplayByName (17:29:37.487):
Loaded braille display driver noBraille, current display has 0 cells.
DEBUG - braille.BrailleHandler.setDisplayByName (17:29:37.489):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
[...]
Contributor

Andre9642 commented Nov 13, 2017

I found another case where it doesn't work fine.

  1. Select "automatic" braille display (of course!).
  2. Disable Bluetooth on VarioUltra and don't connect the device to USB.
  3. Restart NVDA.

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.

DEBUGWARNING - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (17:29:27.233):
Traceback (most recent call last):
  File "brailleDisplayDrivers\baum.pyc", line 86, in __init__
  File "hwIo.pyc", line 160, in __init__
  File "serial\serialwin32.pyc", line 38, in __init__
  File "serial\serialutil.pyc", line 282, in __init__
  File "serial\serialwin32.pyc", line 66, in open
SerialException: could not open port u'COM5': WindowsError(121, 'Le d\xe9lai de temporisation de s\xe9maphore a expir\xe9.')
DEBUGWARNING - braille.BrailleHandler.setDisplayByName (17:29:27.233):
Couldn't initialize display driver for kwargs {'port': DeviceMatch(type='serial', id=u'VarioUltra40 FB2060050000\r\n', port=u'COM5', deviceInfo={'friendlyName': u'Lien s\xe9rie sur Bluetooth standard (COM5)', 'bluetoothName': u'VarioUltra40 FB2060050000\r\n', 'hardwareID': u'BTHENUM\\{00001101-0000-1000-8000-00805f9b34fb}_LOCALMFG&0002', 'bluetoothAddress': 90185979, 'port': u'COM5'})}
Traceback (most recent call last):
  File "braille.pyc", line 1582, in setDisplayByName
  File "braille.pyc", line 2044, in _newWithSupportedKwargs
  File "extensionPoints.pyc", line 135, in callWithSupportedKwargs
  File "brailleDisplayDrivers\baum.pyc", line 121, in __init__
RuntimeError: No Baum display found
DEBUG - braille.BrailleHandler.setDisplayByName (17:29:27.234):
Reinitializing noBraille braille display
INFO - braille.BrailleHandler.setDisplayByName (17:29:27.234):
Loaded braille display driver noBraille, current display has 0 cells.
DEBUG - braille.BrailleHandler.setDisplayByName (17:29:27.234):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
DEBUGWARNING - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (17:29:32.361):
Traceback (most recent call last):
  File "brailleDisplayDrivers\baum.pyc", line 86, in __init__
  File "hwIo.pyc", line 160, in __init__
  File "serial\serialwin32.pyc", line 38, in __init__
  File "serial\serialutil.pyc", line 282, in __init__
  File "serial\serialwin32.pyc", line 66, in open
SerialException: could not open port u'COM10': WindowsError(121, 'Le d\xe9lai de temporisation de s\xe9maphore a expir\xe9.')
DEBUGWARNING - braille.BrailleHandler.setDisplayByName (17:29:32.361):
Couldn't initialize display driver for kwargs {'port': DeviceMatch(type='serial', id=u'VarioUltra40 FB2060050000\r\n', port=u'COM10', deviceInfo={'friendlyName': u'Lien s\xe9rie sur Bluetooth standard (COM10)', 'bluetoothName': u'VarioUltra40 FB2060050000\r\n', 'hardwareID': u'BTHENUM\\{00001101-0000-1000-8000-00805f9b34fb}_LOCALMFG&0002', 'bluetoothAddress': 90185979, 'port': u'COM10'})}
Traceback (most recent call last):
  File "braille.pyc", line 1582, in setDisplayByName
  File "braille.pyc", line 2044, in _newWithSupportedKwargs
  File "extensionPoints.pyc", line 135, in callWithSupportedKwargs
  File "brailleDisplayDrivers\baum.pyc", line 121, in __init__
RuntimeError: No Baum display found
DEBUG - braille.BrailleHandler.setDisplayByName (17:29:32.361):
Reinitializing noBraille braille display
INFO - braille.BrailleHandler.setDisplayByName (17:29:32.361):
Loaded braille display driver noBraille, current display has 0 cells.
DEBUG - braille.BrailleHandler.setDisplayByName (17:29:32.362):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
DEBUGWARNING - brailleDisplayDrivers.baum.BrailleDisplayDriver.__init__ (17:29:37.486):
Traceback (most recent call last):
  File "brailleDisplayDrivers\baum.pyc", line 86, in __init__
  File "hwIo.pyc", line 160, in __init__
  File "serial\serialwin32.pyc", line 38, in __init__
  File "serial\serialutil.pyc", line 282, in __init__
  File "serial\serialwin32.pyc", line 66, in open
SerialException: could not open port u'COM4': WindowsError(121, 'Le d\xe9lai de temporisation de s\xe9maphore a expir\xe9.')
DEBUGWARNING - braille.BrailleHandler.setDisplayByName (17:29:37.487):
Couldn't initialize display driver for kwargs {'port': DeviceMatch(type='serial', id=u'VarioUltra40 FB2060050000\r\n', port=u'COM4', deviceInfo={'friendlyName': u'Lien s\xe9rie sur Bluetooth standard (COM4)', 'bluetoothName': u'VarioUltra40 FB2060050000\r\n', 'hardwareID': u'BTHENUM\\{00001101-0000-1000-8000-00805f9b34fb}_LOCALMFG&0002', 'bluetoothAddress': 90185979, 'port': u'COM4'})}
Traceback (most recent call last):
  File "braille.pyc", line 1582, in setDisplayByName
  File "braille.pyc", line 2044, in _newWithSupportedKwargs
  File "extensionPoints.pyc", line 135, in callWithSupportedKwargs
  File "brailleDisplayDrivers\baum.pyc", line 121, in __init__
RuntimeError: No Baum display found
DEBUG - braille.BrailleHandler.setDisplayByName (17:29:37.487):
Reinitializing noBraille braille display
INFO - braille.BrailleHandler.setDisplayByName (17:29:37.487):
Loaded braille display driver noBraille, current display has 0 cells.
DEBUG - braille.BrailleHandler.setDisplayByName (17:29:37.489):
Possibly detected display 'Terminaux braille Baum/HumanWare/APH/Orbit'
[...]
@bramd

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:

  1. Scan interval for Bluetooth is 5 seconds, but what's the timeout for a BT COM port that's unavailable?
  2. Is there another reliable way to check if a BT device is in range before opening it's COM port?
  3. Do we have any data on the impact of constant BT scanning on laptop batteries?
Show outdated Hide outdated source/bdDetect.py Outdated
try:
return _driverDevices[driver]
except KeyError:
ret = _driverDevices[driver] = defaultdict(set)

This comment has been minimized.

@bramd

bramd Nov 13, 2017

Contributor

Why do you use an intermediate variable here?

@bramd

bramd Nov 13, 2017

Contributor

Why do you use an intermediate variable here?

This comment has been minimized.

@leonardder

leonardder Nov 14, 2017

Collaborator

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.

@leonardder

leonardder Nov 14, 2017

Collaborator

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.

Show outdated Hide outdated source/bdDetect.py Outdated
Show outdated Hide outdated source/brailleDisplayDrivers/baum.py Outdated
Show outdated Hide outdated source/brailleDisplayDrivers/brailliantB.py Outdated
Show outdated Hide outdated source/brailleDisplayDrivers/brailliantB.py Outdated
# 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 = {

This comment has been minimized.

@bramd

bramd Nov 13, 2017

Contributor

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.

@bramd

bramd Nov 13, 2017

Contributor

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.

This comment has been minimized.

@leonardder

leonardder Nov 14, 2017

Collaborator

@michaelDCurran, @jcsteh, thoughts? May be include the device name in detection data as well? That would also help for #7458.

@leonardder

leonardder Nov 14, 2017

Collaborator

@michaelDCurran, @jcsteh, thoughts? May be include the device name in detection data as well? That would also help for #7458.

This comment has been minimized.

@jcsteh

jcsteh Nov 14, 2017

Contributor

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.

@jcsteh

jcsteh Nov 14, 2017

Contributor

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):

This comment has been minimized.

@bramd

bramd Nov 13, 2017

Contributor

Please add a docstring to document the function and parameters and/or refer to the relevant Windows API calls documentation.

@bramd

bramd Nov 13, 2017

Contributor

Please add a docstring to document the function and parameters and/or refer to the relevant Windows API calls documentation.

This comment has been minimized.

@leonardder

leonardder Nov 14, 2017

Collaborator

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?

@leonardder

leonardder Nov 14, 2017

Collaborator

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?

This comment has been minimized.

@feerrenrut

feerrenrut Nov 15, 2017

Contributor

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.

@feerrenrut

feerrenrut Nov 15, 2017

Contributor

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):

This comment has been minimized.

@bramd

bramd Nov 13, 2017

Contributor

Please add a docstring to document the function and parameters and/or refer to the relevant Windows API calls documentation.

@bramd

bramd Nov 13, 2017

Contributor

Please add a docstring to document the function and parameters and/or refer to the relevant Windows API calls documentation.

Show outdated Hide outdated user_docs/en/userGuide.t2t Outdated
@ruifontes

This comment has been minimized.

Show comment
Hide comment
@ruifontes

ruifontes Nov 14, 2017

ruifontes commented Nov 14, 2017

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Nov 14, 2017

Collaborator

@Andre9642 commented on 13 nov. 2017 17:45 CET:

I found another case where it doesn't work fine.

  1. Select "automatic" braille display (of course!).
  2. Disable Bluetooth on VarioUltra and don't connect the device to USB.
  3. Restart NVDA.

Actual behaviour: NVDA tries to connect to the VarioUltra via Bluetooth permanently.

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.

Additionally, if I plug my VarioUltra at this point, auto detection in USB doesn't work.

Now this is interesting behavior. I will try to reproduce this.

Collaborator

leonardder commented Nov 14, 2017

@Andre9642 commented on 13 nov. 2017 17:45 CET:

I found another case where it doesn't work fine.

  1. Select "automatic" braille display (of course!).
  2. Disable Bluetooth on VarioUltra and don't connect the device to USB.
  3. Restart NVDA.

Actual behaviour: NVDA tries to connect to the VarioUltra via Bluetooth permanently.

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.

Additionally, if I plug my VarioUltra at this point, auto detection in USB doesn't work.

Now this is interesting behavior. I will try to reproduce this.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Nov 14, 2017

Collaborator

@ruifontes commented on 14 nov. 2017 01:42 CET:

Hi Leonard!

I assume this wasn't all you had to say? ;)

Collaborator

leonardder commented Nov 14, 2017

@ruifontes commented on 14 nov. 2017 01:42 CET:

Hi Leonard!

I assume this wasn't all you had to say? ;)

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Nov 14, 2017

Collaborator

@bramd reviewed and asked:

  1. Scan interval for Bluetooth is 5 seconds, but what's the timeout for a BT COM port that's unavailable?

That's currently the default, the connection attempt is made by the driver. I assume around five seconds, but this should be investigated.

  1. Is there another reliable way to check if a BT device is in range before opening it's COM port?

@jcsteh: Did you do any research into this while working on this?

  1. Do we have any data on the impact of constant BT scanning on laptop batteries?

No. :) However, since we actually know when we are running on batteries, we can disable polling altogether. May be we'd make this optional.

Collaborator

leonardder commented Nov 14, 2017

@bramd reviewed and asked:

  1. Scan interval for Bluetooth is 5 seconds, but what's the timeout for a BT COM port that's unavailable?

That's currently the default, the connection attempt is made by the driver. I assume around five seconds, but this should be investigated.

  1. Is there another reliable way to check if a BT device is in range before opening it's COM port?

@jcsteh: Did you do any research into this while working on this?

  1. Do we have any data on the impact of constant BT scanning on laptop batteries?

No. :) However, since we actually know when we are running on batteries, we can disable polling altogether. May be we'd make this optional.

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Nov 14, 2017

Contributor

Replying to a bunch of questions.

@leonardder commented on Nov 11, 2017, 7:58 PM GMT+10:

First of all, many thanks to @jcsteh for laying the foundation of this work.

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:

  1. Is there another reliable way to check if a BT device is in range before opening it's COM port?

@jcsteh: Did you do any research into this while working on this?

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.

  1. Do we have any data on the impact of constant BT scanning on laptop batteries?

No. :) However, since we actually know when we are running on batteries, we can disable polling altogether. May be we'd make this optional.

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.

Contributor

jcsteh commented Nov 14, 2017

Replying to a bunch of questions.

@leonardder commented on Nov 11, 2017, 7:58 PM GMT+10:

First of all, many thanks to @jcsteh for laying the foundation of this work.

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:

  1. Is there another reliable way to check if a BT device is in range before opening it's COM port?

@jcsteh: Did you do any research into this while working on this?

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.

  1. Do we have any data on the impact of constant BT scanning on laptop batteries?

No. :) However, since we actually know when we are running on batteries, we can disable polling altogether. May be we'd make this optional.

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.

@Andre9642

This comment has been minimized.

Show comment
Hide comment
@Andre9642

Andre9642 Nov 14, 2017

Contributor

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.

My mistake! In reflecting, it's obvious...
I would have a small request: would it be possible to emit an event when a braille display is autodetected (and why not when connection with its is lost). Unless I didn't search enough and it already exists.

Contributor

Andre9642 commented Nov 14, 2017

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.

My mistake! In reflecting, it's obvious...
I would have a small request: would it be possible to emit an event when a braille display is autodetected (and why not when connection with its is lost). Unless I didn't search enough and it already exists.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Nov 14, 2017

Collaborator

@Andre9642 commented on 14 nov. 2017 09:09 CET:

I would have a small request: would it be possible to emit an event when a braille display is autodetected (and why not when connection with its is lost). Unless I didn't search enough and it already exists.

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:

  1. Restart NVDA with auto detection enabled, USB devices disconnected, bluetooth enabled and bluetooth devices paired but also disabled
  2. Connect a display using USB

Result: A rescan is triggered, but it seems it does not execute properly.

Collaborator

leonardder commented Nov 14, 2017

@Andre9642 commented on 14 nov. 2017 09:09 CET:

I would have a small request: would it be possible to emit an event when a braille display is autodetected (and why not when connection with its is lost). Unless I didn't search enough and it already exists.

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:

  1. Restart NVDA with auto detection enabled, USB devices disconnected, bluetooth enabled and bluetooth devices paired but also disabled
  2. Connect a display using USB

Result: A rescan is triggered, but it seems it does not execute properly.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Nov 14, 2017

Collaborator

14-11 try build:
https://ci.appveyor.com/api/buildjobs/dsc4g4gj60ywn89e/artifacts/output%2Fnvda_snapshot_try-i1271-14615%2Cda6332cb.exe

@Andre9642: This should also fixed the last issue you described. Things should be a bit more reliable now.

Collaborator

leonardder commented Nov 14, 2017

14-11 try build:
https://ci.appveyor.com/api/buildjobs/dsc4g4gj60ywn89e/artifacts/output%2Fnvda_snapshot_try-i1271-14615%2Cda6332cb.exe

@Andre9642: This should also fixed the last issue you described. Things should be a bit more reliable now.

@leonardder leonardder added the Braille label Dec 4, 2017

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Dec 4, 2017

Collaborator

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.

Collaborator

leonardder commented Dec 4, 2017

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.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Dec 4, 2017

Collaborator

Actually, there seems to be an open point.

@jcsteh commented on 14 nov. 2017 07:18 CET:

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.

I actually like the idea to poll on foreground changes. I propose the following:

  1. in NVDAObject.event-foreground, call a new method braille.handler.handleForeground
  2. IN this handleForeground method, start looking for displays again.
Collaborator

leonardder commented Dec 4, 2017

Actually, there seems to be an open point.

@jcsteh commented on 14 nov. 2017 07:18 CET:

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.

I actually like the idea to poll on foreground changes. I propose the following:

  1. in NVDAObject.event-foreground, call a new method braille.handler.handleForeground
  2. IN this handleForeground method, start looking for displays again.
@derekriemer

This comment has been minimized.

Show comment
Hide comment
@derekriemer

derekriemer Dec 5, 2017

Collaborator
  1. in NVDAObject.event-foreground, call a new method braille.handler.handleForeground

What about a extension point?

Collaborator

derekriemer commented Dec 5, 2017

  1. in NVDAObject.event-foreground, call a new method braille.handler.handleForeground

What about a extension point?

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Dec 5, 2017

Collaborator

@derekriemer commented on 5 dec. 2017 05:44 CET:

  1. in NVDAObject.event-foreground, call a new method braille.handler.handleForeground

What about a extension point?

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.

Collaborator

leonardder commented Dec 5, 2017

@derekriemer commented on 5 dec. 2017 05:44 CET:

  1. in NVDAObject.event-foreground, call a new method braille.handler.handleForeground

What about a extension point?

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.

@derekriemer

This comment has been minimized.

Show comment
Hide comment
@derekriemer

derekriemer Dec 5, 2017

Collaborator

true

Collaborator

derekriemer commented Dec 5, 2017

true

@leonardder leonardder requested a review from michaelDCurran Dec 13, 2017

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Dec 13, 2017

Collaborator

@michaelDCurran: Your review is welcome, but I think incubation into Next should wait until at least #7732 and #7489 are in master.

Collaborator

leonardder commented Dec 13, 2017

@michaelDCurran: Your review is welcome, but I think incubation into Next should wait until at least #7732 and #7489 are in master.

@leonardder leonardder assigned dkager and unassigned dkager Jan 2, 2018

@leonardder leonardder requested a review from dkager Jan 2, 2018

@Adriani90

This comment has been minimized.

Show comment
Hide comment
@Adriani90

Adriani90 commented Jan 11, 2018

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Jun 6, 2018

Collaborator

Addressed two additional issues:

  • Since auto detect is now the default, unit tests could fail if a braille display is connected. For unit tests, we now set the braille display back to noBraille before initializing braille. This is a temporary solution and will be fixed by #8240, which will introduce a dummy braille display driver.
  • Auto detection when in browse mode would fail to make the braille display follow the browse mode cursor. This is now solved, and it also fixes the issue that focus context presentation switchting does not work properly while in browse mode.
Collaborator

leonardder commented Jun 6, 2018

Addressed two additional issues:

  • Since auto detect is now the default, unit tests could fail if a braille display is connected. For unit tests, we now set the braille display back to noBraille before initializing braille. This is a temporary solution and will be fixed by #8240, which will introduce a dummy braille display driver.
  • Auto detection when in browse mode would fail to make the braille display follow the browse mode cursor. This is now solved, and it also fixes the issue that focus context presentation switchting does not work properly while in browse mode.
@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Jun 9, 2018

Contributor

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.

Contributor

jcsteh commented Jun 9, 2018

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.

leonardder added some commits Jun 9, 2018

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Jun 9, 2018

Collaborator

@jcsteh commented on 9 Jun 2018, 07:12 CEST:

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.

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.

Collaborator

leonardder commented Jun 9, 2018

@jcsteh commented on 9 Jun 2018, 07:12 CEST:

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.

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 leonardder requested review from michaelDCurran and feerrenrut and removed request for feerrenrut Jun 19, 2018

michaelDCurran added a commit that referenced this pull request Jun 25, 2018

@feerrenrut

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut Jun 27, 2018

Contributor

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.

Contributor

feerrenrut commented Jun 27, 2018

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.

@gregjozk

This comment has been minimized.

Show comment
Hide comment
@gregjozk

gregjozk Jul 3, 2018

Hello,

spotted a spelling error in UG

BRLTYY is not involved in NVDA's automatic background braille display detection functionality.

regards, Jožef

gregjozk commented Jul 3, 2018

Hello,

spotted a spelling error in UG

BRLTYY is not involved in NVDA's automatic background braille display detection functionality.

regards, Jožef

@josephsl

This comment has been minimized.

Show comment
Hide comment
@josephsl

josephsl Jul 3, 2018

Collaborator
Collaborator

josephsl commented Jul 3, 2018

@michaelDCurran michaelDCurran merged commit 846a998 into nvaccess:master Jul 17, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nvaccessAuto nvaccessAuto added this to the 2018.3 milestone Jul 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment