search

nvaccess / nvda

NVDA, the free and open source Screen Reader for Microsoft Windows
https://www.nvaccess.org/
Other
2.11k stars 637 forks source link

Support for serial Papenmeier displays #2679

Closed nvaccessAuto closed 9 years ago

nvaccessAuto commented 12 years ago

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 commented 12 years ago

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

nvaccessAuto commented 11 years ago

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

nvaccessAuto commented 11 years ago

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

nvaccessAuto commented 11 years ago

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

nvaccessAuto commented 11 years ago

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

nvaccessAuto commented 11 years ago

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

nvaccessAuto commented 11 years ago

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

nvaccessAuto commented 11 years ago

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

nvaccessAuto commented 11 years ago

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

nvaccessAuto commented 11 years ago

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

nvaccessAuto commented 11 years ago

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

nvaccessAuto commented 11 years ago

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

nvaccessAuto commented 12 years ago

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

nvaccessAuto commented 12 years ago

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

nvaccessAuto commented 12 years ago

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

nvaccessAuto commented 11 years ago

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 commented 11 years ago

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

nvaccessAuto commented 11 years ago

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

nvaccessAuto commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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

nvaccessAuto commented 11 years ago

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

nvaccessAuto commented 11 years ago

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 commented 11 years ago

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

nvaccessAuto commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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

nvaccessAuto commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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

nvaccessAuto commented 11 years ago

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

nvaccessAuto commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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

nvaccessAuto commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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

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