Opened 13 years ago
Last modified 11 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: | Lucas Taylor, TimAllen, jesstess, Jason | Branch: |
branches/win32er-serialport-3802
branch-diff, diff-cov, branch-cov, buildbot |
Author: | exarkun, ltaylor.volks |
Description (last modified by )
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)
Change History (32)
comment:1 Changed 13 years ago by
Resolution: | → invalid |
---|---|
Status: | new → closed |
comment:2 Changed 13 years ago by
In particular, you need to use this reactor for *serial* support, not for normal usage.
comment:3 Changed 13 years ago by
Description: | modified (diff) |
---|---|
Keywords: | serial win32 added; Serial Win32 removed |
Milestone: | Twisted-8.2+1 |
Priority: | normal → low |
Resolution: | invalid |
Status: | closed → reopened |
Summary: | SerialPort Outdated → 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 13 years ago by
Keywords: | easy added |
---|
Since this is a documentation issue, it should be pretty easy to resolve.
comment:5 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Type: | defect → 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 13 years ago by
Keywords: | documentation added |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
Type: | enhancement → defect |
We should still improve our documentation.
comment:7 Changed 12 years ago by
Keywords: | serialport added; serial removed |
---|
comment:8 Changed 12 years ago by
Keywords: | review added |
---|
twisted-3802.patch does the following:
- Creates a new
IReactorWin32Events
interface w/ 2 win32-specific methods:addEvent
&removeEvent
win32eventreactor
now also implementsIReactorWin32Events
_win32serialport.SerialPort
asserts that the reactor passed in provides theIReactorWin32Events
interface
comment:9 Changed 12 years ago by
Author: | → ltaylor.volks |
---|---|
Owner: | changed from Glyph to TimAllen |
Status: | reopened → new |
comment:10 Changed 12 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from TimAllen to Lucas Taylor |
Thanks for writing this patch!
A few code-review comments:
- I notice the patch you supplied has a few lines with trailing whitespace, you might want to clean that up.
- Twisted likes to keep lines to 79 columns max, so your import in
twisted.internet.win32eventreactor
and your error message intwisted.internet._win32serialport
will need to be wrapped. assert
is a very big stick to wield. Raising something a little less violent likeValueError
would be better.- 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.
- 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 anISerialPort
interface describing all the common functionality of the POSIX, Win32 and JavaSerialPort
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
.
- There should be a unittest in
twisted.internet.test.test_win32eventreactor
asserting thatwin32eventreactor
implementsIReactorWin32Events
(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. Sincewin32eventreactor
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 12 years ago by
Cc: | Lucas Taylor 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 12 years ago by
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 12 years ago by
Keywords: | review added |
---|
Attached patch should address the first review
- Cleaned up trailing whitepace
- Wrapped imports @ 79 chars
ValueError
is raised if user instantiates aSerialPort
instance on Windows w/o installing a correct reactor- Simplified error message in
ValueError
- Added docstring to
twisted.internet.serialport
re: requirements - Added test:
test_win32eventreactor
to verify it provides theIReactorWin32Events
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 12 years ago by
Cc: | jesstess added |
---|---|
Owner: | Lucas Taylor deleted |
comment:15 Changed 12 years ago by
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 12 years ago by
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.
comment:17 Changed 12 years ago by
Author: | ltaylor.volks → exarkun, ltaylor.volks |
---|---|
Branch: | → branches/win32er-serialport-3802 |
(In [29190]) Branching to 'win32er-serialport-3802'
comment:20 follow-up: 22 Changed 12 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Jean-Paul Calderone |
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 12 years ago by
Keywords: | review added |
---|---|
Owner: | Jean-Paul Calderone deleted |
Did that. Build results
comment:22 Changed 12 years ago by
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 doingtrial -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:24 Changed 12 years ago by
Branch: | branches/win32er-serialport-3802 → branches/win32er-serialport-3802-2 |
---|
(In [29423]) Branching to 'win32er-serialport-3802-2'
comment:25 Changed 12 years ago by
Branch: | branches/win32er-serialport-3802-2 → branches/win32er-serialport-3802 |
---|---|
Keywords: | review removed |
Owner: | set to Jean-Paul Calderone |
Status: | new → 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 12 years ago by
Status: | assigned → new |
---|
comment:27 Changed 11 years ago by
Owner: | Jean-Paul Calderone deleted |
---|
comment:28 Changed 11 years ago by
Keywords: | easy removed |
---|
comment:29 Changed 11 years ago by
Cc: | Jason added |
---|
On windows you need to use the win32eventreactor, e.g.:
before you do any other imports, and all the rest of the code can be the same.