Support the BrailleNote Touch and Brailliant BI 14 via the BrailliantB brailledisplayDriver by michaelDCurran · Pull Request #8031 · nvaccess/nvda

michaelDCurran · 2018-02-22T21:32:16Z

Link to issue number:

Summary of the issue:

The BrailleNote touch braille display from Humanware is currently not supported in NVDA.
This display shares a similar protocol to the Brailliant BI/B, though has a different bluetooth name, and is available via USB HID with no drivers necessary).
Similarly, the Brailliant BI 14 is not supported either, nor are the other Brailliant BI/B models with Bluetooth HID.

Description of how this pull request fixes the issue:

Extended the Brailliant BI/B braille display driver to support the BrailleNote Touch and the Brailliant BI 14, as well as Bluetooth HID for the existing Brailliant BI/B devices.
This PR is mostly @jcsteh's original BrailliantBtHid branch, however:

hwIo.Hid now has a setOutputReport method which allows sending data as a Hid output report via HidD_SetOutputReport. This is an alternative to using WriteFile.
When supporting the BrailleNote Touch via USB HID, write display cells via Hid.setOutputReport, as for some reason WriteFile blocks forever when talking to this display.
Append the description of the Brailliant BI/B driver to also mention BrailleNote Touch
Update the userGuide to mention the BrailleNote Touch

Testing performed:

On a recent build of Windows 10, Using the Brailliant BI/B driver selected, connected to and used a BrailleNote Touch with NVDA, via both Bluetooth and USB.

Known issues with pull request:

There was an older BrailleNote Touch software version that announced itself as a Brailliant via bluetooth, which was also supported by this driver, but USB on this older version may not work. The user should update their BrailleNote Touch to the latest version for the best compatibility with NVDA.
it works yet.
Although support for the Brailliant BI 14 was also added here, including new key assignments for the joystick, I am unable to test this due to not having access to a device yet.

Change log entry:

What's new:

Support for the BrailleNote touch braille display via both USB and bluetooth.

…Bluetooth when the protocol is set to OpenBraille.

…lays between attempts as recommended by HumanWare, as init can be quite flakey, particularly via Bluetooth.

…key table.

… HidD_SetOutputReport as an alternative to WriteFile which may freeze on certain USB implementations.

…isplay cells with HidD_SetOutputReport rather than WriteFile which can block forever on these devices.

…es, and BrailleNote Touch

… Touch, splitting them into tables for all models, and extra specific.

LeonarddeR · 2018-02-23T06:05:15Z


 KEY_NAMES = {
-	# Braille keyboard.
+	1: "power", # Brailliant BI 32, 40 and 80.


When will this key be received by NVDA?

The documentation does not say exactly, but clearly suggests it could be sent. The reason I say this is because there is a specific exception in the documentation for BI 14 and BrailleNote touch which says that their power keys are not sent to the host.

When will this key be received by NVDA?

When you press the power button on the Brailliant. It doesn't do anything useful, but it's better than having an unrecognised key.

LeonarddeR

Looks very good. Some general concerns and questions:

How about the Bluetooth HID support? It is there, but your pr description doesn't mention it.
A major difference between how we use writefile and HidD_SetReport is that the latter is not asynchronous, it blocks the caller until True or False is returned. Would be good to know @jcsteh's opininion about this.
In my review of hwIo.Hid.setReport, I mention the size of the buffer that's written to the device. May be you could elaborate on what you've tested to make writefile do its job correctly? Have you also played with the buffer size, (e.g. using a buffer that's as long as the data it contains?

I belief normally, allocating a string buffer that's longer than the data it contains will have the buffer padded up with zero's. For Eurobraille, the buffer had to be padded up with \x55. I've had plenty of help from a USB analysing tool when writing that driver. I belief it operates in trial mode for around two weeks, it should allow you to make a txt dump of all the communication that's taking place between your system and a device, and you can filter it for raw i/o.

LeonarddeR · 2018-02-23T06:06:46Z

-		if portInfo.get("usbID") == "VID_1C71&PID_C006":
+		if portInfo.get("usbID") in USB_IDS_HID:
 			yield "USB HID", portInfo["devicePath"]
+		# In Windows 10, the Bluetooth vendor and product ids don't get recognised.


Has this also been tested on more recent builds of Windows 10, after @jcsteh worked on this?

I tested on an insider build of windows 10 (build 17100). Note that enither I or @jcsteh were ever able to actually test the Braliiant BA 14 (and possibly other models) ourselves. we periodically send builds to Humanware for them to confirm. Though now having a BrailleNote Touch for a while has certainly helped this process along.

Actually, I did test a Brailliant with Bluetooth HID when I was originally writing this, but I ran out of time before they needed the unit back unexpectedly early. It definitely didn't recognise the ids... and HumanWare confirmed they had problems with this too. Someone would need to check this with a Brailliant on newer builds to answer this properly.

LeonarddeR · 2018-02-23T06:11:40Z

 			continue
-		if btName.startswith("Brailliant B") or btName == "Brailliant 80":
-			yield "bluetooth", portInfo["port"]
+		if btName.startswith("Brailliant B") or btName == "Brailliant 80" or "BrailleNote Touch" in btName:


Just out of curiosity, is there a particular reason not to use btName.startswith for the bn touch?

That is its exact Bluetooth name. The rest of the Brailliant devices have their serial number appended I believe.

LeonarddeR · 2018-02-23T06:13:13Z

 	name = "brailliantB"
 	# Translators: The name of a series of braille displays.
-	description = _("HumanWare Brailliant BI/B series")
+	description = _("HumanWare Brailliant BI/B series, and BrailleNote Touch")


I think you can leave out the comma or even change , and into a slash.

I thought slash just looked funny, and that comma may have sounded like BrailleNote touch was another name for these devices.

I changed ", and" into a slash

LeonarddeR · 2018-02-23T06:15:28Z

 PARITY = serial.PARITY_EVEN
+DELAY_AFTER_CONNECT = 1.0
+INIT_ATTEMPTS = 3
+INIT_RETRY_DELAY = 0.2


This is equal to the TIMEOUT constant, so you might as well use that.

LeonarddeR · 2018-02-23T06:21:04Z

+			time.sleep(DELAY_AFTER_CONNECT)
+			# Sometimes, a few attempts are needed to init successfully.
+			for attempt in xrange(INIT_ATTEMPTS):
+				if attempt > 0: # Not the first attempt


You could avoid this if check if you sleep/wait at the end of the loop.

LeonarddeR · 2018-02-23T07:28:08Z

+		This is instead of using the standard WriteFile which may freeze with some USB HID implementations.
+		@ param data: the report string (starting with the report ID).
+		"""
+		buf=ctypes.create_string_buffer(self._writeSize)


In setFeature, the buffer is as long as the report. I guess using writeSize just works, but I think using the length of the data instead deserves a try, since in many cases, especially with the BI 14, it involves less bytes to send.

LeonarddeR · 2018-02-23T07:30:44Z


-++ HumanWare Brailliant BI/B Series ++[HumanWareBrailliant]
-The Brailliant BI and B series of displays from [HumanWare http://www.humanware.com/], including the BI 32, BI 40 and B 80, are supported when connected via USB or bluetooth.
++ HumanWare Brailliant BI/B Series, and BrailleNote Touch  ++[HumanWareBrailliant]


See my comment about the description in the driver file

LeonarddeR · 2018-02-23T07:31:30Z

-The Brailliant BI and B series of displays from [HumanWare http://www.humanware.com/], including the BI 32, BI 40 and B 80, are supported when connected via USB or bluetooth.
++ HumanWare Brailliant BI/B Series, and BrailleNote Touch  ++[HumanWareBrailliant]
+The Brailliant BI and B series of displays  from [HumanWare http://www.humanware.com/], including the BI 14, BI 32, BI 40 and B 80, are supported when connected via USB or bluetooth.
 If connecting via USB with the protocol set to HumanWare, you must first install the USB drivers provided by the manufacturer.


Last time I tried on Windows 10, the BI 40 was plug and play even when in serial mode.

LeonarddeR · 2018-02-23T07:34:04Z

-Following are the key assignments for this display with NVDA.
+The BrailleNote Touch is also supported, and does not require any drivers to be installed.
+
+Following are the key assignments for  the Brailliant BI/B, and BrailleNote touch displays with NVDA.


Note the comma here as well. Now I'm not a native English speaker, but I belief a comma before and is only prescribed in case three or more entities are involved (i.e. one, two, and three).

LeonarddeR · 2018-02-23T07:35:55Z

 | alt+tab key | space+dot2+dot3+dot4+dot5 (space+t) |
+%kc:endInclude
+
+++ Key assignments for Brailliant BI 32, BI 40 and B 80 +++ 


Just to make sure, doesn't the BI 14 have these C keys?

According to the documentation I have, BI 14 does not have command keys.

jcsteh · 2018-02-23T11:09:33Z

How about the Bluetooth HID support? It is there, but your pr description doesn't mention it.

We don't know if it works. It suffered from different problems as compared with the BrailleNote's USB HID support, so we can't assume the same fix will work for both. I'm not sure it's worth stripping out the code, though; we'll want it eventually, even if it needs some tweaking.

A major difference between how we use writefile and HidD_SetReport is that the latter is not asynchronous, it blocks the caller until True or False is returned. Would be good to know @jcsteh's opininion about this.

Actually, hwIo.IoBase.write is synchronous. See the call to GetOverlappedResult at the bottom, which sets bWait to true. Having multiple async writes just makes things complicated, so we don't do it.

In my review of hwIo.Hid.setReport, I mention the size of the buffer that's written to the device. May be you could elaborate on what you've tested to make writefile do its job correctly? Have you also played with the buffer size, (e.g. using a buffer that's as long as the data it contains?

If you mean shortening the buffer for WriteFile, that will break on Windows 7. Note this comment in Hid.__init__:

# On Windows 7, writing any less than caps.OutputReportByteLength is also an error.

I learned that the hard way when I was doing the initial work on that class. In any case, Mick noted in a discussion with me that with the BN Touch, the output report length is the same as the buffer you need to write anyway.

LeonarddeR · 2018-02-23T11:51:49Z

@jcsteh commented on 23 feb. 2018 12:09 CET:

A major difference between how we use writefile and HidD_SetReport is that the latter is not asynchronous, it blocks the caller until True or False is returned. Would be good to know @jcsteh's opininion about this.

Actually, hwIo.IoBase.write is synchronous. See the call to GetOverlappedResult at the bottom, which sets bWait to true. Having multiple async writes just makes things complicated, so we don't do it.

Oow wow, my bad indeed, I assumed that both reading and writing was async for IoBase. In that case, you can safely ignore my comment.

michaelDCurran · 2018-02-26T05:10:25Z

I believe I have addressed your review comments @LeonarddeR .

LeonarddeR

Thanks for addressing these, looks ok now, just only one tiny thing.

LeonarddeR · 2018-02-26T11:57:21Z

+			log.debug("Set output report: %r" % report)
+		if not ctypes.windll.hid.HidD_SetOutputReport(self._writeFile,buf,length):
+			if _isDebug():
+				log.debug("Set feature failed: %s" % ctypes.WinError())


Feature>report

michaelDCurran · 2018-02-26T16:59:04Z

Humanware is currently testing this. They are also testing a small change which makes this driver use SetOutputReport for all HID devices, as they had originally reported not seeing any braille cells come up for bluetooth Hid. For bluetooth it is possible that WriteFile simply fails rather than blocking. However, I'll make the change once I hear back from them.

…HID devices supported by this driver. Not only does WriteFile block for ever with USB HID, but WriteFile simply does nothing at all for Bluetooth HID.

…th space as not all displays have command keys.

michaelDCurran · 2018-02-26T21:49:48Z

@LeonarddeR Humanware confirmed that using HidD_SetOutputReport for all of their braille displays when using HID works better, therefore I have made this change. Also I added some extra key assignments requested by Humanware.

jcsteh · 2018-02-26T23:34:45Z

 			for attempt in xrange(INIT_ATTEMPTS):
 				if attempt > 0: # Not the first attempt
-					time.sleep(INIT_RETRY_DELAY) # Delay before next attempt.
+					self._dev.waitForRead(INIT_RETRY_DELAY) # Delay before next attempt.


@LeonarddeR, why did you want this to be a waitForRead instead of sleep? Yes, it will block the IO thread, but the IO thread shouldn't be trying to do anything else while it's initialising this display anyway. Also, there shouldn't be any data to read here; we're delaying precisely because the display isn't ready yet.

@LeonarddeR, why did you want this to be a waitForRead instead of sleep? Yes, it will block the IO thread, but the IO thread shouldn't be trying to do anything else while it's initialising this display anyway.

In BDDetect, displays are initialized on the IO thread, waitFoRead will be changed as part of bdDetect to allow reading while waiting.

Also, there shouldn't be any data to read here; we're delaying precisely because the display isn't ready yet.

Are you sure that the display simply is not ready and does not respond at all, or is it possible that it's just reacting slowly in the case of bluetooth (i.e. it takes longer than 0.2 seconds to respond and therefore passe the waitForRead at the end of the serial initialisation block?)? In the first case, waitForRead is indeed just an equivalent of sleep. In the latter case, you are wasting time with a sleep. But indeed, for HID, waitForRead doesn't make a difference.

… If it is truly needed we can add it back in in the Autio braille display detection pr.

michaelDCurran · 2018-02-28T04:59:17Z

I have changed the disputed waitForRead back to a time.sleep for now so I can incubate. If we truly need it to be a waitForRead, then this can be changed as part of auto braille display detection. But by itself, this pr works well.

LeonarddeR · 2018-02-28T05:00:48Z

Sounds Ok to me, go ahead :) @bramd @dkager: As you are both Brailliant users I know, your testing would be appreciated as soon as this arrives in next.

jcsteh and others added 8 commits June 27, 2017 15:19

Support for HumanWare Brailliant BI/B braille displays connected via …

19697a1

…Bluetooth when the protocol is set to OpenBraille.

brailliantB: Try a delay after connect, multiple init attempts and de…

b4b3ba8

…lays between attempts as recommended by HumanWare, as init can be quite flakey, particularly via Bluetooth.

Add support for the Brailliant BI 14 and BrailleNote Touch.

12d3d35

brailliantB: Change some key names and add comments about devices to …

aece93e

…key table.

Merge branch 'master' into brailliantBtHid

d051bbc

hwIo.Hid: add a setOutputReport method which allows writing data with…

aeeb6fd

… HidD_SetOutputReport as an alternative to WriteFile which may freeze on certain USB implementations.

Fix support for newer BrailleNote Touch devices via HID, by writing d…

6899df0

…isplay cells with HidD_SetOutputReport rather than WriteFile which can block forever on these devices.

Mention BrailleNote Touch in the UserGuide under Brailliant BI/B Seri…

9aae40b

…es, and BrailleNote Touch

michaelDCurran requested a review from LeonarddeR February 22, 2018 21:32

michaelDCurran added 2 commits February 23, 2018 07:47

Mention Brailliant BI 14 in userGuide

7b530e8

UserGuide: update key assignments for Brailliant BI/B and BrailleNote…

7611496

… Touch, splitting them into tables for all models, and extra specific.

michaelDCurran changed the title ~~Support the BrailleNote Touch via the BrailliantB brailledisplayDriver~~ Support the BrailleNote Touch and Brailliant BI 14 via the BrailliantB brailledisplayDriver Feb 22, 2018

LeonarddeR reviewed Feb 23, 2018

View reviewed changes

LeonarddeR suggested changes Feb 23, 2018

View reviewed changes

michaelDCurran added 3 commits February 26, 2018 15:02

hwIo.Hid.setOutputReport: add debugging.

ed97ab8

Change description of BrailliantB driver.

8a0db52

change time.sleep to waitForRead to not block IO thread.

0c4d1a0

LeonarddeR approved these changes Feb 26, 2018

View reviewed changes

Fix debug string.

6016dda

michaelDCurran added 2 commits February 27, 2018 07:23

BrailliantB braille display driver: use HidD_SetOutputReport for all …

8c0ec51

…HID devices supported by this driver. Not only does WriteFile block for ever with USB HID, but WriteFile simply does nothing at all for Bluetooth HID.

BrailliantB braille display driver: add some extra key assignments wi…

16b42cf

…th space as not all displays have command keys.

michaelDCurran requested a review from LeonarddeR February 26, 2018 21:50

jcsteh reviewed Feb 26, 2018

View reviewed changes

LeonarddeR approved these changes Feb 27, 2018

View reviewed changes

Change waitForRead before retrying init back to a time.sleep for now.…

e6f3fe3

… If it is truly needed we can add it back in in the Autio braille display detection pr.

michaelDCurran added a commit that referenced this pull request Feb 28, 2018

Incubates #8031

5d0aa89

nvaccessAuto added the incubating label Feb 28, 2018

nvaccessAuto assigned michaelDCurran Feb 28, 2018

LeonarddeR added the component/braille label Mar 2, 2018

michaelDCurran merged commit 15060d9 into master Mar 13, 2018

nvaccessAuto removed the incubating label Mar 13, 2018

nvaccessAuto added this to the 2018.2 milestone Mar 13, 2018

Uh oh!

Conversation

michaelDCurran commented Feb 22, 2018 • 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 performed:

Known issues with pull request:

Change log entry:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeonarddeR Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcsteh commented Feb 23, 2018

Uh oh!

LeonarddeR commented Feb 23, 2018

Uh oh!

michaelDCurran commented Feb 26, 2018

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelDCurran commented Feb 26, 2018

Uh oh!

michaelDCurran commented Feb 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelDCurran commented Feb 28, 2018

Uh oh!

LeonarddeR commented Feb 28, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

michaelDCurran commented Feb 22, 2018 •

edited

Loading

LeonarddeR Feb 23, 2018 •

edited

Loading