Skip to content

add bluetooth support for seika notetaker #13191

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
Jan 19, 2022

Conversation

feerrenrut
Copy link
Contributor

@feerrenrut feerrenrut commented Dec 24, 2021

Link to issue number:

fixes #13142

Summary of the issue:

Seika Notetaker can not be autodetected via bluetooth

Description of how this pull request fixes the issue:

Add a way to identify the Seika device when connected by bluetooth

As per #13142 (comment) the bluetooth name for Seika devices should be TSM, a space, then 4 digits.

Testing strategy:

Ask Seika users to test when connected via bluetooth.

Known issues with pull request:

If ALL Seika devices use this bluetooth name, we may not be able to differentiate which driver to use seika vs seika notetaker.

Change log entries:

New features
- Seika Notetaker can now be auto-detected when connected via Bluetooth

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

@moyanming
Copy link
Contributor

Hi, @feerrenrut
The Bluetooth name of the Seika is "TS5 abcd", where "abcd" is a four-digit number.
Seika: TS5, a space, and a four-digit number.
Seika Notetaker: TSM, a space, and a four-digit number.

I can test both Seika and Seika Notetaker Braille display.

Best Regards,

@cary-rowen
Copy link
Contributor

@feerrenrut
I tested the build Version, and unfortunately, NVDA did not connect to my device via Bluetooth.
Shows an error dialog
The following is the log with HWIO enabled:
log_#13191.txt

@AppVeyorBot

This comment has been minimized.

@cary-rowen
Copy link
Contributor

Hello, I tested the latest build version, NVDA seems to behave the same as last time, and it still cannot connect to Seika Notetaker via Bluetooth. Here is the log:

log.txt

@AppVeyorBot
Copy link

See test results for failed build of commit 1f2f203eea

@cary-rowen
Copy link
Contributor

There is a short wait time when connecting, and the final result is that the connection fails.
log-01-09.txt

@feerrenrut
Copy link
Contributor Author

@cary-rowen Thank you for testing and providing the log. I have had to guess about the issue here, could you please test both bluetooth and USB and provide a log for me?

With HID the first and third byte is discarded, for the serial connection the bytes coming through look like the standard command. In the log provided the following was received: \xff\xff\xa2\x0b\x16\x18\x18TSM V1.0. This looks like:

  • Command: \xff\xff\xa2 = SEIKA_INFO
  • argument length: x0b = 11
  • number of buttons: \x16 = 22
  • number of cells: \x18 = 24
  • number of routing keys: \x18 = 24
  • description: "TSM V1.0"

@AppVeyorBot
Copy link

See test results for failed build of commit 82c1e231f7

@cary-rowen
Copy link
Contributor

Hi @feerrenrut , the latest build has been able to successfully connect devices using bluetooth.
But there are multiple optional items in the "Port" combo box, one of which is 'friendlyName': '蓝牙链接上的标准串行 (COM4)'
NVDA freezes when this is checked and the "OK" button is pressed,
The corresponding entry in the log for this item is as follows:
{'hardwareID': 'BTHENUM\{00001101-0000-1000-8000-00805f9b34fb}_LOCALMFG&0000', 'port': 'COM4', 'bluetoothAddress': 0, 'friendlyName': '蓝牙链接上的标准串行 (COM4)'}

That is, there are a total of four items in the Port combo box:

  1. Automatic
  2. Bluetooth
  3. Bluetooth Serial: COM3 (TSM 3366)
  4. Serial: 蓝牙链接上的标准串行 (COM4)
    The first three items can be successfully connected to Seika Notetaker, and the fourth item causes NVDA to freeze.

Grateful

@moyanming
Copy link
Contributor

Hi @feerrenrut
The Seika Notetaker sending back the correct data means the connection is successfully now.
\xff\xff\xa2\x0b\x16\x18\x18TSM V1.0

BTW, both USB-HID and Bluetooth Serial COM port use the same data protocol.

@moyanming
Copy link
Contributor

Hi @cary-rowen
There have two Bluetooth UUIDs, one for Tx and another for Rx, and this is why there are two Bluetooth Serial Com ports.
I think the Bluetooth Serail Com port which has "TSM abcd" is the right one for the communication.

@feerrenrut
Copy link
Contributor Author

@moyanming can you test the latest PR build with bluetooth auto detection and report your findings. I believe this should be working, if so I'd like to merge this PR. Then we can raise a new issue and look at the listing of ports and see if we can prevent the wrong one from being listed.

@cary-rowen
Copy link
Contributor

And, another question, in the case of a successful connection, the user re-selects the same port in the combo box and clicks the "OK" button, then what is our expected performance? Currently makes Seika Notetaker show nothing as if it's disconnected.

@moyanming
Copy link
Contributor

moyanming commented Jan 10, 2022

@moyanming can you test the latest PR build with bluetooth auto detection and report your findings. I believe this should be working, if so I'd like to merge this PR. Then we can raise a new issue and look at the listing of ports and see if we can prevent the wrong one from being listed.

Hi, @feerrenrut
I have tested the PR build and used the USB and BT respectively, I also test the Automatic which is not select the specific Braille display.
The Port list of the Seika Notetatker is: Automatic, USB, Bluetooth, Serial (com1), Bluetooth Serial with TSM 2002, and Serial without TSM abcd.
The following log files all work fine:
BT_SeikaNotetaker_COM port with 'TSM abcd'_OK.log
USB_Automatic Braille display_OK.log
USB_SeikaNotetaker_Automatic_OK.log
USB_SeikaNotetaker_USB_OK.log
BT_Automatic Braille display_OK.log
BT_SeikaNotetaker_Automatic_OK.log
BT_SeikaNotetaker_Bluetooth_OK.log

I got a crash issue when selecting the Bluetooth Serial COM port without TSM abcd:
BT_SeikaNotetaker_COM port without 'TSM abcd' will crash.log

BTW, accidentally disconnecting the USB cable and then reconnecting the USB, the NVDA still can't connect automatically but needs to restart NVDA, #13143 , here is the log:
Accidentally disconnect-USB needs restart NVDA_issue.log

Elsewhere the bytes are in 'big' byte ordering
HID and Serial seem to require different approaches
Use the bluetoothName to filter manual ports,
matching the same approach used by auto detection
of the seika notetaker device.

In order to prevent a circular import,
the initialization of the detection data (and
new import of seika notetaker module method
was moved into a contained method.
Add tests for serial (bluetooth)
@feerrenrut feerrenrut force-pushed the addBluetoothAutoDetect-seikaNotetaker branch from 0627c24 to e544667 Compare January 15, 2022 06:28
@feerrenrut
Copy link
Contributor Author

Can you try the latest build
the Bluetooth Serial COM port without TSM abcd should no longer be listed.

@cary-rowen
Copy link
Contributor

Hi, the build seems to introduce a regression,:
Launching this version of NVDA for the first time, but the Seaka device is not turned on, navigating to "Seika Notetaker" in the braille display combo box NVDA throws an error, the log is as follows:
Device not open, navigate in device selection combo box.txt
The Seaka device enters Bluetooth pairing mode, and then starts this version of NVDA. At this time, Seaka can successfully display content through "Auto Connect", but the device selection dialog cannot be opened:
Unable to open device selection dialog after connecting.txt


def __init__(self, port="hid"):
def __init__(self, port=bdDetect.KEY_HID):
Copy link
Contributor

@lukaszgo1 lukaszgo1 Jan 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I'm not very familiar with NVDA's braille display drivers, this is the only one which specifies HID as the value for the port keyword argument in the constructor. For all others we either leave it with no default value in case of displays where port needs to be specified by the user, or for displays which can be auto-detected we assign "auto" to it. Could you add a comment here to explain why this driver is different?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The short answer is I was trying to avoid making changes if not necessary. Looking at this closer, it seems like it should never be "hid". I have tried to improve the docs and typing elsewhere. One confusing thing in the NVDA braille code is that "port" is used to refer to many different things, in different parts of the code it may be a COM port string, a string description of where to look (auto, usb, bluetooth), a HID device path string, or a DeviceMatch class instance.

There does not seem to be any situation where the port should be hid.
@feerrenrut
Copy link
Contributor Author

@moyanming Is it possible to confirm how this command / gesture shows up in the input gestures dialog. I want to make sure this is consistent with other braille drivers.

@moyanming
Copy link
Contributor

@moyanming I wrote this table based on the gestures in the seikantk driver. The d7 entry is this one: "kb:backspace": ("br(seikantk):d7",),. Other drivers use dot7 to to refer to dots. Can you confirm that dot7 can be used to backspace characters?

Hi @feerrenrut
For the code: there is a "_dotNames" that defined the d7 (means "dot7") and other dot keys, there should be changed everywhere if needed.
For the User guider: it is fine to use "dot7" to represent the dot 7 key of the Seika Notetaker.
Thanks!

@feerrenrut
Copy link
Contributor Author

Looking at the other drivers, the hims, papenmeier, and brailleNote drivers use dx eg d7 as well. I'll update the user guide to refer to this as "dot 7"

For seika notetaker, userguide should refer to dot7 and dot8 not d7 and d8
@cary-rowen
Copy link
Contributor

@moyanming

Hi, There are currently two options with the word "Seika" in the braille device selection combo box, we don't seem to have reflected their differences in the user guide, can you try to mention this in the user guide, or do we The options that can be in the combo box themselves reflect their range of support, sorry this may be off topic. If you want, we can contact privately, my email is:
manchen_0528@outlook.com

Thanks

@feerrenrut
Copy link
Contributor Author

@cary-rowen can you get a screenshot of this, or set NVDA to English and copy the speech output from the speech viewer. I want to make sure I understand the request.

I think there will be one entry for "seika" and one for "seika notetaker"?

@feerrenrut
Copy link
Contributor Author

Apologies, the last time I asked to check the user guide I linked to the changes file, I have corrected that link.

Can @cary-rowen and @moyanming please confirm the accuracy of the user guide:

@moyanming
Copy link
Contributor

@moyanming

Hi, There are currently two options with the word "Seika" in the braille device selection combo box, we don't seem to have reflected their differences in the user guide, can you try to mention this in the user guide, or do we The options that can be in the combo box themselves reflect their range of support, sorry this may be off topic. If you want, we can contact privately, my email is: manchen_0528@outlook.com

Thanks

Hi @cary-rowen
For historical reasons, there is a Seika representing Seika v3/5/80 Braille display.
There is a new device type named Seika Notetaker Braille display published later and uses a different protocol compared to Seika Braille display.
We recommend using two different items for easy adding/modifying the driver.
BTW, you can select "Automatic" when connecting to Seika Notetaker now and this will work fine.

@moyanming
Copy link
Contributor

Hi @feerrenrut
Yes, there are two entry items: "Seika" and "Seika Notetaker".

@moyanming
Copy link
Contributor

Hi, @feerrenrut
There are sub-type of Seika Notetaker: 16 cells Seika Notetaker, 24 cells Seika Notetaker, and 40 cells Seika Notetaker. There is no need to install the driver when using Seika Notetaker because those devices use USB-HID.

So please change from:
15.11. Seika Braille Displays
The Seika Version 3, 4 and 5 (40 cells), Seika80 (80 cells), and Notetaker (16 cells) braille displays from Nippon Telesoft are supported. You can find more information about the displays and the required drivers at https://en.seika-braille.com/down/index.html You must first install the USB drivers provided by the manufacturer.

Should change to:
15.11. Seika Braille Displays
The Seika Version 3, 4, and 5 (40 cells), Seika80 (80 cells), and Notetaker (16, 24, 40 cells) braille displays from Nippon Telesoft are supported. You can find more information about the displays and the required drivers at https://en.seika-braille.com/down/index.html. You must first install the USB drivers when using Seika v3/4/5/80 provided by the manufacturer. There is no need to install the driver when using Seika Notetaker.

@cary-rowen
Copy link
Contributor

@moyanming @feerrenrut
Ok, I see, but I understand that seika also has a V6Pro model in China, do you know about it? Is it possible to mention the device in the user guide as well?

Grateful

@moyanming
Copy link
Contributor

@moyanming @feerrenrut Ok, I see, but I understand that seika also has a V6Pro model in China, do you know about it? Is it possible to mention the device in the user guide as well?

Grateful

Hi @cary-rowen
Seika V6Pro is kind of Seika Notetaker has 40 cells.
The Seika Notetaker device name may vary but those devices should use "Seika notetaker" or "Automatic" compare to Seika v3/4/5/80 devices.

@feerrenrut
Copy link
Contributor Author

I have split the information for the two different drivers. @moyanming Can you review this?

installer
direct user guide html

@moyanming
Copy link
Contributor

Hi @feerrenrut
That's great.

V6 and V6Pro are 40 cells Seika Notetaker, so please add V6 as well.
BTW, V6Pro has TTS voice supported but V6 hasn't.

So, please change:
15.11.2. MiniSeika (16, 24 cells), V6Pro (40 cells)
to:
15.11.2. MiniSeika (16, 24 cells), V6 and V6Pro (40 cells)

@feerrenrut
Copy link
Contributor Author

@moyanming done. The build will complete shortly.

@moyanming
Copy link
Contributor

As per #13142 (comment) the bluetooth name for Seika devices should be TSM, a space, then 4 digits.

Testing strategy:

Ask Seika users to test when connected via bluetooth.

Known issues with pull request:

If ALL Seika devices use this bluetooth name, we may not be able to differentiate which driver to use seika vs seika notetaker.

Hi @feerrenrut
Thanks for your great work and the "Seika Notetaker" already supported by Bluetooth connection.

But the "Seika"( means "Seika v3/v5/v80") still can't connect with NVDA by using Bluetooth COM port, which is no the same type as "Seika Notetaker".
The Seika uses Bluetooth name: "TS5 abcd", but Seika Notetaker uses: "TSM abcd", where "abcd" are four digital numbers.
Do I add a new Issue about this?

Best Regards

@cary-rowen
Copy link
Contributor

@moyanming
Can you create a new question for this? NVAccess cannot triage closed issues.
Also, I would like to get in touch with you privately, my email is manchen 0528@outlook.com

@moyanming
Copy link
Contributor

Hi @cary-rowen
I have report an issue #14242 and send to your email manchen_0528@outlook.com and please check you Inbox.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Bluetooth COM Port function for Seika and Seika Notetaker Braille display
7 participants