Rewrite parallel transport, linux dropped data fix (#59)

* Fix losing data in last chunk due to how  linux usblp driver works in non blocking mode
* Use twisted.internet.abstract.FileDescriptor instead of StdIO as base
* Add an option not to close connection after job for serial/parallel transports. It has caused multiple issues with various transport backends. And for some interactive usecases it might be more convenient

Author: karliss

inkcut/device/plugin.py

@@ -88,6 +88,14 @@ def disconnect(self):
         """
         raise NotImplementedError
 
+    @property
+    def always_disconnect_after_job(self) -> bool:
+        True
+
+    @property
+    def auto_disconnect_after_job(self) -> bool:
+        return True
+
 
 class TestTransport(DeviceTransport):
     """ A transport that captures protocol output """
@@ -910,7 +918,7 @@ def submit(self, job, test=False):
                 for path in model.toSubpathPolygons(m):
                     for i, p in enumerate(path):
                         whole_path.lineTo(p)
-                        total_lines += 1
+                    total_lines += 1
                 total_length = whole_path.length()
                 total_moved = 0
                 log.debug("device | Path length: {}".format(total_length))
@@ -1033,7 +1041,7 @@ def submit(self, job, test=False):
                         log.error(traceback.format_exc())
                         raise
                     finally:
-                        if connection.connected:
+                        if connection.connected and connection.auto_disconnect_after_job:
                             yield defer.maybeDeferred(self.disconnect)
 
             #: Set the origin

inkcut/device/transports/parallelport/plugin.py

@@ -15,12 +15,13 @@
 import traceback
 import subprocess
 from glob import glob
-from atom.api import Atom, List, Str, Instance
+from atom.api import Atom, List, Str, Instance, Value
 from inkcut.core.api import Plugin, log
+from inkcut.device.plugin import DeviceTransport
+from twisted.internet import abstract, fdesc
+import twisted.internet
 
-from inkcut.device.transports.raw.plugin import (
-    RawFdTransport, RawFdProtocol, RawFdConfig
-)
+from inkcut.device.transports.raw.plugin import RawFdProtocol, RawFdConfig
 
 
 class ParallelPortDescriptor(Atom):
@@ -32,7 +33,7 @@ def __str__(self):
 
 
 def find_dev_name(dev):
-    """ Use udevadm to lookup info on a device
+    """Use udevadm to lookup info on a device
 
     Parameters
     ----------
@@ -46,33 +47,33 @@ def find_dev_name(dev):
 
     """
     try:
-        cmd = 'udevadm info -a %s' % dev
+        cmd = "udevadm info -a %s" % dev
         manufacturer = ""
         product = ""
 
         output = subprocess.check_output(cmd.split())
         if sys.version_info.major > 2:
             output = output.decode()
-        for line in output.split('\n'):
+        for line in output.split("\n"):
             log.debug(line)
             m = re.search(r'ATTRS{(.+)}=="(.+)"', line)
             if m:
                 k, v = m.groups()
-                if k == 'manufacturer':
+                if k == "manufacturer":
                     manufacturer = v.strip()
-                elif k == 'product':
+                elif k == "product":
                     product = v.strip()
             if manufacturer and product:
-                return '{} {}'.format(manufacturer, product)
-        log.warning('Could not lookup device info for %s' % dev)
+                return "{} {}".format(manufacturer, product)
+        log.warning("Could not lookup device info for %s" % dev)
     except Exception as e:
         tb = traceback.format_exc()
-        log.warning('Could not lookup device info for %s  %s' % (dev, tb))
-    return 'usb%s' % dev.split('/')[-1]
+        log.warning("Could not lookup device info for %s  %s" % (dev, tb))
+    return "usb%s" % dev.split("/")[-1]
 
 
 def find_ports():
-    """ Lookup ports from known locations on the system
+    """Lookup ports from known locations on the system
 
     Returns
     -------
@@ -81,22 +82,22 @@ def find_ports():
 
     """
     ports = []
-    if 'win32' in sys.platform:
+    if "win32" in sys.platform:
         pass  # TODO
-    elif 'darwin' in sys.platform:
+    elif "darwin" in sys.platform:
         pass  # TODO
     else:
-        for p in glob('/dev/lp*'):
+        for p in glob("/dev/lp*"):
             # TODO: Get friendly device name
-            name = p.split('/')[-1]
+            name = p.split("/")[-1]
             ports.append(ParallelPortDescriptor(device=p, name=name))
 
-        for p in glob('/dev/parport*'):
+        for p in glob("/dev/parport*"):
             # TODO: Get friendly device name
-            name = p.split('/')[-1]
+            name = p.split("/")[-1]
             ports.append(ParallelPortDescriptor(device=p, name=name))
 
-        for p in glob('/dev/usb/lp*'):
+        for p in glob("/dev/usb/lp*"):
             name = find_dev_name(p)
             ports.append(ParallelPortDescriptor(device=p, name=name))
 
@@ -122,15 +123,122 @@ def refresh(self):
         self.ports = self._default_ports()
 
 
-class ParallelTransport(RawFdTransport):
-    """ This is just a wrapper for the RawFdTransport
+class ParallelTwistedTransport(abstract.FileDescriptor):
+    connected = 1
+
+    def __init__(
+        self,
+        port_name: str,
+        protocol: RawFdProtocol,
+        reactor=None,
+    ):
+        abstract.FileDescriptor.__init__(self, reactor)
+        self.fd = open(port_name, "r+b")
+        fdesc.setNonBlocking(self.fileno())
+        self.protocol = protocol
+        self.written_something = False
+        self.protocol.makeConnection(self)
+        self.startReading()
+
+    def fileno(self):
+        return self.fd.fileno()
+
+    def writeSomeData(self, data):
+        res = fdesc.writeToFD(self.fileno(), data)
+        self.written_something = self.written_something or res > 0
+        return res
+
+    def doRead(self):
+        return fdesc.readFromFD(self.fileno(), self.protocol.dataReceived)
+
+    def doWrite(self):
+        self.written_something = False
+        return abstract.FileDescriptor.doWrite(self)
+
+    def stopWriting(self):
+        abstract.FileDescriptor.stopWriting(self)
+
+    def connectionLost(self, reason):
+        if (
+            self.written_something
+            and "linux" in sys.platform
+            and isinstance(reason.value, twisted.internet.error.ConnectionDone)
+        ):
+
+            # Queue up one more iteration in select loop to wait until write is really done.
+            # Linux usblp driver has a documented quirk where closing it in nonblocking mode can cause dropping
+            # pending data.
+            # https://github.com/torvalds/linux/blob/df87d843c6eb4dad31b7bf63614549dd3521fe71/drivers/usb/class/usblp.c#L893C1-L898C1
+            # https://github.com/karliss/inkcut/issues/56
+            self.startWriting()
+            return
+
+        abstract.FileDescriptor.connectionLost(self, reason)
+        self.fd.close()
+        self.protocol.connectionLost(reason)
+        log.debug("Closed {}".format(self.fd.name))
+
+
+class ParallelTransport(DeviceTransport):
+    """This is just a wrapper for the RawFdTransport"""
 
-    """
     #: Default config
     config = Instance(ParallelConfig, ()).tag(config=True)
 
+    #: Current path
+    device_path = Str()
+    #: Wrapper which forwards twisted protocol to inkcut
+    _wrapper_protocol = Instance(RawFdProtocol)
+
+    #: A raw device connection
+    connection = Instance(ParallelTwistedTransport)
+
+    def connect(self):
+        config = self.config
+        device_path = self.device_path = config.device_path
+        try:
+            log.debug("-- {} | opened".format(device_path))
+            self._wrapper_protocol = RawFdProtocol(self, self.protocol)
+            self.connection = ParallelTwistedTransport(
+                device_path, self._wrapper_protocol
+            )
+        except Exception as e:
+            #: Make sure to log any issues as these tracebacks can get
+            #: squashed by twisted
+            log.error(
+                "Parallel port open failed {} | {}".format(
+                    device_path, traceback.format_exc()
+                )
+            )
+            raise
+
+    def write(self, data):
+        if not self.connection:
+            raise IOError("{} is not opened".format(self.device_path))
+        log.debug("-> {} | {}".format(self.device_path, data))
+        if hasattr(data, "encode"):
+            # TODO: cleanup str/byte handling transport should always receive bytes not strings
+            data = data.encode()
+        self.last_write = data
+        self.connection.write(data)
+
+    def disconnect(self):
+        if self.connection:
+            log.debug("-- {} | closed by request".format(self.device_path))
+            self.connection.loseConnection()
+            self.connection = None
+
+    def __repr__(self):
+        return self.device_path
+
+    @property
+    def always_disconnect_after_job(self) -> bool:
+        return False
+
+    @property
+    def auto_disconnect_after_job(self) -> bool:
+        return self.config.close_after_job
 
-class ParallelPlugin(Plugin):
-    """ Plugin for handling parallel port communication
 
-    """
+class ParallelPlugin(Plugin):
+    """Plugin for handling parallel port communication"""

inkcut/device/transports/parallelport/settings.enaml

@@ -49,3 +49,8 @@ enamldef ParallelPortSettingsView(Container):
                 text = QApplication.translate("parallelport", "Refresh")
                 icon = load_icon("arrow_refresh")
                 clicked :: model.refresh()
+        Label:
+            pass
+        CheckBox:
+            text = QApplication.translate("transport", "Disconnect after job")
+            checked := model.close_after_job

inkcut/device/transports/qtserialport/plugin.py

@@ -151,6 +151,14 @@ def disconnect(self):
     def __repr__(self):
         return self.device_path
 
+    @property
+    def auto_disconnect_after_job(self) -> bool:
+        return self.config.close_after_job
+
+    @property
+    def always_disconnect_after_job(self) -> bool:
+        return False
+
 
 class QtSerialPlugin(Plugin):
     """ Plugin for handling serial port communication

inkcut/device/transports/raw/plugin.py

@@ -14,24 +14,26 @@
 import sys
 import traceback
 from atom.atom import set_default
-from atom.api import Value, Instance, Str, Enum
+from atom.api import Value, Instance, Str, Enum, Bool
 from inkcut.core.api import Plugin, Model, log
-from inkcut.device.plugin import DeviceTransport
+from inkcut.device.plugin import DeviceTransport, DeviceProtocol
 from twisted.internet import reactor, stdio
 from twisted.internet.protocol import Protocol, connectionDone
 
 
 class RawFdConfig(Model):
     device_path = Str("/dev/null").tag(config=True)
-    mode = Enum('r+b', 'wb', 'r+', 'w').tag(config=True)
+    mode = Enum("r+b", "wb", "r+", "w").tag(config=True)
+    close_after_job = Bool(True).tag(config=True)
 
 
 class RawFdProtocol(Protocol, object):
     """Make a twisted protocol that delegates to the inkcut protocol
     implementation to have a consistent api (and use proper pep 8 formatting!).
 
     """
-    def __init__(self, transport, protocol):
+
+    def __init__(self, transport: DeviceTransport, protocol: DeviceProtocol):
         self._transport = transport
         self.delegate = protocol
 
@@ -47,7 +49,8 @@ def dataReceived(self, data):
 
     def connectionLost(self, reason=connectionDone):
         self._transport.connected = False
-        self._transport.fd = None
+        if hasattr(self._transport, "fd"):
+            self._transport.fd = None
         device_path = self._transport.device_path
         log.debug("-- {} | dropped: {}".format(device_path, reason))
         self.delegate.connection_lost()
@@ -73,7 +76,7 @@ class RawFdTransport(DeviceTransport):
     def connect(self):
         config = self.config
         device_path = self.device_path = config.device_path
-        if 'win32' in sys.platform:
+        if "win32" in sys.platform:
             # Well, technically it works, but only with stdin and stdout
             raise OSError("Raw device support cannot be used on Windows")
 
@@ -93,7 +96,7 @@ def write(self, data):
         if not self.connection:
             raise IOError("{} is not opened".format(self.device_path))
         log.debug("-> {} | {}".format(self.device_path, data))
-        if hasattr(data, 'encode'):
+        if hasattr(data, "encode"):
             data = data.encode()
         self.last_write = data
         self.connection.write(data)
@@ -104,6 +107,13 @@ def disconnect(self):
             self.connection.loseConnection()
             self.connection = None
 
+    @property
+    def auto_disconnect_after_job(self) -> bool:
+        return self.config.close_after_job
+
+    @property
+    def always_disconnect_after_job(self) -> bool:
+        return False
+
     def __repr__(self):
         return self.device_path
-

inkcut/device/transports/raw/settings.enaml

@@ -12,7 +12,7 @@ Created on Feb 2, 2019
 """
 from inkcut.core.utils import load_icon
 from enaml.qt.QtWidgets import QApplication
-from enaml.widgets.api import Container, Form, Label, Field, ObjectCombo
+from enaml.widgets.api import Container, Form, Label, Field, ObjectCombo, CheckBox
 
 
 enamldef RawFdSettingsView(Container):
@@ -28,3 +28,8 @@ enamldef RawFdSettingsView(Container):
         ObjectCombo:
             items << list(model.get_member('mode').items)
             selected := model.mode
+        Label:
+            pass
+        CheckBox:
+            text = QApplication.translate("transport", "Disconnect after job")
+            checked := model.close_after_job