Attachment changes.diff added by orcauser on 2011-10-24 12:17
Description:
suggested improvements

Comment 3 by orcauser on 2011-10-24 12:19
Hi Sebastian,

I have never written a braille driver before, so the below suggestions are simply regarding python. I'm sure you will get further feedback from Jamie or Mick.

attached is a small diff that:

• removes hard coded values such as 80, 84, and replaces them by the constants defined at the top.
• removes slow concatunations of strings by faster operations.

Best,

Mesar

Comment 4 by orcauser on 2011-10-24 12:25
sorry, forgot to mension, NVDA code uses tabs for indentation, you are using 4 spaces. When everything is sorted out please convert into tabs.
Thanks.

Comment 5 by sebastian.kruber@... on 2011-10-24 14:10
Hi Mesar,

I added the source code with the changes you suggested.

Can you recommend a Windows tool to edit Python code and to apply diff files?

Thank you!

Sebastian

Comment 6 by jteh on 2011-10-25 05:13
Thanks for your contribution. Excellent work.

First, probing serial ports like this is time consuming and could potentially confuse another device. This is one of the reasons we choose not to support serial displays in general. I assume the newer !ProfiLine is USB emulating a serial port?

Here's some code review.

HEDO_NAME = "hedoProfiLine"
HEDO_DESCRIPTION = _("hedo / hedoProfiLine")

nit: I don't think these two constants are necessary. I'd just assign those strings directly to the name and description class variables.

def enumerate_serial_ports():

Why not use hwPortUtils.listComPorts? Is there something that this function doesn't do that you need? I'd prefer to avoid unnecessary code duplication, so it'd be good to get this functionality into hwPortUtils so it can be used by others.

      display_found = False
      ports = enumerate_serial_ports()
      for portname in ports:
          display_found = serial_check(self._ser, full_port_name(portname))
          if display_found:
              break
      if display_found == False:
          log.debug("No display found")
          raise RuntimeError("No hedo braille display found")

This code can be simplified/shortened quite a bit:

        for portname in enumerate_serial_ports():
            if serial_check(self._ser, full_port_name(portname))
                break
        else:
            log.debug("No display found")
            raise RuntimeError("No hedo braille display found")

Note the use of for/else. The else clause gets executed if no break is reached. Also, avoid var == False; instead, just do not var.

      self._keymap = dict([           [0x04, "K1"](
), # K1
...

I think this dict can be defined as a module level variable, rather than redefining it each time in the constructor. Also, constructing a dict this way is inefficient, since you're creating many lists which will just be discarded. Syntax for defining a basic dict is:

d = {
    key1: val1,
    key2: val2,
    ...
}

  def _get_numCells(self):
      return HEDO_CELL_COUNT

Since this is a hard-coded property, you can just define it as a class attribute:

    numCells = HEDO_CELL_COUNT

_get_xxx is just a special method that gets converted to a property. (I realise this isn't clear from reading the !BrailleDisplayDriver class documentation.)

  def handleData(self, data):
      keys = None
      index = None
...

Because you know early in the method whether you're delaing with a routing key, it's probably easier to just return early. For example:

        if data >= HEDO_CR_BEGIN and data <= HEDO_CR_END:
            # Routing key is pressed
            index = data - HEDO_CR_BEGIN
            try:
                inputCore.manager.executeGesture(InputGestureRouting(index))
            except inputCore.NoInputGestureAction:
                pass
            return

        elif data >= (HEDO_CR_BEGIN + HEDO_RELEASE_OFFSET) and data <= (HEDO_CR_END + HEDO_RELEASE_OFFSET):
            # Routing key is released
            return

Thanks again!

Comment 7 by sebastian.kruber@... on 2011-10-25 10:15
Hi James,

Thank you for the review.

I updated the source code like you suggested. Our serial braille displays are no longer supported, but the newer USB ones are.

Looking forward to hear from you again.

Sebastian

Comment 8 by jteh on 2011-10-27 02:12
Excellent! Looks great. Just a few final comments:

  def __init__(self):
...
      for portInfo in sorted(hwPortUtils.listComPorts(onlyAvailable=True), key=lambda item: "bluetoothName" in item):

You only need this bluetooth sorting thing if your display supports bluetooth and you want those ports to appear last in the list. It looks like you don't support bluetooth, so just do:

for portInfo in hwPortUtils.listComPorts(onlyAvailable=True):

              log.info("Found hedoProfiLine connected via {port}".format(port=port))
              break;

nit: Unnecessary semi-colon after break statement.

With these changes, the driver is good to be included in the main distribution.
Changes:
Milestone changed from None to 2012.1

Comment 9 by jteh on 2011-10-27 02:15
One other thing. You have the description as "hedo / hedoProfiLine". I don't quite follow this. Should it just be "hedo !ProfiLine USB"? Also, should hedo have a capital H or not?

Comment 10 by sebastian.kruber@... on 2011-10-27 08:33
Servus James!

Thank you again for the C to Python migration support. I changed the code like you suggested and deleted the semi-colon and the port list sorting. The drivers name is now "hedo ProfiLine USB". Our company hedo is written in small letters. We also have a hedo PrivatLine in our portfolio, for which I want to write a driver after this process is finished.

I am looking forward for the driver to be included in the main distribution.

Greetings from ice cold bavaria,
Sebastian

Comment 11 by jteh on 2011-10-31 08:59
Sorry, Sebastian. I missed one more thing.

Currently, the driver probes all devices that have a hardware ID beginning with "FTDIBUS\COMPORT". However, these may not necessarily be your display, as other devices can use FTDI serial ports. It'd be great if you could restrict this check so it only covers your displays. See the baum driver for how I achieved this.

Attachment hedoProfiLine.py added by sebastian.kruber@... on 2011-11-02 13:13
Description:
Updated source code

Comment 12 by sebastian.kruber@... on 2011-11-02 13:13
Yes I see... we have our own PID. I added the checking to the code.

Comment 13 by jteh on 2011-11-04 07:47
Committed in 5195d39. The only change I made was commenting out the log.info which logs all scanned serial ports, as the user doesn't care about this.

I added documentation in 41157d2.

Thanks for the contribution!
Changes:
State: closed

Comment 14 by sebastian.kruber@... on 2011-11-04 09:59
Nice to hear... thank you for your support!

Comment 15 by sebastian.kruber@... on 2011-11-10 15:52
I wrote a second braille display driver for the hedo MobilLine USB.

http://www.nvda-project.org/ticket/1897