Skip to content
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

Prevent crash when selecting papenmeier braille display driver, remove brxcom wrapper #13348

Merged

Conversation

Stefan-Kliesch-FHP
Copy link
Contributor

@Stefan-Kliesch-FHP Stefan-Kliesch-FHP commented Feb 16, 2022

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 Stefan-Kliesch-FHP requested a review from a team as a code owner February 16, 2022 17:51
@Stefan-Kliesch-FHP Stefan-Kliesch-FHP changed the title Add files via upload Update papenmeier braille display driver Feb 16, 2022
@Stefan-Kliesch-FHP
Copy link
Contributor Author

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

@AppVeyorBot

This comment was marked as off-topic.

@lukaszgo1
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Collaborator

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

@LeonarddeR
Copy link
Collaborator

@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
Copy link
Contributor Author

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

Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

source/brailleDisplayDrivers/papenmeier.py Outdated Show resolved Hide resolved
restored file
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
@@ -3272,4 +3272,3 @@ Die folgenden Werte können geändert werden:
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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it fine now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -2858,10 +2858,6 @@ The following Braille displays are supported:

These displays do not support NVDA's automatic background braille display detection functionality.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

See test results for failed build of commit 1908a760a6

restore de/userGuide.t2t from nvaccess:master
@seanbudd
Copy link
Member

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

@Stefan-Kliesch-FHP
Copy link
Contributor Author

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

@Stefan-Kliesch-FHP
Copy link
Contributor Author

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
Copy link
Contributor Author

Stefan-Kliesch-FHP commented Feb 21, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor Author

How do we continue here?

@LeonarddeR
Copy link
Collaborator

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
Copy link
Collaborator

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:

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

seanbudd pushed a commit that referenced this pull request Feb 23, 2022
…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
Copy link
Contributor Author

@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
Copy link
Member

seanbudd commented Mar 2, 2022

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.

Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, thank you!

@seanbudd seanbudd merged commit aa7049d into nvaccess:master Mar 7, 2022
@nvaccessAuto nvaccessAuto added this to the 2022.1 milestone Mar 7, 2022
@seanbudd seanbudd changed the title Update papenmeier braille display driver Prevent crash when selecting papenmeier braille display driver, remove brxcom wrapper Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants