[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.

[EdKweon]

Note: English translation to follow:
@josephsl
안녕하세요. 관심 가져주셔서 감사합니다.

우선 사용설명서에 모델명, 통신방법, 명령어 세트 까지 작성해서 PR이 요청되야 한다면, device에 관한 코드를 좀 더 명확히 작성 후 review 신청하도록 하겠습니다.

• 추후 나올 힘스 제품 관련 PR: yes
• 곧 상용화 또는 한국 스크린 리더(예: 센스리더) 외 타 스크린 리더 지원 준비를 위한 PR: yes

두가지 사항 모두 맞습니다. 추후 PR사용설명서(user guide), 변경 내역(changelog) 작성 후 오작성 되거나 오해의 소지가 있는 부분이 있는지 요청드려도 될까요? NVDA PR을 하는 것이 처음이라 과정들이 아직 익숙치가 않습니다.

감사합니다.

English Translation:

First of all, if a PR is requested by writing the model name, communication method, and command set in the user manual, I will write the code for the device more clearly and then apply for review.

• PR related to Hims products to be released in the future: yes
• PR for preparation for upcoming commercialization or support for screen readers other than Korean screen readers (e.g. Sense Reader): yes

Both points are correct. After writing the PR user guide and change log in the future, may I ask if there are any incorrect or misleading parts? This is my first time doing NVDA PR, so I'm not familiar with the process yet.

Thanks.

[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