Skip to content

Fix port listing for seikantk driver#13156

Merged
feerrenrut merged 3 commits into
rcfrom
seikaPortFix
Dec 17, 2021
Merged

Fix port listing for seikantk driver#13156
feerrenrut merged 3 commits into
rcfrom
seikaPortFix

Conversation

@feerrenrut

@feerrenrut feerrenrut commented Dec 10, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Link to issue number:

fixes #13121

Summary of the issue:

Although the braille display can be used via automatic detection, trying to configure manually results in an error.
Braille expects to be able to call ports.update(cls.getManualPorts()), other drivers all return an iterator of tuples, each containing two strings. However the seikantk driver attempts to return the device path, from the log:

DEBUG - hwPortUtils.listHidDevices (12:56:39.981) - MainThread (1252):
{'hardwareID': 'HID\\VID_10C4&PID_EA80&REV_0100', 'devicePath': '\\\\?\\hid#vid_10c4&pid_ea80#6&392c7606&5&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}', 'provider': 'usb', 'usbID': 'VID_10C4&PID_EA80', 'vendorID': 4292, 'productID': 60032, 'versionNumber': 256, 'manufacturer': 'Silicon Laboratories', 'product': 'CP2110 HID USB-to-UART Bridge', 'HIDUsagePage': 65280}

Description of how this pull request fixes the issue:

  • Use the same approach as other device drivers, relying on braille.getSerialPorts()
  • Corrects / adds missing type information.

Testing strategy:

Since I do not have a device, I'll rely on reporters to test this PR.

Known issues with pull request:

None

Change log entries:

New features
Changes
Bug fixes
For Developers

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

@lukaszgo1

lukaszgo1 commented Dec 10, 2021

Copy link
Copy Markdown
Contributor

While you're at it would it be possible to register the Seika driver for auto-detection the same way as other drivers i.e. by providing its data in bdDetect rather than inside the driver?

@feerrenrut

feerrenrut commented Dec 10, 2021

Copy link
Copy Markdown
Contributor Author

@lukaszgo1 That sounds like a separate issue. Is there one open already? I think it should be done in an independent PR.

@lukaszgo1

lukaszgo1 commented Dec 10, 2021

Copy link
Copy Markdown
Contributor

It looks like there are more mistakes in this driver i.e. the check method is overridden to unconditionally return True even when no Seika display is connected to the machine.

@feerrenrut

feerrenrut commented Dec 13, 2021

Copy link
Copy Markdown
Contributor Author

@lukaszgo1 it seems like many of the drivers do this. I agree it doesn't look quite right, but it seems unlikely to be a unique problem. I think perhaps we should consider tidying that up in the 2022.1 release.

@feerrenrut

feerrenrut commented Dec 14, 2021

Copy link
Copy Markdown
Contributor Author

I'm not confident that this change substantially fixes this driver, without a device or clear descriptions / logs of the remaining issues we won't know in the short term. I don't want to delay the 2021.3.1 release further, so I'll retarget this to master (it will become available in alpha builds).

It seems there are a number of issues remaining when using the seika notetaker device, anyone with one of these devices that is willing to work with us to test / and provide logs, please get in touch.

@feerrenrut feerrenrut changed the base branch from beta to master December 14, 2021 01:47
@cary-rowen

cary-rowen commented Dec 14, 2021

Copy link
Copy Markdown
Contributor

Hello, as @moyanming said in #13121: This PR solves #13121 but there is another problem, which is unable to connect to Seika Notetaker using Bluetooth.
I have Seika Notetaker and I am very willing to assist with the test.

@cary-rowen

cary-rowen commented Dec 14, 2021

Copy link
Copy Markdown
Contributor

@feerrenrut

This fix is ​​completely effective for #13121, and I very much hope that it will appear in 2021.3.1 because #13121 is enough to bother all users who use Seika Notetaker.
Just imagine how boring you can’t open the dialog box as long as you use Seika. This is a serious bug, and 2022.1 is an add-on compatibility broken version. Many people may need to wait for the add-ons they depend on to be released before updating To 2022.1.
I very much hope to assist NVAccess in providing test logs, because I have Seika Notetaker.

Thanks

@feerrenrut feerrenrut mentioned this pull request Dec 16, 2021
Closed
@feerrenrut feerrenrut changed the base branch from master to rc December 16, 2021 10:45
@feerrenrut feerrenrut marked this pull request as ready for review December 16, 2021 11:07
@feerrenrut feerrenrut requested a review from a team as a code owner December 16, 2021 11:07
@feerrenrut feerrenrut requested review from seanbudd and removed request for a team December 16, 2021 11:07
@feerrenrut
move registration of seikantk to bdDetect
c6ab88c 
This reflects how the other drivers are registered
@feerrenrut

feerrenrut commented Dec 16, 2021

Copy link
Copy Markdown
Contributor Author

@lukaszgo1 I have moved the addUsbDevices call to bdDetect.

@lukaszgo1

lukaszgo1 commented Dec 16, 2021

Copy link
Copy Markdown
Contributor

it seems like many of the drivers do this. I agree it doesn't look quite right, but it seems unlikely to be a unique problem.

Driver for a Seika Notetaker is the only driver for which we can auto-detect the display yet check returns True rather than fall back to the superclass implementation which ensures that display is listed only when connected. All other drivers for which check is shadowed either do not support auto-detection yet or cannot be detected passively i.e. because they're using a serial connection.

@feerrenrut
Fall back to base implementation for check method
0d34e25 
All other drivers for which check is shadowed either do not support
auto-detection yet or cannot be detected passively
i.e. because they're using a serial connection.
@feerrenrut

feerrenrut commented Dec 17, 2021

Copy link
Copy Markdown
Contributor Author

Thanks @lukaszgo1, it's a helpful observation. I have removed the override for the check method.

@feerrenrut feerrenrut merged commit 2b4434e into rc Dec 17, 2021
@feerrenrut feerrenrut deleted the seikaPortFix branch December 17, 2021 11:07
@nvaccessAuto nvaccessAuto added this to the 2022.1 milestone Dec 17, 2021
@feerrenrut feerrenrut modified the milestones: 2022.1, 2021.3.1 Dec 17, 2021
@feerrenrut

feerrenrut commented Dec 17, 2021

Copy link
Copy Markdown
Contributor Author

build of RC to test merged changes: https://ci.appveyor.com/api/buildjobs/pl56jgcjg2ym6xap/artifacts/output%2Fnvda_snapshot_rc-24378%2Ccaf14822.exe

feerrenrut added a commit that referenced this pull request Dec 22, 2021
@feerrenrut
Fix port listing for seikantk driver (PR #13156)
f6d47c5 
Add type information and fix port listing for seikantk driver

Move registration of seikantk to bdDetect
  - This reflects how the other drivers are registered

Fall back to base implementation for check method
All other drivers for which check is shadowed either do not support
auto-detection yet or cannot be detected passively
i.e. because they're using a serial connection.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2021.3.1

Development

Successfully merging this pull request may close these issues.

getPossiblePorts error with Seika Notetaker driver

5 participants

@feerrenrut @lukaszgo1 @cary-rowen @seanbudd @nvaccessAuto