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

Driver for Tivomatic Caiku Albatross 46/80 displays #13045

Merged
merged 164 commits into from
Jan 18, 2023

Conversation

burmancomp
Copy link
Contributor

@burmancomp burmancomp commented Nov 9, 2021

Link to issue number:

Summary of the issue:

No support for Tivomatic Caiku Albatross 46/80 braille display models.

Description of how this pull request fixes the issue:

General

Different to many other displays, Albatross models do not wait for the driver to send a query of their presence to the device. Instead, the device continuously sends initialization packets until driver requests to them quit, as the connection has been established.

These displays also expect regular data packets, with frequency of at least one every 2 seconds. Otherwise they fall back to "wait for connection" state and start sending the initialization packets until the driver sends a quit packet.

Similarly, if the user enters the device internal menu, after exiting, initialization packets are continuously sent until driver sends quit packet.

Init packets are also sent when device is powered off and then on.

Display init packets consist of two bytes: the first one is \xff which tells that this is an init packet. The second one is a settings byte which contains display settings like length of display and number of status cells. The most meaningful setting is the length of display. Other settings contained by settings byte can be regarded as notes to screenreader, and it is screenreader or driver job to use them when applicable. For example, there are no separate status cells in the device but if screenreader supports using status cells, it can be notified to use them by settings byte.

The settings byte can be anything between \x00 and \xff. Thus it could be the same as init byte.

It is possible that settings byte may have same value with any display buttons. Because init packets may be sent also during session (user exits display internal menu for example), it is essential to know if byte is button press or part of init packet.

When display is in "wait for connection" state, it sends init packets continuously. As such, there may be hundreds of bytes to handle. There are several rx buffers between device and driver which seems to cause situation that all data cannot be read with one read operation. It cannot be known when all data has been read. This is the case with init packets but also with key combinations.

There are other devices with same PID&VID. When automatic braille display detection is used, other displays with same PID&VID are tried before Albatross. Those drivers try to send queries to the port to detect their own displays. These queries may cause Albatross to send unexpected init packets which in turn could disturb this driver - it could get inappropriate settings byte. This is tried to prevent by resetting I/O buffers so that strange packets would be discarded.

If however, there are still strange init packets, Albatross should be manually selected from the display list.

To reduce complexity of data handling this driver accepts only settings bytes <= \xfe. From user perspective this means that with 80 model user can ask screenreader to use at most 14 status cells when without limitation user could ask 15. Limitation is applied only if user has switched all other settings to values that cause value of byte to be \xff. From settings byte for status cells there are reserved the most right 4 bits. Limitation does not affect on 46 cells model because the most left bit of byte defines the length of display (0 for 46 and 1 for 80 cells model).

Note: This driver ignores status cells related settings because NVDA does not use status cells at the moment.

Driver requirements

  • support for both 46 and 80 cells models
  • support for both automatic and manual detection
  • when connected allows device plugging out and in, and power switching off and on so that display content is up-to-date and buttons work as expected after these operations.

Design

Driver has modular structure:

constants.py contains all the constant definitions, for example button values and names.

driver.py is the main part of the code. It implements BrailleDisplayDriver class which is in response of all read and write operations. It also takes care to format data which is meant to be displayed on the braille line.

Important main functions of BrailleDisplayDriver are:

  • _readHandling; performs connecting/reconnecting to the device, and all read operations during connection
  • _somethingToWrite; performs all write operations to the display
  • display; prepares data to be displayed on the braille line

In gestures.py numeric values of pressed buttons are interpreted as gestures so that they can be forwarded to NVDA input system.

_threads.py defines two threads. Thread called albatross_read calls BrailleDisplayDriver _readHandling function when it gets signaled that port has data to be read. Idea is somewhat similar to hwIo onReceive function. For deeper read and write operations control own thread was implemented. In addition, it calls _readHandling if it detects port problems so that _readHandling can try to reconnect. Albatross_read thread sleeps most of the time because user does not press buttons continuously, and connection problems occur rarely.

The second thread is timer which checks periodically (after approximately 1.5 seconds) that some data has been sent to display so that it keeps connected. _SomethingToWrite function updates time of last write operation. If there is at least 1.5 seconds from last write operation, display is feeded data packet containing START_BYTE and END_BYTE which enclose data which is sent to be displayed on the braille line.

Testing strategy:

Daily use testing in Windows 7/10.

Known issues with pull request:

Braille Extender addon may not be compatible with this driver at the moment.

Change log entries:

New features
"Added support for Tivomatic Caiku Albatross 46/80 braille displays.
Changes
Bug fixes
For Developers

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

…ic detection. English and Finnish user guides, and bdDetect.py updated.
@burmancomp burmancomp closed this Nov 9, 2021
@burmancomp burmancomp reopened this Nov 9, 2021
@burmancomp burmancomp marked this pull request as ready for review November 9, 2021 18:55
@burmancomp burmancomp requested review from a team as code owners November 9, 2021 18:55
@lukaszgo1
Copy link
Contributor

@burmancomp Please fill out the PR template and revert changes to the Finnish user guide edits to the translated documentation are handled separately via SVN.

@AppVeyorBot
Copy link

See test results for failed build of commit 1bc7e5c706

@burmancomp burmancomp marked this pull request as draft November 12, 2021 21:56
@burmancomp
Copy link
Contributor Author

No idea how to modify information on pr template.

@burmancomp Please fill out the PR template and revert changes to the Finnish user guide edits to the translated documentation are handled separately via SVN.

@codeofdusk
Copy link
Contributor

@burmancomp Click the context menu button (labelled by NVDA as "show options") above the first comment in this thread, then click "edit comment" from the revealed menu. In the edit field:

  • Delete "…ic detection. English and Finnish user guides, and bdDetect.py updated." on the first line.
  • Fill in the template according to the comments provided.

@burmancomp burmancomp marked this pull request as ready for review November 12, 2021 23:29
Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

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

User Guide changes look fine. Is there a website for the display manufacturer or its documentation? If so, I'd request adding a link (I didn't find one from a quick look myself, and your use of the past tense "...these displays were manufactured by..." suggests there might not be much online, so that's fine.

Note: Just approving for the user guide change, check the requested changes from others.

@burmancomp
Copy link
Contributor Author

Unfortunately there is no online resources anymore.

@lukaszgo1
Copy link
Contributor

@burmancomp Would you mint resolving conflicts here? @michaelDCurran Is this on your radar?

@seanbudd seanbudd self-requested a review February 16, 2022 23:07
@seanbudd seanbudd self-assigned this Mar 17, 2022
@feerrenrut feerrenrut added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Mar 18, 2022
@burmancomp
Copy link
Contributor Author

@seanbudd,

Maybe this could be reviewed in the near future?

Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Sorry for the late review, I have picked up a few more trivial changes.

Other changes include:

- When connection was disabled, numCells was set to 0, and it was then used
in efforts to block some useless write operations. Due to changes in
braille.py, setting numCells to 0 caused issue where display was not
updated properly when switching device off and back on. In this context
numCells is replaced with _disabledConnection boolean variable.
- Modified comment in _threading.py.
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Once all these outstanding review comments are resolved, I think this can be merged.
Thanks for all your hard work on this @burmancomp

@burmancomp
Copy link
Contributor Author

Once all these outstanding review comments are resolved, I think this can be merged. Thanks for all your hard work on this @burmancomp

Thank you @seanbudd. This is my first open source and python work and as such very useful.

@seanbudd seanbudd merged commit ffc8fab into nvaccess:master Jan 18, 2023
@nvaccessAuto nvaccessAuto added this to the 2023.1 milestone Jan 18, 2023
@burmancomp burmancomp deleted the albatrossDriver branch January 19, 2023 08:43
michaelDCurran pushed a commit that referenced this pull request Mar 1, 2023
As part of Pull request #14531, I ordered display order in bdDetect alphabetically. However, it seems that querying Albatross first has a major negative impact on detecting other displays using the same Serial to USB converter (see Pull request #13045)

Description of user facing changes
Handy Tech displays such as the Modular Evolution instantly auto detect again.

Description of development approach
Restored the order, Albatross is last in the dictionary again. I'm pretty unhappy with how these displays work, but it looks like it's a design thing in the displays itself.
seanbudd pushed a commit that referenced this pull request Mar 7, 2023
Follow-up of #13045

Summary of the issue:
Driver sent to Albatross display byte string b"\xfe\xfd\xfe\xfd" to tell device that there is active connection and it should not send init packets anymore. Byte string contained two establishment packets, and one should be enough. It should not cause harm to send two packets because display ignores latter one but it is redundant anyway.

In stead of b"\xfe\xfd\xfe\xfd" driver sends now b"\xfe\xfd" to display.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants