Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.Sign up
Braille Display Driver Hedo Germany #1863
Comment 3 by orcauser on 2011-10-24 12:19
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:
Comment 6 by jteh on 2011-10-25 05:13
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.
nit: I don't think these two constants are necessary. I'd just assign those strings directly to the name and description class variables.
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.
This code can be simplified/shortened quite a bit:
Note the use of for/else. The else clause gets executed if no break is reached. Also, avoid
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:
Since this is a hard-coded property, you can just define it as a class attribute:
_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.)
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:
Comment 7 by sebastian.kruber@... on 2011-10-25 10:15
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.
Comment 8 by jteh on 2011-10-27 02:12
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:
nit: Unnecessary semi-colon after break statement.
With these changes, the driver is good to be included in the main distribution.
Comment 10 by sebastian.kruber@... on 2011-10-27 08:33
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,
Comment 11 by jteh on 2011-10-31 08:59
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.