Braille driver for Eco display

[nvaccessAuto]

Reported by jjimenez0 on 2014-04-16 05:47

EcoBraille display are very common in Spain but it doesn’t work with 64 bits systems, so they were falling obsolete. An addon for Eco Braille display would be very important in order to get working those displays in NVDA even in 64 bits operating systems.

[nvaccessAuto]

Comment 39 by norrumar (in reply to comment 38) on 2015-02-02 06:55 Replying to jteh:

So did this new driver actually cause a crash? I don't see any mention of a crash, just an error. Otherwise, a crash dump wouldn't be useful anyway.

What's the status of this? Is it ready for review and potential inclusion or are there still issues that need to be resolved?

I'm providing feedback to developers of this drive, who send versions fixing issues according to my feedback. The last version that I have received has no issues for me: just an exception writen in the log, which perhaps can be handled. I'm waiting some days before providing feedback again, to ensure there are no issues at this point. Thanks.

[nvaccessAuto]

Comment 40 by norrumar on 2015-02-09 11:50 Hi, I have tested this driver, and I think it's ready to be included in NVDA core. In previous versions, I got errors for instance when I started my computer, but now this is fixed, though there is a key error which appears in the log; but this is not harmful and Ecobraille display works fine. Thanks.

[nvaccessAuto]

Comment 41 by jjimenez0 on 2015-02-09 11:51 Hi James, We have tested this new version of the python file for the EcoBraille displays and we think that it's ok. Only one problem was found with the EcoBraille 80plus displays and the final solution is the same that we use in the Windows driver for those displays: if we have an Eco display that doesn´t follow the initial protocol, we assume that it's an EcoBraille 80plus display. So we only have an error message that appears sometimes but it doesn't look important. I think it's possible to include it in NVDA when you want and we keep trying to solve this problem from now. I have adde the final file ecoBraille.2.py right now Thanks

[nvaccessAuto]

Comment 43 by jteh on 2015-04-14 07:42 Thanks for all of your work. I've finally reviewed this. Sorry for the ridiculously long wait.

In general, this looks pretty good to me. I have a few concerns/issues, however:

1. A very major concern is that the eco_in function sleeps for 1 second if there are less than 8 bytes to read. This gets called in a timer, so unless I'm missing something, it's entirely possible that this could happen at some random point. The displaymight send entire chunks, but that doesn't mean you'll read exactly at the right time; it might be in the middle of flushing a packet. If that happens, the code will sleep, which will block NVDA's main thread, causing a very noticeable "freeze" for the user.[Is there some way you can avoid this? If you know you need 8 bytes, just always read 8 bytes (assuming there is at least 1 byte waiting); the timeout will prevent it from blocking indefinitely if the display misbehaves. I see you do need to read an extra byte in some cases. Is there no way you can detect this based on the previous data in the packet?[br] When initialising, I understand you might need this sleep. If so, that should be made specific to initialising.
2. Please use Python comments (i.e. lines prepended with #) rather than strings for comments in your code. You can use a string at the top of a function/class/module for code documentation, but comments should be comments. For what it's worth, I think the only code doc string in your code is for the BrailleDisplayDriver class; the rest should be comments.
3. Is there a more descriptive name for table_translation; e.g. output_dots_map?
4. There are a few whitespace issues; e.g. double blank lines, trailing spaces/tabs at the end of lines, etc. This isn't major, but it'd be great if they could be removed.

Thanks!

[nvaccessAuto]

Comment 44 by jteh (in reply to comment 43) on 2015-04-15 02:24 Replying to jteh:

If you know you need 8 bytes, just always read 8 bytes ...

I see you do need to read an extra byte in some cases.

I just re-read and realised you always need 9 bytes; you return 0 if you only get 8 bytes or less. In what cases do you get less? Can you detect those based on the first byte or similar?

[nvaccessAuto]

Comment 45 by jjimenez0 on 2015-04-15 10:05 Hi James, You don't have to apologize for the time. I know that there are a lot of things more urgent in this great project and the day only have 24 hours (smile) I am glad that the file looks pretty to you. I have changed all the things you say me about the white spaces, extra tabs, blank lines and comments. Also i have changed the name for the table_translation. And about the important issue: I have changed a couple of things and it looks better than before ( i need that Noelia tests it to confirm the progress) 1.- I always need to read 9 bytes: in the init function and in the read from display one. But the important information is in the 5 bytes of the middle. Two first and two last bytes are the head and the tail of the display protocol (always 0x10 0x02 and 0x10 0x03 respectively). So the first improvement is read only 8 bytes, flush the last, and don't check if the tail is complete. I am going to upload a file called ecoBraille.py.read 8 bytes.py with this changes. 2.- I have tested to read 9 bytes exactly and it works. I feel that i have tried this and did not work, but, obviously this is the perfect solution. I have tested it a little and it looks fine. I am going to upload a file called ecoBraille.py.read 9 bytes.py with this version.

I need that Noelia tests both files, especially the one that read 9 bytes, because if this one works fine, is the one that we have to include in NVDA. I left in both files the flushInput() call in order to avoid another bug that Noelia found with the last version. I hope that those files may be included in NVDA in short time

[nvaccessAuto]

Comment 46 by jteh on 2015-04-15 10:23 In that case, the 9 bytes version should work, but if it doesn't, that suggests you might need to increase your timeout a little.

[nvaccessAuto]

Comment 47 by jteh on 2015-04-17 04:24 Did anyone ever provide a User Guide section for this display? I can't seem to find it if they did.

From the 9 bytes version:

  if dev.inWaiting() < 9 :

This might not work. The problem is that the display might not be finished sending a packet yet. For example, it might have only sent 5 bytes of the 9 total. You already check whether there is at least 1 byte waiting before calling eco_in in _handleResponses; that just prevents pointless blocking if there is no packet at all. After that, reading 9 bytes will simply wait until all 9 bytes arrive unless the timeout is reached. To put it another way, if there's at least 1 byte, there should be 9 bytes coming, assuming the display never sends a packet shorter than that.

In practice, because this is running on a repeating timer, I guess it'll just pick up the full packet next time the timer runs. However, there's probably no point in making the user wait unnecessarily.

  status = []
  status.append(dev.read(9))

nit: No point in using a list if you're only appending one thing to it. I know you used to append two strings, but now it's just one.

What happens without the flushInput? Calling flushInput concerns me, as you could potentially be losing a subsequent packet or part thereof. Losing part of a packet is even worse because your reading will then all be offset by 1 or more bytes, so all future packets will be invalid.

[nvaccessAuto]

Comment 48 by jteh on 2015-04-17 04:25 It's too late to get this into 2015.2, but let's try very hard for 2015.3. Changes: Milestone changed from next to 2015.3

[nvaccessAuto]

Comment 49 by jjimenez0 on 2015-04-17 12:07 No problem about when release the code. Better be sure that the code is well. I have no manual or guide for the eco displays. I am using the sources from windows so i am sure about the strings to send (the protocol) but i do not know another things like timers, for example. I have changed the input in order to look for an only byte in the buffer and, if it's true, read 9 bytes. But this doesn't work propertly with the initialization because the read is not blocking. I think because of i have set a timeout when open the serial port. I have separated the eco_in function into two smallers functions: one for the initialization (eco_in_init) and another for the rest (eco_in) So, in the eco_in_init i have to use an sleep for the case that i have no initialization sequence in the serial port buffer. I hope it's not a problem because only happens once in a session and we can be sure that get the real device in the serial port. I use this changes to improve the code and send a message with 'No display found' when nothing is in the serial port buffer. I attach a new version of ecoBraille.py file

[nvaccessAuto]

Comment 50 by jteh on 2015-04-20 00:01 Thanks! Two final things.

In eco_in, I don't think you need the dev.inWaiting() check at all, since you already check this before you call eco_in (in _handleResponses).

In eco_in_init, rather than checking for 9 bytes waiting, I'd set the timeout higher and just read 9 bytes. So, in the constructor, something like this:

        self._dev = serial.Serial(self._port, baudrate = 19200, bytesize = serial.EIGHTBITS, parity = serial.PARITY_NONE, stopbits = serial.STOPBITS_ONE)
        # Use a longer timeout when waiting for initialisation.
        self._dev.timeout = self._dev.writeTimeout = 3
        self._ecoType  = eco_in_init(self._dev)
        # Use a shorter timeout hereafter.
        self._dev.timeout = self._dev.writeTimeout = TIMEOUT

Then, in eco_in_init, you can remove the inWaiting stuff and just read. This is preferable because it means the user doesn't always have to wait 3 seconds. At present, the user will almost always have to wait 3 seconds, even if the display only takes 1 second to send the packet.

[nvaccessAuto]

Comment 51 by jjimenez0 on 2015-04-28 06:50 Ok. All changes has been made. Now works for all models of Ecobraille display. It has different timeout for initialization process and for the rest. Also doesn't have a dev.inwaiting in eco_in. I think it has no more spaces or blank lines than the necessary (i hope) Noelia has tested it in the ecoBraille 80 plus (the most problematic) and it's fine. Only one advice in the user manual so that the user switch on the display before NVDA. Attach a new file ecoBraille.py

[nvaccessAuto]

Comment 52 by jteh on 2015-04-30 03:48 In eco_in_init:

    msg = dev.read(9)
    if  (len(msg) < 9 ):
        return ecoTypes.TECO_80

Doesn't this mean you'll assume TECO_80 even if there were no bytes at all? I guess you might not have any other way to detect this, but it does mean NVDA might think it's talking to a display when it isn't. From a user's perspective, this means they won't get an error when they try to use the display (e.g. if they selected the wrong port), even though they get no output.

In the constructor:

        self._dev = serial.Serial(self._port, baudrate = 19200,  bytesize = serial.EIGHTBITS, parity = serial.PARITY_NONE, stopbits = serial.STOPBITS_ONE)
        self._dev.setTimeout(3)
        # Use a longer timeout when waiting for initialisation.
        self._dev.timeout = self._dev.writeTimeout = 3

The self._dev.setTimeout(3) is redundant, since it gets set below the comment. That was a typo in my original comment. I edited this out, but you might have seen the version before the edit. :)

In qite a few places, you have:

        if(self._dev!=None):

Sorry I neglected to mention this one before. Unless I'm missing something, this shouldn't be None unless the display failed to initialise or it has been terminated, in which case the functions won't get called, making this check unnecessary. Did you find otherwise during your development?

Also, is someone providing User Guide documentation for this display?

[nvaccessAuto]

Comment 53 by jjimenez0 on 2015-04-30 05:55 Hi James Yes. In the eco_in_init I assume that if there is no bytes, we have a Eco 80. This is because there is a model (eco 80 Plus) that have an error sending the initial protocol: first time you switch on those displays they send the initial message and work fine but if you, for example, reload NVDA, the displays don't send this initial message anymore, so, although you have an eco, NVDA would say that there is no display. In other software (JAWS) this is the solution we have given. Another solution would be write instructions in the user manual and return TECO_NODISPLAY if there is no bytes at all. Which do you prefer?

About the self._dev.setTimeout(3),... ok, too much trust in your words (smile). I delete it in the next version.

And finally, about the None check, I never find this case in my tests, I wrote this in the first versions making tests and I guess i would feel safe leaving it there, but i can deleted it because i also think that it is unnecessary now.

I am going to talk with Noelia in order to write this guide documentation.

[nvaccessAuto]

Comment 54 by jteh (in reply to comment 53) on 2015-04-30 06:21 Replying to jjimenez0:

Yes. In the eco_in_init I assume that if there is no bytes, we have a Eco 80. This is because there is a model (eco 80 Plus) that have an error sending the initial protocol: first time you switch on those displays they send the initial message and work fine but if you, for example, reload NVDA, the displays don't send this initial message anymore, so, although you have an eco, NVDA would say that there is no display.

I notice that you expect the display to send the message as soon as you open the port. My guess is that these displays don't detect opening the port correctly. Is there not some other way to force the display to identify itself? (With most displays, you ask the display to identify itself rather than just opening the port and waiting for it to do so.) I'm guessing there isn't--if there were, you'd probably be using it--but I'm just double checking.

In other software (JAWS) this is the solution we have given. Another solution would be write instructions in the user manual and return TECO_NODISPLAY if there is no bytes at all. Which do you prefer?

I think consistency with what you've done for other screen readers is best. This was more a query than an actual objection.

Thanks.

[nvaccessAuto]

Comment 55 by jjimenez0 on 2015-04-30 07:11 Last file i have attached (ecoBraille.py) works fine for me in all the models that I have. Also with the ecoBraille 80 Plus if you switch on the display before run NVDA.

[nvaccessAuto]

Comment 56 by norrumar (in reply to comment 55) on 2015-04-30 16:48 Replying to jjimenez0:

Last file i have attached (ecoBraille.py) works fine for me in all the models that I have. Also with the ecoBraille 80 Plus if you switch on the display before run NVDA.

Hi, I don't get this result with Ecoplus using your last file when restarting NVDA. Here is the log.

[nvaccessAuto]

Comment 57 by jjimenez0 on 2015-04-30 21:10 Hi, Noelia, are you sure that the ecoBraille is displaying the "PC no conectado" message before start NVDA? With the last code is mandatory restart the display before reconnect to NVDA.

[nvaccessAuto]

Comment 58 by norrumar (in reply to comment 57) on 2015-05-01 03:45 Replying to jjimenez0:

Hi,

Noelia, are you sure that the ecoBraille is displaying the "PC no conectado" message before start NVDA? With the last code is mandatory restart the display before reconnect to NVDA.

No, the message displayed correspond to the normal text, NOT PC NO CONECTADO. The error is produced when restarting NVDA. Steps to reproduce:

1. Switch on Ecoplus when Ecobraille displays are selected from NVDA configuration.
2. Start NVDA, for instance turning on the PC.
3. Restart NVDA, i.e. pressing control+nvda+n.

BTW, about documentation, long time ago I created t4078 branch at https://bitbucket.org/nvdaaddonteam/nvda There I wrote the key commands for Ecobraille displays and commit an old driver. Tell me if I should update all this. Thanks.

[nvaccessAuto]

Comment 59 by jteh on 2015-05-01 03:55 Thanks Noelia. I can pull the documentation from your branch.

[nvaccessAuto]

Comment 60 by norrumar (in reply to comment 59) on 2015-05-01 09:40 Replying to jteh:

Thanks Noelia. I can pull the documentation from your branch.

I have updated the documentation noticing that Ecoplus must be connected before starting NVDA. I hve also included the new driver with some minor changes, that we can revert if needed.

1. I have change f1 and f3 key commands according to the user guide. We discussed about this on spanish mailing list, and I thought that using f1 and f3 for switching to next and previous review modes make sense, since these kind of keys are used for object navigation and so, for moving the review cursor.
2. I have deleted some blank lines and spaces, i.e. inside brackets, a semicolon at the end of a line, etc.
3. If lengh(msg) < 9, TECO_80 is returned, as in previous files. I use this modification in my job to restart NVDA properly with Ecoplus display.

Some questions:

1. As I said, if you want we can delete encoding declaration if needed, if if can contribute to future versions, when NVDA uses Python 3.
2. In the year of Copyright, it's writen 2014. We can write 2014-2015 if you want. Thanks.

[nvaccessAuto]

Comment 61 by jjimenez0 on 2015-05-01 10:15 Thanks Noelia for the documentation and for the changes.

1. About the functionallity of the key commands I let you total freedom as I believe that the users are who must set them.
2. Again sorry for the blank lines
3. In this point we must decide one of two solutions. Due to the EcoBraille 80 Plus hardware is broken (at least the software inside them) we can solve it from NVDA in two ways: a.- We return TECO_80 when no data is waiting in the initialization. This allows restart NVDA with all EcoBraille displays working fine but if there is no display or another display is connected to the serial port, NVDA will assume that there is a EcoBraille 80 display connected. NVDA will send data to the serial port but it will be consummed by nobody. This is the solution that this kind of displays use in JAWS, for example. b.- We return TECO_NO_DISPLAY when no data is waiting in the initialization. This make NVDA coherent about the real displays connected to the PC but the user of a eco80 plus can not restart NVDA because those displays will not send the initialization bytes after a software disconnect.

I don't care which one we select. Both has a good side and a bad side. Right now is only change a line in the source code.

And ok for the encoding and the copyright date ( I hope we must not change it to 2014-2016, (smile))

[nvaccessAuto]

Comment 62 by nvdakor on 2015-05-01 10:38 Hi, As you pointed out, both options have advantages and downsides. Personally, I'd go against the first option not only for communication reasons, but also for resource use. An open serial port connection could become a potential security issue, in that a device claming to be Eco 80 could send malicious data that could be used against NVDA (highly unlikely, but we might as well be open to that possibility). I'd like to propose a third solution: updating Eco firmware. If the firmware on Eco 80 is updatable, then I believe the most effective way to solve this is to update the Eco 80's firmware to resolve this from Eco's end. Not only this should resolve the serial port issue, it should let Eco 80 comply to whatever protocol is being used. Thanks.

[nvaccessAuto]

Comment 63 by norrumar (in reply to comment 62) on 2015-05-01 10:51 Replying to nvdakor:

Hi,

As you pointed out, both options have advantages and downsides. Personally, I'd go against the first option not only for communication reasons, but also for resource use. An open serial port connection could become a potential security issue, in that a device claming to be Eco 80 could send malicious data that could be used against NVDA (highly unlikely, but we might as well be open to that possibility).

I'd like to propose a third solution: updating Eco firmware. If the firmware on Eco 80 is updatable, then I believe the most effective way to solve this is to update the Eco 80's firmware to resolve this from Eco's end. Not only this should resolve the serial port issue, it should let Eco 80 comply to whatever protocol is being used.

Thanks.

Hi, I agree with you and finally I reverted the modification used in my job. While firmware is not updated, I think this driver should be included in NVDA core when possible. I have leave UTF-8 encoding declaration for now, since perhaps it can be needed in Python 2 if the file is modified. There are other files in NVDA core with this declaration, and they perhaps should be changed when upgrading to Python 3. I have added a note about restarting Ecoplus in the user guide.

BTW, one time (just one), NVDA gets restarted and a dump was produced in my job, but I think it doesn't is related to Eco display. In the log appears something about braille update, but I didn't report it because it just happened one time. Thanks.

[nvaccessAuto]

Comment 64 by jjimenez0 on 2015-05-01 11:30 I am agree about the real solution should be change the firmware of the EcoBraille 80 Plus but this is a very, very difficult solution. They are very good displays but has the same firmware and hardware as 15-20 years ago. Many years ago we don´t made new EcoBraille displays but a lot of people in Spain use them in their home or jobs. We should ask them to bring those displays to update the firmware. Really complicated.

[nvaccessAuto]

Comment 65 by jteh on 2015-05-01 11:48 I don't think the security argument holds. That's theoretically possible for any device, regardless of which option you choose. A device could just as well respond to the protocol correctly and still be malicious.

Personally, if there's really no command you can send to these displays to force them to re-initialise/identify themselves, I'd go with the option adopted for other screen readers. It's ugly, but it's probably the better of the two evils.

[nvaccessAuto]

Comment 66 by norrumar (in reply to comment 65) on 2015-05-01 12:02 Replying to jteh:

I don't think the security argument holds. That's theoretically possible for any device, regardless of which option you choose. A device could just as well respond to the protocol correctly and still be malicious.

Personally, if there's really no command you can send to these displays to force them to re-initialise/identify themselves, I'd go with the option adopted for other screen readers. It's ugly, but it's probably the better of the two evils.

If sequrity reasons don't hold, I think it's better adopt the other option as in other screen readers in order to restart NVDA without problems. I will make changes on my branch. Thanks.

[nvaccessAuto]

Comment 67 by norrumar (in reply to comment 66) on 2015-05-01 12:41 Replying to norrumar:

Replying to jteh:

I don't think the security argument holds. That's theoretically possible for any device, regardless of which option you choose. A device could just as well respond to the protocol correctly and still be malicious.

Personally, if there's really no command you can send to these displays to force them to re-initialise/identify themselves, I'd go with the option adopted for other screen readers. It's ugly, but it's probably the better of the two evils.

If sequrity reasons don't hold, I think it's better adopt the other option as in other screen readers in order to restart NVDA without problems.

I will make changes on my branch.

Thanks.

Done. I will test this the next Monday in my job, placing the driver into the add-on. I assume ther won't be differences in the results obtained with the add-on and placing the driver into NVDA core. Thanks.

[nvaccessAuto]

Comment 68 by norrumar on 2015-05-02 07:03 I think t4078 branch is finished for now. I will test the driver. Regarding documentation, I have changed the key names in the user guide according to Ecoplus documentation available at ftp://ftp.once.es/pub/utt/bibliotecnia/Lineas_Braille/ECO/ecoplus.doc

Routing keys are described, but I think that not named consistently, so I have writen Route in NVDA user guide, as in the driver with capital R, since key names are capitalized in Ecoplus documentation. Anyway, ONCE has an add-on explained at http://cidat.once.es/home.cfm?id=1711&nivel=2 It has been posted on mailing list. I think it's not needed to tell users that they must uninstall it in the user guide, but I don't know. BTW, as Ecoplus is used for instance in people jobs and perhaps there stable versions of NVDA are preferred, I wonder in you want to update your add-on when the driver is included in development version for wide testing, telling people that it's just a test. Perhaps in the add-on the display description should be different, i.e. Ecobraille displays for testing. Thanks.

[nvaccessAuto]

Comment 69 by norrumar on 2015-05-04 10:15 I'm testing the driver in my job. A timeout set to 0.1 is right for me also in initialization. Thanks.

[nvaccessAuto]

Comment 70 by jjimenez0 on 2015-05-11 11:00 After some tests and a few minor changes made by Noelia and me we think that the file is correct. I explain here the last changes:

• In eco_in_init it returns ECO 80 if there is any problem because of the hardware failure with the Eco 80 Plus displays.
• For initialization we found 2.7 seconds as a good time in order to recognize all displays.
• Changed the name of the keys

This file and the changes of the files changes.t2t and userGuide.t2t are in the branch t4078 Hope it's ok. Thanks

[nvaccessAuto]

Comment 71 by jteh on 2015-06-26 07:22 Note to self: The What's New entry for this ticket is:

Support for the EcoBraille 20, EcoBraille 40, EcoBraille 80 and EcoBraille Plus braille displays. (#4078)

[nvaccessAuto]

Comment 72 by James Teh <jamie@... on 2015-06-26 07:34 In [b720d04ed8b122aa235547377361ef46564cbe69]:

Merge branch 't4078' into next

Incubates #4078.

Changes: Added labels: incubating

[nvaccessAuto]

Comment 73 by jteh on 2015-06-26 07:36 Okay! We're finally incubating. Thanks to both of you for the great work. :)

[nvaccessAuto]

Comment 74 by norrumar (in reply to comment 73) on 2015-06-26 09:26 Replying to jteh:

Okay! We're finally incubating. Thanks to both of you for the great work. :)

Thank you! Juan Ramón Jiménez (ONCE-CIDAT) is not included in contributors.txt on t4078 branch. I think this could be included later. Cheers.

[nvaccessAuto]

Comment 75 by norrumar on 2015-06-26 13:49 Sorry, please forget my previous comment. This is included, but I hadn't seen it properly in your commit. Thanks.

[nvaccessAuto]

Comment 76 by jteh on 2015-06-28 23:11 Should i include Juan Ramón Jiménez, ONCE-SIDAT or both? At this stage, it just says ONCE-SIDAT, but I'd prefer to include Juan's name instead/as well if that is okay.

[nvaccessAuto]

Comment 77 by norrumar (in reply to comment 76) on 2015-06-29 02:08 Replying to jteh:

Should i include Juan Ramón Jiménez, ONCE-SIDAT or both? At this stage, it just says ONCE-SIDAT, but I'd prefer to include Juan's name instead/as well if that is okay.

Hi, I agree with you. I would include: Juan Ramón Jiménez García (ONCE-CIDAT)

He works with other people in CIDAT (a technical center from ONCE, a spanish organization for blind people). Thanks.

[nvaccessAuto]

Comment 78 by jjimenez0 on 2015-06-30 06:44

Hi, sorry for the delay. For me it's perfect with Juan Ramón Jiménez García (ONCE-CIDAT) I take this comment to thank you for the help you have given me to write this source. I hope to contribute with more things in this wonderful project. Thank you so much.

[nvaccessAuto]

Comment 79 by James Teh <jamie@... on 2015-07-10 04:26 In [8cc16755fa3efdd54c76ca9f1da17c46118e5248]:

Support for the EcoBraille 20, EcoBraille 40, EcoBraille 80 and EcoBraille Plus braille displays.

Authors: ONCE-CIDAT cidat.id@once.es, Noelia Ruiz Martínez nrm1977@gmail.com
Fixes #4078.

Changes: Removed labels: incubating State: closed