Skip to content

Try new HID protocol after, not before all other driver matches#13150

Merged
feerrenrut merged 4 commits into
nvaccess:betafrom
LeonarddeR:degradeHid
Dec 13, 2021
Merged

Try new HID protocol after, not before all other driver matches#13150
feerrenrut merged 4 commits into
nvaccess:betafrom
LeonarddeR:degradeHid

Conversation

@LeonarddeR

@LeonarddeR LeonarddeR commented Dec 9, 2021 •
edited by feerrenrut
Loading

Copy link
Copy Markdown
Collaborator

Link to issue number:

Closes #13153

Summary of the issue:

Auto detection is broken for Humanware Brailliant and APH Mantis q40 when working over Bluetooth. It looks like they implement an incomplete version of the HID protocol.

Description of how this pull request fixes the issue:

to be honest, my belief has always been that the new HID protocol should not overrule a custom protocol offered by a vendor. This could lead to several side effects, including a vendor updating a firmware with support for the new protocol, causing different behavior in NVDA for example.

Testing strategy:

Tested with the try build that detection of the Mantis Q40 over Bluetooth is instant and stable again.

Known issues with pull request:

  1. It should be considered to create a point release for this if many people are affected. Therefore I'm filing this against RC for now.
  2. It should be investigated whether what I report in Unstable auto detect for Humanware Brailliant and APH Mantis Q40 braille displays #13153 about the HID driver connecting, then disappearing from the list of available displays, is a bug in the display or in the HID driver.

Change log entries:

Changes
- The new HID Braille protocol is no longer preferred when another braille display driver can be used.
Bug fixes
- Fixed unstable braille display auto detection for Humanware Brailliant and APH Mantis Q40 braille displays when using Bluetooth.

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

@LeonarddeR LeonarddeR requested a review from a team as a code owner December 9, 2021 19:03
@LeonarddeR LeonarddeR requested review from michaelDCurran and removed request for a team December 9, 2021 19:03
@michaelDCurran

michaelDCurran commented Dec 9, 2021

Copy link
Copy Markdown
Member

This is probably the right thing to do, but very annoying that Humanware ended up officially releasing firmware with initial or half-finished Braille HID support. We had asked them on many occasions about their plans for this, and after we had supplied them with several bug reports for test firmware which I'm guessing they must not have addressed.
I'm going on leave for 3 weeks, so I'll let @feerrenrut decide as to whether this should warrant a point release.
To do the right thing by our users, it probably should.

michaelDCurran
michaelDCurran previously approved these changes Dec 9, 2021
View reviewed changes
@michaelDCurran

michaelDCurran commented Dec 9, 2021

Copy link
Copy Markdown
Member

I would however want this tested with an Orbit Reader 40, in both Braille HID and Orbit HID modes, as Orbit Reader so far is the only real-world official display implementing Braille HID.

feerrenrut
feerrenrut reviewed Dec 10, 2021
View reviewed changes
Comment thread source/bdDetect.py Outdated
feerrenrut
feerrenrut reviewed Dec 10, 2021
View reviewed changes
Comment thread source/bdDetect.py Outdated
@feerrenrut

feerrenrut commented Dec 10, 2021

Copy link
Copy Markdown
Contributor

This will mean that braille device manufacturers will need to change device IDs if they wish to support / test HID with NVDA.
I think this will be fine for 2021.3.1. We may add an advanced toggle for this in 2022.1. Device makers should be changing the ID anyway when they change the expected communications approach.

@feerrenrut

feerrenrut commented Dec 10, 2021

Copy link
Copy Markdown
Contributor

If we are going to have a changes file, then we'll want to give translators a chance, this will need to target beta (where translations happen).

@feerrenrut feerrenrut changed the base branch from rc to beta December 10, 2021 07:21
@feerrenrut

feerrenrut commented Dec 13, 2021

Copy link
Copy Markdown
Contributor

It would be good to have some instructions for Braille manufacturers about device IDs. I'd like a strong recommendation that device IDs should be changed when moving from legacy custom driver approach to a HID Braille approach. I'm ok with this going into alpha since it may take some time to get the wording / location of the advice right.

@feerrenrut feerrenrut merged commit 391ca63 into nvaccess:beta Dec 13, 2021
@nvaccessAuto nvaccessAuto added this to the 2022.1 milestone Dec 13, 2021
@feerrenrut feerrenrut modified the milestones: 2022.1, 2021.3.1 Dec 13, 2021
feerrenrut pushed a commit that referenced this pull request Dec 14, 2021
@LeonarddeR @feerrenrut
Prefer custom braille drivers to HID braille. (PR #13150)
3f2856c 
Try to use new HID protocol after, not before all other driver matches

Closes #13153
Summary:
Auto detection was broken for Humanware Brailliant and APH Mantis q40 when working over Bluetooth. It looks like they implement an incomplete version of the HID protocol.
@LeonarddeR LeonarddeR deleted the degradeHid branch August 23, 2025 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelDCurran michaelDCurran michaelDCurran left review comments

+1 more reviewer

@feerrenrut feerrenrut feerrenrut approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2021.3.1

Development

Successfully merging this pull request may close these issues.

4 participants

@LeonarddeR @michaelDCurran @feerrenrut @nvaccessAuto