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

Native driver for Eurobraille Esys, Esytime and newer Iris displays #7859

Merged
merged 14 commits into from Feb 9, 2018

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Dec 15, 2017

Link to issue number:

Closes #7488.
Note that this also contains the commits from #7732, which is currently incubating. I'm sure git merging will deal with that properly.

Summary of the issue:

For Eurobraille displays, users currently rely on an add-on packaged driver that has some drawbacks for NVDA users, including:

  1. The inability to assign gestures for many keys using NVDA's input gesture framework
  2. The use of a custom braille input layer
  3. The add-on is quite huge to distribute and, due to its size, can't be integrated in NVDA core
  4. The add-on based driver won't support braille display auto detection (Give NVDA the ability to automatically detect braille displays. #1271)

Description of how this pull request fixes the issue:

This pr implements a native driver for Eurobraille displays, with support for automatic and manual port selection. Note that for Iris displays, port selection should always occur manually, as these are serial displays.

For Esytime, the driver supports switching between protocol based input (i.e. input handled by NVDA) and HID simulation input. Support for HID simulation might be added to Esys in the future.

The driver also contains key bindings as requested by Eurobraille. There ought to be proper support for several firmware versions, including newer braille displays which send acknowledgement packets. Due to the acknowledgement handling required, this driver requires #7732, and therefore this pr contains commits from that pr. All changes to braille.py are already incubating and thus don't need additional review.

Eurobraille wants us to provide other applications access to a device while NVDA is using it. Because of that, hwIo.Hid now takes an additional parameter exclusive, which defaults to True. If set to False as in the Eurobraille driver, access to the device handle is not restricted for other applications. As this is a specific manufacturer request, Any issues caused by this behaviour should be discussed with Eurobraille.

Testing performed:

I tested an Esys 12 and Esys 40 myself. I also owe many thanks to @clementb49 and @Andre9642, the former tested with an Esytime Evo I belief. Last but not least, many thanks go out to Eurobraille, as they performed tests with Esys 64 and 80, Esytime and Iris displays.

Known issues with pull request:

Eurobraille requested some key assignments which could not yet be made (e.g. toggling control or shift). This will be possible whenever #7843 is implemented.

Change log entry:

Copy link
Member

@michaelDCurran michaelDCurran left a comment

Choose a reason for hiding this comment

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

@LeonarddeR: could you please merge master into this pr again now that acknowledgement packets has been merged to master? This will make the diff easier to read. Strangely git diff and github are still showing a lot of code being added which is already in master, such as CreateWaitableTimer.

| switch to next review mode | NVDA+numpad7 | NVDA+pageUp | 2-finger flick up | switches to the next available review mode |
| switch to previous review mode | NVDA+numpad1 | NVDA+pageDown | 2-finger flick down | switches to the previous available review mode |
| Switch to next review mode | NVDA+numpad7 | NVDA+pageUp | 2-finger flick up | switches to the next available review mode |
| Switch to previous review mode | NVDA+numpad1 | NVDA+pageDown | 2-finger flick down | switches to the previous available review mode |
Copy link
Member

Choose a reason for hiding this comment

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

What changed here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The initial command descriptions did not start with an upper case letter. Now they do. I actually had forgotten I had changed this. I can revert if you like, but it looked more consistent.

@@ -55,6 +56,58 @@ def CreateFile(fileName,desiredAccess,shareMode,securityAttributes,creationDispo
return res


def createWaitableTimer(securityAttributes=None, manualReset=False, name=None):
Copy link
Member

Choose a reason for hiding this comment

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

I am confused with this diff. Isn't this all already in master? Did github get this wrong?

@@ -55,6 +56,58 @@ def CreateFile(fileName,desiredAccess,shareMode,securityAttributes,creationDispo
return res


def createWaitableTimer(securityAttributes=None, manualReset=False, name=None):
Copy link
Member

Choose a reason for hiding this comment

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

I think github is not showing this right... but it looks like CreateWaitableTimer and SetWaitableTimer are being added here, yet they already exist in master I think.

@michaelDCurran
Copy link
Member

@LeonarddeR: could you please merge master into this pr again now that acknowledgement packets has been merged to master? This will make the diff easier to read. Strangely git diff and github are still showing a lot of code being added which is already in master, such as CreateWaitableTimer.

@LeonarddeR
Copy link
Collaborator Author

Just did :)

@@ -46,6 +46,7 @@ def GetStdHandle(handleID):
GENERIC_WRITE=0x40000000
FILE_SHARE_READ=1
FILE_SHARE_WRITE=2
FILE_SHARE_DELETE=4
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one isn't used, but it was the only access right constant that was missing, so people who use winKernel might expect it should be there.

Leonard de Ruijter added 2 commits January 26, 2018 19:37
…se appModules simply set NVDA into sleep mode, as these applications are self-voicing/self-brailling
michaelDCurran added a commit that referenced this pull request Feb 1, 2018
@michaelDCurran michaelDCurran merged commit da5f33b into nvaccess:master Feb 9, 2018
@nvaccessAuto nvaccessAuto added this to the 2018.1 milestone Feb 9, 2018
@LeonarddeR LeonarddeR added the BabbageWork Pull requests filed on behalf of Babbage B.V. label Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BabbageWork Pull requests filed on behalf of Babbage B.V.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Native driver for Eurobraille Esys displays
3 participants