Update seika braille driver for python 3 by moyanming · Pull Request #10961 · nvaccess/nvda

moyanming · 2020-04-08T13:49:54Z

Update the seika.py by using the Python v3. Have well tested with Seika v3/5/80 devices.
Refer to issue #10787

Link to issue number:

#10787

Summary of the issue:

Seika Braille Display can't be loaded in NVDA 2019.3

Description of how this pull request fixes the issue:

Update the seika.py by using the Python v3.

Testing performed:

Have well tested with the Seika v3/5/80 devices by the manufacturer of the Seika device. The Seika v3/5/80 can be connected automatically after selected.

Known issues with pull request:

None.

Change log entry:

Section: New features, Changes, Bug fixes
Bug fixes.

Update the seika.py by using the Python v3. Have well tested with Seika v3/5/80 devices.

AppVeyorBot · 2020-04-08T13:58:37Z

PR introduces Flake8 errors
Build (for testing PR)

See test results for failed build of commit a9d35d0c53

Fixed errors: source/brailleDisplayDrivers/seika.py:6:1: E265 block comment should start with '# ' #Copyright (C) 2012/20 Ulf Beckmann <beckmann@flusoft.de> ^ source/brailleDisplayDrivers/seika.py:105:13: E203 whitespace before ':' lineBytes : bytes = self.s40 + bytes(cells) + cellPadding ^ source/brailleDisplayDrivers/seika.py:199:1: W391 blank line at end of file ^ 1 E203 whitespace before ':' 1 E265 block comment should start with '# ' 1 W391 blank line at end of file

feerrenrut · 2020-04-08T14:36:23Z

Thanks @moyanming. Since it seems that the intention of this PR to replace #9479, I'll close that PR and communication can continue here.

feerrenrut · 2020-04-08T14:38:45Z

+			if versionS.startswith(b"seika80"):
 				log.info("Found Seika80 connected via {port} Version {versionS}".format(port=port, versionS=versionS))
 				self.numCells = 80
+				self.s40 = b"\xff\xff\x73\x38\x30\x00\x00\x00"


Can you add a comment to explain what this is for, or what these values mean? This will be very helpful if the community has to maintain this file in the future.

There needs handshake between Seika v3/5/80 and NVDA before any actual Braille data. The handshake data (\xff\xff\x73\x38\x30\x00\x00\x00) is one part of the transmit protocol of the Seika v3/5/80.

Can you add this as a comment to the source code please.

s40 as a property name doesn't make much sense when it applies both to seika80 and 40.
Please rename to sendHeader based on the comment on line 105:

# every transmitted line consists of the preamble SEIKA_SENDHEADER and the Cells

Looking at this again, I have realized what this encodes:

Two bytes of b'\xFF'

The code for the display b"s80"

Null characters until the length is 8.

Note, this value:

>>> b"\xff\xff\x73\x38\x30\x00\x00\x00" b'\xff\xffs80\x00\x00\x00' >>> "s80".encode() == b"\x73\x38\x30" True

Then from the later value where s40 is defined for 40 cell v3, v5.

>>> b"\xFF\xFF\x73\x65\x69\x6B\x61\x00" b'\xff\xffseika\x00' >>> b'\x73\x65\x69\x6B\x61' == b'seika' True

It would be way clearer if these could be constructed:

def createDeviceVersionHandshake(deviceVersion:bytes)->bytes: """ Creates an 8 byte string containing the device version appropriately formatted for the handshake. if 5 < len(deviceVersion) < 1: raise ArgumentError("At least one character must be supplied. Only space in handshake for a total of 8 bytes. Requires 2 Leading 0xFF chars, and one trailing 0x0 char") totalChars = 8 prefixChars = b"\xFF" * 2 trailingChars = b"\x00" * (totalChars - len(prefixChars) - len(deviceVersion)) return = prefixChars + deviceVersion + trailingChars

Then here you can do something like:

Suggested change

self.s40 = b"\xff\xff\x73\x38\x30\x00\x00\x00"

self.sendHeader= createDeviceVersionHandshake(b's80')

And a bit further down for the v3, v5:

self.sendHeader=createDeviceVersionHandshake(b'seika')

We have understood how to construct these headers, and what they encode. I would like this to be more obvious in the code.

Please add the following function:

def createDeviceVersionHandshake(deviceVersion:bytes)->bytes: """ Creates an 8 byte string containing the device version appropriately formatted for the handshake. if 5 < len(deviceVersion) < 1: raise ArgumentError( "At least one character must be supplied." " Only space in handshake for a total of 8 bytes." " Requires 2 Leading 0xFF chars, and one trailing 0x0 char" ) totalChars = 8 prefixChars = b"\xFF" * 2 data = prefixChars + deviceVersion # pad to required length data = data.ljust(totalChars, b'\x00') return data

Then where b"\xff\xff\x73\x38\x30\x00\x00\x00" is used change to s80Header = createDeviceVersionHandshake(b"s80")
and where b"\xFF\xFF\x73\x65\x69\x6B\x61\x00" is used change to seikaHeader = createDeviceVersionHandshake(b"seika")

feerrenrut · 2020-04-08T16:23:37Z

+			if versionS.startswith(b"seika80"):
 				log.info("Found Seika80 connected via {port} Version {versionS}".format(port=port, versionS=versionS))
 				self.numCells = 80
+				self.s40 = b"\xff\xff\x73\x38\x30\x00\x00\x00"


Can you add this as a comment to the source code please.

JulienCochuyt · 2020-05-12T16:30:56Z

@moyanming,
Could you please address the remaining requested changes on this PR?
We're receiving more and more complaints from our customers of these devices, disappointed to have support dropped for three versions of NVDA now (2019.3, 2019.3.1, 2020.1)

feerrenrut · 2020-05-13T14:12:29Z

+			if versionS.startswith(b"seika80"):
 				log.info("Found Seika80 connected via {port} Version {versionS}".format(port=port, versionS=versionS))
 				self.numCells = 80
+				self.s40 = b"\xff\xff\x73\x38\x30\x00\x00\x00"


s40 as a property name doesn't make much sense when it applies both to seika80 and 40.
Please rename to sendHeader based on the comment on line 105:

# every transmitted line consists of the preamble SEIKA_SENDHEADER and the Cells

Looking at this again, I have realized what this encodes:

Two bytes of b'\xFF'

The code for the display b"s80"

Null characters until the length is 8.

Note, this value:

>>> b"\xff\xff\x73\x38\x30\x00\x00\x00" b'\xff\xffs80\x00\x00\x00' >>> "s80".encode() == b"\x73\x38\x30" True

Then from the later value where s40 is defined for 40 cell v3, v5.

>>> b"\xFF\xFF\x73\x65\x69\x6B\x61\x00" b'\xff\xffseika\x00' >>> b'\x73\x65\x69\x6B\x61' == b'seika' True

It would be way clearer if these could be constructed:

def createDeviceVersionHandshake(deviceVersion:bytes)->bytes: """ Creates an 8 byte string containing the device version appropriately formatted for the handshake. if 5 < len(deviceVersion) < 1: raise ArgumentError("At least one character must be supplied. Only space in handshake for a total of 8 bytes. Requires 2 Leading 0xFF chars, and one trailing 0x0 char") totalChars = 8 prefixChars = b"\xFF" * 2 trailingChars = b"\x00" * (totalChars - len(prefixChars) - len(deviceVersion)) return = prefixChars + deviceVersion + trailingChars

Then here you can do something like:

Suggested change

self.s40 = b"\xff\xff\x73\x38\x30\x00\x00\x00"

self.sendHeader= createDeviceVersionHandshake(b's80')

And a bit further down for the v3, v5:

self.sendHeader=createDeviceVersionHandshake(b'seika')

Adriani90 · 2020-08-17T10:54:23Z

@moyanming just a reminder, if you'd be so kind to follow up on the remaining things to fix on this PR? Thanks.

Ulf-Beckmann · 2020-08-31T15:18:58Z

Hello,
I am unsure whether I have understood the GitHub system correctly and I am even more unsure about the news about the Python3 and Seika customization. Therefore, here again the two current drivers for the Seika Braille displays and the Seika Notetaker.
I deliberately didn't put both of them in one file, which would certainly make sense, but I had to struggle enough with the changes. Unfortunately I have not found any further specifications for adaptation. When studying the other drivers, however, I noticed that some (e.g. hedo) do not work with Python3. But that's a different matter.

best regards, ulf

seika.py.zip
seikantk.py.zip

AppVeyorBot · 2020-11-12T10:28:43Z

PR introduces Flake8 errors
Build (for testing PR)

See test results for failed build of commit ee9926a8db

dpy013 · 2020-11-12T10:45:49Z

hello
@moyanming
Please fix the flake8 error.
thanks

lukaszgo1

Aside from failing Linter checks and my comments it looks like most changes requested by @feerrenrut were just marked as resolved but no changes to the source code were made. Please address these.

moyanming

have checked

feerrenrut · 2021-03-17T09:33:53Z

Given the time this is taking, would you like me to make the changes I suggest directly?

moyanming · 2021-03-18T04:22:27Z

Given the time this is taking, would you like me to make the changes I suggest directly?

Hi @feerrenrut ,
Yes, please make the change directly.
I am sorry for not familiar with GitHub.

moyanming · 2021-03-31T04:41:28Z

Given the time this is taking, would you like me to make the changes I suggest directly?

Hi @feerrenrut ,
Yes, please make the change directly.
I am sorry for not familiar with GitHub.

Hi @feerrenrut ,
Is there new information about this PR?
Any questions please let me know.
Best Regards

moyanming · 2021-04-02T05:47:21Z

I use "Seika Notetaker Mini24" to test and select "Seika" in the list. It can't be used at all.
@moyanming Does the PR support the device?

Hi @cary-rowen
Please test this PR build for Seika Notetaker Mini24.

BTW, Mini16/24 and V6 (40-cells) are kind of Seika Notetaker, please chose the Seika Notetaker within the NVDA.

feerrenrut · 2021-04-09T07:18:30Z

@moyanming Can you confirm this driver is working? I'd like to get this reviewed and merged.

moyanming · 2021-04-09T07:31:36Z

@moyanming Can you confirm this driver is working? I'd like to get this reviewed and merged.

Hi @feerrenrut ,
We have tested Seika v3/5/80 Braille display with the PR build and all work fine.
The code is ready for merge now.

Since I'm a contributor now, I shouldn't be a reviewer.

feerrenrut · 2021-04-09T08:08:55Z

I'm aware, thanks @dingpengyu. It is just the changes file, I'll get these changes reviewed, then I'll resolve conflicts in the changes file before merging.

JulienCochuyt · 2021-05-04T06:44:29Z

Sorry for the late report.
Everything seems good here with a Seika v5.

@moyanming, could you please confirm if the CP210x USB to UART Bridge Driver is required or not to use the Seika Displays driver as of this PR?
Long story short, we've recently faced some broken installation of this CP210x USB to UART Bridge Driver after updating to Windows 10 20H2 19042.928. Reinstalling this bridge driver fixed the issue and allows again to use the Seika Displays driver that we ship as an extension to NVDA 2019.3 > 2020.4, and we'd like to be sure on what to communicate to the users regarding the future release.

moyanming · 2021-05-06T07:42:58Z

Sorry for the late report.
Everything seems good here with a Seika v5.

@moyanming, could you please confirm if the CP210x USB to UART Bridge Driver is required or not to use the Seika Displays driver as of this PR?
Long story short, we've recently faced some broken installation of this CP210x USB to UART Bridge Driver after updating to Windows 10 20H2 19042.928. Reinstalling this bridge driver fixed the issue and allows again to use the Seika Displays driver that we ship as an extension to NVDA 2019.3 > 2020.4, and we'd like to be sure on what to communicate to the users regarding the future release.

Hi @JulienCochuyt
The Seika v5, and Seika v3/v80, need the CP210x driver insalled.
It seems that the updating of Windows 10 OS broken/deleted the CP210x driver, please reinstall the CP210x driver to fix it.

lukaszgo1 · 2021-06-01T18:55:53Z

@feerrenrut @michaelDCurran May I ask when this is going to be looked at? As it stands people who've purchased this display cannot use it with NVDA for year and a half now. People are complaining about support for Seika displays on users list from time to time and this seems to be ready aside from being reviewed.

cary-rowen · 2021-06-02T00:48:22Z

 I have manually tested the build of this PR and it works as expected. Why can't we resolve the conflict as soon as possible to integrate it into NVDA as soon as possible?

cary-rowen · 2021-06-02T00:52:18Z

The other is Seika Notetaker. I also tested it. I think this is an important feature. It will affect those NVDA users who use Seika. Does anyone care about this PR?

moyanming · 2021-06-02T03:43:21Z

I'm aware, thanks @dingpengyu. It is just the changes file, I'll get these changes reviewed, then I'll resolve conflicts in the changes file before merging.

Hi, @feerrenrut
I think it's time to merge this PR now.
If there need my work on this PR please let me know.
Thank you for your great work.

michaelDCurran

Looks okay to me, including @seanbudd 's suggestions.

cary-rowen · 2021-06-03T06:32:29Z

@moyanming Can you change this?
Thanks.

feerrenrut · 2021-06-03T06:42:11Z

@cary-rowen, No NV Access is addressing the items on this PR now.

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

Add files via upload

900e9e9

Update the seika.py by using the Python v3. Have well tested with Seika v3/5/80 devices.

feerrenrut mentioned this pull request Apr 8, 2020

Add driver for seika notetaker braille display #9479

Closed

feerrenrut suggested changes Apr 8, 2020

View reviewed changes

moyanming requested a review from feerrenrut April 8, 2020 15:27

feerrenrut suggested changes Apr 8, 2020

View reviewed changes

fixed for the requested changes.

479a86e

feerrenrut added component/braille-display-drivers feature labels Apr 14, 2020

feerrenrut previously requested changes May 13, 2020

View reviewed changes

LeonarddeR mentioned this pull request Aug 22, 2020

Add original Seika Notetaker driver #11514

Merged

8 tasks

add the update by ulf.

5cbdbe6

lukaszgo1 suggested changes Nov 15, 2020

View reviewed changes

fixed flake8 errors, request, and bug fixed.

f1e1a92

lukaszgo1 suggested changes Nov 20, 2020

View reviewed changes

Comment thread source/brailleDisplayDrivers/seika.py Outdated

Comment thread source/brailleDisplayDrivers/seika.py Outdated

fix requests.

37f0284

moyanming requested a review from feerrenrut January 11, 2021 14:06

moyanming commented Mar 4, 2021

View reviewed changes

feerrenrut reviewed Mar 17, 2021

View reviewed changes

Comment thread source/brailleDisplayDrivers/seika.py Outdated

moyanming closed this Mar 31, 2021

feerrenrut self-assigned this Apr 8, 2021

feerrenrut requested a review from michaelDCurran April 9, 2021 07:36

seanbudd changed the title ~~Update the seika.py by using the Python v3~~ Update seika braille driver for python 3 Jun 3, 2021

feerrenrut changed the base branch from master to beta June 3, 2021 03:21

feerrenrut requested a review from a team as a code owner June 3, 2021 03:21

Merge remote-tracking branch 'origin/beta' into moyanming/master

16606cf

seanbudd reviewed Jun 3, 2021

View reviewed changes

michaelDCurran previously approved these changes Jun 3, 2021

View reviewed changes

feerrenrut dismissed michaelDCurran’s stale review via 6b82d64 June 3, 2021 06:43

feerrenrut and others added 2 commits June 3, 2021 14:43

Review actions

6b82d64

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

review actions

78b54ec

seanbudd approved these changes Jun 3, 2021

View reviewed changes

feerrenrut merged commit ed0f900 into nvaccess:beta Jun 3, 2021

nvaccessAuto added this to the 2021.2 milestone Jun 3, 2021

seanbudd modified the milestones: 2021.2, 2021.1 Jun 4, 2021

moyanming mentioned this pull request Jun 4, 2021

Seika Braille Display can't be loaded in NVDA 2019.3 #10787

Closed

	self.s40 = b"\xff\xff\x73\x38\x30\x00\x00\x00"
	self.sendHeader= createDeviceVersionHandshake(b's80')

Uh oh!

Conversation

moyanming commented Apr 8, 2020

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

AppVeyorBot commented Apr 8, 2020

Uh oh!

feerrenrut commented Apr 8, 2020

Uh oh!

Uh oh!

feerrenrut Apr 8, 2020

Choose a reason for hiding this comment

Uh oh!

moyanming Apr 8, 2020

Choose a reason for hiding this comment

Uh oh!

feerrenrut Apr 8, 2020

Choose a reason for hiding this comment

Uh oh!

feerrenrut May 13, 2020

Choose a reason for hiding this comment

Uh oh!

feerrenrut Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

feerrenrut Apr 8, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JulienCochuyt commented May 12, 2020

Uh oh!

feerrenrut May 13, 2020

Choose a reason for hiding this comment

Uh oh!

Adriani90 commented Aug 17, 2020

Uh oh!

Ulf-Beckmann commented Aug 31, 2020

Uh oh!

AppVeyorBot commented Nov 12, 2020

Uh oh!

dpy013 commented Nov 12, 2020

Uh oh!

lukaszgo1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moyanming left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

feerrenrut commented Mar 17, 2021

Uh oh!

moyanming commented Mar 18, 2021

Uh oh!

moyanming commented Mar 31, 2021

Uh oh!

moyanming commented Apr 2, 2021

Uh oh!

feerrenrut commented Apr 9, 2021

Uh oh!

moyanming commented Apr 9, 2021

Uh oh!

feerrenrut commented Apr 9, 2021

Uh oh!

JulienCochuyt commented May 4, 2021

lukaszgo1 left a comment •

edited

Loading