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

Several improvements for the handy tech driver #8016

Merged
merged 22 commits into from Apr 20, 2018

Conversation

Projects
None yet
5 participants
@leonardder
Collaborator

leonardder commented Feb 20, 2018

Link to issue number:

None

Description of this pull request

This pr improves or extends the handy tech driver in the following way:

  1. Support for the old Handy Tech protocol has been re-added. This means, support for older Modular and Braillino devices has been restored. The EasyBraille now also makes use of the older protocol, as the new protocol seemed to cause some instabilities for older firmware models here and there.
  2. Support for time synchronisation. Similar to the ALVA displays, time of supported Handy Tech displays (the active line in particular) will be synchronised if out of sync more than five seconds.
  3. Preparation for #7452. This is not user visible, but gives a first glance on what #7452 will be like from a implementation perspective. Most notably, atc is now a magic property on the driver. driver.atc=True enables it for supported displays, driver.atc=False disables it. For unsupported displays, only the internal state is toggled. I might change this a bit as part of #7452, as raising a NotImplementedError might be preferred instead.
  4. Fixes an issue where a lot of rapid braille input on displays with a HID serial converter would lead to display connection loss.
  5. Some changes to make the driver somewhat more Python 3 ready, explicitly making all strings such as constants byte strings.
  6. This bundles another attempt to kill the Handy Tech COM server. Since displays for all displays has been restored, I think this can be done as part of this pr.

Testing performed:

Although I've not tested a Braillino myself, I've used an Easy Braille with HID serial converter for around a day without any problems or connection loss. I belief that's a setup that comes quite close to the implementation of a Braillino, although the latter might be a device with an USB serial implementation. At least, no issues did occur with the old protocol regarding input and output.

Known issues with pull request:

None

Change log entry:

  • New features

    • Restored support for Handy Tech Braillino and Modular (with old firmware) displays. (#8016)
    • Date and time for supported Handy Tech devices (such as Active Braille and Active Star) will now automatically be synchronized by NVDA when out of sync more than five seconds. (#8016)
  • Bug fixes

    • Fixed connection stability issues for Handy Tech Easy Braille and Braille Wave displays. (#8016)
@zstanecic

This comment has been minimized.

Show comment
Hide comment
@zstanecic

zstanecic Feb 20, 2018

Contributor
Contributor

zstanecic commented Feb 20, 2018

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Feb 20, 2018

Collaborator
Collaborator

leonardder commented Feb 20, 2018

@zstanecic

This comment has been minimized.

Show comment
Hide comment
@zstanecic

zstanecic Feb 20, 2018

Contributor
Contributor

zstanecic commented Feb 20, 2018

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Feb 20, 2018

Collaborator

Sounds like a good rationale to me. Would you be able to test this? I've now updated the pr to make the BrailleWave use the old protocol as well. Would be great if you could report whether this fixes your issue.

Collaborator

leonardder commented Feb 20, 2018

Sounds like a good rationale to me. Would you be able to test this? I've now updated the pr to make the BrailleWave use the old protocol as well. Would be great if you could report whether this fixes your issue.

@zstanecic

This comment has been minimized.

Show comment
Hide comment
@zstanecic

zstanecic Feb 20, 2018

Contributor
Contributor

zstanecic commented Feb 20, 2018

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Feb 20, 2018

Collaborator

Here is a try build

Collaborator

leonardder commented Feb 20, 2018

Here is a try build

@zstanecic

This comment has been minimized.

Show comment
Hide comment
@zstanecic

zstanecic Feb 20, 2018

Contributor
Contributor

zstanecic commented Feb 20, 2018

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Feb 20, 2018

Collaborator

Except for the native HID devices, all devices still support the old protocol. I'm not going to use a particular protocol based on what firmware version is in use. The newest Braille Wave firmware will still work with the old protocol and probably doesn't have any benefits from the new protocol.

Collaborator

leonardder commented Feb 20, 2018

Except for the native HID devices, all devices still support the old protocol. I'm not going to use a particular protocol based on what firmware version is in use. The newest Braille Wave firmware will still work with the old protocol and probably doesn't have any benefits from the new protocol.

@leonardder leonardder added the Braille label Mar 2, 2018

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Mar 8, 2018

Contributor

What is the status on this pr? I don't believe any review has been asked for yet.

Contributor

michaelDCurran commented Mar 8, 2018

What is the status on this pr? I don't believe any review has been asked for yet.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Mar 8, 2018

Collaborator

Thanks for poking.

I'd really appreciate @FelixGruetzmacher's feedback on this pr first. Felix, would you be able to review the current state?

Collaborator

leonardder commented Mar 8, 2018

Thanks for poking.

I'd really appreciate @FelixGruetzmacher's feedback on this pr first. Felix, would you be able to review the current state?

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Mar 26, 2018

Collaborator

Looks like @FelixGruetzmacher hasn't yet been able to review. @bramd already did a quick review I believe.

Collaborator

leonardder commented Mar 26, 2018

Looks like @FelixGruetzmacher hasn't yet been able to review. @bramd already did a quick review I believe.

@leonardder leonardder requested review from feerrenrut and michaelDCurran and removed request for feerrenrut Mar 26, 2018

@bramd

This comment has been minimized.

Show comment
Hide comment
@bramd

bramd Mar 27, 2018

Contributor

@leonardder commented on Mar 26, 2018, 11:51 AM GMT+2:

Looks like @FelixGruetzmacher hasn't yet been able to review. @bramd already did a quick review I believe.

That's correct, I looked at it a while ago. I will gladly look at it again if you have doubts about some parts of the code.

Contributor

bramd commented Mar 27, 2018

@leonardder commented on Mar 26, 2018, 11:51 AM GMT+2:

Looks like @FelixGruetzmacher hasn't yet been able to review. @bramd already did a quick review I believe.

That's correct, I looked at it a while ago. I will gladly look at it again if you have doubts about some parts of the code.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Apr 5, 2018

Collaborator

Thanks for approving. I think it would be nice to incubate this ASAP in order to have it properly merged before braille display auto detection lands in next.

Collaborator

leonardder commented Apr 5, 2018

Thanks for approving. I think it would be nice to incubate this ASAP in order to have it properly merged before braille display auto detection lands in next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment