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

Fix Seika Notetaker buttons #12598

Merged
merged 11 commits into from
Jul 1, 2021
Merged

Fix Seika Notetaker buttons #12598

merged 11 commits into from
Jul 1, 2021

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Jun 30, 2021

Link to issue number:

None

Summary of the issue:

A spec for the braille device, seika notetaker, was provided here. The driver was hard to maintain and understand. An issue was raised in that comment which noted that keys that did not exist and were not labelled were being checked for.

Description of how this pull request fixes the issue:

The driver and testing has been updated to match it using examples from the spec.

Testing strategy:

Unit tests have been added which follow the spec:

  • note, it seems a byte in the spec was wrong, this has been noted in the transcribed markdown

Known issues with pull request:

None

Change log entries:

Bug fixes

- Improvements to the Seika Notetaker driver

Code Review Checklist:

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual testing.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers

@seanbudd seanbudd requested a review from a team as a code owner June 30, 2021 08:04
@seanbudd seanbudd changed the title Add seika notetaker spec Add seika notetaker specification Jun 30, 2021
@seanbudd seanbudd requested a review from feerrenrut June 30, 2021 08:04
@AppVeyorBot
Copy link

  • PASS: Translation comments check.
  • PASS: Unit tests.
  • FAIL: Lint check. See test results for more information.
  • PASS: System tests.
  • Build (for testing PR)

See test results for failed build of commit 2ff7f421e0

@seanbudd
Copy link
Member Author

@moyanming - would you like to test this PR build and see if the issue with buttons is still an issue?

@moyanming
Copy link
Contributor

@moyanming - would you like to test this PR build and see if the issue with buttons is still an issue?

Sure, I will test the PR build right now.

source/brailleDisplayDrivers/seikantk.py Outdated Show resolved Hide resolved


def _getRoutingIndexes(routingKeys: bytes) -> Set[int]:
return {i * 8 + j for i in range(len(routingKeys)) for j in range(8) if routingKeys[i] & (1 << j)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is a little harder, I suspect it could be simplified. There are likely only so many values for routingKeys, I'd suggest a brute force unit test that tests them all (eg that the new algorithm gives the same out put as this one). Once simplified the test can probably be deleted.

Probably something like:

Suggested change
return {i * 8 + j for i in range(len(routingKeys)) for j in range(8) if routingKeys[i] & (1 << j)}
routingKeyBitFlags = routingKeys
bitValues = [1 << i for i in range(8)]
routingIndexes = []
routingIndexTest = 0
for byte in routingKeyBitFlags:
for bitValue in bitValues:
if bitValue & byte:
routingIdexes.append(routingIndexTest)
routingIndexTest += 1

Or if you want to still do it in a list comprehension style

Suggested change
return {i * 8 + j for i in range(len(routingKeys)) for j in range(8) if routingKeys[i] & (1 << j)}
bitsPerByte = 8
bitValues = [1 << i for i in range(bitsPerByte)]
return {
bitIndex + (byteIndex * bitsPerByte)
for bitIndex, bitValue in enumerate(bitValues)
for byteIndex, bitFlags in enumerate(routingKeys)
if bitValue & bitFlags

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a different approach here, closer to the approach suggested above. Let me know what you think.

The current testing should provide enough coverage here I think, unless Nippon releases a seika notetaker device with a very large number of keys.

tests/unit/test_brailleDisplayDrivers.py Show resolved Hide resolved
source/brailleDisplayDrivers/seikantk.py Show resolved Hide resolved
source/brailleDisplayDrivers/seikantk.py Outdated Show resolved Hide resolved
source/brailleDisplayDrivers/seikantk.py Show resolved Hide resolved
@seanbudd seanbudd requested a review from feerrenrut July 1, 2021 02:55
@seanbudd
Copy link
Member Author

seanbudd commented Jul 1, 2021

@moyanming - is it okay that we are publishing the pdf of this driver within our repository (and the transcribed version)?

@moyanming
Copy link
Contributor

@moyanming - is it okay that we are publishing the pdf of this driver within our repository (and the transcribed version)?

Hi, @seanbudd
Please do not publish this pdf (Seika Notetaker protocol.pdf) file because there have:
Copyright © Seika Braille
Please note that the Seika Notetaker protocol must be only used on the Seika devices.

Regards

@moyanming
Copy link
Contributor

@moyanming - would you like to test this PR build and see if the issue with buttons is still an issue?

Hi, @seanbudd
We have tested this PR build and works fine now, includes the Braille display and key functions are all work fine.
Thanks for your great work.

@seanbudd seanbudd changed the title Add seika notetaker specification Fix SeikaNotetaker buttons Jul 1, 2021
@seanbudd seanbudd changed the title Fix SeikaNotetaker buttons Fix Seika Notetaker buttons Jul 1, 2021
@nvaccess nvaccess deleted a comment from AppVeyorBot Jul 1, 2021
@seanbudd seanbudd merged commit 462643b into master Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants