Braille Display Driver Hedo Germany

[sebastian.kruber@hedo.de]

Driver to access braille display hedoProfiLine

[orcauser]

suggested improvements

[orcauser]

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

[orcauser]

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.

[sebastian.kruber@hedo.de]

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

[jteh]

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!

[sebastian.kruber@hedo.de]

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

[jteh]

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.

[jteh]

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?

[sebastian.kruber@hedo.de]

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

[jteh]

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.

[sebastian.kruber@hedo.de]

Updated source code

[sebastian.kruber@hedo.de]

Yes I see... we have our own PID. I added the checking to the code.

[jteh]

Committed in changeset:main,4777. 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 changeset:main,4778.

Thanks for the contribution!

[sebastian.kruber@hedo.de]

Nice to hear... thank you for your support!

[sebastian.kruber@hedo.de]

I wrote a second braille display driver for the hedo MobilLine? USB.

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