Attachment ecoBrailleAddon-1.0.nvda-addon added by jjimenez0 on 2014-04-16 05:48
Description:
Addon for Eco Braille displays

Comment 1 by jjimenez0 on 2014-04-16 05:53

In ONCE (National Organization of Spanish blind) we have developed a little addon that allow us to use the Eco Braille display with NVDA.
I have attached the addon in this ticket. I'm working now in documentation.
Only two files are neccesary: a dll with the functionallity and a python file to attach this library to the NVDA core.

It's the first addon that we develop for NVDA, so it could be possible that we have fail in something. Please, tell us in order to solve it and improve the driver.

Comment 2 by norrumar on 2014-04-21 11:35
Hi, many thanks. I'm a spanish user of Ecobraille, and now I can work properly in my job. Before this add-on I used Jaws just for braille. I haven't reviewed the add-on code, but it seams to work find. I'm jus starting to use it now, but it works much better than BRLTTY.
If you want, we can store the add-on in the community website after reviewing the code, following our guidelines. For this we usually upload add-ons to http://bitbucket.org/nvdaaddonteam
I'm really pleased for this.

Thanks a lot.

Comment 4 by jjimenez0 on 2014-04-21 12:23

Hi
I am very glad to read that (smile). Of course you can store the addon in the community website. Do you need the source of the dll library or you only review the Python file of the addon? The Python file is very simple and perhaps you might consider changing the key assignment of the EcoBraille display.
Please, tell me anything I can do in order to improve the addon and get it in NVDA as soon as possible.

Thanks in advance

Comment 5 by norrumar on 2014-04-21 13:05
Hi, I have read the Python file a first time.
I think we could fix small spelling errors in comments, and remove several spaces in variable assignments. Imo it would be easier for reviewers.
It seems that dll functions are writen in Spanish. Perhaps it would be better writing their names in English for reviewers.
Furthermore, a log.warning message is in Spanish.
According to our guidelines, we don't use the word addon in add-on names...
But the add-on is great. I needed it in my job, cause I prefer NVDA and not JAWS, and I had to start JAWS for braille, as BRLTTY doesn't work fine.
I will post the add-on in a Bitbucket repository of addonteam site, and we can upload it to our addon site while it's not incorporated in NVDA core.
I will send the add-on to the addons email list too.
Great great work.

Comment 6 by norrumar on 2014-04-21 15:25
Hi, I will create a public repo with your attached add-on without modifications. But I can add feed-back:

1. f0 key doesn't work: I have fixed it with this assignment: "braille_toggleTether" : ("br(ecoBraille):f0",),
2. The same thing occurs with point key. It can be fixed using: "kb:enter" : ("br(ecoBraille):point",), as now appears "punto", in Spanish, instead of "point".
3. You can import * from cTypes, though you can also use just: from ctypes import windll, sizeof, byref
4. From winUser, you can just import WNDCLASSEXW, WNDPROC#. Or do you need other functions for mainteinance?
5. We don't need to import wx in ecobraille.py.
6. Perhaps it could be cleaner to use join to find the path of the library, instead of plus (+). I don't know; just an idea.
   If you want to work on the public repo that I will create, let us know to give you writing access.
   Thanks.

Comment 7 by jteh on 2014-04-21 21:43
You may want to hold off on deploying this as an add-on. I'm going to try to review this in hopes of getting it into the core soon er rather than later. I've already done some preliminary review, but haven't had a chance to provide comments yet.

Comment 8 by jteh on 2014-04-21 22:12
Thanks for the code!

A few general questions:

1. Are these displays serial only or are there USB or Bluetooth displays as well?
2. If the dll was written specifically for the NVDA driver, is there any reason you opted for a dll rather than doing everything in Python? We prefer to avoid additional dlls if we can. This one is small enough that it's not a major problem, but if it's all serial, it seems it could all be done in Python fairly easily. If the dll was written before the NVDA driver, I certainly understand this.

For the most part, the code looks pretty good. In addition to comment:6, here is some code review:

log.warning(dllabspath)

I assume this was debugging code that was accidentally left. It should be removed.

      if (wParam == ECO_KEY_STATUS1 ) or (wParam == ECO_KEY_STATUS2) or (wParam == ECO_KEY_STATUS3 ) or (wParam == ECO_KEY_STATUS4) :

Rather than multiple equality checks, use the in operator; e.g. wParam in (foo, bar, bas)

      else:
          if (wParam > 0):

Unless I'm missing something, you can rewrite this as an elif and get rid of one indentation level. You can also get rid of the parentheses.

        elif wParam > 0:

  description = _("Lineas ecoBraille 20/40/80")

Any translatable strings need a translators comment above them. This will do:

    # Translators: The name of a braille display.

Does the display support pressing multiple keys at once? This doesn't seem to be supported by the driver. It's fine if you don't see a need to implement this. I mention it because most drivers do.

There is also some extraneous whitespace, but I can remove that easily enough.

Thanks again!

Comment 9 by jjimenez0 (in reply to comment 6) on 2014-04-22 06:26

Yes, of course I want to work in the public repo. It will be the best for all.
If you could give me writing access i will be very grateful

Comment 10 by jjimenez0 on 2014-04-22 06:55
Hi, here are some answers:

1. EcoBraille displays are good displays but they are quite old, so it has only serial connection.
2. We alredy had the source written for another projects, so it was much easier for us to use a dll library and a little python file to attach to NVDA. But we have no problem writing again all in Python. The only problem may be the time. We have anothers open projects and perhaps would take a little to finish it. I even think that there would be no problem in upload the source of the dll in order to whoever wanted could help us to develop it.
3. Sorry for the logs and for the spanish comments (smile) Hurry are not good for nothing...
4. EcoBraille also allows pressing multiple keys at once, but we alredy have to develop this functionality.
5. Just now we have change the exported functions names of the dll into english. I will test it and upload a new dll with the name of the functions in english.

Comment 11 by jteh (in reply to comment 10) on 2014-04-22 07:11
Replying to jjimenez0:

1. We alredy had the source written for another projects, so it was much easier for us to use a dll library and a little python file to attach to NVDA.

This is fine. Keep using the dll for now if you're already using it elsewhere. At least it's a very small dll.

Comment 12 by norrumar on 2014-04-22 09:04
Hi, a few points:

1. If this driver is incorporated soon in NVDA core, I think we don't need to post the add-on on our add-ons website.
2. Finally I can't create a public repo, since my computer (at home) is unfortunately broken now.
   If you want, I can request this on add-ons list. I tried to send an email about this add-on, but I couldn't do it.
   About the code, there is feed-back reported on NVDA spanish mailing list:
   a. Each routing key moves the cursor to the next position, not to the exact one. I have fixed this using:
   self.routingIndex = index-1 in the right class.
   b. The description of the driver should be writen in English, and imo it would be better, for instance, just to use: _("EcoBraille displays").
   I have an Ecoplus display, wich has 80 cells, as EcoBraille 80, and I can work fine.
   Thanks.

Comment 13 by jjimenez0 on 2014-04-22 09:52
Of course our final target is that the driver is built in NVDA core, but at this moment, details still must be developed, and we need a place to share the source and allow people improve the code. For example, do you have (or perhaps James) a file with all those details in this ticket added? I'm working right now with an old version of the driver (smile). Which is the best way to get this?

Comment 14 by jteh on 2014-04-22 09:55
Generally, unless you aren't able to for some reason, it'd be best if you implement the code review suggestions yourself. Thanks.

Comment 15 by jjimenez0 on 2014-04-22 09:57
I have attached a new dll file in which we have changed the spanish names of the dll functions into the english ones.
The only changes in the python file are those:

• EB_Inicializar moves to EB_Init
• EB_Cerrar moves to EB_Close
• EB_Escribir moves to EB_Write
• EB_ObtenerCeldas moves to EB_GetNumCells

Changes work fine for me

Attachment EcoBraille.dll added by jjimenez0 on 2014-04-22 09:58
Description:
EcoBraille dll with english names for the exported functions

Comment 16 by jjimenez0 on 2014-04-22 10:00
Ok James, I collect all fixed bugs and attach a new python file

Attachment ecoBrailleAddon-1.1.nvda-addon added by jjimenez0 on 2014-04-24 06:12
Description:
New version with fixed bugs and dll functions in english

Comment 17 by jjimenez0 on 2014-04-24 06:14
Hi, I have attached a new version of the addon with all the modifications that Noelia have made and (I hope) all bugs fixed. Also includes the new version of the dll with the name of the functions in english. I hope it works

Comment 18 by norrumar (in reply to comment 17) on 2014-04-24 17:35
Replying to jjimenez0:

Hi, I have attached a new version of the addon with all the modifications that Noelia have made and (I hope) all bugs fixed. Also includes the new version of the dll with the name of the functions in english. I hope it works

Hi, I can't review my modifications: key assignment and deletion of extra spaces.
However, I have a few points to add:

1. I think pass is not necessary after other instructions used to handle exceptions. Or not?
2. I think there are still extra spaces to remove.
3. For consistency with other braille drivers, we can remove the encoding declaration which I added, and fix the path if the driver is to be incorporated in NVDA core.
4. EB_init causes an exception if the display is disconnected. Perhaps it could be handled in other way, though it's certainly a minor point.

If you need it, I can create a branch for this ticket in NVDA addon team, and send commit notifications to a mailing list. It's more difficult to work on binary add-ons. I have my personal computer now and can do it if needed.
Thanks.

Comment 19 by norrumar on 2014-04-28 17:47
Hi, according to our conversation on NVDA's spanish mailing list, I have created t4078 branch for this ticket at
https://bitbucket.org/nvdaaddonteam/nvda
I haven't commited the DLL file, as I suppose it should be included in miscDeps using the git submodule.
Thanks.

Comment 20 by nvdakor on 2014-04-28 21:04
Hi,
Few questions/comments:

• If you don't mind, could you add copyright headers?
• Would you like to add what's new entry and/or user guide entry for Eco displays now or later? At least adding the entry in user guide would help users with knowing NVDA commands for the displays (in my opinion) so they can help you with testing.
  Thanks.

Comment 21 by jteh on 2014-05-01 07:16
The latest version looks good. One tiny issue: there is a comment which says "Braille input", but this is misleading, since it suggests actual braille dots, whereas this is actually dealing with buttons on the display.

Please include a User Guide section for this similar to those for other displays. The What's New entry shouldn't be added to the branch; that will be committed once it goes to master. Btw, this is the purpose of the "Changes document entry" field in tickets.

Thanks.
Changes:
Milestone changed from None to next

Comment 22 by norrumar (in reply to comment 21) on 2014-05-01 09:10
Replying to jteh:

The latest version looks good. One tiny issue: there is a comment which says "Braille input", but this is misleading, since it suggests actual braille dots, whereas this is actually dealing with buttons on the display.

Please include a User Guide section for this similar to those for other displays. The What's New entry shouldn't be added to the branch; that will be committed once it goes to master. Btw, this is the purpose of the "Changes document entry" field in tickets.

Thanks.

I have changed the comment about braille buttons. I have added try... except to handle a possible exception in EB_Init function when the display is disconnect. Thanks for including this driver into NVDA. My job is now happy and better without switching to JAWS just to use Ecobraille.

Comment 23 by jteh (in reply to comment 22) on 2014-05-01 10:10
Replying to norrumar:

I have added try... except to handle a possible exception in EB_Init function when the display is disconnect.

I don't follow. If the display is disconnected, the constructor should fail, so the exception should pass through so NVDA knows the display failed to initialise. We can't do this for BRLTTY, but it should be done for all other displays.

Comment 24 by norrumar (in reply to comment 23) on 2014-05-01 10:21
Replying to jteh:

Replying to norrumar:

I have added try... except to handle a possible exception in EB_Init function when the display is disconnect.

I don't follow. If the display is disconnected, the constructor should fail, so the exception should pass through so NVDA knows the display failed to initialise. We can't do this for BRLTTY, but it should be done for all other displays.

OK. It's reverted now. I was testing this driver (in the add-on), loading it, disconnecting the display and restarting NVDA, connecting, etc. I thought that the information shown in the log file was not needed. But your comment is right for me.
Thanks.

Comment 25 by norrumar on 2014-06-09 16:22
Hi, branch t4078, available at
https://bitbucket.org/nvdaaddonteam/nvda
is finished for now. We'd like to request its revision to be included in NVDA's core.
The DLL for the display can be included into miscDeps.
Many thanks.

Comment 26 by norrumar on 2014-08-05 06:27
Hi, I have added ONCE-CIDAT as a contributor on t4078 branch, at
https://bitbucket.org/nvdaaddonteam/nvda
Thanks.

Comment 27 by jjimenez0 on 2014-12-11 10:47

Hi all,
I have included all the functionality in the python file, so there is no need for the .dll library.
It is a first version of the braille driver and i am sure that it needs some improvements. Please, feel free to modify anything you want.
I have kept the same keys combination that the pugin had.
Hope it works fine!

Comment 28 by norrumar (in reply to comment 27) on 2014-12-11 11:09
Replying to jjimenez0:

Hi all,

I have included all the functionality in the python file, so there is no need for the .dll library.

It is a first version of the braille driver and i am sure that it needs some improvements. Please, feel free to modify anything you want.

I have kept the same keys combination that the pugin had.

Hope it works fine!

!
Thanks!
If you have the add-on with the Python file instead of the library, you can attach it to test this in my job.
I will create it otherwise, since I have installed a stable version of NVDA here, and cannot test a branch.
Great!

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.

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))

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.

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.

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.

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.

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.

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.

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.

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.

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

Attachment ecoBraille.py added by jjimenez0 on 2015-05-11 11:00
Description:
EcoBraille driver

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)

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

Merge branch 't4078' into next

Incubates #4078.

Changes:
Added labels: incubating

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

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.

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.

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.

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.

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.

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

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