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.

Sign up for GitHub

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

Jump to bottom

NLS eReader Zoomax braille driver #17509

Draft

florin-trutiu wants to merge 9 commits into nvaccess:master from florin-trutiu:master

+327 −2

florin-trutiu commented Dec 12, 2024 •

edited

Loading

Link to issue number:

Closes #15863

Summary of the issue:

This is the NVDA driver for the NLS eReader Zoomax braille display.
It supports both USB and Bluetooth automatic detection.

Description of user facing changes

With this driver the user can use directly the NLS eReader Zoomax display without the need to manually install it as an addon.

Description of development approach

The driver is similar with the existing braille display drivers for NVDA.

Testing strategy:

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.

@coderabbitai summary


          Added the NLS eReader Zoomax braille driver

7ab13e6

florin-trutiu requested a review from a team as a code owner

December 12, 2024 11:23

florin-trutiu requested a review from SaschaCowley

December 12, 2024 11:23


          Pre-commit auto-fix

0c5fa0e

seanbudd self-requested a review

December 12, 2024 22:56

Member

seanbudd commented Dec 13, 2024

Thanks for opening this PR @florin-trutiu !

Please fill out the PR template
Please remove this line from the userGuide:

nvda/user_docs/en/userGuide.md

Line 4284 in c17b423

* Note that the Zoomax is currently not supported without external drivers
Please add a changelog entry in changes.md noting that support for this braille device has been added

Member

seanbudd commented Dec 13, 2024

pre-commit.ci autofix

pre-commit-ci bot and others added 3 commits

December 13, 2024 02:14


          Pre-commit auto-fix

d175093


          Edited the user guide.

0b62584


          Merge branch 'master' of https://github.com/florin-trutiu/NLSeReaderZ…

9ad1d7f

…oomaxDriver

florin-trutiu requested a review from a team as a code owner

December 13, 2024 11:34

florin-trutiu requested a review from Qchristensen

December 13, 2024 11:34

florin-trutiu added 3 commits

December 13, 2024 13:39


          Edited the user guide again.

5bf66cb


          Added an entry for the NLS eReader Zoomax display in the user guide.

96fda96


          Added an entry in the changes.md file.

dc30cf7

Author

florin-trutiu commented Dec 13, 2024 •

edited

Loading

Thanks for opening this PR @florin-trutiu !

Please fill out the PR template

Please remove this line from the userGuide:

nvda/user_docs/en/userGuide.md

Line 4284 in c17b423

* Note that the Zoomax is currently not supported without external drivers

Please add a changelog entry in changes.md noting that support for this braille device has been added

@seanbudd
Removed the mentioned line.
I noticed that the line was in the Huwanware section because of a confusion, there are two NLS eReader braille displays. one is manufactured by Humanware and one by Zoomax.
So to avoid confusion I appended the Humanware name to the NLS eReader device mentioned in the Humanware displays section.

In the user guide I have now added a section for the NLS eReader Zoomax braille display.

Collaborator

CyrilleB79 commented Dec 13, 2024

@florin-trutiu, you have added the template, but for completeness, could you please writing the required information in its sections rather than writing before it? Thanks!

SaschaCowley added the conceptApproved label

SaschaCowley reviewed

View reviewed changes

Member

SaschaCowley left a comment

Thanks for submitting this, @florin-trutiu! A lot of questions and stylistic questions on this to begin with

source/brailleDisplayDrivers/nlseReaderZoomax.py Outdated

Comment on lines 1 to 4

+              # -*- coding: UTF-8 -*-
+              # brailleDisplayDrivers/nlseReaderZoomax.py
+              # Description:
+              # NLS eReader Zoomax driver for NVDA.

Member

SaschaCowley Jan 10, 2025

Please update this to be in line with the copyright headers in the rest of NVDA's source.

Suggested change

      
            # -*- coding: UTF-8 -*-
          
            # brailleDisplayDrivers/nlseReaderZoomax.py
          
            # Description:
          
            # NLS eReader Zoomax driver for NVDA.
          
            # A part of NonVisual Desktop Access (NVDA)
          
            # Copyright (C) 2025 NV Access Limited, Florin Trutiu
          
            # This file is covered by the GNU General Public License.
          
            # See the file COPYING for more details.
          
            # NLS eReader Zoomax driver for NVDA.

source/brailleDisplayDrivers/nlseReaderZoomax.py Outdated

+              # NLS eReader Zoomax driver for NVDA.
+              import time
+              from typing import Union, List, Optional

Member

SaschaCowley Jan 10, 2025

Union type expressions are preferred over typing.Union. Likewise, T | None is preferred over typing.Optional[T]. And typing.List is a deprecated alias of list.

source/brailleDisplayDrivers/nlseReaderZoomax.py Outdated

+              from typing import Union, List, Optional
+              import braille
+              from hwIo import intToByte, boolToByte

Member

SaschaCowley Jan 10, 2025

You import hwIo at line 14.

source/brailleDisplayDrivers/nlseReaderZoomax.py Outdated

+              import bdDetect
+              import serial
+              TIMEOUT = 0.2

Member

SaschaCowley Jan 10, 2025

Please add units.

Suggested change

      
            TIMEOUT = 0.2
          
            TIMEOUT_SEC = 0.2

source/brailleDisplayDrivers/nlseReaderZoomax.py Outdated

+              TIMEOUT = 0.2
+              BAUD_RATE = 19200
+              CONNECT_RETRIES = 5
+              TIMEOUT_BETWEEN_RETRIES = 2

Member

SaschaCowley Jan 10, 2025

Please add units

Suggested change

      
            TIMEOUT_BETWEEN_RETRIES = 2
          
            TIMEOUT_BETWEEN_RETRIES_SEC = 2

source/brailleDisplayDrivers/nlseReaderZoomax.py

+              		if not isinstance(command, bytes):
+              			raise TypeError(typeErrorString.format("command", "bytes", type(command).__name__))
+              		arg = arg.replace(ESCAPE, ESCAPE * 2)

Member

SaschaCowley Jan 10, 2025

Can you please explain why you are doing this?

source/brailleDisplayDrivers/nlseReaderZoomax.py Outdated

+              	def _onReceive(self, data: bytes):
+              		if data != ESCAPE:
+              			log.debugWarning("Ignoring byte before escape: %r" % data)

Member

SaschaCowley Jan 10, 2025

Please use an f-string instead

source/brailleDisplayDrivers/nlseReaderZoomax.py Outdated

+              			# If not, we wish to know about it, allow decode to raise.
+              			self._deviceID = arg.decode("latin-1", errors="strict")
+              		elif command in KEY_NAMES:
+              			arg = sum(byte << offset * 8 for offset, byte in enumerate(arg))

Member

SaschaCowley Jan 10, 2025

Why not

Suggested change

      
            			arg = sum(byte << offset * 8 for offset, byte in enumerate(arg))
          
            			arg = int.from_bytes(reversed(arg))

source/brailleDisplayDrivers/nlseReaderZoomax.py

Comment on lines +216 to +236

+              			if arg < self._keysDown.get(command, 0):
+              				# Release.
+              				if not self._ignoreKeyReleases:
+              					# The first key released executes the key combination.
+              					try:
+              						inputCore.manager.executeGesture(InputGesture(self._deviceID, self._keysDown))
+              					except inputCore.NoInputGestureAction:
+              						pass
+              					# Any further releases are just the rest of the keys in the combination being released,
+              					# so they should be ignored.
+              					self._ignoreKeyReleases = True
+              			else:
+              				# Press.
+              				# This begins a new key combination.
+              				self._ignoreKeyReleases = False
+              			if arg > 0:
+              				self._keysDown[command] = arg
+              			elif command in self._keysDown:
+              				# All keys in this group have been released.
+              				# #3541: Remove this group so it doesn't count as a group with keys down.
+              				del self._keysDown[command]

Member

SaschaCowley Jan 10, 2025

I'm having a lot of trouble following what's going on here. Could you please comment a bit more on what's going on?

source/brailleDisplayDrivers/nlseReaderZoomax.py

+              		self.keyNames = names = []
+              		for group, groupKeysDown in keysDown.items():
+              			if group == LOC_BRAILLE_KEYS and len(keysDown) == 1 and not groupKeysDown & 0xF8:

Member

SaschaCowley Jan 10, 2025

Why 0xf8?

SaschaCowley requested changes

View reviewed changes

user_docs/en/changes.md Outdated

    
            @@ -27,6 +27,7 @@ To use this feature, "allow NVDA to control the volume of other applications" mu
          
              * Automatic language switching is now supported when using Microsoft Speech API version 5 (SAPI5) and Microsoft Speech Platform voices. (#17146, @gexgd0419)

              * NVDA can now be configured to speak the current line or paragraph when navigating with braille navigation keys. (#17053, @nvdaes)

              * In Word, the selection update is now reported when using Word commands to extend or reduce the selection (`f8` or `shift+f8`). (#3293, @CyrilleB79)

              * NLS eReader Zoomax driver has been added. Previously it was available only as a separate addon.

Member

SaschaCowley Jan 10, 2025

Suggested change

      
            * NLS eReader Zoomax driver has been added. Previously it was available only as a separate addon.
          
            * Support for the NLS eReader Zoomax braille display has been added. (#17509, @florin-trutiu)

user_docs/en/userGuide.md Outdated

+              The NLS eReader Zoomax device supports USB or bluetooth connections.
+              The Windows 10 and Windows 11 operating systems will automatically detect and install the necessary drivers for this display.
+              For computers where the Internet connection is disabled or not available, for the USB connection, a driver can be download from the USB to serial CH340 chip manufacturer  (https://www.wch-ic.com/downloads/CH341SER_EXE.html)

Member

SaschaCowley Jan 10, 2025

Suggested change

      
            For computers where the Internet connection is disabled or not available, for the USB connection, a driver can be download from the USB to serial CH340 chip manufacturer  (https://www.wch-ic.com/downloads/CH341SER_EXE.html)
          
            For computers where the Internet connection is disabled or not available, you can manually [download and install the USB to serial CH340 chip driver](https://www.wch-ic.com/downloads/CH341SER_EXE.html) to support this display over USB.

SaschaCowley marked this pull request as draft

January 13, 2025 00:31


          Made the requested changes.

e3001c8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conceptApproved