Make automatic braille display detection more pythonic

[LeonarddeR]

Link to issue number:

Related to #14130

Summary of the issue:

Parts of the braille display detection code are hard to read and rely on Win32 APCs to be queued. A major drawback about APCS is that, as soon as theyre queued, they can not be canceled.

Description of user facing changes

None, though I hope things will be more stable after this change

Description of development approach

In an attempt to make the code more pythonic as well as changing the code in such a way that it no longer depends on the braille background thread, I changed the following:

• The detection work now has a dedicated thread, using a thread pool under the hood. This is a thread pool with only one thread. This might sound stupid, but the major advantage of a ThreadPoolExecutor is that it allows to execute and queue functions on the thread
• The detection function is no longer a loop. It was a loop to avoid queuing an apc when we knew one was already running, but as the ThreadPoolExecutor has an internal queue and we can cancel futures now, this decreases compexity.

Testing strategy:

I hope I can make @dkager test this, as he is the one who mostly complaint about auto detection instability in my network.

Known issues with pull request:

None known

Change log entries:

For Developers

• Somewhat decreased the complexity of the background braille display auto detection logic.

Code Review Checklist:

• [x] Pull Request description:
  • description is up to date
  • change log entries
• [] Testing:
  • Unit tests
  • System (end to end) tests
  • Manual testing
• [x] API is compatible with existing add-ons.
• [x] Documentation:
  • User Documentation
  • Developer / Technical Documentation
  • Context sensitive help for GUI changes
• [x] UX of all users considered:
  • Speech
  • Braille
  • Low Vision
  • Different web browsers
  • Localization in other languages / culture than English
• [x] Security precautions taken.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/r4v3at6oyn0t3hg6/artifacts/output/nvda_snapshot_pr14147-26575,57b72685.exe
• CI timing (mins): INIT 0.0, INSTALL_START 0.8, INSTALL_END 0.8, BUILD_START 0.0, BUILD_END 22.5, TESTSETUP_START 0.0, TESTSETUP_END 0.2, TEST_START 0.0, TEST_END 13.4, FINISH_END 0.1

See test results for failed build of commit 57b7268556

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/2uabhwjbmvwuwg8x/artifacts/output/nvda_snapshot_pr14147-26576,cfefdb5e.exe
• CI timing (mins): INIT 0.0, INSTALL_START 0.9, INSTALL_END 0.9, BUILD_START 0.0, BUILD_END 23.2, TESTSETUP_START 0.0, TESTSETUP_END 0.2, TEST_START 0.0, TEST_END 10.1, FINISH_END 0.1

See test results for failed build of commit cfefdb5e9b

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/0egnr80gg523p8iv/artifacts/output/nvda_snapshot_pr14147-26580,2f2ac000.exe
• CI timing (mins): INIT 0.0, INSTALL_START 0.8, INSTALL_END 0.8, BUILD_START 0.0, BUILD_END 22.0, TESTSETUP_START 0.0, TESTSETUP_END 0.2, TEST_START 0.0, TEST_END 13.4, FINISH_END 0.1

See test results for failed build of commit 2f2ac00002

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/vcqm50jnsco1xq5k/artifacts/output/nvda_snapshot_pr14147-26602,8a08c1db.exe
• CI timing (mins): INIT 0.0, INSTALL_START 0.8, INSTALL_END 0.8, BUILD_START 0.0, BUILD_END 21.2, TESTSETUP_START 0.0, TESTSETUP_END 0.1, TEST_START 0.0, TEST_END 0.7, FINISH_END 0.1

See test results for failed build of commit 8a08c1db77

[LeonarddeR]

This was tested by @dkager. He reported that crashes on his system that happened on Alpha still occur with this branch, so it looks like there is no real benefit in this pr other than that it decouples auto detection from braille._BgTrhead. I leave it up to @feerrenrut or someone else from NV Access to decide about this pr.

[lukaszgo1]

@leonardder Have you considered creating an issue describing these crashes? I understand that you don't have a solution, but not reporting at all means that unless either you or @dkager would create a fix the problem will remain unsolved.

[LeonarddeR]

@dkager Would you be able to do this? As far as I understood you correctly, the crashes are very rare and hard to reproduce reliably.

[dkager]

While the code from @leonardder does not fix everything, I did notice that it was much rarer for NVDA to fall over altogether. With 2022.3 I estimate there to be a 20-30% chance of NVDA crashing after waking the Brailliant BI 40X from sleep while on bluetooth. I haven't found any log entries pointing to the cause, though.

[dkager]

Update: I got this straight after booting the PC with NVDA auto-starting. Obviously, Leonard's code fixes this.

INFO - braille.BrailleHandler.setDisplayByName (11:15:37.686) - braille._BgThread (9276):
Loaded braille display driver noBraille, current display has 0 cells.
INFO - braille.BrailleHandler.setDisplayByName (11:15:39.143) - braille._BgThread (9276):
Loaded braille display driver noBraille, current display has 0 cells.
INFO - braille.BrailleHandler.setDisplayByName (11:15:39.159) - braille._BgThread (9276):
Loaded braille display driver noBraille, current display has 0 cells.
INFO - brailleDisplayDrivers.brailliantB.BrailleDisplayDriver.__init__ (11:15:40.259) - braille._BgThread (9276):
Found display with 40 cells connected via hid (\\?\hid#{00001124-0000-1000-8000-00805f9b34fb}_vid&00021d6b_pid&0246&col01#8&f2c30de&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030})
INFO - braille.BrailleHandler.setDisplayByName (11:15:40.264) - braille._BgThread (9276):
Loaded braille display driver brailliantB, current display has 40 cells.
ERROR - stderr (11:15:40.269) - braille._BgThread (9276):
Exception in thread braille._BgThread:
Traceback (most recent call last):
  File "threading.pyc", line 926, in _bootstrap_inner
  File "threading.pyc", line 870, in run
  File "braille.pyc", line 2368, in func
OSError: exception: access violation writing 0xDBB3EFC4

[dkager]

And interestingly, when disabling HID braille support:

ERROR - braille.executor (12:15:09.681) - braille._BgThread (7440):
Error displaying cells. Disabling display
Traceback (most recent call last):
  File "braille.pyc", line 2344, in executor
  File "brailleDisplayDrivers\brailliantB.pyc", line 240, in display
  File "hwIo\hid.pyc", line 301, in setOutputReport
OSError: [WinError 1167] The device is not connected.
ERROR - braille.BrailleHandler.handleDisplayUnavailable (12:15:09.681) - braille._BgThread (7440):
Braille display unavailable. Disabling
Traceback (most recent call last):
  File "braille.pyc", line 2344, in executor
  File "brailleDisplayDrivers\brailliantB.pyc", line 240, in display
  File "hwIo\hid.pyc", line 301, in setOutputReport
OSError: [WinError 1167] The device is not connected.
DEBUG - autoSettingsUtils.autoSettings.AutoSettings._registerConfigSaveAction (12:15:09.681) - braille._BgThread (7440):
registering pre_configSave action: <class 'brailleDisplayDrivers.noBraille.BrailleDisplayDriver'>
DEBUG - braille.BrailleHandler.setDisplayByName (12:15:09.681) - braille._BgThread (7440):
Switching braille display from brailliantB to noBraille
ERROR - braille.BrailleDisplayDriver.terminate (12:15:09.681) - braille._BgThread (7440):
Display driver <brailleDisplayDrivers.brailliantB.BrailleDisplayDriver object at 0x08CA3870> failed to display while terminating.
Traceback (most recent call last):
  File "braille.pyc", line 2344, in executor
  File "brailleDisplayDrivers\brailliantB.pyc", line 240, in display
  File "hwIo\hid.pyc", line 301, in setOutputReport
OSError: [WinError 1167] The device is not connected.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "braille.pyc", line 2504, in terminate
  File "brailleDisplayDrivers\brailliantB.pyc", line 240, in display
  File "hwIo\hid.pyc", line 301, in setOutputReport
OSError: [WinError 1167] The device is not connected.
DEBUG - autoSettingsUtils.autoSettings.AutoSettings._loadSpecificSettings (12:15:09.692) - braille._BgThread (7440):
loading braille noBraille
INFO - braille.BrailleHandler.setDisplayByName (12:15:09.692) - braille._BgThread (7440):
Loaded braille display driver noBraille, current display has 0 cells.
DEBUG - autoSettingsUtils.autoSettings.AutoSettings._registerConfigSaveAction (12:15:09.702) - braille._BgThread (7440):
registering pre_configSave action: <class 'brailleDisplayDrivers.brailliantB.BrailleDisplayDriver'>
DEBUG - braille.BrailleHandler.setDisplayByName (12:15:11.161) - braille._BgThread (7440):
Reinitializing noBraille braille display
DEBUG - autoSettingsUtils.autoSettings.AutoSettings._registerConfigSaveAction (12:15:11.161) - braille._BgThread (7440):
registering pre_configSave action: <class 'brailleDisplayDrivers.noBraille.BrailleDisplayDriver'>
DEBUG - autoSettingsUtils.autoSettings.AutoSettings._loadSpecificSettings (12:15:11.166) - braille._BgThread (7440):
loading braille noBraille
INFO - braille.BrailleHandler.setDisplayByName (12:15:11.166) - braille._BgThread (7440):
Loaded braille display driver noBraille, current display has 0 cells.
DEBUG - autoSettingsUtils.autoSettings.AutoSettings._registerConfigSaveAction (12:15:11.166) - braille._BgThread (7440):
registering pre_configSave action: <class 'brailleDisplayDrivers.brailliantB.BrailleDisplayDriver'>
DEBUGWARNING - brailleDisplayDrivers.brailliantB.BrailleDisplayDriver.__init__ (12:15:11.166) - braille._BgThread (7440):
Traceback (most recent call last):
  File "brailleDisplayDrivers\brailliantB.pyc", line 101, in __init__
  File "hwIo\hid.pyc", line 150, in __init__
PermissionError: [WinError 5] Access is denied.
DEBUG - braille.BrailleHandler.setDisplayByName (12:15:11.166) - braille._BgThread (7440):
Reinitializing noBraille braille display
DEBUG - autoSettingsUtils.autoSettings.AutoSettings._registerConfigSaveAction (12:15:11.166) - braille._BgThread (7440):
registering pre_configSave action: <class 'brailleDisplayDrivers.noBraille.BrailleDisplayDriver'>
DEBUG - autoSettingsUtils.autoSettings.AutoSettings._loadSpecificSettings (12:15:11.171) - braille._BgThread (7440):
loading braille noBraille
INFO - braille.BrailleHandler.setDisplayByName (12:15:11.172) - braille._BgThread (7440):
Loaded braille display driver noBraille, current display has 0 cells.
DEBUG - autoSettingsUtils.autoSettings.AutoSettings._registerConfigSaveAction (12:15:11.172) - braille._BgThread (7440):
registering pre_configSave action: <class 'brailleDisplayDrivers.brailliantB.BrailleDisplayDriver'>
INFO - brailleDisplayDrivers.brailliantB.BrailleDisplayDriver.__init__ (12:15:12.252) - braille._BgThread (7440):
Found display with 40 cells connected via hid (\\?\hid#{00001124-0000-1000-8000-00805f9b34fb}_vid&00021d6b_pid&0246&col01#8&f2c30de&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030})
DEBUG - braille.BrailleHandler.setDisplayByName (12:15:12.252) - braille._BgThread (7440):
Switching braille display from noBraille to brailliantB
DEBUG - autoSettingsUtils.autoSettings.AutoSettings._loadSpecificSettings (12:15:12.262) - braille._BgThread (7440):
loading braille brailliantB
INFO - braille.BrailleHandler.setDisplayByName (12:15:12.262) - braille._BgThread (7440):
Loaded braille display driver brailliantB, current display has 40 cells.

[LeonarddeR]

These permissions aren't that worrying. The access violation is much more concerning, though. Ok, let's give this a spin.

[dkager]

An access violation in the Windows sleep function especially. :)

[LeonarddeR]

An access violation in the Windows sleep function especially. :)

I guess it is crashing because the sleep waits for a handle that has been destroyed. That's why I want to get rid of this APC stuff altogether, it requires a lot of specific Win32 knowledge while in contrast, the Pythonic implementation this pr is much more readable IMO.

[dkager]

Perhaps refactoring should be a separate step beyond implementing a fix for the very real issue of the display detection crashing. I imagine that making braille more deterministic could cause some interesting new issues at first.

[LeonarddeR]

@seanbudd Not sure why you're marking this as draft? It is ready for review and currently under review.

[seanbudd]

@seanbudd Not sure why you're marking this as draft? It is ready for review and currently under review.

This comment is unresolved: https://github.com/nvaccess/nvda/pull/14147#discussion_r995364727

[LeonarddeR]

I think this is ready again if you agree this case is too complex for unit tests.

[seanbudd]

For Developers

Reduced the complexity of the background braille display auto detection logic

Other than API breaking change I raised, I don't believe this PR affects developers. Would a braille display driver developer notice any changes other than better performance from this PR?

[LeonarddeR]

Other than API breaking change I raised, I don't believe this PR affects developers. Would a braille display driver developer notice any changes other than better performance from this PR?

Not likely. There will be a difference in that an automatically detected braille display driver will now be initialized on a different thread (i.e. the threadpool thread, not braille._BgThread). However:

1. This shouldn't be problematic as a braille display that is selected explicitly from the braille display selection dialog is initialized on the main thread.
2. These drivers are supposed to be threadsafe

[seanbudd]

As we don't expect any changes for developers, i.e. this is changes to internal code only, I am going to avoid adding a \change log entry.