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

Projects
None yet
7 participants
@bramd
Copy link
Contributor

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 some commits Jul 28, 2017

Basic native Handy Tech driver, work in progress. Outputs braille to …
…Modular Evolution 88 and logs received packets and key presses
Another try to fix sending of HID packaets, not yet accounted for HID…
… specific packets such as firmware version lookup
Moved the Actilino joystick keys into its own mixin, since ht plan to…
… add a joystick to a new active braille at a later date. Also, fixed numcells, which should be 16
Delegate sending braille cells to the Model class, use AutoPropertyOb…
…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

This comment has been minimized.

Copy link
Contributor

FelixGruetzmacher commented Oct 16, 2017

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

This comment has been minimized.

Copy link
Collaborator

leonardder commented Oct 16, 2017

@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

This comment has been minimized.

Copy link
Collaborator

leonardder commented Oct 16, 2017

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

@FelixGruetzmacher

This comment has been minimized.

Copy link
Contributor

FelixGruetzmacher commented Oct 16, 2017

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

This comment has been minimized.

Copy link
Collaborator

leonardder commented Oct 17, 2017

@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

This comment has been minimized.

Copy link
Contributor

FelixGruetzmacher commented Oct 17, 2017

@leonardder

This comment has been minimized.

Copy link
Collaborator

leonardder commented Oct 17, 2017

@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

This comment has been minimized.

Copy link
Contributor

FelixGruetzmacher commented Oct 17, 2017

@leonardder

This comment has been minimized.

Copy link
Collaborator

leonardder commented Nov 1, 2017

@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

This comment has been minimized.

Copy link
Collaborator

leonardder commented Nov 1, 2017

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

This comment has been minimized.

Copy link
Contributor

zstanecic commented Nov 1, 2017

@michaelDCurran

This comment has been minimized.

Copy link
Contributor

michaelDCurran commented Nov 1, 2017

@zstanecic

This comment has been minimized.

Copy link
Contributor

zstanecic commented Nov 1, 2017

@michaelDCurran

This comment has been minimized.

Copy link
Contributor

michaelDCurran commented Nov 2, 2017

@leonardder

This comment has been minimized.

Copy link
Collaborator

leonardder commented Nov 2, 2017

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

This comment has been minimized.

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
You can’t perform that action at this time.