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
Conversation
# Translators: Names of braille displays. | ||
description = _("SuperBraille") | ||
isThreadSafe=True | ||
_dev=None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "SuperBralle"
return | ||
data=self._dev.read(1) | ||
if data!='\x05': | ||
log.info("Unknown second byte") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
user_docs/en/userGuide.t2t
Outdated
@@ -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 ++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This heading calls it "Super Braille" (with a space), whereas elsewhere, it's "SuperBraille" (camel case). Which one? :)
user_docs/en/userGuide.t2t
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bah. Forgot to hit approve.
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: