Opened 5 years ago

Last modified 3 years ago

#3802 defect new

win32 SerialPort requires a reactor that provides "addEvent" method, but doesn't have a good error message if it gets a different one

Reported by: tiendalinux Owned by:
Priority: low Milestone:
Component: core Keywords: serialport win32 documentation
Cc: ltaylor.volks, TimAllen, jessica.mckellar@…, jason.heeris@… Branch: branches/win32er-serialport-3802
(diff, github, buildbot, log)
Author: exarkun, ltaylor.volks Launchpad Bug:

Description (last modified by glyph)

SerialPort raises the following exception on Windows if it is used with the default reactor:

  File "twisted\internet\_win32serialport.py", line 56, in __init__
    self.reactor.addEvent(self._overlappedRead.hEvent, self, 'serialReadEvent')
AttributeError: 'SelectReactor' object has no attribute 'addEvent'

It should say something more helpful when given a SelectReactor, explaining that win32eventreactor is required for serial port support on Windows.

Attachments (3)

twisted-3802.patch (3.5 KB) - added by ltaylor.volks 5 years ago.
See comments below
twisted-3802.2.patch (5.6 KB) - added by ltaylor.volks 4 years ago.
Patch addresses review
3802_docstring.patch (2.7 KB) - added by ltaylor.volks 4 years ago.
SerialPort docstring

Download all attachments as: .zip

Change History (32)

comment:1 Changed 5 years ago by itamar

  • Resolution set to invalid
  • Status changed from new to closed

On windows you need to use the win32eventreactor, e.g.:

from twisted.internet import win32eventreactor
win32eventreactor.install()

before you do any other imports, and all the rest of the code can be the same.

comment:2 Changed 5 years ago by itamar

In particular, you need to use this reactor for *serial* support, not for normal usage.

comment:3 Changed 5 years ago by glyph

  • Description modified (diff)
  • Keywords serial win32 added; Serial Win32 removed
  • Milestone Twisted-8.2+1 deleted
  • Priority changed from normal to low
  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Summary changed from SerialPort Outdated to win32 SerialPort requires a reactor that provides "addEvent" method, but doesn't have a good error message if it gets a different one

There's a valid problem here, that's both a documentation and specification problem: twisted.internet.serialport's documentation is unclear as to what is required of the reactor parameter to SerialPort, and it differs between platforms. addEvent is not specified on any reactor interface.

To close this ticket I would say that we need an addEvent method that provides an error message that explains what itamar just did in response to this ticket. There may also be a need for a separate ticket for an interface that explains the semantics of addEvent; ideally this could be added to e.g. the IOCP reactor as well.

comment:4 Changed 5 years ago by glyph

  • Keywords easy added

Since this is a documentation issue, it should be pretty easy to resolve.

comment:5 Changed 5 years ago by tiendalinux

  • Resolution set to fixed
  • Status changed from reopened to closed
  • Type changed from defect to enhancement

Sorry, i initially didn't understand how to use the Win32Reactor, i finally was able to use it, this is a code example, that reads from the serial port and print everything on the screen:

#!/usr/bin/python2.4

from twisted.internet.win32eventreactor import Win32Reactor
from twisted.internet import protocol
from twisted.internet.serialport import SerialPort

class MyProtocol( protocol.Protocol ) :

def dataReceived(self, data):

print data

def connectionLost(self, reason='connectionDone'):

print reason


if name == "main":

port = 0
protocol = MyProtocol()
reactor = Win32Reactor()
myserial = SerialPort (protocol, port, reactor)
print "Server running, press ctrl-C to stop."
reactor.run()

comment:6 Changed 5 years ago by exarkun

  • Keywords documentation added
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Type changed from enhancement to defect

We should still improve our documentation.

comment:7 Changed 5 years ago by exarkun

  • Keywords serialport added; serial removed

Changed 5 years ago by ltaylor.volks

See comments below

comment:8 Changed 5 years ago by ltaylor.volks

  • Keywords review added

twisted-3802.patch does the following:

  • Creates a new IReactorWin32Events interface w/ 2 win32-specific methods: addEvent & removeEvent
  • win32eventreactor now also implements IReactorWin32Events
  • _win32serialport.SerialPort asserts that the reactor passed in provides the IReactorWin32Events interface

comment:9 Changed 5 years ago by TimAllen

  • Author set to ltaylor.volks
  • Owner changed from glyph to TimAllen
  • Status changed from reopened to new

comment:10 Changed 5 years ago by TimAllen

  • Keywords review removed
  • Owner changed from TimAllen to ltaylor.volks

Thanks for writing this patch!

A few code-review comments:

  1. I notice the patch you supplied has a few lines with trailing whitespace, you might want to clean that up.
  2. Twisted likes to keep lines to 79 columns max, so your import in twisted.internet.win32eventreactor and your error message in twisted.internet._win32serialport will need to be wrapped.
  3. assert is a very big stick to wield. Raising something a little less violent like ValueError would be better.
  4. I think your error message is a bit verbose - it's quite possible that some other reactor will work, other than win32eventreactor, and the reactor-installation instructions are well-covered in other parts of the documentation, so no need to repeat them here. Something like this should be sufficient:
    SerialPort on Windows requires a reactor that implements IReactorWin32Events, such as win32eventreactor.
    
  5. I think people investigating Twisted's serial support would best be served by having some actual documentation in twisted.internet.serialport. In an ideal world, there would be an ISerialPort interface describing all the common functionality of the POSIX, Win32 and Java SerialPort implementations, with full docstrings and all that stuff. At the very least, however, twisted.internet.serialport should have a module docstring that mentions that:
    • Twisted's serial port support requires pyserial (perhaps linking to its homepage)
    • On Windows, you need to use a reactor that supports IReactorWin32Events.
  6. There should be a unittest in twisted.internet.test.test_win32eventreactor asserting that win32eventreactor implements IReactorWin32Events (zope.interface.verify.verifyClass should come in handy here). That test file doesn't exist yet, so you'll probably want to read about adding new modules in the Coding Standard. Since win32eventreactor imports Win32-specific modules, you'll need prevent the tests from running on other platforms. I can't find a good example of this in the code at the moment, but I imagine it would look something like this:
    from twisted.trial import unittest
    from twisted.python.runtime import platform
    
    if platform.isWindows():
        from twisted.internet import win32eventreactor
    else:
        win32eventreactor = None
    
    class TestWin32EventReactor(unittest.TestCase):
    
        if win32eventreactor is None:
    	skip = "Win32EventReactor tests can only run on Windows"
    
        # ...actual test methods...
    

Again, thank you for tackling this. Better error messages make life more pleasant for everybody involved!

comment:11 Changed 5 years ago by ltaylor.volks

  • Cc ltaylor.volks TimAllen added

Thanks for the review.

re: 5
A docstring added to twisted.internet.serialport is easy enough.

I may work on an Interface, but it would probably be good to modify the implementations to actually use it. Should that go in a separate ticket?

comment:12 Changed 5 years ago by TimAllen

If it's up to me, I say this ticket is for making a helpful error message on Win32, and re-factoring the code to use interfaces would be a separate ticket.

comment:13 Changed 5 years ago by ltaylor.volks

  • Keywords review added

Attached patch should address the first review

  1. Cleaned up trailing whitepace
  2. Wrapped imports @ 79 chars
  3. ValueError is raised if user instantiates a SerialPort instance on Windows w/o installing a correct reactor
  4. Simplified error message in ValueError
  5. Added docstring to twisted.internet.serialport re: requirements
  6. Added test: test_win32eventreactor to verify it provides the IReactorWin32Events interface

I think this covers the error msg and doc issue and provides a base for fleshing out IReactorWin32Events and tests elsewhere. ISerialPort will have to wait for another day/ticket.

comment:14 Changed 4 years ago by jesstess

  • Cc jessica.mckellar@… added
  • Owner ltaylor.volks deleted

comment:15 Changed 4 years ago by amaury

In the IReactorWin32Events interface, the description of @param action is inaccurate: the actual code passes a string, and the action is run as getattr(fd, action)().

@param action is not a callable, but the name of a method.

comment:16 Changed 4 years ago by ltaylor.volks

Thanks for the correction. Modified as follows:

@param action: a string that is a method name of the fd instance.
               This method is called in response to the event.

Changed 4 years ago by ltaylor.volks

Patch addresses review

comment:17 Changed 4 years ago by exarkun

  • Author changed from ltaylor.volks to exarkun, ltaylor.volks
  • Branch set to branches/win32er-serialport-3802

(In [29190]) Branching to 'win32er-serialport-3802'

comment:18 Changed 4 years ago by exarkun

(In [29191]) Apply twisted-3802.2.patch

refs #3802

comment:19 Changed 4 years ago by exarkun

(In [29192]) Add the test module as well

refs #3802

comment:20 follow-up: Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner set to exarkun

Thanks. This mostly seems good. We should probably document the new exception that SerialPort can raise. I think we also need to write the tests in the reactormixins.ReactorBuilder style, because that way our existing Windows build slaves can run the new test. We don't actually have anything doing trial -r win32er ... at the moment.

comment:21 Changed 4 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

Did that. Build results

Changed 4 years ago by ltaylor.volks

SerialPort docstring

comment:22 in reply to: ↑ 20 Changed 4 years ago by ltaylor.volks

Replying to exarkun:

We should probably document the new exception that SerialPort can raise.

We should probably document SerialPort in general ;)

I've attached a docstring patch, but there's an issue in that the signatures for _posixserialport and _win32serialport differ for some reason (@param timeout). Fixing _win32serialport would reorder the parameters...

I think we also need to write the tests in the reactormixins.ReactorBuilder style, because that way our existing Windows build slaves can run the new test. We don't actually have anything doing trial -r win32er ... at the moment.

I'm pretty sure I don't understand... doesn't buildbot just run everything in the test packages? What is the benefit of reactormixins.ReactorBuilder esp. considering how simple test_win32eventreactor is?

Thanks for the review.

comment:23 Changed 4 years ago by exarkun

(In [29421]) Apply 3802_docstring.patch

refs #3802

comment:24 Changed 4 years ago by exarkun

  • Branch changed from branches/win32er-serialport-3802 to branches/win32er-serialport-3802-2

(In [29423]) Branching to 'win32er-serialport-3802-2'

comment:25 Changed 4 years ago by exarkun

  • Branch changed from branches/win32er-serialport-3802-2 to branches/win32er-serialport-3802
  • Keywords review removed
  • Owner set to exarkun
  • Status changed from new to assigned

I'm pretty sure I don't understand... doesn't buildbot just run everything in the test packages?

Yep. But "trial twisted" runs all of the tests using the default reactor - currently select on all platforms. So tests written in the obvious style, using the global reactor, get run against the select reactor, not the win32 event reactor. In the past we've dealt with this using the trial --reactor option, configuring slaves that run "trial --reactor win32er twisted" alongside slaves that use the default reactor or other reactors. However, it's been a long time since we had a slave configured to actually do "trial --reactor win32er twisted".

The reactormixins.ReactorBuilder style of writing tests generates a test case for each reactor at runtime, so that even when doing "trial twisted", the tests are still run against every available reactor. This is how we prefer to write new tests, since it simplifies the buildbot configuration (and simplifies running the tests in general).

For the rest of this, I'm going to do some refactoring to reduce some code duplication between posix and windows implementations. Thanks also for pointing out the mixed up parameter order. I'm pretty sure the correct fix here is to make the windows order match the posix order, since these are supposed to be implementations of a single interface, and posix was the better working and more widely used of the two implementations.

comment:26 Changed 4 years ago by exarkun

  • Status changed from assigned to new

To make it easier to find reviews, I split the code in this branch up into #4523 and #4525. Once those are resolved, we can update this branch to just include the new Windows serialport interface checking, which will make this branch much shorter and focused on this issue.

comment:27 Changed 4 years ago by <automation>

  • Owner exarkun deleted

comment:28 Changed 4 years ago by exarkun

  • Keywords easy removed

comment:29 Changed 3 years ago by detly

  • Cc jason.heeris@… added
Note: See TracTickets for help on using tickets.