Add com port option in Braille Display Select Dialog and fix Serial Connection error

[EdKweon]

Link to issue number:

Summary of the issue:

Add com port in Hims Braille Display and fix Serial Connection error

Description of user facing changes

• Users can always perform serial connections normally.
• User can choose com port in Braille Display Selection Dialog(Hims).

Description of development approach

• In the HimsDisplayDriver, due to a chip issue, there were instances where packet data was intermittently split arbitrarily during the serial connection verification process, necessitating modifications to the _onReceive function.

Testing strategy:

Lint, System, Unit

Known issues with pull request:

Code Review Checklist:

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

Summary by CodeRabbit

• New Features

  • Introduced fallback device management for USB device drivers to improve reliability when primary drivers are unavailable.

• Enhancements

  • Improved handling of multiple device types and identifiers in automatic detection.
  • Enhanced the serial data management to handle data fragments more effectively.

• Bug Fixes

  • Enhanced logic to ensure fallback devices are correctly utilized and cleared when necessary.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• PASS: System tests (tags: installer NVDA).
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/0p5p21mw2yfqtpp5/artifacts/output/nvda_snapshot_pr16340-31523,0b632001.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 1.1,
  INSTALL_END 1.0,
  BUILD_START 0.0,
  BUILD_END 32.0,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.4,
  TEST_START 0.0,
  TEST_END 2.5,
  FINISH_END 0.2

See test results for failed build of commit 0b6320011c

[LeonarddeR]

1. As far as I understand the product, you can choose two connection modes, either HID or Serial. Furthermore, for countries that don't want to use HID, you hide the HID method. Is that correct?
2. If the desired operating mode is Serial, shouldn't the device reject requests to connect using HID?
3. Should people who don't want to use HID have the possibility to disable HID discoverability of the device altogether?

[EdKweon]

@LeonarddeR
Your points accurately. However, the current issue is that this device cannot hide the HID mode.

1. Yes, that is correct.
2. Currently, the system team has stated that it is challenging to implement a method to reject HID connection requests.
3. As it stands, when the device is connected via USB, the HID connection is inevitably established. It would be ideal to disable it, but currently, it seems difficult.

Given these issues, there is a need for a software solution. Consequently, my current plan is to modify the code to check via Serial if HID is connectable in software terms. Would this be acceptable?

[LeonarddeR]

If you could come up with a solution that doesn't touch ordering for other drivers, feel free to give it a try, though I"m afraid that it will be almost impossible to do so given how the detection system functions.
Have you considered leaving out the HID option out of automatic detection?

[EdKweon]

@LeonarddeR
Yes . But I think that's impossible leaving out. I will commit code tomorrow. Thank you for your help

stale

[lukaszgo1]

@EdKweon I've looked briefly at the code and what you're trying to do seems doable but difficult. Here is how I'd go about de-prioritizing HID for a particular driver:

• Add an additional argument (something like useAsAFallBack which should default to False) on DriverRegistrar.addUsbDevices (for consistency the same can be done for addBluetoothDevices, though this is secondary)
• No longer store union of callables and sets of IDs in the _driverDevices for each device - abstract it into a class like DeviceMatcher - it should remember if the given match is a fallback and obviously allow to check if the device in question has a matching ID / Bluetooth name
• Then, in getConnectedUsbDevicesForDriver when iterating through found devices, for each one check if it should be used as a fallback, if so don't yield it immediately, but only at the end when there is no non-fallback match
• Finally, when registering HID ID for Hims, set it as a fallback device, which ensures it will be picked only when serial is not found.

Comments are appreciated.

[EdKweon]

@seanbudd
I added docstring of FallbackDevicesStore class.

• "can you answer this please?"
  I can't understand what you saying. Can you tell me what I need to respond to?"

[seanbudd]

@EdKweon - you may need to uncollapse hidden comments. search the page for "6 hidden conversations"

[seanbudd]

Alternatively, open the PR files page, select conversations, and check through the unresolved comments

[seanbudd]

@EdKweon - this is looking almost ready other than the last comment. can you do a final round of testing?

@coderabbitai full review

[coderabbitai]

@seanbudd, I've reviewed the PR based on the available information and discussions in the comments. Here are the key points and recommendations:

1. Fallback Device Handling: It's crucial to ensure that the handling of fallback devices is consistent across the functions getConnectedUsbDevicesForDriver and getDriversForConnectedUsbDevices. The modifications should be thoroughly reviewed to confirm that fallback logic works as intended, especially during device disconnections and reconnections.

2. Testing: Given the changes involve critical functionality for device detection and connection stability, a comprehensive testing strategy should be employed. This includes:

   • Unit Tests: Ensure there are unit tests that specifically test the fallback logic under various scenarios, including disconnecting and reconnecting devices.
   • System Tests: Conduct system tests to observe the behavior in a simulated or actual user environment to ensure no regression or unintended behavior.
   • Manual Testing: Manual testing should be conducted to verify the user interface changes, particularly the new options in the Braille Display Select Dialog.

3. Code Review and Documentation: Ensure that all new code is well-documented, especially the parts dealing with fallback devices. This includes updating inline comments and external documentation to reflect the new logic.

4. Linting Issues: Address the linting issues that have caused the build to fail in recent attempts. Ensuring code quality and standards are met will aid in maintaining the codebase health.

Please ensure these areas are addressed comprehensively before finalizing the PR. If there are any specific areas you would like further in-depth analysis or testing on, let me know!

Analysis chain

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Fetch the latest commits and show the changes in the files bdDetect.py and hims.py
git log -p -2 -- bdDetect.py hims.py

Length of output: 1688

Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 4

Outside diff range and nitpick comments (8)

source/brailleDisplayDrivers/hims.py (3)

Line range hint 318-318: Remove unused loop variable.

The loop variable i is not used within the loop body. Consider renaming it to _ to indicate that it's intentionally unused:

- for i in range(3):
+ for _ in range(3):

---

Line range hint 338-339: Optimize string formatting using f-strings.

Use Python's f-string for better readability and performance:

- log.info("Found {device} connected via {type} ({port})".format(
-     device=self._model.name, type=portType, port=port))
+ log.info(f"Found {self._model.name} connected via {portType} ({port})")

---

Line range hint 364-364: Use Python's built-in exception type for consistency.

Replace WindowsError with Python's built-in OSError for better consistency and clarity:

- except WindowsError:
+ except OSError:

source/bdDetect.py (5)

Line range hint 30-30: Remove unused import of typing.Set.

- from typing import Set

---

Line range hint 33-33: Remove unused import of typing.Union.

- from typing import Union

---

Line range hint 151-155: Remove extraneous parentheses to simplify expression.

- usbHidDeviceMatches, usbHidDeviceMatchesForCustom = itertools.tee((
+ usbHidDeviceMatches, usbHidDeviceMatchesForCustom = itertools.tee(

---

Line range hint 217-221: Remove extraneous parentheses to simplify expression.

- btHidDevMatchesForHid, btHidDevMatchesForCustom = itertools.tee((
+ btHidDevMatchesForHid, btHidDevMatchesForCustom = itertools.tee(

---

Line range hint 367-368: Replace yield over for loop with yield from for cleaner and more efficient code.

- for driver, match in btDevs:
-     yield (driver, match)
+ yield from btDevs

[seanbudd]

Thanks @EdKweon