Add ability to add more constraints to bdDetect USB device registration #17537
Conversation
SaschaCowley
added
the
conceptApproved
|
You can also remove VID_AND_PID definition from albatross/constants.py because it is not used elsewhere. |
See test results for failed build of commit 2574273c55 |
|
@coderabbitai review |
WalkthroughThe pull request introduces a comprehensive redesign of the device detection system in NVDA, focusing on the Changes
Sequence DiagramsequenceDiagram
participant Driver as Braille Display Driver
participant BdDetect as Device Detection System
participant Device as USB/Bluetooth Device
Driver->>BdDetect: Register device with ProtocolType
BdDetect->>Device: Match device using protocol criteria
Device-->>BdDetect: Return device match details
BdDetect->>Driver: Provide matched device information
Assessment against linked 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: 3
🧹 Nitpick comments (5)
source/brailleDisplayDrivers/albatross/driver.py (1)
87-91: Refine match function usage
ThematchFunchere ensures detection is confined to devices matching"Albatross Braille Display". This is a suitable solution if exact matching is desired. Consider whether partial or case-insensitive matching might be necessary in certain edge cases. Otherwise, this looks solid.source/brailleDisplayDrivers/brailliantB.py (1)
122-147: Improved Bluetooth matching logic.The multi-part checks for manufacturer, product, and usage page yield precise device detection. Ensure that any expansions of the product list remain consistent and tested, so as not to mistakenly exclude newly added models.
source/brailleDisplayDrivers/seikantk.py (1)
137-138: Consider handling other protocol types explicitly.
If future protocols are supported, you might introduce anelsebranch or additional checks to prevent silent misconfigurations.source/hwPortUtils.py (1)
524-534: Use a narrowed exception or conditional check for handle creation.You are handling device sharing violations by returning cached info. This fallback is useful and potentially prevents runtime errors. However, consider narrowing the exception to handle only known handle creation issues and adding logging for other unexpected failures to aid debugging.
user_docs/en/changes.md (1)
130-134: API Enhancement: Improved braille display registrationThe changes to
bdDetect.DriverRegistrarimprove the handling of USB device detection by:
- Adding
addUsbDevicefor single device registration- Adding
matchFuncparameter to enable more granular device constraints- Supporting cases where VID/PID combinations are shared across devices
This is a significant improvement for braille display drivers. Consider reviewing the documentation and examples in the albatross and brailliantB drivers for implementation details.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
source/bdDetect.py(18 hunks)source/braille.py(1 hunks)source/brailleDisplayDrivers/albatross/driver.py(2 hunks)source/brailleDisplayDrivers/alva.py(2 hunks)source/brailleDisplayDrivers/baum.py(3 hunks)source/brailleDisplayDrivers/brailleNote.py(1 hunks)source/brailleDisplayDrivers/brailliantB.py(3 hunks)source/brailleDisplayDrivers/eurobraille/driver.py(6 hunks)source/brailleDisplayDrivers/eurobraille/gestures.py(1 hunks)source/brailleDisplayDrivers/freedomScientific.py(2 hunks)source/brailleDisplayDrivers/handyTech.py(4 hunks)source/brailleDisplayDrivers/hidBrailleStandard.py(1 hunks)source/brailleDisplayDrivers/hims.py(2 hunks)source/brailleDisplayDrivers/nattiqbraille.py(1 hunks)source/brailleDisplayDrivers/seikantk.py(3 hunks)source/brailleDisplayDrivers/superBrl.py(1 hunks)source/hwPortUtils.py(4 hunks)source/winAPI/constants.py(1 hunks)tests/unit/test_bdDetect.py(1 hunks)user_docs/en/changes.md(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/unit/test_bdDetect.py
🧰 Additional context used
📓 Path-based instructions (19)
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/brailleDisplayDrivers/nattiqbraille.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/superBrl.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/brailleNote.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/eurobraille/gestures.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/baum.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/hidBrailleStandard.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/eurobraille/driver.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/freedomScientific.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.
source/brailleDisplayDrivers/hims.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/brailleDisplayDrivers/alva.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/seikantk.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/albatross/driver.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/handyTech.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/braille.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/bdDetect.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 (56)
source/brailleDisplayDrivers/albatross/driver.py (1)
15-15: Consistent import usage
Good call replacing the old type imports with ProtocolType from bdDetect. This aligns well with the new enums introduced for device protocols.
source/bdDetect.py (16)
15-16: New imports for dataclass and partial
Importing dataclass, field, and partial is appropriate given the usage of dataclasses and partial application. This adds clarity to object construction and custom match functions.
Also applies to: 20-20
56-63: Introduction of ProtocolType
Defining a dedicated ProtocolType enum helps separate concerns related to different device protocols (HID, SERIAL, CUSTOM). This is clearer and more maintainable than the previous approach.
65-70: Addition of CommunicationType
Splitting out device communication channels (e.g., USB, BLUETOOTH) into CommunicationType further clarifies how devices interact. This separation reduces confusion between protocol and communication layers.
72-95: Use of _DeviceTypeMeta for backward compatibility
Retaining a metaclass to provide backwards compatibility for the now-deprecated DeviceType is a good strategy. It avoids breaking existing code while encouraging adoption of ProtocolType and CommunicationType. Logging warnings also help users migrate.
107-110: Deprecation map for constants
Mapping old constants to new enums and emitting warnings preserve backward compatibility. This approach helps developers transition away from older keys without abruptly breaking their code.
Also applies to: 115-117
124-125: Extended DeviceMatch
The additions to DeviceMatch are straightforward and self-explanatory, holding protocol type and related metadata. This is clear and fosters uniform handling of device attributes.
137-166: Introducing _UsbDeviceRegistryEntry
This class encapsulates information about a given USB device and includes a matches method to apply additional checks. It enhances both clarity and modularity. The optional matchFunc attribute is particularly useful for refining detection logic.
168-169: New DriverDictT and _driverDevices
Using a defaultdict keyed on CommunicationType with entries of _UsbDeviceRegistryEntry (or a callable for Bluetooth) is an elegant solution for storing registry info. OrderedDict helps preserve insertion order, beneficial for fallback logic.
Line range hint 201-217: Enhanced iteration over USB devices
Generating DeviceMatch objects for custom, HID, and COM ports is more robust alongside the standard HID detection. Enumerating possible device matches this way prevents missing any potential braille device.
226-232: Fallback driver usage
Storing fallback drivers in a separate list is an excellent approach to handle devices that do not match primary drivers. This ensures maximum compatibility without complicating the main detection flow.
242-245: Final HID check
Placing the braille HID protocol driver last promotes vendor-specific drivers over the generic fallback. This design choice respects a user’s likely preference for specialized support.
255-257: Abstracting HID usage page checks
Separating _isHIDUsagePageMatch allows flexible usage page matching for different specialized protocols in the future.
260-261: Specific HID braille check
Narrowing the usage page to HID_USAGE_PAGE_BRAILLE ensures only appropriate HID devices are included. This helps reduce false positives.
263-264: HIDUsagePageMatchFuncFactory helper
Providing a factory function for partial usage page matching is a clean, extensible approach to generating match functions on the fly.
Line range hint 278-302: Handling Bluetooth device matching
Allowing custom logic for Bluetooth devices ensures broad coverage, including vendor-specific detection. Caching results optimizes repeated queries. The approach is consistent with the new fallback logic for USB.
Line range hint 548-572: getConnectedUsbDevicesForDriver fallback flow
This logic properly attempts standard HID after enumerating custom drivers, then yields fallback matches last. The arrangement is comprehensive and prevents overshadowing specialized drivers.
source/winAPI/constants.py (1)
31-32: New SHARING_VIOLATION system error code
Defining SHARING_VIOLATION = 0x20 with a descriptive docstring ensures smoother error handling for concurrency or file-sharing conflicts.
source/brailleDisplayDrivers/superBrl.py (1)
37-37: Switched to ProtocolType.SERIAL
This update aligns with the new protocol classification, enhancing clarity for driver registration.
source/brailleDisplayDrivers/nattiqbraille.py (1)
42-42: Use of ProtocolType.SERIAL for USB
Registration now distinctly identifies the device using the improved protocol enum. This is consistent with broader changes in bdDetect and other drivers.
source/brailleDisplayDrivers/eurobraille/gestures.py (1)
165-165: Check for potential type conversion edge cases.
display.ProtocolType may be an enum or another non-string type. Calling .lower() and splitting could raise an exception if it's not truly a string or if it's None. Also, if its string representation is empty, split(" ")[0] could raise an IndexError. Consider validating the type or using a safer approach.
source/brailleDisplayDrivers/brailleNote.py (1)
133-133: Migration to ProtocolType.SERIAL looks consistent.
Switching from DeviceType.SERIAL to ProtocolType.SERIAL aligns with the refactoring throughout the codebase. Tests covering device detection should validate that this still correctly registers the targeted hardware.
source/brailleDisplayDrivers/brailliantB.py (5)
38-38: New constant definition for HID usage page.
Defining HID_USAGE_PAGE = 0x93 is fine. Ensure that no other usage pages need coverage, and confirm that this value is correct for all relevant Humanware devices.
93-101: Refined HID registration with match function.
Using bdDetect.ProtocolType.HID along with HIDUsagePageMatchFuncFactory(HID_USAGE_PAGE) provides a more precise filter for the new models. This is a good practice to prevent accidental detection of non-compatible devices.
102-114: Consistent switch to ProtocolType.SERIAL.
This ensures that purely serial-based devices are registered properly. Validate that the newly included device IDs are correct for the intended hardware.
161-161: HID detection flag.
Assigning self.isHid based on portType == bdDetect.ProtocolType.HID looks correct. Confirm that no device is misclassified in practice, especially if new protocol types are added later.
165-167: Validate presence of HIDUsagePage in portInfo.
The usage of assignment expressions is concise, but ensure that any older Python versions (if applicable) support them. Also confirm that ignoring devices lacking a HIDUsagePage value is intentional.
source/brailleDisplayDrivers/baum.py (3)
87-87: Switch from DeviceType.HID to ProtocolType.HID.
This change follows the new classification system. Verify that all references to HID device detection now point to ProtocolType.HID.
114-114: Serial device registration upgrade.
Referring to bdDetect.ProtocolType.SERIAL is on par with the broader refactor. Good to see consistent usage across drivers.
167-167: HID detection flag updated.
self.isHid = portType == bdDetect.ProtocolType.HID aligns with the new protocol type approach. Ensure QA coverage for transitions from legacy code or for add-ons still referencing the old device type.
source/brailleDisplayDrivers/hidBrailleStandard.py (1)
98-98: Looks correct for filtering non-HID ports.
This ensures that non-HID devices are skipped during initialization. No issues spotted here.
source/brailleDisplayDrivers/seikantk.py (1)
19-19: Import statement is appropriate.
No issues spotted. This import provides the necessary references for registering devices.
source/brailleDisplayDrivers/eurobraille/driver.py (3)
48-48: Registering USB devices for HID looks good.
No red flags from a security or logic perspective.
70-70: Registering USB devices for SERIAL is appropriate.
Still aligned with the new protocol-based approach.
100-100: Assigning isHid for HID protocol type is valid.
No concurrency or security issues detected.
source/brailleDisplayDrivers/alva.py (2)
161-161: Registers USB HID devices properly.
No issues noted.
219-219: Assigning isHid for HID protocol is coherent.
No concerns from a correctness or security standpoint.
source/hwPortUtils.py (4)
19-19: No immediate issues with the new import.
This import for SystemErrorCodes is consistent with usage in subsequent error handling logic.
500-503: Initialize caching dictionary outside the function.
Defining _getHidInfoCache at the module level is acceptable. It effectively allows caching across multiple calls. Ensure that usage remains thread-safe if this module can be imported in multi-threaded scenarios, as dictionary writes might cause concurrency issues if done in parallel.
535-545: Ensure consistent fallback logic during concurrency.
When the device is currently in use (SHARING_VIOLATION), returning cached info is a good approach to avoid failing. However, ensure that any update to _getHidInfoCache is thread-safe. If multiple threads attempt to fetch this info for the same device simultaneously, concurrency issues may arise.
572-572: Cache is updated after reading from the device.
Storing the retrieved info in _getHidInfoCache improves performance on subsequent lookups. Verify that calling code invalidates or refreshes this cache entry as needed when device attributes change (e.g., firmware updates, hot plugging).
source/brailleDisplayDrivers/hims.py (7)
284-284: Device type mapping for HID is correct.
Registering "VID_045E&PID_940A" under bdDetect.ProtocolType.HID is consistent with the changes across other drivers. Confirm the correct detection if the device enumerates under multiple VID/PID pairs.
[approve]
290-290: Consider fallback usage carefully.
For bdDetect.ProtocolType.CUSTOM, you allow useAsFallback=False. If devices return invalid USB descriptors, you could fail detection. Validate that no backward-compatible fallback is needed for older devices.
297-297: Appropriate grouping under SERIAL.
The listed VIDs for the FTDI/serial chips are properly mapped to bdDetect.ProtocolType.SERIAL. This aligns with other braille drivers.
332-333: Check for a potential mismatch between isBulk and isHID.
The booleans are set separately based on the port type. If a device enumerates with partial HID + Bulk endpoints, ensure your detection is robust so that only one flag is true at a time.
337-337: HID instantiation logic.
Creating hwIo.Hid for HID port type is correct. Confirm that the device’s usage pages are properly supported for braille input and that the usage is consistent with the vendor’s documentation.
339-339: Ensure Bulk endpoints are correct.
When using hwIo.Bulk, verify that endpoint indices (0 in, 1 out) match the actual device descriptors. If mismatch occurs, data might not be transmitted properly.
342-342: Proper fallback for serial.
This final fallback to a standard serial interface is consistent with hardware that might present USB-based serial bridging. Check for any concurrency issues in hwIo.Serial, specifically if multiple braille devices might map to the same COM port.
source/brailleDisplayDrivers/freedomScientific.py (2)
202-202: Replaced bdDetect.DeviceType with bdDetect.ProtocolType.CUSTOM.
This aligns with the broader shift away from DeviceType. Ensure all references to ProtocolType.CUSTOM properly support the older device families.
246-246: USB detection approach remains consistent with other drivers.
The assignment self.isUsb = portType == bdDetect.ProtocolType.CUSTOM is functionally correct if the FreedSci displays are recognized as custom USB endpoints. Confirm that none require HID logic.
source/brailleDisplayDrivers/handyTech.py (4)
692-692: Correct association with ProtocolType.SERIAL.
Mapping the FTDI-based IDs to SERIAL is standard. Verify if additional variants of these chips might exist in the ecosystem.
701-701: Appropriate HID mapping for modern Handy Tech displays.
This update is consistent with newer Handy Tech models that rely on native HID. Confirm that older devices using custom endpoints have been accounted for.
726-726: Unified approach for older displays with a HID adapter.
Listing the USB adapter IDs ensures coverage of older Handy Tech devices. Validate that the driver logic recognizes both the “native HID” and “adapter-based HID” approach.
777-777: Combined HID vs HID-serial logic.
self.isHidSerial = portId in USB_IDS_HID_CONVERTER ensures correct fallback. Watch out for concurrency or timing issues when switching from the HID-serial converter flush to normal HID usage.
source/braille.py (1)
3520-3523: Check for potential KeyError in portInfo usage
The expression portInfo["bluetoothName" if "bluetoothName" in portInfo else "friendlyName"] assumes that friendlyName is always present. If this assumption doesn't always hold, a KeyError might occur.
Below is a script to verify that relevant dictionary assignments for friendlyName always exist in the code and that no KeyError paths are possible:
✅ Verification successful
KeyError is not possible in the current implementation
Looking at the code in hwPortUtils.py, we can see that friendlyName is always set in the dictionary. There are two cases:
- If
SPDRP_FRIENDLYNAMEexists, it's set tobuf.value - If
SPDRP_FRIENDLYNAMEdoesn't exist, it falls back to using theportvalue
This means the portInfo dictionary will always have a friendlyName key, making the expression portInfo["bluetoothName" if "bluetoothName" in portInfo else "friendlyName"] safe from KeyError exceptions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verifies that portInfo dictionaries always define "friendlyName".
# 1. Search for lines where portInfo is assigned or updated.
# 2. Look for references to "friendlyName" for those assignments.
rg -A 5 -F "portInfo" | rg "friendlyName"
Length of output: 141
Script:
#!/bin/bash
# Let's search for where portInfo is constructed or populated to understand its structure
# Search for portInfo assignments and dictionary creation
rg -A 10 "portInfo\s*=\s*"
# Also search for where friendlyName is assigned to understand if it's always set
rg -B 5 -A 5 'friendlyName.*='
Length of output: 2666
user_docs/en/changes.md (1)
84-85: LGTM: Braille display driver selection fix
The fix ensures that when the Standard HID Braille Display driver is explicitly selected, NVDA correctly identifies it as the selected driver in the braille display list. This improves the user experience by providing accurate feedback about the selected driver.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
See test results for failed build of commit 99611c1c2f |
Link to issue number:
Fixes #17521
Summary of the issue:
In several cases, registering a USB device in bdDetect with just VID and PID is not enough. We need an extra filter to ensure that only the right devices are detected. This was fixed at the driver level until now, but fixing this in bdDetect improves performance and stability.
Description of user facing changes
More stability and quicker detection times.
Description of development approach
Added the ability to register a match func that further constraints device detection for specific VID/PID combinations.
Testing strategy:
Known issues with pull request:
None known
Code Review Checklist:
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
New Features
Bug Fixes
Refactoring
Performance