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!

Uh oh!

Braille Display Driver Hedo Germany #1863

Description

Activity

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions