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

Added a Braille display driver for the SuperBraille device used primarily in Taiwan. #7352

Merged
merged 3 commits into from Jul 18, 2017

Conversation

@michaelDCurran
Copy link
Contributor

@michaelDCurran michaelDCurran commented Jul 4, 2017

Summary of the issue:

In Taiwan, the primary braille display that is used is the SuperBraille. this is Taiwan-made and is very popular in Government and education etc, despite its large size and lack of typing and scroll buttons.
Due to other Taiwan-specific screen readers now no longer being maintained, NVDA is fast becoming a very popular screen reader. However, for NVDA uptake it is essential that the SuperBraille is supported as individuals and governments have spent significant finances on these displays.

Description of how this pull request fixes the issue:

There was a 3rd party add-on written for this display. However, it had issues when restarting NVDA, and was not using the most modern braile display driver techniques in NVDA.
This pull request introduces that code, at the same time addressing the restart bug, and modernizing the driver.
This new driver also stops probing all serial ports, rather only allowing automatic for one particular USB hardware ID, and then allowing manual selection for other serial ports.

Testing performed:

A SuperBraille v3.2 was used to test while updating this driver.

Known issues with pull request:

  • I have seen this SuperBraille unit start showing garbage after using this driver for a while once. Further testing by the Taiwan community is necessary.
  • As this driver has no physical scroll buttons at all, it is necessary to bind scrolling to keyboard gestures. Currently to maintain compatibility with other screen readers in Taiwan, numpadMinus scrolls back and numpadPlus scrolls forward. Obviously the use of numpadPlus eclipses review sayAll in desktop layout. However, as a vast majority of the users of this display expect this behavior, there would be some work in getting a change accepted by the users.
@michaelDCurran michaelDCurran requested a review from jcsteh Jul 4, 2017
# Translators: Names of braille displays.
description = _("SuperBraille")
isThreadSafe=True
_dev=None

This comment has been minimized.

@jcsteh

jcsteh Jul 4, 2017
Contributor

I don't think this needs to be initialised here; it should be set in the constructor. There's a risk that this might hide a logic error somewhere.

for portInfo in comPorts:
# Translators: Name of a serial communications port.
ports[portInfo["port"]] = _("Serial: {portName}").format(portName=portInfo["friendlyName"])
print "ports: %s"%ports

This comment has been minimized.

@jcsteh

jcsteh Jul 4, 2017
Contributor

Please remove.

log.debug("Checking port %s for a SuperBraille", port)
try:
self._dev = hwIo.Serial(port, baudrate=BAUD_RATE, stopbits=serial.STOPBITS_ONE, parity=serial.PARITY_NONE, timeout=TIMEOUT, writeTimeout=TIMEOUT, onReceive=self._onReceive)
log.debug("Port opened.")

This comment has been minimized.

@jcsteh

jcsteh Jul 4, 2017
Contributor

I don't think these debug messages are needed because we can get this info (and more) from the hwIo debug logging.

found = True
break
else:
self._dev.close()

This comment has been minimized.

@jcsteh

jcsteh Jul 4, 2017
Contributor

Each port needs to be closed after a failed attempt, not just the last one. So, this else block needs to be indented one more so it's inside the loop iteration.

else:
self._dev.close()
if not found:
raise RuntimeError, "No SuperBralle found"

This comment has been minimized.

@jcsteh

jcsteh Jul 4, 2017
Contributor

Typo: "SuperBralle"

return
data=self._dev.read(1)
if data!='\x05':
log.info("Unknown second byte")

This comment has been minimized.

@jcsteh

jcsteh Jul 4, 2017
Contributor

I'd log these as debugWarning, rather than info, as they are meaningless to users and probably harmless unless the user is seeing a problem.

self._dev.write(DISPLAY_TAG + "".join(out))
except EnvironmentError as e:
self._closeComPort()
raise e

This comment has been minimized.

@jcsteh

jcsteh Jul 4, 2017
Contributor

You shouldn't need to catch exceptions here and close the device; braille core now handles that for you. If the display method throws an exception, it switches to noBraille, which calls terminate on the display. This also means you can get rid of the checks for _dev being None and probably merge _closeComPort into terminate.

@@ -2231,6 +2231,16 @@ Please see the [EcoBraille documentation ftp://ftp.once.es/pub/utt/bibliotecnia/
| Toggle braille tethered to | A |
%kc:endInclude

++ Super Braille ++

This comment has been minimized.

@jcsteh

jcsteh Jul 4, 2017
Contributor

This heading calls it "Super Braille" (with a space), whereas elsewhere, it's "SuperBraille" (camel case). Which one? :)

@@ -2231,6 +2231,16 @@ Please see the [EcoBraille documentation ftp://ftp.once.es/pub/utt/bibliotecnia/
| Toggle braille tethered to | A |
%kc:endInclude

++ Super Braille ++
The SuperBraille device, mostly available in Taiwan, can be connected to by either USB or serial.

This comment has been minimized.

@jcsteh

jcsteh Jul 4, 2017
Contributor

Are there any links we need to/can provide here for USB drivers or the like? I get this thing isn't overly well documented (at least in English), so we can always add that later if you have no idea and someone asks for it.

…ile to superBrl,matching its name attribute. It can't internally be called superBraille as clashing with the existing add-on would make debugging tricky.
Copy link
Contributor

@jcsteh jcsteh left a comment

Looks good barring one nit below. Just a question: I understand why you wouldn't want them to be the same name, since the add-on would override, but how will the users tell the difference between the two? Do they have a different description or something?

self._dev.close()
if not found:
raise RuntimeError, "No SuperBralle found"
raise RuntimeError, "No SuperBraille found"

This comment has been minimized.

@jcsteh

jcsteh Jul 4, 2017
Contributor

Sorry, missed this. This is old Python syntax. Please change to:

raise RuntimeError("No SuperBraille found")

That is, pass the message to the exception's constructor.

@jcsteh
jcsteh approved these changes Jul 4, 2017
Copy link
Contributor

@jcsteh jcsteh left a comment

Bah. Forgot to hit approve.

michaelDCurran added a commit that referenced this pull request Jul 4, 2017
michaelDCurran added a commit that referenced this pull request Jul 4, 2017
@michaelDCurran michaelDCurran merged commit 45f0787 into master Jul 18, 2017
1 check passed
1 check passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
@nvaccessAuto nvaccessAuto added this to the 2017.3 milestone Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants