Prevent crash when selecting papenmeier braille display driver, remove brxcom wrapper by Stefan-Kliesch-FHP · Pull Request #13348 · nvaccess/nvda

Stefan-Kliesch-FHP · 2022-02-16T17:51:25Z

Link to issue number:

None.

Summary of the issue:

BrxCom wrapper is not compatible with Python 3 anymore and causes NVDA to crash.
Furthermore BrxCom is deprecated and is no longer supported.

Description of how this pull request fixes the issue:

BrxCom support via wrapper dll removed from braille display driver.

Testing strategy:

Modified braille display driver tested via Developer Scratchpad.

Known issues with pull request:

None.

Change log entries:

Fixed a bug where selecting the Papenmeier Braille Display Driver caused NVDA to crash when BrxCom is installed.

Pull Request description:
- description is up to date
- change log entries
Testing:
- Manual testing
API is compatible with existing add-ons.
Documentation:
- User Documentation
UX of all users considered:
- Braille

BrxCom via wrapper dll removed. Wrapper was not compatible with Python 3 anymore.

Stefan-Kliesch-FHP · 2022-02-16T17:54:36Z

This is my first pull request.
Please provide some help if something is not as it should be or as it be expected.

lukaszgo1 · 2022-02-16T18:58:07Z

Given that BrxCom wrapper is just a dll it cannot be incompatible with Python 3 and removing support for it, while certainly the easiest solution, may introduce its own regressions. Could you please share the exception you got when trying to use BrxCom with Python 3 version of NVDA - it would be much better to fix the code rather than remove it?

Stefan-Kliesch-FHP · 2022-02-16T19:39:10Z

BrxCom is deprecated, so we (F.H. Papenmeier) don't support it anymore.
All our Braille displays work with NVDA without BrxCom.
BrxCom handled the braille and qwerty input from the keyboards with other screenreaders.
The wrapper stopped working with the transition from Python 2 to Python 3.

I work at F.H. Papenmeier as software engineer, I am responsible for the braille display software and it's drivers.

lukaszgo1 · 2022-02-16T20:07:58Z

Thanks @Stefan-Kliesch-FHP That is very helpful to know. Could you please update the description of this PR to mention that BrxCom is deprecated and therefore support for it is removed? Also please remove mentions of BrxCom from user_docs\enuserGuide.t2t

LeonarddeR · 2022-02-16T20:09:32Z

@Stefan-Kliesch-FHP Great to have you on board here! I'm glad to review the driver later this week!

LeonarddeR · 2022-02-16T20:14:18Z

@Stefan-Kliesch-FHP Could you also elaborate on the papenmeier_serial driver? Do you think it is still used, or is it obsolete?

BrxCom removed from user guide.

Stefan-Kliesch-FHP · 2022-02-16T20:39:47Z

@LeonarddeR I think only a very few user will use it with the old braille dispays. I never tested it via serial port.
Looking forward for your testing.

I tested the braille driver with a BRAILLEX EL40c, BRAILLEX Trio, BRAILLEX Live+.
But only with USB connection. I will perform a bluetooth test later this week.

I removed the BrxCom from the english and german userGuide.t2t.

seanbudd

Thanks for this PR @Stefan-Kliesch-FHP.
I've suggested 2 minor changes, I think this should be good to merge after these are fixed up, but I'll wait for @LeonarddeR to do the review he wants to do.

restored file

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

lukaszgo1 · 2022-02-17T12:49:26Z

 Wenn Sie weitere Informationen oder Hilfe bezüglich NVDA benötigen, besuchen Sie bitte die NVDA-Homepage unter NVDA_URL.
 Neben Ressourcen der Community und dem technischen Support finden Sie auch zusätzliche Dokumentationen.
 Auf diesen Seiten werden Informationen und Ressourcen zur NVDA-Entwicklung ebenfalls bereitgestellt.
-


Please reintroduce this line - there should be no changes to the German documentation in this PR as it would introduce conflicts for translators.

Is it fine now?

I'm afraid it isn't. This should be just empty line not hyphen. Essentially when looking at the list of files modified by this PR German user guide should not be mentioned at all.

So I restored it from nvaccess:master and it's gone from the "files changed" list!

lukaszgo1 · 2022-02-17T12:51:34Z


-If BrxCom is installed, NVDA will use BrxCom.
-BrxCom is a tool that allows keyboard input from the braille display to function independently from a screen reader.
-Keyboard input is possible with the Trio and BRAILLEX Live models.


Just to make sure - since you've removed this line are you saying that braille input is supported with other devices as well?

BrxCom is a kind of connection chain. BrxCom connects to the braille display and the screenreader connects to BrxCom.
All keyboard inputs are handled by BrxCom and not passed through to the screenreader. NVDA is able to handle the qwerty and the braille input itself. Other screenreaders also implemented this function, so BrxCom is not needed anymore.

AppVeyorBot · 2022-02-17T18:03:59Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests. See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/gcjg7tvwnn5wjbwa/artifacts/output/nvda_snapshot_pr13348-24716,1908a760.exe

See test results for failed build of commit 1908a760a6

restore de/userGuide.t2t from nvaccess:master

seanbudd · 2022-02-21T05:25:33Z

How did the bluetooth tests go? This looks ready to merge otherwise.

Stefan-Kliesch-FHP · 2022-02-21T11:15:01Z

Sorry, been very busy last week. I will test it, today or tomorrow.

Stefan-Kliesch-FHP · 2022-02-21T12:17:22Z

So I tested it immediately, but I had a problem with the "Build (for testing PR)" version. The settings dialog wasn't available.
So I tested it with the 2021.3.1 and added the papenmeier.py to the developer scratchpad. Which worked fine with a bluetooth connection to the BRAILLEX Live+. Keyboard inputs work also fine.
Any ideas why I couldn't use the the testbuild?

Stefan-Kliesch-FHP · 2022-02-21T12:27:05Z

I encountered an other strange behavior. If there was a working bluetooth connection via NVDA and the BRAILLEX Live+ and I switch off the braille display, NVDA starts to act strange. After a few seconds it lags with keyboard inputs and GUI updates. And it's not recovering. I have to switch on the braille display again or end the NVDA task in the Task-Manager. I think it's due to the lost connection.

Stefan-Kliesch-FHP · 2022-02-23T11:35:44Z

Hello everyone,
I was able to test it with the PR Build.
I have to press Ctrl + NVDA + A to selecct the braille display.
And it works so far, but the bluetooth disconnect problem remains.
It seems that either the HWPortUtils nor the Serial-object throw an exception when the bluetooth device is disconnected.
Any ideas about that?

When I try to select the braille display via the main settings dialog I get thje following traceback:
-Base configuration saved
-ERROR - unhandled exception (12:21:42.928) - MainThread (21620):

Traceback (most recent call last):
File "gui_init_.pyc", line 224, in onNVDASettingsCommand
File "gui_init_.pyc", line 166, in _popupSettingsDialog
File "gui\settingsDialogs.pyc", line 463, in init
File "gui\settingsDialogs.pyc", line 193, in init
File "gui\settingsDialogs.pyc", line 4000, in makeSettings
File "gui\settingsDialogs.pyc", line 532, in makeSettings
File "gui\settingsDialogs.pyc", line 641, in _doCategoryChange
File "gui\settingsDialogs.pyc", line 569, in _getCategoryPanel
File "gui\settingsDialogs.pyc", line 327, in init
File "gui\settingsDialogs.pyc", line 337, in _buildGui
File "gui\settingsDialogs.pyc", line 805, in makeSettings
File "config_init_.pyc", line 391, in getStartOnLogonScreen
-UnboundLocalError: local variable 'k' referenced before assignment
-INFO - globalCommands.GlobalCommands.script_navigatorObject_devInfo (12:21:44.871) - MainThread (21620):

This can't be caused by the papenmeier driver, so any ideas?

Stefan-Kliesch-FHP · 2022-02-23T16:28:11Z

How do we continue here?

LeonarddeR · 2022-02-23T16:51:01Z

Hey @Stefan-Kliesch-FHP,

To get rid of the issues with bluetooth, I'd suggest a more thorough rewrite of the driver that is more like the other native drivers (e.g. Handy Tech, Hims, Baum). In short, make use of the hwIo module to ensure asynchronous communication with the display. Unfortunately, the FTDI2 module is a bit of a show stopper here, since it doesn't support overlapped IO I believe.
Now I wonder, is it really necessary to communicate with the device with this FTDI2 module or is it also possible to use generic Win32 serial communication somehow? I once tried to get the Win32 handle from the FTDI2 handle in order to write to it with the Win32 Serial APIs, but that was slightly hacky. How do you communicate with these display with the other screen readers?

LeonarddeR · 2022-02-23T18:35:41Z

I noticed that the FTDI driver should support both D2XX and virtual COM port access. So if we can switch to the virtual COM port model somehow, that would be greatly preferred and bring the following benefits. Note that these things shouldn't be impossible with D2XX, it will just take much more research and effort to get it working:

Implementing braille display auto detection.
Unifying the bluetooth and usb code as both use virtual COM ports
Probably better performance because we can easily switch to async I/O.

@Stefan-Kliesch-FHP

…creen (#13384) Fix-up of PR #13242 Originally reported by @Stefan-Kliesch-FHP in #13348 Summary of the issue: getStartOnLogonScreen can fail with UnboundLocalError when the autostart key does not exist. Description of how this pull request fixes the issue: If we failed to open autostart key we no longer try to access it.

Stefan-Kliesch-FHP · 2022-02-25T14:08:13Z

@LeonarddeR
This would mostly effect the USB connections, which works fine so far.
I would suggest that we just merge this pull request.
And if I find some time, I will consider to use HWio for the Bluetooth communication in the future. Including a new PR.

seanbudd · 2022-03-02T04:45:14Z

Hi @Stefan-Kliesch-FHP -

Could you update the template to include a changelog entry on how this affects the user?
Perhaps:

Fixed a bug where selecting the Papenmeier Braille Display Driver caused NVDA to crash.

seanbudd

Thanks @Stefan-Kliesch-FHP

…BrxCom-Removed

Qchristensen

Great work, thank you!

Add files via upload

9bcb6de

BrxCom via wrapper dll removed. Wrapper was not compatible with Python 3 anymore.

Stefan-Kliesch-FHP requested a review from a team as a code owner February 16, 2022 17:51

Stefan-Kliesch-FHP requested a review from seanbudd February 16, 2022 17:51

Stefan-Kliesch-FHP changed the title ~~Add files via upload~~ Update papenmeier braille display driver Feb 16, 2022

This comment was marked as off-topic.

Sign in to view

Update userGuide.t2t

8943bd9

BrxCom removed from user guide.

Stefan-Kliesch-FHP requested a review from a team as a code owner February 16, 2022 20:33

Stefan-Kliesch-FHP requested a review from Qchristensen February 16, 2022 20:33

Update userGuide.t2t

871daa2

seanbudd reviewed Feb 16, 2022

View reviewed changes

Comment thread user_docs/de/userGuide.t2t

seanbudd requested a review from LeonarddeR February 16, 2022 22:21

seanbudd reviewed Feb 16, 2022

View reviewed changes

Comment thread source/brailleDisplayDrivers/papenmeier.py Outdated

Stefan-Kliesch-FHP and others added 2 commits February 17, 2022 10:02

Update userGuide.t2t

efae903

restored file

Update source/brailleDisplayDrivers/papenmeier.py

519062f

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

lukaszgo1 reviewed Feb 17, 2022

View reviewed changes

Update userGuide.t2t

656246c

Add files via upload

efc9232

restore de/userGuide.t2t from nvaccess:master

lukaszgo1 mentioned this pull request Feb 23, 2022

No longer try to access non-existing registry key in getStartOnLogonScreen #13384

Merged

5 tasks

seanbudd approved these changes Mar 7, 2022

View reviewed changes

seanbudd added 2 commits March 8, 2022 09:56

Merge remote-tracking branch 'origin/master' into Stefan-Kliesch-FHP-…

463e76b

…BrxCom-Removed

update changes

44f5e3d

Qchristensen approved these changes Mar 7, 2022

View reviewed changes

seanbudd merged commit aa7049d into nvaccess:master Mar 7, 2022

nvaccessAuto added this to the 2022.1 milestone Mar 7, 2022

seanbudd changed the title ~~Update papenmeier braille display driver~~ Prevent crash when selecting papenmeier braille display driver, remove brxcom wrapper Mar 7, 2022

Uh oh!

Conversation

Stefan-Kliesch-FHP commented Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entries:

Uh oh!

Stefan-Kliesch-FHP commented Feb 16, 2022

Uh oh!

This comment was marked as off-topic.

lukaszgo1 commented Feb 16, 2022

Uh oh!

Stefan-Kliesch-FHP commented Feb 16, 2022

Uh oh!

lukaszgo1 commented Feb 16, 2022

Uh oh!

LeonarddeR commented Feb 16, 2022

Uh oh!

LeonarddeR commented Feb 16, 2022

Uh oh!

Stefan-Kliesch-FHP commented Feb 16, 2022

Uh oh!

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lukaszgo1 Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

Stefan-Kliesch-FHP Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

lukaszgo1 Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

Stefan-Kliesch-FHP Feb 18, 2022

Choose a reason for hiding this comment

Uh oh!

lukaszgo1 Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

Stefan-Kliesch-FHP Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Feb 17, 2022

Uh oh!

seanbudd commented Feb 21, 2022

Uh oh!

Stefan-Kliesch-FHP commented Feb 21, 2022

Uh oh!

Stefan-Kliesch-FHP commented Feb 21, 2022

Uh oh!

Stefan-Kliesch-FHP commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Stefan-Kliesch-FHP commented Feb 23, 2022

Uh oh!

Stefan-Kliesch-FHP commented Feb 23, 2022

Uh oh!

LeonarddeR commented Feb 23, 2022

Uh oh!

LeonarddeR commented Feb 23, 2022

Uh oh!

Stefan-Kliesch-FHP commented Feb 25, 2022

Uh oh!

seanbudd commented Mar 2, 2022

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

Qchristensen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Stefan-Kliesch-FHP commented Feb 16, 2022 •

edited

Loading

Stefan-Kliesch-FHP commented Feb 21, 2022 •

edited

Loading