Comment 2 by jteh on 2012-06-18 03:01
Thanks for your great work. Here's some code review:

The module should start with a lower case letter; i.e. seika.py instead of Seika.py. This also means BrailleDisplayDriver.name should be "seika" as well.

CELL_COUNT80 = 80
CELL_COUNT40 = 40
CELL_COUNT16 = 16

Is there any need for these constants? IN this case, they don't seem to serve any purpose, since they include the number anyway.

SEIKA_QUERY  = chr(0xFF) + chr(0xFF) + chr(0x1C)

This could be better written as:

SEIKA_QUERY = "\xFF\xFF\x1C"

Similar for the other constants just below it.

SEIKA_SENDHEADER = SEIKA_SENDHEADER40

This is never used, so should be removed.

SEIKA_KEYMAP = [   "LEFT",

This map doesn't seem to correspond in any way with the bytes received from the display, which might make the code cleaner. I'll cover this more below. Also a cosmetic issue: the entries are indented with a tab and a space instead of just a tab.

USB_IDS = frozenset((
  "USB\\Vid_10c4&Pid_ea60", # Seika USB to Ser
        "BTHENUM\\{00001101-0000-1000-8000-00805f9b34fb}"

I assume the BTHENUM entry is debugging? Also, the USB\ part seems redundant if this is for USB ids only.

class BrailleDisplayDriver(braille.BrailleDisplayDriver):
  name = "Seika"

Should start with a lower case letter as above.

  description = "Seika Braille"

Perhaps _("Seika braille displays")

  numCells = CELL_COUNT80

Defining this here seems redundant, as it's always set in __init__().

In __init__():

              usbID = hwID.split("&R", 1)[0](
)

I don't understand what this code does. It doesn't seem like it will ever succeed. Can you explain?

          cells = SEIKA_QUERY
          self._ser.write(cells)

No need for the cells variable; better written simply as:

          self._ser.write(SEIKA_QUERY)

          if versionS.startswith(r"seika80"):
...
              SEIKA_SENDHEADER = SEIKA_SENDHEADER80

This won't have the intended effect because you haven't declared it as a global at the top of the function. Anyway, it's never used, so should be removed.

In display():

      else:
          line = SEIKA_SENDHEADER40 + "".join(chr(0)+chr(cell) for cell in cells)

I'd probably write chr(0) as "\0" to avoid an unnecessary function call.

      expectedLength = SEIKA_SENDHEADER_COUNT + 80
      line += chr(0) * (expectedLength - len(line))

I'm possibly misunderstanding this, but I don't think it's necessary. Braille will always send exactly numCells to the display, padding with 0 if necessary.

In handleResponses():

I know this stuff is always nearly impossible to write elegantly. However, there are a few important problems here:

There is a sleep after reading each byte. While small, this will cause lag every time a key is pressed. I'm hoping there must be some way to know how many bytes need to be read in advance. Does the first byte provide information about this perhaps?
There is quite a lot of hard-coding here. The determination of keys is done in a big if/elif block. In addition, the indexes into SEIKA_KEYMAP are hard-coded, which makes the key map pointless as a constant.
One option might be to have a list of the keys corresponding to each bit, combine the ordinal values for all the bytes and then enumerate through the list of keys checking for each bit. You don't even need to combine the ordinal values if you prefer; you could just have a list of keys for each byte. This is how I handle it in the baum driver. At the very least, if the rest of the if/elif block stays the same, I would remove SEIKA_KEYMAP, as it doesn't seem to serve any real purpose.

Btw, another cosmetic thing: j += 1 is probably nicer than j = j + 1. You can do similarly with other operators.

Please let me know if you need further explanation on any of these points. Thanks once again.

Uh oh!

Braille Display Driver for Seika40/80 #2452

Description

Metadata

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

Issue actions