Support for serial Papenmeier displays

[nvaccessAuto]

Reported by jteh on 2012-09-19 10:52
Spun off #1265. Filing now so we don't forget.

A driver for serial Papenmeier displays was provided in #1265. #426 must first be implemented and then the driver updated accordingly. The driver also probably needs some other fixes similar to those suggested for the non-serial driver.
Blocked by #426, #1265

[nvaccessAuto]

Attachment PAPENMEIER_SERIAL.PY added by halim on 2012-09-24 06:57
Description:
NEW VERSION FIXED SOME PROBLEMS REPORTED IN 1265

[nvaccessAuto]

Comment 2 by halim on 2012-10-10 09:18
:ll attach a new version with some problems fixed.

• removed unused code
• fixed upperRouting gesture.

[nvaccessAuto]

Comment 3 by halim on 2012-10-14 06:35
Add new Version of Papenmeier serial Driver
Changes:

• catch exception "inputCore.NoInputGestureAction:"

[nvaccessAuto]

Comment 4 by jteh on 2012-10-26 03:01
Changes:
Milestone changed from None to 2013.1

[nvaccessAuto]

Comment 5 by halim (in reply to comment 4) on 2012-12-06 10:02
This is the driver updated to work with brailleports branch..

[nvaccessAuto]

Attachment papenmeier_serial.2.py added by halim on 2012-12-06 10:05
Description:
Papenmeier serial fixed

[nvaccessAuto]

Comment 6 by halim on 2012-12-06 11:53
New Version: added comment about used code from braillenote driver

[nvaccessAuto]

Attachment papenmeier_serial.3.py added by halim on 2012-12-06 11:55
Description:

[nvaccessAuto]

Comment 7 by halim on 2012-12-06 12:07
minor cleanups and added log message about used port

[nvaccessAuto]

Attachment papenmeier_serial.4.py added by halim on 2012-12-06 12:07
Description:

[nvaccessAuto]

Attachment papenmeier_serial.5.py added by halim on 2012-12-06 14:18
Description:

[nvaccessAuto]

Comment 8 by halim on 2012-12-06 14:22
last attachment fixes one small issue with papenmeier Tiny Displays.
removed also unused vaiable.

[nvaccessAuto]

Comment 9 by ragb on 2012-12-06 14:37
Quic code review and sugestions:

On the braille driver constructor

    def __init__(self, port="auto"):
    ```
I think it doesn't make sence to have port argument defaulting to "auto", sincew this driver doesn't support automatic port selectio, only serial.

Still on the constructor:
        portsToTry = (port,)
        for port in portsToTry:
            log.info("papenmeier_serial using port "+port)
            self._port = port
        #try to connect to braille display
        ...
        ```

I think you don't need the for cycle and !portsTotry, since you are only considering one serial port at a time, always. From my understanding, this driver only supports serial communication.This only makes sence when you have posible more than one port to try.

Moreover you should per haps put the log statement to debug level.

I don't now the protocol neither the display in question so I can't make any comments about the implementation.

[nvaccessAuto]

Comment 10 by halim on 2012-12-06 15:40
thx, will change it and post a new version soon.

[nvaccessAuto]

Attachment papenmeier-serial.py added by halim on 2012-12-07 06:53
Description:
new patcvh

[nvaccessAuto]

Attachment papenmeier_serial.6.py added by halim on 2012-12-07 06:57
Description:
sorry newest patch

[nvaccessAuto]

Comment 11 by halim on 2012-12-07 06:58
thx again see attached latest file

[nvaccessAuto]

Attachment papenmeier_serial.7.py added by halim on 2012-12-07 07:57
Description:

[nvaccessAuto]

Comment 12 by jteh on 2012-12-07 11:10
Some thought also needs to be given to how to handle the documentation for this. It supports different displays and I seem to recall it has less gesture bindings, so I believe it should have a separate documentation section to the existing driver unless anyone has a better idea that doesn't lead to confusion.

[nvaccessAuto]

Comment 13 by aliminator on 2012-12-07 16:02
This is the updated user documentation both in English and German.

[nvaccessAuto]

Comment 14 by halim on 2012-12-07 16:47
Aliminator: Thx for the user documentation update
I.ll look into the driver code next week. There seems to be an error in the display detection code.

@jteh: some of your suggestions from the review process of 1265 are included in current driver as well. Hope to get more useful feedback from you :-).

[nvaccessAuto]

Comment 15 by jteh on 2012-12-10 05:24
Code review:

brl_poll and brl_poll2 make a lot of read(1) calls. This is very inefficient. If at all possible, you should try to read several bytes at once. For example, if you know the length returned by a particular command, read that many bytes. At the very least, inWaiting() should give you the number of bytes currently waiting to be read, so you can read that many.

In the !BrailleDisplayDriver class:

The description attribute should have a translators comment above it:

    # Translators: Names of braille displays.

I missed this when reviewing the other Papenmeier driver. :)

In the constructor, you set self.numcells and you then have _get_numCells return this. This isn't necessary. Just use self.numCells directly and remove _get_numCells altogether.

nit: The constructor sets dic = -1 at the top of the loop, but then initialises this inside the loop anyway. I'd probably remove the one outside the loop.

      for baud in [38400](19200,):

nit: Use a tuple instead of a list here; i.e. parentheses instead of square brackets. Tuples are more lightweight unless you need the extra features of mutable lists.

                  if(baud == 19200): self._eab = False
                  if(baud == 38400): self._eab = True

nit: Can be simplified to just: self._eab = (baud == 38400). Alternatively, if you don't like that, at least make the second if an else.

Could you explain what's going on in executeGesture; i.e. changing "r1,left" to "left2"? At the very least, the subsequent ifs should probably be elifs. Also, I'm wondering whether this code would be better in the gesture class.

Consider changing the multiple if checks in brl_keyname into a dict lookup, though whether you do this is up to you.

As always, thanks for your great work.

[nvaccessAuto]

Comment 16 by halim on 2012-12-10 12:13
Thx for your review: Here some notes:

• brl_poll: I was able to change it to read more than one byte in the loop. However using same aproach for poll2 doesn't work. The loop expects 10 bytes to read but it can't be read at a time (I don't know why). When changing this, I don't get all keys and the driver can't detect release of these keys reliable.
• in executegesture the changes are for implementing same functionality for all Papenmeier Displays. Eg: Braillex Tiny has no right2,left2,up2,down2 keys. These keys were used for object navigation. The code emulates right2 by pressing r1+right.
  Elifs added in same Method.
  I.ll attach a new Version soon.
  Thx again for you help!!!

[nvaccessAuto]

Comment 17 by jteh (in reply to comment 16) on 2012-12-14 10:06
Replying to halim:

• brl_poll: I was able to change it to read more than one byte in the loop. However using same aproach for poll2 doesn't work. The loop expects 10 bytes to read but it can't be read at a time (I don't know why). When changing this, I don't get all keys and the driver can't detect release of these keys reliable.

Strange. Are you saying you get more or less than 10 bytes from read(10)?

• in executegesture the changes are for implementing same functionality for all Papenmeier Displays. Eg: Braillex Tiny has no right2,left2,up2,down2 keys. These keys were used for object navigation. The code emulates right2 by pressing r1+right.

I wonder whether it might be less confusing to just map r1+right to the same script in the gesture map?

[nvaccessAuto]

Comment 18 by halim on 2012-12-17 07:55
here is the new version: The bigest change:
reading improoved in both brl_poll methods. The documentation update will be attached soon. TODO: fix autodetect code and I.ll merge the brl_poll methodss into one brl_poll method.

[nvaccessAuto]

Attachment papenmeier_serial.8.py added by halim on 2012-12-17 07:57
Description:

[nvaccessAuto]

Attachment papenmeier_serial.9.py added by halim on 2012-12-18 07:47
Description:

[nvaccessAuto]

Comment 19 by halim on 2012-12-18 07:52
Next Version:

• merged brl_poll method in one method
• moved special keybindings for displays without EAB like tiny into gesture map
• Some minor cleanups
  It would be nice if some users could test it as well :-).

[nvaccessAuto]

Comment 20 by aliminator on 2012-12-18 08:41
This is the updated user documentation in German and English. Ignore former diff.

[nvaccessAuto]

Attachment userguide.diff added by aliminator on 2012-12-18 08:42
Description:
user guide bzr diff

[nvaccessAuto]

Comment 21 by halim on 2012-12-20 07:13
So that's it, If nobody else reports an error, this you can merge it now!
Thx a lot.

[nvaccessAuto]

Attachment papenmeier_serial.py added by halim on 2012-12-20 07:13
Description:

[nvaccessAuto]

Comment 22 by aliminator on 2013-01-07 10:02
James, have you reviewed the code already?

Could you please merge it into main if there are no more points to fix?

[nvaccessAuto]

Comment 23 by jteh on 2013-01-07 11:19
Sorry. I haven't had a chance to review it due to holidays and other work. I'll try to do it in the coming days. Feel free to ping me again in a couple of weeks if I haven't commented here. Thanks.

[nvaccessAuto]

Comment 24 by jteh on 2013-01-09 01:13
Having reviewed the code, I have two questions:

1. Why are you using comma as a separator for combined keys instead of plus; e.g. r1,right instead of r1+right? If you use plus, you do have to make sure they're in Python set order, but it also means that r1+right is the same as right+r1.
2. For some displays, there is a reportf key. I'm guessing reportf isn't the name of the key on the display. Does this need to be clarified or is that actually what the key is called?

[nvaccessAuto]

Comment 25 by halim on 2013-01-09 08:42
Hi,

1. We wanted to implement r1,right != right,r1. Using plus had some unwanted effects so we decided to use komma instead.
2. reportf key is not labeled on the braille display but using nvda's input help should clarify the function.
   No need for adding more info here.
   Thx for your review!!!

[nvaccessAuto]

Comment 26 by jteh (in reply to comment 25) on 2013-01-09 09:53
Replying to halim:

1. We wanted to implement r1,right != right,r1. Using plus had some unwanted effects so we decided to use komma instead.

Yeah. Using plus means the order is indeterminate. I never thought there'd be a case where it mattered. If it does, comma is fine.

1. reportf key is not labeled on the braille display but using nvda's input help should clarify the function.

How does the User Guide for the display refer to it? How do other screen readers refer to it? The problem is that the documentation mentions reportf as a key binding, but reportf seems NVDA specific and a user won't know where to find it.

[nvaccessAuto]

Comment 27 by aliminator on 2013-01-09 12:24
As far as we know there are neither standardized labels nor standards how to label braille keys.
In JAWS, labels are assigned from left to right and it seems to be that the labels were choosen randomly.
Nevertheless, I could include a note in the documentation mentioning this.

[nvaccessAuto]

Comment 28 by jteh on 2013-01-09 22:13
My confusion is that you've named the other keys, for example, l1, l2, r1 and r2, so reportf seems odd because it describes the function, not the placement. Where is the reportf key located relative to the rest?

[nvaccessAuto]

Comment 29 by halim on 2013-01-10 06:53
Any suggestions what to do here?
In my opinion we should reflect this in the Documentation.
It only affects two very old braille displays!
We could also rename that key any suggestions for new label

[nvaccessAuto]

Comment 30 by aliminator on 2013-01-10 08:01
We could rename it to rf and include it in the key list.

[nvaccessAuto]

Comment 31 by jteh on 2013-01-10 08:13
Where is the key located in relation to the others?

I guess I'm happy for it to be left as reportf if there's really no better name. We can change it if it confuses users.

[nvaccessAuto]

Comment 32 by aliminator on 2013-01-10 08:26
It depends on the braille display. Depending on the model the key is located differently.
As for the Tiny display the key is the first one on the rightand side.
The source
self._keymap = ['l2', 'left', 'up', 'r2', 'dn', 'right', 'r1', 'reportf']('l1',)

clarifies this.
It is not possible to create a unified schema due to differences of key amount and key structure.

[nvaccessAuto]

Comment 33 by jteh on 2013-01-11 01:34
Merged in 3bebcc3 with changes to the documentation by me.

Thanks for your work once again.
Changes:
State: closed