OpenCyphal / pycyphal

Python implementation of the Cyphal protocol stack.
https://pycyphal.readthedocs.io/
MIT License
123 stars 106 forks source link

python 2.7 - TypeError: monotonic() takes no arguments (1 given) #22

Closed JamesStewy closed 7 years ago

JamesStewy commented 7 years ago

Hi,

I am trying to use pyuavcan with python 2.7 but I am getting an error upon node initialization. I installed pyuavcan using pip install uavcan and pip install pyserial for SLCAN.

test.py:

#!/usr/bin/python
import uavcan

node_id = 123
node_info = uavcan.protocol.GetNodeInfo.Response()
node_info.name = 'test'
node_info.software_version.major = 1
node_info.hardware_version.unique_id = map(ord,'12345')

node = uavcan.make_node('/dev/ttyACM0', node_id=node_id, node_info=node_info)

Output:

No handlers could be found for logger "uavcan.driver.slcan"
Traceback (most recent call last):
  File "./test.py", line 10, in <module>
    node = uavcan.make_node('/dev/ttyACM0', node_id=node_id, node_info=node_info)
  File "/usr/local/lib/python2.7/dist-packages/uavcan/node.py", line 501, in make_node
    can = driver.make_driver(can_device_name, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/uavcan/driver/__init__.py", line 33, in make_driver
    return SLCAN(device_name, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/uavcan/driver/slcan.py", line 655, in __init__
    raise sig
TypeError: monotonic() takes no arguments (1 given)

In the README it says

If Python 2.7 is used, additional dependencies are needed - refer to setup.py for more info.

but I can't find any additional information.

I have tried using python 3 and everything works fine.

pavel-kirienko commented 7 years ago

The additional dependencies that are recommended for Python 2.7 are listed here, at the end of the file: https://github.com/UAVCAN/pyuavcan/blob/master/setup.py#L50

If monotonic is not available, Pyuavcan will resort to the wall time instead, which is very unsuitable for this purpose because it may change phase and rate when the system clock is being corrected. Monotonic time is always available in Python 3.3 and newer.

The stack trace you posted looks outstandingly weird. How come the function is invoked with an argument, I wonder. Do you have the monotonic package installed or not? If not, could you install it and see if the exception goes away?

Thanks.

JamesStewy commented 7 years ago

Turns out I had monotonic already installed (pip install monotonic returns that requirements are already satisfied). I opened up a python shell and I can import it just fine.

I'm glad the (1 given) part makes just as little sense to you as it did to me as I couldn't find a single instance of this occurring anywhere.

One extra note, I ran the script without the Babel plugged in and got this traceback

No handlers could be found for logger "uavcan.driver.slcan"
Traceback (most recent call last):
  File "./test.py", line 10, in <module>
    node = uavcan.make_node('/dev/ttyACM0', node_id=node_id, node_info=node_info)
  File "/usr/local/lib/python2.7/dist-packages/uavcan/node.py", line 501, in make_node
    can = driver.make_driver(can_device_name, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/uavcan/driver/__init__.py", line 33, in make_driver
    return SLCAN(device_name, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/uavcan/driver/slcan.py", line 655, in __init__
    raise sig
serial.serialutil.SerialException: [Errno 2] could not open port /dev/ttyACM0: [Errno 2] No such file or directory: '/dev/ttyACM0'

The final error makes sense but interestingly the traceback is identical.

pavel-kirienko commented 7 years ago

The traceback is identical because these exceptions are thrown from a worker process, so the stack trace information is lost, and what you see is the point of re-raising in the control (main) process.

Anyway, in order to debug this, we need to find out who is invoking the function monotonic() with an unwanted argument. I propose the following approaches:

A. Override the function there with a wrapper that logs the stack trace of every invocation: https://github.com/UAVCAN/pyuavcan/blob/master/uavcan/__init__.py#L30. The code should look roughly like this:

def monotonic_wrapper(*args, **kwargs):
    import traceback
    if len(args) or len(kwargs):
        traceback.print_stack(file=sys.stderr)
    return time.monotonic()

B. Try to extract the stack trace information from the exception object re-thrown from the worker thread. I'm not sure it is possible at all, I suspect the stack trace information is not stored in the exception object. Perhaps the function tracebacl.format_exception() should be of help here? https://docs.python.org/2/library/traceback.html#traceback.format_exception

C. Try to remove the monotonic package and see if anything changes.

JamesStewy commented 7 years ago

The issue is caused right here: https://github.com/pyserial/pyserial/blob/master/serial/serialutil.py#L116. Presumably the argument being passed to monotonic() is self.

Replacing TIME = time.monotonic with

def TIME(self):
    return time.monotonic()

fixes the the issue.

Simply removing the trace section from your suggested code also works around the issue.

# noinspection PyPackageRequirements,PyUnresolvedReferences
import monotonic                    # 3rd party dependency for old versions @UnresolvedImport
def monotonic_wrapper(*args, **kwargs):
    return monotonic.monotonic()
time.monotonic = monotonic_wrapper
pavel-kirienko commented 7 years ago

Great work James. :+1:

I suppose the behavior of PySerial is invalid, since neither time() nor monotonic() expect to receive any arguments, but normally it doesn't cause any issues because the native implementations just ignore extra arguments. Does this assumption make sense? If so, are you going to open a ticket in the PySerial repo?

Also, are you up to sending a pull request to this repository? If so, it would be helpful to add a comment or two explaining why the wrapper is needed.

JamesStewy commented 7 years ago

I am happy to be guided on this one. Do you think it is reasonable to expect pyserial to work when a standard function is overridden? And as a result, should pyuavcan provide the fix?

pavel-kirienko commented 7 years ago

I just tested the assumption made above and it turned out to be invalid:

>>> import time
>>> time.time()
1495585243.3116841
>>> time.time(123)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: time() takes no arguments (1 given)

So the reason why the extra argument is being passed remains unclear. Could you perhaps print this argument in the wrapper to make sure that it is indeed self as you proposed?

Do you think it is reasonable to expect pyserial to work when a standard function is overridden? And as a result, should pyuavcan provide the fix?

I think yes, it is reasonable to expect that, especially if the replacement function is fully identical and features the same interface as the original. Regardless of that, PyUAVCAN still should provide the fix, because I don't anticipate our fix, whatever it would be, to be released as a part of PySerial anytime soon. Their release cadence is rather slow.

JamesStewy commented 7 years ago

Ok I will submit a PR to both.

pavel-kirienko commented 7 years ago

Thanks James!