Handle BrailliantB devices that do not report their number of cells #17526
Conversation
LeonarddeR
left a comment
LeonarddeR
left a comment
There was a problem hiding this comment.
This works like a charm with a Brailliant Bi 20X. I confirmed that the cell count is ok. just one suggestion for improved logging.
|
I'm a bit confused with the expected behaviour on Firmware 2.4 for the Brailliant 20X. Via USB, Standard HID Braille works fine. But the Humanware Brailliant driver (although listed and loads) causes a blank display. However, via Bluetooth, it is the exact opposite. Brailliant B driver works fine, but Standard HID braille results in a blank display. I don't remember what it was before this firmware update. I thought it was the other way around. The Brailliant does not seem to allow the user to configure what protocol it offers on the display? Does it actually offer both at the same time? |
|
@michaelDCurran This is quite interesting. I recall it being the other way around as well.
This makes me think that while the device offers both protocols at the same time, first come, first served. Long story short, I think our observations are due to shortcomings in the device/firmware, not in NVDA per se. |
SaschaCowley
added
the
conceptApproved
…ze. This solves the problem where some devices and firmware versions do not report a cell count in their capabilities report. The behavior is unchanged for devices that report their cell count correctly.
Co-authored-by: Leonard de Ruijter <[email protected]>
SaschaCowley
force-pushed
the
brailliantCellCount
branch
from
2aa496d to
8904674
Compare
WalkthroughThe pull request introduces modifications to the Brailliant B braille display driver initialization process, focusing on improving the method for determining the number of display cells. The changes enhance error handling and logging when retrieving device capabilities. Additionally, the minor version number of NVDA has been incremented, and the changes documentation has been updated to reflect recent improvements in the software. Changes
Assessment against linked issues
Possibly related issues
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: 1
🧹 Nitpick comments (4)
source/brailleDisplayDrivers/brailliantB.py (2)
195-200: Consider enhancing error loggingWhile the fallback to _writeSize is a good approach, the error message could be more informative about why this method was attempted and what the actual values were.
- log.debugWarning("Could not get _writeSize from HID device") + log.debugWarning( + "Could not get _writeSize from HID device. " + "This is expected for some connection types." + )
193-213: Consider adding unit testsWhile the implementation looks solid, it would be beneficial to add unit tests covering:
- Device reporting zero cells
- Device with missing _writeSize
- Device with both fallback mechanisms failing
Would you like me to help create these unit tests?
user_docs/en/changes.md (2)
7-7: Inconsistent capitalization in product name"Humanware" should be "HumanWare" to match the company's official branding.
-* Humanware Brailliant BI/B series braille displays can now be used when connected via bluetooth. +* HumanWare Brailliant BI/B series braille displays can now be used when connected via Bluetooth.
7-7: Bluetooth should be capitalizedThe word "bluetooth" should be capitalized as "Bluetooth" since it is a proper noun.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
source/brailleDisplayDrivers/brailliantB.py(1 hunks)source/buildVersion.py(1 hunks)user_docs/en/changes.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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.
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 (4)
source/buildVersion.py (1)
68-68: LGTM: Version bump is appropriate
The minor version increment is consistent with the addition of new functionality for handling BrailliantB devices that don't report their cell count.
source/brailleDisplayDrivers/brailliantB.py (2)
193-194: Good practice: Preventing premature display writes
Moving the self.numCells assignment to the end of the method prevents potential issues with display writes before the device capabilities are fully determined.
202-210: LGTM: Robust device capabilities handling
The implementation properly handles cases where the device doesn't report its cell count, with appropriate fallback and logging.
Let's verify the compatibility with different device models:
✅ Verification successful
Let me gather more context about the HIMS driver since it also uses HR_CAPS.
Verified: Cell count handling is consistent with HIMS driver implementation
The implementation in brailliantB.py correctly follows the established pattern for HID device cell count detection, with two key observations:
- Both drivers use HR_CAPS feature report to get cell count information
- The offset difference (24 vs 9) is intentional as these are different device families
- Both implement appropriate fallback and error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential device-specific handling in the codebase
# Look for other places where HR_CAPS is used
rg "HR_CAPS" --type py
# Check for any device-specific cell count handling
ast-grep --pattern 'numCells = $_'
Length of output: 2726
Script:
#!/bin/bash
# Check how HIMS driver handles HR_CAPS for cell count
rg -B 5 -A 5 "HR_CAPS" source/brailleDisplayDrivers/hims.py
Length of output: 522
user_docs/en/changes.md (1)
Line range hint 1-7000: Overall changelog format looks good
The changelog follows a consistent format with:
- Clear version headers
- Categorized sections (New Features, Changes, Bug Fixes, etc.)
- Bullet points for individual changes
- Issue/PR references where applicable
5 participants
Link to issue number:
Partially fixes #17518
Summary of the issue:
Some HumanWare devices do not report the number of cells in their HID capabilities report. This is the case for Brailliant BI 40X since firmware 2.4.
Description of user facing changes
The Brailliant BI 40X and similar display with firmware version 2.4 is working correctly again.
Description of development approach
This PR fixes the issue by using the HID output report size to calculate the number of cells. If the device reports a cell count, this is still being used.
Testing strategy:
Tested with a Brailliant BI 40X over Bluetooth.
Known issues with pull request:
Since firmware 2.4 this device is recognized by the Standard HID driver over Bluetooth. However, the keys do not work using that driver. This PR does not fix the Standard HID driver.
Code Review Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Version Update