Filter for the correct usage page in the Brailliant driver #17522
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc @Adriani90 @bramd @dkager @jcsteh I would really appreciate your help with testing with both recent and previous generation Brailliants. |
See test results for failed build of commit 59f2c588bd |
|
This approach looks good, but I agree with you that it should be handled more generally outside of the drivers. |
|
@LeonarddeR My device is still at firmware 2.4 to see if I can do something about the broken driver and/or non-working keys in Standard HID. This looks reasonable to me and I agree that we should build this in bdDetect in the long term. When I downlgrade or have a working driver for 2.4 I'll test your changes. |
LeonarddeR
force-pushed
the
brailliantUsagePage
branch
from
73c4152 to
791e5d7
Compare
|
I don't own a Brailliant, so unfortunately I can't help. (Well, I do own one of the old ones, but that's basically a rebadged Baum device and doesn't use the Brailliant protocol.) I wrote the original drive using a device I loaned from HumanWare, but I haven't had that device for years. |
|
Thanks Leonard for the ping, I‘ll test next week when I am back home. |
bramd
approved these changes
Dec 15, 2024
SaschaCowley
approved these changes
Dec 16, 2024
5 tasks
SaschaCowley
added
the
conceptApproved
SaschaCowley
force-pushed
the
brailliantUsagePage
branch
from
9a35fb3 to
7f92915
Compare
WalkthroughThis pull request introduces several improvements to NVDA's device handling and version management. The changes focus on enhancing the Brailliant B HID driver's connection logic by adding a usage page check, improving HID device information retrieval with a caching mechanism, and updating the minor version number. The modifications aim to increase device connection reliability and provide more robust error handling when interacting with hardware devices. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
user_docs/en/changes.md (2)
7-8: Inconsistent sentence structure in bullet points.The bullet points should start with verbs for consistency. Consider rewording to:
- * When the Standard HID Braille Display driver is explicitly selected as the braille display driver, and the braille display list is opened, NVDA correctly identifies the HID driver as the selected driver instead of showing no driver selected. (#17522, @LeonarddeR) - * The Humanware Brailliant driver is now more reliable in selecting the right connection endpoint, resulting in better connection stability and less errors. (#17522, @LeonarddeR) + * Fixed issue where NVDA did not correctly identify the HID driver as selected when opening the braille display list with Standard HID Braille Display driver explicitly selected. (#17522, @LeonarddeR) + * Improved reliability of the Humanware Brailliant driver when selecting connection endpoints, resulting in better stability and fewer errors. (#17522, @LeonarddeR)
8-8: Grammar: "less errors" should be "fewer errors"."Less" is used for uncountable nouns while "fewer" is used for countable nouns like errors.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
source/brailleDisplayDrivers/brailliantB.py(2 hunks)source/buildVersion.py(1 hunks)source/hwPortUtils.py(4 hunks)source/winAPI/constants.py(1 hunks)user_docs/en/changes.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
source/winAPI/constants.py (2)
Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>
Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.
source/buildVersion.py (2)
Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>
Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.
source/brailleDisplayDrivers/brailliantB.py (2)
Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>
Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.
source/hwPortUtils.py (2)
Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>
Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.
user_docs/en/changes.md (3)
Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>
Pattern **/*.md: Focus on grammar, spelling, and punctuation. Also consider readability and clarity of contents. Ensure that changes follow the style of one sentence per line, suggest changes if this is not met.
Pattern user_docs/en/changes.md: Ensure each change log entry references an issue or pull request number. Change log entries can also include a reference to a GitHub author. Examples of valid change log entries: * Item with sub-items (#123, @username): * sub-item * bar (#342) * Item with. Multiple lines. (#143)
🔇 Additional comments (5)
source/winAPI/constants.py (1)
31-32: Addition of SHARING_VIOLATION error code is appropriate
The constant SHARING_VIOLATION correctly represents the error code 0x20 and includes an appropriate docstring.
source/buildVersion.py (1)
68-68: Version number updated appropriately
The version_minor variable has been updated from 1 to 2, reflecting the new minor version.
source/brailleDisplayDrivers/brailliantB.py (2)
38-38: Definition of HID_USAGE_PAGE is appropriate
The constant HID_USAGE_PAGE is correctly defined as 0x93, which will be used for filtering devices.
151-153:
Typo in variable name usasePage
There is a typo in the variable name usasePage; it should be usagePage.
Apply this diff to fix the typo:
- if (usasePage := portInfo.get("HIDUsagePage")) != HID_USAGE_PAGE:
+ if (usagePage := portInfo.get("HIDUsagePage")) != HID_USAGE_PAGE:Additionally, ensure consistency in the variable name throughout the code if it's used elsewhere.
Ensure compatibility with Python versions before 3.8
The use of the walrus operator (:=) requires Python 3.8 or later. If the project needs to maintain compatibility with earlier Python versions, consider refactoring to avoid using the walrus operator.
Suggested refactor:
- if (usagePage := portInfo.get("HIDUsagePage")) != HID_USAGE_PAGE:
+ usagePage = portInfo.get("HIDUsagePage")
+ if usagePage != HID_USAGE_PAGE:Likely invalid or redundant comment.
user_docs/en/changes.md (1)
1-8: Missing issue/PR references in changelog entries.
The second bullet point is missing an issue/PR reference number that should be included for traceability.
|
I'm not sure whether the usage page constraint brings other issues here. I'd really like it if someone like @dkager can test against an older display, may be someone with a Braille Note as well? |
|
@LeonarddeR testing on a Braillina BI40x with firmware 2.3, it doesn't work when choosing standard HID, but indeed the Standard HID is shown properly after applying changes and checking the current braille display in braille settings panel. It doesn't disappear from the list of braille device drivers anymore. However, nothing is shown oin the Braille display when standard HID is selected. is this expected? Just in case, here is my advanced logging: |
|
@Adriani90 This is expected from NVDA"s perspective, see #17526 (comment) |
|
Ah, I am not sure whether that runtime error at the end has anything to do with this issue. It seems when I choose standard HID and press ok, the braille display is shown as |
|
That would be weird. I'll see whether I can reproduce that, but it should have nothing to do with the changes in this pr. |
|
After a while, I restarted NVDA, and suddenly now the Standard HID driver seems to work somehow, at least my Braille display shows characters and I can read. At this stage I try to choose back the Humanware driver, and the Braille display is again empty, the humanware driver doesn't work anymore. |
|
I could catch following error: |
|
Not sure yet whether this was happening always before, I never tested switching back and forth between standard HID and the native driver, I shall try with the NVDA stable version as well. I think NVDA can properly close a braille display device only when choosing "no braille display", but not when changing back and forth between standard HID and the native driver. At least this is my conclulusion but this might be wrong. |
|
Now after restarting NVDA, looking at the selected braille display in the settings panel, it says |
|
And the keys of the braille display are not working but it shows braille characters. |
|
Now testing on the stable version of NVDA 2024.4.1, it seems the humanware driver is not working anymore. It is shown on the settings panel as selected, but in fact NVDA uses the standard HID driver. The error about the time limit for the semaphore being reached is logged again and again when choosing the humanware driver manually. I guess this is the issue that is there for a while and is not impcated by this PR. I need to reinstall NVDA stable and clean my user config to make the humanware driver work again and then I can perform further tests if needed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link to issue number:
Related to #17518
Summary of the issue:
The Brailliant driver doesn't filter for HID usage page properly, meaning that attempts are made to connect to device entries with other usage pages, such as HID braille and keyboard.
Also, when explicitly selecting the HID driver and then opening the display list again results in no display driver being selected at all and the HID driver being unavailable to select again.
Description of user facing changes
Quicker USB and Bluetooth connection.
Description of development approach
hwPortUtilsthat caches device info like usage page, manufacturer, vendor, etc. This is necessary because when the display is connected, NVDA is unable to open a second handle to the device to get this information. This means that for example, when a HID braille driver device is connected, NVDA is unable to detect the same HID device again because it can't fetch the usage page.Testing strategy:
Known issues with pull request:
Code Review Checklist:
Summary by CodeRabbit
Release Notes for NVDA 2024.4.2
New Features
Bug Fixes
Documentation