Papenmeier Braille driver support

nvaccessAuto commented 13 years ago

Reported by Casalino on 2010-12-08 13:25 Here my solution to support braillex II series braille devices. They are divided in revision A and revision B models. This driver should support completely all revision A devices as braillex II 40El as I had at disposal one model of them. Revision B should work except keys handling that requires a different protocol that at this time I can not test. I hope to have soon at my disposal a revision B model to complete this work. The native usb drivers have to be already installed. Blocking #2679

nvaccessAuto commented 13 years ago

Attachment BraillexNvdaDriver_v0.5.zip added by Casalino on 2010-12-08 13:26 Description:

nvaccessAuto commented 12 years ago

Attachment papenmeier-driver.zip added by aliminator on 2012-08-24 12:55 Description:

nvaccessAuto commented 12 years ago

Attachment papenmeier.py added by halim on 2012-09-11 09:00 Description: updated Papenmeier usb braille driver

nvaccessAuto commented 12 years ago

Attachment userguide.diff added by halim on 2012-09-11 10:26 Description:

nvaccessAuto commented 12 years ago

Attachment papenmeier.2.py added by aliminator on 2012-09-17 15:47 Description:

nvaccessAuto commented 13 years ago

Comment 1 by winman3000 on 2011-06-17 20:48 Is it possible to add this driver into NVDA? I mean can you commit this driver to NVDA?

nvaccessAuto commented 13 years ago

Comment 2 by jteh on 2011-06-19 23:51 Somehow, the files in this ticket got overlooked. My apologies.

Unfortunately, this driver needs to be updated for the new braille input gesture code before it can be added. I can't do this because i don't have a display to work with, so someone with a display and the required expertise will need to update it.

nvaccessAuto commented 13 years ago

Comment 3 by drein on 2011-06-23 10:45 Well, until now, nobody was interested on this ticket. Nobody told us if the driver worked well, if there were problems, etc. So we stopped working. If you think it is useful, Gianluca can work on it again, but some users should provide feedback because now we haven't any more PapenMeier display., and we should ask it another time.

nvaccessAuto commented 13 years ago

Comment 4 by schulle4u (in reply to comment 3) on 2011-08-01 08:41 I'm interested in this driver, unfortunately it doesn't seem to work for my braillex el80 (Rev. 117). The device doesn't appear in the list of available braille displays in NVDA 2011.1.1. However after copying the .py file to the brailleDisplayDrivers in NVDA's userdata, a .pyo file is created after starting NVDA. Using Win7 64 bit. This is all feedback I can give at the moment since I'm not a programmer.

nvaccessAuto commented 13 years ago

Comment 5 by Bernd on 2011-11-11 15:48 @Drein

There are some german persons who want to test this driver. Could you say on which nvda version this driver worked? Could you modify so the driver will work with version 2011.3? In this case I'm sure that German users will benefit from your and Gianlucas work and test it. In this case we'd have to know wheather drivers have to be installed and so on.

nvaccessAuto commented 12 years ago

Comment 6 by jteh (in reply to comment 4) on 2012-06-15 11:42 Replying to schulle4u:

I'm interested in this driver, unfortunately it doesn't seem to work for my braillex el80 (Rev. 117). The device doesn't appear in the list of available braille displays in NVDA 2011.1.1. However after copying the .py file to the brailleDisplayDrivers in NVDA's userdata, a .pyo file is created after starting NVDA.

Did you copy both .py files (braillex.py and ftdi.py) into brailleDisplayDrivers?

nvaccessAuto commented 12 years ago

Comment 7 by schulle4u (in reply to comment 6) on 2012-06-15 11:56 Replying to jteh:

Did you copy both .py files (braillex.py and ftdi.py) into brailleDisplayDrivers?

Yes, and both files result in an appropriate .pyo file after starting NVDA.

nvaccessAuto commented 12 years ago

Comment 8 by aliminator on 2012-06-19 07:51 Hello,

a re-deveopment of the driver is not necessary since a new one has already been developed and currently tested.

It even works with the new el40c displays. It will be published as soon as testing has been finished. It won't take much time.

nvaccessAuto commented 12 years ago

Comment 9 by Casalino on 2012-06-19 08:05 Hi, this is not the right way to behave. This is not a re-implamantation of the code, but eventually an update. Contribution means working togheter on an existing project and not ignoring who has worked before. Anyway I stop here my work as the important thing is the result. Good work Gianluca

nvaccessAuto commented 12 years ago

Comment 10 by aliminator (in reply to comment 9) on 2012-06-19 09:30 Replying to Casalino:

the driver developed is based on your development code. The reason it was done this way due to the protocols of the displays we got from the company.

nvaccessAuto commented 12 years ago

Comment 11 by aliminator on 2012-06-26 09:15 A t2t file was created containing the documentation. A German localisation can be also be provided. The question which arises is in which format should this localisation be? Gettext, HTML etc...?

nvaccessAuto commented 12 years ago

Comment 12 by MHameed on 2012-06-27 16:04 What is the information documenting? If it is simple information such as the basic key bindings, then it would be good to have this as a patch to the userGuide.t2t, just like what we already have for the other supported braille displays.

If the driver will be packaged as an add-on, then an independant t2t is probably best.

In any case, can we please have both English and German, to save our German translators some work. Thanks.

nvaccessAuto commented 12 years ago

Comment 13 by aliminator on 2012-08-24 12:57 Hello,

first of all sorry for the delay and thanks to Gianluca Casalino who did the work before. We are pleased to announce that the Papenmeier braille driver was created, tested and documented and is located in the attachment. Driver creation was initiated by the "Blindenzentrum" department of the University of Applied Sciences in Giessen, Germany.

Papenmeier did support its development by providing both technical documentation and braille devices.

The archive contains the following:

papenmeier.py: The braille driver for USB/Bluetooth (newer) devices.
ftdi.py: Python bindings in order to talk to ftd2xx.dll, required for papenmeier.py and was taken from the driver created by Gianluca
papenmeier_serial.py: The braille driver for serial devices (older models).
documentation.diff: Contains patches for both the English and German user guide with further instructions.

Please integrate the braille driver and its user guide in the upcoming NVDA release. Changes: State: closed

nvaccessAuto commented 12 years ago

Comment 14 by aliminator on 2012-08-24 13:02 Changes: Changed title from "Braillex II family braille display driver" to "Papenmeier Braille driver support"

nvaccessAuto commented 12 years ago

Comment 15 by jteh on 2012-08-26 22:36 This shouldn't be closed until it is reviewed and made part of the main distribution.

Note that we will not accept the serial driver at present, as we've chosen not to support serial displays for now due to configuration difficulty and the dangers of probing arbitrary serial ports. Changes: State: reopened

nvaccessAuto commented 12 years ago

Comment 16 by aliminator on 2012-08-27 10:53 Sorry for that...

The serial driver supplied does not go through all ports on a system; it merely uses the one specified in the papenmeier_serial.ini file. Hence, there should not be any security consideration.

The very positive responses from the German mailing lists confirm that those serial braille devices which are usually older than the USB ones are still in use. Therefore they should be supported.

Do you have other suggestions regarding this topic? Normally, one would implement a list box showing connection types (such as com1, USB ...) and saving those values to the main configuration file. The braille display ddriver is queried with the interface selected and acts accordingly.

nvaccessAuto commented 12 years ago

Comment 17 by jteh (in reply to comment 16) on 2012-08-27 23:21 Replying to aliminator:

The serial driver supplied does not go through all ports on a system; it merely uses the one

specified in the papenmeier_serial.ini file. Hence, there should not be any security consideration.

This requires manual configuration by the user and will result in a lot of confusion.

The very positive responses from the German mailing lists confirm that those serial braille devices which are usually older than the USB ones are still in use.

For now, you're still welcome to submit an add-on containing this driver. This way, it is still available to those who want it.

Do you have other suggestions regarding this topic? Normally, one would implement a list box showing connection types (such as com1, USB ...) and saving those values to the main configuration file. The braille display ddriver is queried with the interface selected and acts accordingly.

The problem with this is that it requires the user to know what port the device is connected to. Determining this can be a fairly involved process which isn't realistic for new users. (This in turn results in a higher support load.) There are also implementation details which are difficult to resolve; e.g. where to store the port setting in the config, how to handle it in the Braille Settings dialog when the display is changed, etc. This part should be discussed in #426.

nvaccessAuto commented 12 years ago

Comment 18 by jteh on 2012-08-28 23:29 Btw, is the protocol implementation in the USB/bluetooth and serial versions the same (except the function calls for writing/reading data)? If we do support serial displays (see discussion in #426), it'd be good to combine these two drivers if the raw protocol is identical.

nvaccessAuto commented 12 years ago

Comment 19 by aliminator on 2012-08-29 07:43 During development it was thought combining those two drivers but unfortunately, the raw protocol is not identical.

nvaccessAuto commented 12 years ago

Comment 20 by aliminator on 2012-09-07 07:41 Were the drivers already been reviewed? It would be great when they could be included in the upcoming release.

nvaccessAuto commented 12 years ago

Comment 21 by jteh on 2012-09-11 05:40 First, thanks for your great work.

I've briefly reviewed the non-serial driver. I can provide detailed code review, but there are a few larger issues that need to be discussed.

The serial driver definitely won't be included for 2012.3. It needs to wait until proper support for serial displays is implemented. See #426.
The driver includes support for braille input. While I understand the desire for this, we will not include the driver with this code. Support for braille input for all drivers should wait until NVDA has core support for braille input; see #808. This allows for a unified implementation, rather than inconsistency between drivers.
Can you explain why there needs to be support for a COM server? We do this for Handy Tech, but your driver already has support for talking to the devices directly. We don't want to include a dll (now or in the future) if it isn't absolutely necessary due to file size. If you are unhappy with these restrictions, you may wish to distribute the drivers as add-ons for this release. Otherwise, please submit a version of the driver without braille input and possibly COM server support, depending on your answer to (3).

Btw, a minor point: there's no reason for us to include ftdi.py in NVDA's repository, as you can download it from its original source which we will list as a dependency. It's actually called ftdi2.py, but this is easily changed in the driver.

Thanks again.

nvaccessAuto commented 12 years ago

Comment 22 by halim on 2012-09-11 07:02 Hello, to 2. The brailleinput stuff works without external dependencies and do you really want us to remove that code? Can you tell us about nvda's brailleinput support and how long the users should wait to get brailleinput working with papenmeier displays?

BrxCom support is optional. There is no additional dll to ship with nvda. The required libs will be shipped with brxcom.

You minor issue is a big one in my opinion. If you decide to integrate the driver without ftdi python binding it will not work. It makes no difference if you add a comment in the documentation or dependency list. It would be really hard for many users to download and integrate the bindings.

I.ll check if we can remove the braille input code but this needs some work.

nvaccessAuto commented 12 years ago

Comment 23 by aliminator (in reply to comment 21) on 2012-09-11 07:36 Replying to jteh:

The driver includes support for braille input. While I understand the desire for this, we will not include the driver with this code. Support for braille input for all drivers should wait until NVDA has core support for braille input; see #808. This allows for a unified implementation, rather than inconsistency between drivers.

I understand you want to have drivers consistently but how long do users have to wait till the braille input is implemented into core? The actual driver would at least be an interim solution till the core gets braille input support.

Can you explain why there needs to be support for a COM server? We do this for Handy Tech, but your driver already has support for talking to the devices directly. We don't want to include a dll (now or in the future) if it isn't absolutely necessary due to file size.

As Halim has mentioned the dll file does not need to be included (and was not planned to include it) in NVDA. It will be shipped with the BRXCOM server and is a second approach to talk to Papenmeier displays. Both approaches definitely should be supported. What do you mean by dependecies of the ftdi2.py? Are you going to include it as dependency in the NVDA source code (such as thirdparty lib) or are you not going to include it at all? In the second case the driver will not work if BRXCOM is not installed. Installing BRXCOM when there is an alternative available creates unnecessary difficulties for the user.

nvaccessAuto commented 12 years ago

Comment 24 by jteh on 2012-09-11 08:23 Regarding braille input, I don't have an estimate on when this will be done. There's no reason this can't be done in an add-on for now; the whole point of an add-on is to add functionality that isn't included in the core. Aside from inconsistency, doing it in individual drivers means that users will expect the functionality to be the same in a future release, which is a problem if things change somewhat once the core support is implemented.

Regarding BRXCOM, I still don't understand why this should be supported. If I understand correctly, the user can choose to install BRXCOM on their system. What is the advantage of doing this as opposed to using the direct support? If there's a good reason/advantage, I'm not opposed to it, but I'm not understanding this.

Regarding ftdi2.py, I mean that there's no reason to include it in NVDA's source repository. It'll just be an additional source dependency that users running from source have to download, just like they do for configobj, etc. Users running from binary builds will not have to do anything extra. This is why I said it was a minor point.

nvaccessAuto commented 12 years ago

Comment 25 by halim on 2012-09-11 09:04 Hi, BrxCom is needed for brailledisplays which can be used as notetaker. It support displaying braille and handles file transfer. If brxcom is not installed our nvda driver can only be used to display braille. I attached a modified version to comment 24 (sorry) with brlinput removed. It contains also your sugested change to ftdi2 and contains some small fixes.

nvaccessAuto commented 12 years ago

Comment 26 by halim on 2012-09-11 10:31 Hello, So I've attached userGuid.diff because of removal of the brailleinput stuff. Please see also my updated papenmeier.py attached to comment 24 which contains the suggested changes. Thx

nvaccessAuto commented 12 years ago

Comment 27 by jteh on 2012-09-12 04:59 Thanks! Code review:

The copyright doesn't mention Gianluca Casalino. Was any of his code used in the creation of this new driver? If so, he needs to be recognised as well.

xrange() should generally be used instead of range(), as range() creates a list, which is wasteful.

def brl_out(data,nrk,nlk,nv):
...
  d2 = len(data)
  #reserve space for braille displays with dummy cells
  d2+=nv
  d2+=2*nlk
  d2+=2*nrk

There is some inefficiency here with no gain in readability. This is probably better written as:

d2 = len(data) + nv + 2 * nlk + 2 * nrk

Also, using += on a string is inefficient when done a lot, as strings in Python are immutable. You may wish to consider appending strings to a list and then using "".join to improve performance, though it's a relatively short string, so it probably won't be that noticeable.

def brl_poll(dev):
...
          while(dev.inWaiting()==0): pass

Spinning like this is really expensive and will cause unnecessarily high CPU load. It's better to do a blocking read (even if only 1 byte), as the OS will then resume the process only when data is available.

In the brl_decode_key_names* functions, there are a lot of if/elif blocks for specific values. It is more efficient to use a constant dict here which maps one value to another. Also, code like this:

dec += [be written using dec.append(val), as creating a new list and concatenating it is unnecessary.

In the !BrailleDisplayDriver class:

> ```
>   description = _("Papenmeier BRAILLEX NEW")
> ```

What does the "NEW" mean here and why in upper case? Perhaps "Papenmeier BRAILLEX newer models" might be better if I understand correctly.

> ```
>   def connectBrxCom(self):
> ...
>           if(self._brxnvda.brxnvda_init(str(value+"\\BrxCom.dll")) == 0):
> ```

The ```str(value+"\\BrxCom.dll")``` will break if this path contains non-ASCII characters. This should probably be ```(value+"\\BrxCom.dll").decode("mbcs")```. This will convert it from Unicode to ANSI based on the system's ANSI code page.

> ```
>           log.info("BRXCom is not installed")
> ```

I don't think messages like this stating that something wasn't found should be logged at level info. They should instead be logged at level debugWarning. It's okay to log what *was* found, since the user may wish to know, but what wasn't found is only useful for debugging. The same applies to messages when USB and bluetooth aren't found.

> ```
>   def connectBluetooth(self):
>       if(self._dev == None and self._baud == 0):
> ```

When checking for equality against None, use ```blah is None``` rather than ```blah == None```, as None will never have more than one address, so it's more efficient. Also, it might be nicer to reverse this check and return early, thus eliminating one level of indentation.

> ```
>   def __init__(self):
> ...
>               self._dev.write(brl_auto_id())
>               time.sleep(1)
> ```

Sleeping like this means you're potentially delaying a lot longer than you need to. I guess this is tricky because it looks like the Trio gives no response to this command. Does it not even send an ETX? Generally, you should do a blocking read (even if just 1 byte), as this way, you only wait as long as it takes for data to arrive. If the Trio really sends nothing and there's a way to do a read with timeout with ftdi2, I would try that instead of incurring the delay for all devices.

> ```
>               log.info('BROKEN PIPE - THIS SHOULD NEVER HAPPEN')
> ```

Again, this shouldn't be logged at level info. It should either be error or debugWarning depending on how important it is.

The terminate method should call super first, as super tries to clear the display, etc. This should also eliminate the need for the display filling code in terminate.

> ```
>   def _get_numCells(self):
>       return self._numcells
> ```

This suggests the _numcells variable should be removed and self.numCells should be used directly. self.numCells doesn't have to be a property; it can just be an attribute.

> ```
>   def display(self, cells):
> ...
>           newcells = ''
>           for cell in cells: newcells += chr(cell)
> ```

This is a little inefficient and is better written as:

newcells = "".join([chr(cell) for cell in newcells]('dn']

should))

Regarding the InputGesture class, it shouldn't call braille.handler.routeTo directly. Instead, it should map routing keys to a gesture identifier and then map that gesture to the braille_routeTo script in the gestureMap. The code also sleeps for 0.5 seconds after handling routing keys. It definitely shouldn't do this.

Here are some minor nits that I'm not too concerned about, but should be noted nevertheless:

Generally, we tend to use camel case for names in NVDA; e.g. brlDecodeKeyNamesARepeat instead of brl_decode_key_names_A_repeat.
There are a few blank lines containing only spaces. These should be removed. There are also some lines that have spaces as indentation, which should be changed to tabs. Some lines end with unnecessary whitespace.
There's no need to put brackets around conditions in conditional satements. For example, you can just do: if blah and blah and blah: instead of: if (blah and blah and blah):

Thanks again for your work!

nvaccessAuto commented 12 years ago

Comment 28 by aliminator (in reply to comment 27) on 2012-09-14 13:17 Replying to jteh:

The copyright doesn't mention Gianluca Casalino. Was any of his code used in the creation of this new driver? If so, he needs to be recognised as well.

The driver has been completely newly created.

Regarding the InputGesture class, it shouldn't call braille.handler.routeTo directly. Instead, it should map routing keys to a gesture identifier and then map that gesture to the braille_routeTo script in the gestureMap.

The upper routing keys do have actually two actions associated with. First, routing the braille cursor to actual position (where the key was pressed). Second, announcing formatting information of current character. We did not find any other solution to solve this. Is there any possibility to map two subsequent scripts to one key?

nvaccessAuto commented 12 years ago

Comment 29 by jteh (in reply to comment 28) on 2012-09-17 01:14 Replying to aliminator:

Regarding the InputGesture class, it shouldn't call braille.handler.routeTo directly. Instead, it should map routing keys to a gesture identifier and then map that gesture to the braille_routeTo script in the gestureMap.

The upper routing keys do have actually two actions associated with.

First, routing the braille cursor to actual position (where the key was pressed). Second, announcing formatting information of current character. We did not find any other solution to solve this.

So when you press the key, both of these things occur. Is that correct? If that is the case, I would do two things:

Make sure the upper routing keys are mapped to a different gesture identifier (e.g. upperRouting); and
Define your own script on the !BrailleDisplayDriver class which performs both of these actions, then bind the gesture in __gestures. The script might be something like this:

 def script_upperRouting(self, gesture):
    globalCommands.commands.script_braille_routeTo(gesture)
    globalCommands.commands.script_reportFormatting(gesture)

__gestures would be something like this:

 __gestures = {
    "br(papenmeier):upperRouting": "upperRouting",
 }

nvaccessAuto commented 12 years ago

Comment 30 by aliminator (in reply to comment 29) on 2012-09-17 15:46 First of all, thanks for the review! We tried to incooperate the changes you suggested (see attachment). The only thing which cannot be removed/changed completely is the way of device discovery. To wait one second is indeed definetely too long. So it was reduced to 50 ms (this should be sufficient and necessary till the response is sent back from the braille display -- for all devices).

It would be great if the driver is now ready to be included in the upcoming release.

nvaccessAuto commented 12 years ago

Comment 31 by jteh on 2012-09-18 06:27 Thanks. Merged in d6e854dd213c613c3382ac4690988869eff27194 with several changes by me. You can see the changes I made by examining 1b7374d87c66ff7a216e56f152f9c77f6583c5ad through 6a759cd626cd78f2f813b9aee02cb46b39072924. My changes shouldn't have broken anything, but please test and reopen if I broke something. Changes: Milestone changed from None to 2012.3 State: closed

nvaccessAuto commented 12 years ago

Comment 32 by jteh on 2012-09-19 01:11 aliminator, can we please have your name for inclusion in the contributors file? It isn't required if you don't want to be known, but we can't recognise your contribution properly without it.

nvaccessAuto commented 12 years ago

Comment 33 by aliminator on 2012-09-19 07:44 Ok... My name is Ali-Riza Ciftcioglu.

nvaccessAuto commented 11 years ago

Comment 35 by ragb on 2012-11-28 17:17 Hi,

I think with the changes in #426 (see the !braillePorts branch), the serial version of the driver may be included, if the authors modifies it to reflect possible ports and such. The braillenote driver on that branch may serve as an example.

nvaccessAuto commented 11 years ago

Comment 36 by jteh on 2012-11-28 21:21 The serial driver was split off into #2679.

nvaccess / nvda

Papenmeier Braille driver support #1265