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 Handy Tech braille displays #7590

Merged
merged 95 commits into from Nov 2, 2017

Conversation

bramd
Copy link
Contributor

@bramd bramd commented Sep 11, 2017

Link to issue number:

Closes #7458.
fixes #5806.
closes #5418.

Summary of the issue:

A rewrite of the driver for Handy Tech braille displays. This is a native implementation which doesn't depend on the Handy Tech COM server.

Description of how this pull request fixes the issue:

Extensive discussion can be found in #7458. This is a first implementation of a native driver, which supports most Handy Tech displays. Notable changes for users include:

  • Some very old displays have been dropped, but can be used via Handy Techs COM server and NVDA add-on
  • Braille input is now available
  • The Basic Braille displays will have working key bindings by default
  • The Actilino display is supported
  • Threadsafe, native driver which can benefit from things like braille display autodetection in the future

Testing performed:

  • I did extensive testing with a Modular Evolution 88 braille display
  • @LeonarddeR tested extensively with an Active Braille and did some testing with a Basic Braille 40 and a Basic Braille 80
  • I did some testing with a Basic Braille 40 on USB and Bluetooth
  • @zstanecic tested an earlier version with a Braille Wave

Known issues with pull request:

  • Support for some very old displays has been dropped
  • This will need more work in the future to support advanced features like ATC, setting dot firmness etc

Change log entry:

New features

  • Support for the Actilino braille display from Handy Tech has been added
  • Braille input for Handy Tech braille displays is now supported

Changes

  • The Braillino, Bookworm and Modular (with old firmware) braille displays from Handy Tech are no longer supported out of the box. Install the Handy Tech Universal Driver and NVDA add-on to use these displays

bramd and others added 30 commits July 28, 2017 17:29
…Modular Evolution 88 and logs received packets and key presses
… specific packets such as firmware version lookup
… add a joystick to a new active braille at a later date. Also, fixed numcells, which should be 16
…ject to rename get_keys to a keys property, some whitspace fixes, implemented old protocol for writing braille cells, construct name dynamically for models where this makes sense.
@FelixGruetzmacher
Copy link
Contributor

Completed code inspection and all looks good except one potential issue: When a device id packet, the one consisting of 0xfe followed by the model id, is received from a hid to serial converter, the code currently does not check if there are more bytes beyond those two, so in theory, if that packet got lumped together with another one, that last one might get lost. Is this a ghost I'm seeing or actually relevant?

@LeonarddeR
Copy link
Collaborator

@FelixGruetzmacher commented on 16 okt. 2017 15:41 CEST:

Completed code inspection and all looks good except one potential issue: When a device id packet, the one consisting of 0xfe followed by the model id, is received from a hid to serial converter, the code currently does not check if there are more bytes beyond those two, so in theory, if that packet got lumped together with another one, that last one might get lost.

Hmm, very true indeed.

Is this a ghost I'm seeing or actually relevant?

I think this should be considered. Is it true that non-extended packages are always two bytes in size?

@LeonarddeR
Copy link
Collaborator

@michaelDCurran: Could you please review the last few commits?

@FelixGruetzmacher
Copy link
Contributor

Yes, I believe this to be the case in all scenarios we are considering here. Also, just looked at your change and I believe this addresses it. Thank you.

michaelDCurran added a commit that referenced this pull request Oct 16, 2017
@LeonarddeR
Copy link
Collaborator

@bramd and @FelixGruetzmacher, I wonder whether we should put a timer or a safety guard on the _awaitingAck property. To give an example:

  1. Connect a Modular Evolution 88
  2. Disable the device with the right side switch
  3. Change focus, braille is sent to the device
  4. awaitingAck is stuck in the True position. Setting it back to False raises a serial error and NVDA falls back to no braille

Note that this is currently not very relevant, as it is an edge case and the connection is lost when disabling and enabling the device anyway. However, when when #1271 gets into view, this is certainly an issue, as braille auto detection won't be triggered and thus the connection won't be reinstated.

We really should come up with a generic framework for protocols where we rely on ACKs

@FelixGruetzmacher
Copy link
Contributor

FelixGruetzmacher commented Oct 17, 2017 via email

@LeonarddeR
Copy link
Collaborator

@FelixGruetzmacher commented on 17 okt. 2017 09:14 CEST:

Maybe we should stop waiting for an ack after, say, 0.2 s, and increase a
missing_ack counter. If an ack is received at some point, that counter
should drop back to zero. If it reaches 3, meaning 3 consecutive missing
acks, that amounts to a connection loss. That stops the edge case from
causing trouble, but still honors the reason why an ack was put in place at
all, namely, detecting fatal errors.

My problem with a timer is that it might, or might not, conflict with thread safety. I'd rather only work with a counter or with waitForRead. However, calling wiatForRead from the background thread simply has no effect, as it doesn't allow reading to take place while waiting. :P This is an issue that will be solved in #1271 as well.

If you guys agree, I don't think this issue should block this driver from landing into a release. I've been working with this driver for weeks now and I haven't seen any missed ACKs except for the edge case I mentioned.

@FelixGruetzmacher
Copy link
Contributor

FelixGruetzmacher commented Oct 17, 2017 via email

@LeonarddeR
Copy link
Collaborator

@michaelDCurran: I did some small typo fixes to the user guide yesterday and added some gestures that weren't yet in there.

I think the user guide is clear about the old devices (Modular with old firmware and Braillino) not being supported). But if you consider this to be to early for 2017.4, I can understand this. However, note that we should also consider that landing too much braille changes in one release increases the chance of unresolved bugs.

@LeonarddeR
Copy link
Collaborator

Actually, I think a good intermediate solution would be merging this pr while keeping the com server for now. This way, people only have to install the add-on provided by handy tech, and this is clearly documented.

@zstanecic
Copy link
Contributor

zstanecic commented Nov 1, 2017 via email

@michaelDCurran
Copy link
Member

michaelDCurran commented Nov 1, 2017 via email

@zstanecic
Copy link
Contributor

zstanecic commented Nov 1, 2017 via email

@michaelDCurran
Copy link
Member

michaelDCurran commented Nov 2, 2017 via email

@LeonarddeR
Copy link
Collaborator

There is a commit on the way where I reverted the removal of the com server. Note that I still removed one line from the readme, stating that you need the handy tech universal driver when running from source. This is no longer the case. You still need the handy tech universal driver installed when running from source with the Handy Tech add-on, but that's no longer our responsibility.

@LeonarddeR
Copy link
Collaborator

LeonarddeR commented Nov 2, 2017

Ah, and there's another file, comInterfaces_sconscript. The handy tech com server python file is no longer delivered with NVDA, but built on demand again. This has actually always been the case before NVDA 2016.4 or so, so I see no problems with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants