Skip to content
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

Add support for new braille display from Nattiq #10778

Merged
merged 20 commits into from Mar 20, 2020
Merged

Conversation

@m-noman
Copy link
Contributor

m-noman commented Feb 11, 2020

Link to issue number: /

Summary of the issue: /

Description of how this pull request fixes the issue: /

Testing performed: Testing is done with the Nattiq Braille Display (USB)

Known issues with pull request: /

Change log entry:

Section: New features, Changes, Bug fixes

Adding support for the new Braille display from Nattiq Technologies
http://www.nattiq.com/en/node/1694

m-noman added 2 commits Feb 11, 2020
Nattiq Braille display support
@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Feb 11, 2020

See test results for failed build of commit 2fcbea94db

@AppVeyorBot

This comment has been minimized.

@AppVeyorBot

This comment has been minimized.

@AppVeyorBot

This comment has been minimized.

@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Feb 11, 2020

See test results for failed build of commit ab23fe6b3b

@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Feb 12, 2020

See test results for failed build of commit 5d7479a19c

@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Feb 12, 2020

See test results for failed build of commit 9bb6d15688

@m-noman

This comment has been minimized.

Copy link
Contributor Author

m-noman commented Feb 13, 2020

After further checking and testing, it seems the driver is stable and good to merge,
so that NVDA can officially have the support for the Nattiq nBraille systems.
@leonardder @feerrenrut @Adriani90
if someone can review,
Thanks.

Copy link
Collaborator

leonardder left a comment

Overall, this looks pretty nice.

Could you say anything about how widely this display is used?

source/brailleDisplayDrivers/nattiqbraille.py Outdated Show resolved Hide resolved
source/brailleDisplayDrivers/nattiqbraille.py Outdated Show resolved Hide resolved
source/brailleDisplayDrivers/nattiqbraille.py Outdated Show resolved Hide resolved
source/brailleDisplayDrivers/nattiqbraille.py Outdated Show resolved Hide resolved
source/brailleDisplayDrivers/nattiqbraille.py Outdated Show resolved Hide resolved
source/brailleDisplayDrivers/nattiqbraille.py Outdated Show resolved Hide resolved
source/brailleDisplayDrivers/nattiqbraille.py Outdated Show resolved Hide resolved
source/brailleDisplayDrivers/nattiqbraille.py Outdated Show resolved Hide resolved
source/brailleDisplayDrivers/nattiqbraille.py Outdated Show resolved Hide resolved
@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Feb 15, 2020

See test results for failed build of commit e0c10840c5

@m-noman

This comment has been minimized.

Copy link
Contributor Author

m-noman commented Feb 15, 2020

@leonardder
Thank you for the feedback,
This Braille display is actually New.
It was released around December last year, so normally it is rather new and does not yet have a huge user-base, but I expect it may get more exposure due to its low price comparing to other brands.
It also have support for other screen readers like HAL/Supernova, and so this is how I created this driver. All testing I am doing is on the braille display unit.

I edited out the comments, I am not sure if it is now better, or need more comments/explanations?
I will keep testing and will await the feedback for the changes.

Thank you.

@m-noman m-noman requested a review from leonardder Feb 15, 2020
source/brailleDisplayDrivers/nattiqbraille.py Outdated Show resolved Hide resolved
source/brailleDisplayDrivers/nattiqbraille.py Outdated Show resolved Hide resolved
source/brailleDisplayDrivers/nattiqbraille.py Outdated Show resolved Hide resolved
source/brailleDisplayDrivers/nattiqbraille.py Outdated Show resolved Hide resolved
source/brailleDisplayDrivers/nattiqbraille.py Outdated Show resolved Hide resolved
source/brailleDisplayDrivers/nattiqbraille.py Outdated Show resolved Hide resolved
source/brailleDisplayDrivers/nattiqbraille.py Outdated Show resolved Hide resolved
@leonardder

This comment has been minimized.

Copy link
Collaborator

leonardder commented Feb 17, 2020

Are there actually any docs about this display, i.e. this driver is implemented using serial communication, but is this a usb to serial converter? If so, you might want to add the vid and pid of the converter to bdDetect in order for automatic braille display detection to work.

@m-noman

This comment has been minimized.

Copy link
Contributor Author

m-noman commented Feb 17, 2020

Are there actually any docs about this display, i.e. this driver is implemented using serial communication, but is this a usb to serial converter? If so, you might want to add the vid and pid of the converter to bdDetect in order for automatic braille display detection to work.

I don't think there are things that are publicly available.
but the Braille display is not using a usb to serial converter, rather it is using a hardware based usb controller. It is based on Atmel micro-controllers. Though its PID/VID will be kind of generic as they are for the Atmel micro-chip, but I checked the bdDetect.py file and it is possible to add them there for auto-detection. Though manually, the braille display seems to works fine, on starts/reboots/ and even re-installs ( not sure how? or this might be normal )

@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Feb 17, 2020

See test results for failed build of commit 049ea2751c

@m-noman

This comment has been minimized.

Copy link
Contributor Author

m-noman commented Feb 17, 2020

@leonardder
I just noticed that now NVDA uses python 3 instead of 2.
I guess that was the reason why it worked for me with either bytes or strings.
Now when I updated to python 3 and VS2019, I can not write strings like before,
and because the unit expect a string encoded value, so I will just use the dash and cell data as strings then encode the output for serial.write().
I tried it out and it seems to works fine. I will await your review.
Also the auto-detect works now.

Thank you.

m-noman and others added 2 commits Feb 17, 2020
Co-Authored-By: Leonard de Ruijter <leonardder@users.noreply.github.com>
@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Feb 17, 2020

See test results for failed build of commit 93a1d731a1

@m-noman m-noman requested a review from leonardder Feb 19, 2020
Copy link
Collaborator

leonardder left a comment

I think the code is nice as it is now, thanks for your contribution.

Next steps would be updating the user guide

  • Add a paragraph about the display to chapter 15 (supported braille displays) of the user guide: userDocs\en\userGuide.t2t
  • Add the driver to the Displays supporting automatic detection in the background section
added Nattiq nBraille displays description.
@m-noman

This comment has been minimized.

Copy link
Contributor Author

m-noman commented Feb 19, 2020

I think the code is nice as it is now, thanks for your contribution.

Next steps would be updating the user guide

* Add a paragraph about the display to chapter 15 (supported braille displays) of the user guide: userDocs\en\userGuide.t2t

* Add the driver to the `Displays supporting automatic detection in the background` section

I added the display to the list of automatic supported displays, and also a small paragraph with the keys for the display.
I put it above BRLTTY, I do not think this needs to be ordered by alphabetic? because the others are not in order. I only choose it to be above BRLTTY, as that is not a display but rather a protocol/program, so technically, it is now the last ( if the order was meant to be from older to newer )
@leonardder if there are any problems, or it needs to have more information, I will try to add more. but mostly I do not think we need to have more than this, as the link I attached ( in the very first post ) have all the technical details etc.

Thanks.

@m-noman m-noman requested a review from leonardder Feb 23, 2020
Copy link
Collaborator

leonardder left a comment

Three small things:

  • Please start a new line after every sentence in the user guide
  • manufacturer website > manufaturer's website
  • Also, see my comment about missing assignments.
user_docs/en/userGuide.t2t Outdated Show resolved Hide resolved
user_docs/en/userGuide.t2t Show resolved Hide resolved
added the previous/back assignment, and edited the first line spelling error.
@m-noman m-noman requested a review from leonardder Feb 24, 2020
@leonardder leonardder requested a review from michaelDCurran Mar 4, 2020
Copy link
Contributor

feerrenrut left a comment

This looks fine to me, though I am unable to test it. I will wait for a review from @michaelDCurran

The release process has already started for 2020.1 I propose this goes into the 2020.2 release.

@feerrenrut feerrenrut added this to the 2020.2 milestone Mar 9, 2020
@michaelDCurran

This comment has been minimized.

Copy link
Contributor

michaelDCurran commented Mar 9, 2020

@feerrenrut to be clear, obviously I can't test this either as I don't have access to one of these displays.

@feerrenrut feerrenrut merged commit 8ae6dca into nvaccess:master Mar 20, 2020
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
feerrenrut added a commit that referenced this pull request Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.