SerialPort never calls connectionLost(reason)

[jerub]

Here is a minimal working example showing the problem.

from twisted.internet.serialport import SerialPort
from twisted.protocols.basic import LineOnlyReceiver
from twisted.internet import reactor

class SerialProtocol(LineOnlyReceiver):
    def connectionMade(self):
        print 'connection made'
        self.transport.loseConnection()

    def connectionLost(self, reason):
        print 'connectionLost called'

port = SerialPort(SerialProtocol(), '/dev/ttyUSB2', reactor)

reactor.callLater(10, reactor.stop)
reactor.run()

This prints:

# python testserial.py 
Connection made
#

And it should print:

# python testserial.py 
Connection made
connectionLost called
#

[glyph]

I don't have a serial port handy. Do you? :)

[exarkun]

(In [28054]) Branching to 'serialport-connectionlost-3690'

[exarkun]

(In [28055]) A test for serialport loseConnection and a fix to make it pass

refs #3690

[exarkun]

Here's sort of an attempt. Maybe there should be a bit better testing (eg for exception handling around connectionLost). I wonder if this testing strategy is viable, though.

[jesstess]

• On my OSX laptop running 2.6 the test errors out with exceptions.ImportError: No module named serial.  buildbot results suggest that this is a problem for all non-2.5 machine.
• copyright bump in _posixserialport.py
• addition of copyright in test_serialport.py

How many build machines will pass the check on the existence of "/dev/ttyUSB0"?

[exarkun]

(In [28110]) Address review comments

1. Skip if pyserial is not installed
2. Update copyright date in _posixserialport.py
3. Add copyright to test_serialport.py
4. Switch to trying to open serial port "0" which pyserial will interpret in a platform dependent manner, which should make the test runnable on more systems

refs #3690

[exarkun]

Thanks for the review. Addressed everything, I think, including the USB0 thing. Most systems should have ttyS0 or COM1. ttyUSB0 was a silly choice, but the machine I developed on had it. :)

I'll also see what I can do about getting pyserial installed on as many of the build slaves as possible.

[exarkun]

That wasn't very successful. I wasn't able to get any of the slaves to be able to run this test. Apparently none of them have real serial controllers. I'm going to look into addressing this on at least one of the slaves, but it might be a little while before that happens.

Assuming a reviewer has access to a machine with a serial controller, I don't think this necessarily needs to block this ticket from being resolved.

[jesstess]

I tested this on my Mac and on an Ubuntu machine. On the Mac, after installing pySerial and changing portName to "/dev/tty.usbserial" it passed under SelectReactor and skipped on everything else. On the Ubuntu machine, after changing portName to "/dev/ttyUSB0" it passed under Glib2Reactor, Gtk2Reactor, PollReactor, and SelectReactor and skipped everything else.

So: the test looks good and passes, but only if I hardcode the right device name. On my Mac, leaving portName as 0 I get: Cannot open serial port: could not open port 0: [Errno 2] No such file or directory: '/dev/cuad0'

and on the Ubuntu machine I get: Cannot open serial port: Port not open

[exarkun]

For actually testing the serial port code, there's #2462.

[exarkun]

This will conflict with #4525, but perhaps this should be resolved first since it introduces some unit tests for the serial port code, whereas now there are none, even though the tests won't run on any build slaves until we do something about their configuration.

I think jesstess's last comment was basically an endorsement for merge, but I want to make sure.

[ltaylor.volks]

I had some trouble running the test on both OSX and Windows...

OSX 10.5.8

• SelectReactor passes, everything but PollReactor is skipped.
• PollReactor results in an error:

  [ERROR]: twisted.internet.test.test_serialport.SerialPortTestsBuilder_PollReactor.test_loseConnection
  
  Traceback (most recent call last):
  Failure: twisted.internet.error.ConnectionLost: Connection to the other side was lost in a non-clean fashion.
  

I also had to explicitly set the portName for my usb adapter (/dev/tty.usbserial-FTCDCGWA)

Windows XP:

I can't get the Win32event reactor test to run due to some weirdness.

So when specifying portName="COM3" (a valid port), the test skips:

[SKIPPED]: twisted.internet.test.test_serialport.SerialPortTestsBuilder_Win32Reactor.test_loseConnection

Cannot open serial port: could not open port: (5, 'CreateFile', 'Access is denied.')

But if I directly instantiate the Serial object, the port is OK:

>>> import serial
>>> s = serial.Serial("COM3")
>>> s
Serial<id=0x13c12d0, open=True>(port='COM3', baudrate=9600, bytesize=8, parity='
N', stopbits=1, timeout=None, xonxoff=0, rtscts=0, dsrdtr=0)

So, I don't know what's going on there. I'll investigate further.

But equally important is that the Select reactor test will always ERROR due to problem addressed in #3802

[ERROR]: twisted.internet.test.test_serialport.SerialPortTestsBuilder_SelectReac
tor.test_loseConnection

Traceback (most recent call last):
  File "C:\Development\Projects\Twisted\branches\serialport-connectionlost-3690\
twisted\internet\test\test_serialport.py", line 41, in test_loseConnection
    port = SerialPort(protocol, self.portName, reactor)
  File "C:\Development\Projects\Twisted\branches\serialport-connectionlost-3690\
twisted\internet\_win32serialport.py", line 56, in __init__
    self.reactor.addEvent(self._overlappedRead.hEvent, self, 'serialReadEvent')
exceptions.AttributeError: 'SelectReactor' object has no attribute 'addEvent'

Should the test be rewritten to skip unless run under win32eventreactor?

One more thing... self.protocol.connectionLost(reason) needs to be added to _win32serialport.py as with _posixserialport.py

[jesstess]

You're good to go from an OS X 10.6 perspective. I can't speak for 10.5 (we also don't have a buildbot for it...), but this + ltaylor.volks' comments ==> reviewed.

[exarkun]

(In [29598]) Branching to 'serialport-connectionlost-3690-2'

[exarkun]

Should the test be rewritten to skip unless run under win32eventreactor?

Yep. I changed the skip logic in r29600.

PollReactor results in an error

I'd forgotten OS X 10.5 ships with select.poll. It really shouldn't, since poll is quite broken on OS X. This isn't a problem on 10.6, which is the only version of OS X we're claiming to support now, so I don't know if it's worth going out of our way to try to make this test pass (or skip, as would likely be necessary, it seems plausible that serialports are not well supported by poll on os x) in this case.

I can't get the Win32event reactor test to run due to some weirdness.

Blargh. This seems to be worth figuring out before merging anything, though. Let me know if you figure anything out. I don't have a Windows machine to do testing on, so I probably won't be able to investigate this any time soon.

Thanks very much for taking a look. Let me know if you'd rather not be made nosy on issues.

[jknight]

BTW: Poll is no more broken than kqueue on modern OSX versions. It used to be implemented as a wrapper over select, which was completely busted. Then it was implemented as a kernel call but was broken in various ways.

But now it works great...on sockets. Not serial ports, not ptys, pretty much not anything else. Presumably some day Apple will get around to creating an OS that doesn't suck, but until then, yea, ignore poll.

[michaelnt]

I have added a patch that should resolve this ticket and tested that it passes on Windows (7) and Linux.

• modify test_serialport so that only the win32 reactor is tested on windows.
• modify the win32reactor to suppress the traceback when trying to write to a closed fd, and added the missing connectionLost call to _win32serialport.
• added a function which returns a valid serial port for each platform.

[michaelnt]

This ticket is blocking

#4525 and #3802

#2462 is also related (describing mocking the serial port)

[jerub]

There is both a patch and a branch for this ticket, and both must be applied in order to test.

Please provide either a patch against trunk with all changes, or a branch integrating the patch. I can't effectively review this by looking at the changes in both the patch and the branch simultaneously.

From the branch:

+# Gross sibling Twisted import
 from serialport import BaseSerialPort

Instead of commenting that there is interesting code here, change the code please. Make it an absolute import.

I don't like how importing the test causes serial ports to be opened on posix based systems, think that should happen in the setUp, please move this out of the class suite intwisted/internet/test/test_serialport.py class SerialPortTestsBuilder:

    portName = getDefaultPort() 

If these things are fixed up, and either a patch or a branch that can be cleanly applied to trunk are provided, I think this change should be merged.

[michaelnt]

I don't have svn access so I can't update the branch, I'll create a patch against trunk.

I'll move the getDefaultPort call to a setUp method.

Is it OK to leave the definition of getDefaultPort in the same file as the serial port or should it be in a test file?

[exarkun]

Please provide either a patch against trunk with all changes, or a branch integrating the patch.

Blaugh. michaelnt's patch against the branch is what we always ask for, and is preferable for many reasons. It lets him check out our branch and work against it, instead of having to maintain patches to trunk. It means the new changes being submitted are obvious and separate from the previous work on the ticket. It means the branch can actually be updated with the patch. When you see a ticket in this state, you should apply the patch to the branch and then do the review.

I don't have svn access so I can't update the branch, I'll create a patch against trunk.

Please keep doing what you were doing before. :)

Is it OK to leave the definition of getDefaultPort in the same file as the serial port or should it be in a test file?

Probably having it in the test file is better, unless there's a reason that Twisted should be providing this API to people. Also, I think you can simplify it quite a bit - serial.Serial will accept integers and construct the platform-appropriate device name based on them. eg, serial.Serial(0) is like serial.Serial('/dev/ttyS0') on Linux.

[michaelnt]

I've added a patch that implements a mock serial port on windows and posix. My plan is to extend the test cases to run against a real serial port if an environment variable is set and run against this mock and write some more tests.

Would be nice to get some feedback if this is heading in the right direction and ideally run across some buildslaves?

Also any pointers on how to eliminate the need for _MockWindowsEchoPort would be good.

[lvh]

Hi! I'm a recovering serial port victim.

First off: this code is *definitely* going in the right direction, please carry on :)

There's a few style issues:

1. Default value arguments should not have spaces before and after the =, so not def f(x = 1): ... but def f(x=1): ... -- both when defining and when calling. Many of these are existing problems, but this patch should fix them.
   1. In your patch, line 27 of _posixserialport, the __init__ method
   2. In your patch, line 31 of _posixserialport, self._serial = self.SerialClass(...)
   3. In your patch, line 44 of _win32serialport, SerialPort.__init__
2. Attributes should be in mixedCase, even if they're a class. This means the SerialClass classattr should actually be serialClass (or maybe even serialFactory)
3. Operators should have spaces around them.
   1. In your patch, line 44 of _win32serialport, SerialPort.doWrite, there's no spaces around n==0. I had to look up n was the number of bytes received. Perhaps it should be renamed bytesReceived. Perhaps that line should be if not bytesReceived:.
   2. In that same doWrite method, if len(self.outQueue)> 0: needs a space. I'd replace it with if self.outQueue:, though.
   3. in the _serial_win test method, 175, 177, 191: missing space around | operator
4. Long import lines get several lines.
   1. In your patch, line 27 of test_serialport, the import should match  the coding standard

In the tests, _serial_win, man, win32pipe.CreateNamedPipe and win32file.CreateFile make me want to cry. This is not your fault at all, but perhaps the way they are called could be a bit cleaner. Perhaps using kwargs and creating local variables for those things?

There's a few implementation questions:

1. In your patch, line 110 of _win32serialport, except Exception as e: around a ReadFile call. Is that really the exception you want to catch?
2. In doWrite, you only write the first thing you pop from the queue. Is that right? The method's docstring appears to suggest it should write everything. Also, if it's a queue and not a stack, why is it pop and not popleft?

Thanks in any case for your awesome contribution to this previously hairier piece of code :)

[itamar]

This ticket appears to have mostly evolved into testing serial port support, rather than the original reported problem... so I'm going to use #4249 for the actual connectionLost problem. #2462 is another ticket for testing serial ports with its own suggestions.

[itamar]

I moved the branch from here, that was mostly about testing, to #2462 which is a serial port testing ticket.

This ticket, which is about connectionLost not being called, now has a new branch limited to that fix only, so it'll be easier to get review approval.

[exarkun]

(In [32977]) Branching to 'serialport-connectionlost-3690-4'

[exarkun]

Oops. When you asked about renaming the branches, I should have thought a little harder. You should never "svn mv" a branch. Or more generally, never use anything except Combinator (or the exact operations Combinator uses ;) on a branch.

I created a -4 using Combinator so the final merge can be done with Combinator.

[exarkun]

Thanks for working on this. I know a lot of people want to use Twisted with serial ports, so it's nice to make some progress here.

1. Please document _serialFactory on BaseSerialPort
2. SerialPortTests class docstring is funny. :)
3. Both twisted.internet._posixserialport.SerialPort.connectionLost and twisted.internet._win32serialport.SerialPort.connectionLost could benefit from docstrings
4. I've come to prefer the style of putting skips at the top of the class definition, rather than at the end of the module. If you agree and think it's worth it, maybe change the style of this skip to match that.
5. The news fragment should have a different ticket number in it now

Otherwise seems good (though unfortunately I am still unable to test using a real serial device), including  build results - at least if we ignore the always interesting, always fresh potpourri of Windows garbage. Please merge when the above points are addressed.

[itamarst]

(In [33004]) Merge serialport-connectionlost-3690-4.

Author: itamar Review: exarkun Fixes: #3690

Serial ports now correctly call connectionLost on the protocol when disconnected.