[Hims] Add SPP connection [BrailleEdge]

[EdKweon]

Link to issue number:

Summary of the issue:

Support for spp connection with a Braille Edge device.

Description of user facing changes

Support for all BrailleEdge devices NVDA's usb connection.

Description of development approach

Support for Braille Edge device SPP connection in the Hims display. Only BrailleEdge devices with changed hardware chips can connect to SPP(Not use Hims Driver). Original BrailleEdge should use Hims driver to connect.

Testing strategy:

How have you tested to ensure that your change works as intended?
yes.
Have you ensured testing coverage across all supported operating systems?
yes.
Have you considered possible regressions (related features or behaviours that may break)?
yes.

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.

[LeonarddeR]

Could you please elaborate on how you tested this? I think this pr is not complete yet. You might need to add an extra model class and add it to the modelMap.

[EdKweon]

Could you please elaborate on how you tested this? I think this pr is not complete yet. You might need to add an extra model class and add it to the modelMap.

First of all I'm sorry for not checking the code more accurately.
I tested that new device can get NVDA braille data in connectivity app. And check unit test using rununittests.bat.

I agree with you that new model class and that should be added in the modelMap. I'll add your feedback code. Any other things I should check?

[LeonarddeR]

Apart from the model map, I guess this is ok now.

[EdKweon]

I added device to model map. Thank you for your review.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• PASS: System tests (tags: installer NVDA).
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/r86rh2bwkr617hpn/artifacts/output/nvda_snapshot_pr16033-30671,cf7fc13e.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 1.0,
  INSTALL_END 1.0,
  BUILD_START 0.0,
  BUILD_END 28.0,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.3,
  TEST_START 0.0,
  TEST_END 16.7,
  FINISH_END 0.2

See test results for failed build of commit cf7fc13e36

[EdKweon]

I added blank line because of Lint error. I don't know why this happend. Please review my code.

[seanbudd]

Hi @EdKweon,

It appears this code is just for internal testing for your organisation, rather than for wider NVDA users.
As such, I don't think we can merge this until it is a complete feature targeted at wider NVDA users.

If you need signed builds to test with, we are happy to create them - I've created a signed build from this PR - download.

You can also create your own self signed builds for testing if you don't want to wait on us.

Feel free to push further changes to this PR and ask us to create another signed build if you need to make any further changes for testing.
When this PR is ready for the public, feel free to mark it as ready for review to convert it from a draft.

Thanks,
Sean

[michaelDCurran]

@EdKweon I think there is some confusion here. Could you please clarify if the BrailleEmotion is a real-world Braille display on the market, or it is just an emulated test device? If it is a real Braille display on the market, of course we will accept this PR.
If it is a device / emulator purely for testing, then we cannot take this code.
Apologies if we have miss-understood the intention of this PR.

[EdKweon]

@michaelDCurran
This is a device that will actually be sold on the market. Of course, the code for that device may be PR with additional requirements and testing. It is not simply a device for testing.

[seanbudd]

Thanks for more information, please let us know when testing is complete and the pull request is complete

[EdKweon]

@seanbudd
I think first PR and test is complete.

[josephsl]

안녕하세요,

알겠습니다. 만약 시간이 있으시면 NVDA 한국 사용자들과 NVDA 개발 및 이 PR에 대해서 이야기를 나누어 보시면 좋겠다고 생각합니다(참고로 저는 미국에 있고 현재 대학원생이라 자세한 코드 리뷰는 해드릴 수 없지만). 도움이 되셨으면 합니다.

English: Got it. If you have time, I think it would be great to talk to Korean NVDA users about the screen reader devleopment process and this PR (I'm a graduate student in United States so can't review the PR in detail at the moment). Hope this helps.

Tanks.

[EdKweon]

This may be misleading from what I said before. I will summarize the current situation in the article below. Currently, the device I am trying to add is almost same as the BrailleEdge product. However, hardware was little bit changed. So the product is connected to nvda via serial (not use Hims Driver). Therefore, there is no key to add or modify separately in the user_docs document.

Thanks for members.

[seanbudd]

Can you please also document this device in the user guide?

[EdKweon]

@seanbudd
There are no other keys to add or modify in user_guide. Because this is completely same with BrailleEdge except not using Hims driver(hardware chip replacement).

[seanbudd]

This adds support for a new braille device right - eMotion? We like to include all of the supported braille devices in the user guide.

[EdKweon]

@seanbudd
What I said before was incorrect information. It's just a BrailleEdge not a new device Braille eMotion.

[seanbudd]

Thanks for explaining

[seanbudd]

can you please update the PR description to be more accurate? thanks

[EdKweon]

@seanbudd
Thanks. I updated PR description.

[seanbudd]

Great - when this is merged it will become available in NVDA alpha releases. As long as no major issues are found it will be in NVDA 2024.2

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/2nx23fjsjff2vn7a/artifacts/output/nvda_snapshot_pr16033-30877,5527eaa9.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 1.0,
  INSTALL_END 1.0,
  BUILD_START 0.0,
  BUILD_END 28.4,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.4,
  TEST_START 0.0,
  TEST_END 10.4,
  FINISH_END 0.1

See test results for failed build of commit 5527eaa9cf