search

OpenCyphal / pycyphal

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

python-can support implemented #123

Closed Polarisru closed 3 years ago

Polarisru commented 4 years ago

Tested with PCAN-USB Pro and Kvaser Leaf Light v2, some problems with Kvaser detected.

pavel-kirienko commented 4 years ago

Hey @Polarisru, is it ready for review? Do you need any assistance with this? Thanks!

Polarisru commented 4 years ago

Hello, yes, I have committed the last version a week ago, can you see it? I have fixed probably all of your comments, but python-can has some problems regarding CAN FD implementation, the settings are very device-specific for different adapters, so I have just added CAN FD support for adapters I have only (PCAN and Kvaser).

pavel-kirienko commented 4 years ago

Thanks. Yes, I've seen your commits but you didn't request another review so I thought that you are still working on it. I will give it a look soon.

pavel-kirienko commented 4 years ago

Meanwhile, can you please fix the typing issues detected by MyPy? You can find them listed at https://ci.appveyor.com/project/Zubax/pyuavcan/builds/34717452/job/ryxadmg0fxnesff7 if you scroll to the very end:

tests/cli/_subprocess.py: note: In member "interrupt" of class "BackgroundChildProcess":
tests/cli/_subprocess.py:123: error: Module has no attribute "CTRL_BREAK_EVENT"
pyuavcan/transport/can/media/pythoncan/_pythoncan.py: note: In member "__init__" of class "PythonCANMedia":
pyuavcan/transport/can/media/pythoncan/_pythoncan.py:53: error: Value of type "Union[int, Tuple[int, int]]" is not indexable
pyuavcan/transport/can/media/pythoncan/_pythoncan.py:54: error: Value of type "Union[int, Tuple[int, int]]" is not indexable
pyuavcan/transport/can/media/pythoncan/_pythoncan.py:56: error: Argument 1 to "int" has incompatible type "Union[int, Tuple[int, int]]"; expected "Union[str, bytes, SupportsInt, _SupportsIndex]"
pyuavcan/transport/can/media/pythoncan/_pythoncan.py:57: error: Argument 1 to "int" has incompatible type "Union[int, Tuple[int, int]]"; expected "Union[str, bytes, SupportsInt, _SupportsIndex]"
pyuavcan/transport/can/media/pythoncan/_pythoncan.py: note: In member "send_until" of class "PythonCANMedia":
pyuavcan/transport/can/media/pythoncan/_pythoncan.py:144: error: Returning Any from function declared to return "object"
Found 6 errors in 2 files (checked 364 source files)

Also, at least some rudimentary test suite would help.

pavel-kirienko commented 4 years ago

Hey @Polarisru! Please let me know if you need assistance with any of the issues I mentioned in my last review. I think it's very close to completion, just a few minor things are left to fix.

Polarisru commented 4 years ago

Hello Pavel, sorry for this delay, I was really busy with other projects, but I will continue to process the issues on this weekend, I also would like to finish this implementation.

Polarisru commented 4 years ago

I don't see any errors with mypy locally:

e:\Work\pyuavcan-master\pyuavcan\transport\can\media\pythoncan>mypy --ignore-missing-imports _pythoncan.py
Success: no issues found in 1 source file

Is it really so?

pavel-kirienko commented 4 years ago

I don't see any errors with mypy locally:

e:\Work\pyuavcan-master\pyuavcan\transport\can\media\pythoncan>mypy --ignore-missing-imports _pythoncan.py
Success: no issues found in 1 source file

Is it really so?

No. MyPy needs more context for a comprehensive analysis, so invoking it on a single file is no good. Look at the CI output:

pyuavcan/transport/can/media/pythoncan/_pythoncan.py: note: In member "send_until" of class "PythonCANMedia":
pyuavcan/transport/can/media/pythoncan/_pythoncan.py:147: error: Returning Any from function declared to return "object"

It found another error but it is not related to your code so just ignore it (that's because certain things here are not quite stable yet).

Polarisru commented 3 years ago 
 * [x]  Add a few words about the parameters in the constructor documentation, like it is done in https://github.com/UAVCAN/pyuavcan/blob/d887f1c2dea9341b7482116bcaae3230aecfd191/pyuavcan/transport/can/media/socketcan/_socketcan.py#L37-L46
   .

 * [x]  Refactor the interface construction logic -- see my suggestions.

 * [x]  Get rid of the useless logic around `_loopback_enabled`.

 * [x]  Some minor things I mentioned in the review.

 * [ ]  From my earlier review: Can we somehow make the MTU optional so that if the adapter supports CAN FD, it defaults to 64, otherwise to 8 bytes, or is it going to get convoluted? We can make the MTU argument optional and then if it's None, make it default to 8 or 64 depending on `_is_fd`.

 * [ ]  Add a test suite. We do not require full coverage but it is necessary to make sure that it is at least importable and somehow runnable.

Could you please give me some examples from the existing code to write a test suite? Regarding the MTU: there are not so many adapters supporting CAN FD, so I am afraid of some inconvenencies using such kind of MTU definitions. We could probably use default settings and distinguish FD and Classic based on the bitrate parameter (single int or a tulpe). What do you think about it?

I have also added some more adapters: SocketCAN, NI-CAN and SLCAN, I probably have a possibility to test them too, so it would be not bad to have a brighter set of supported adapters I guess. I will get it working during the next week.

pavel-kirienko commented 3 years ago

Could you please give me some examples from the existing code to write a test suite?

For quicker real-time communication, I recommend joining https://t.me/uavcan_ru (a public channel in Russian)

pavel-kirienko commented 3 years ago

@Polarisru hi Alex, are you planning to continue working on this? We are only missing a basic test suite here.

Polarisru commented 3 years ago

Yes, I have completed all other projects and want finally to fulfill this commit at this weekend. Sorry for this huge delay.

pavel-kirienko commented 3 years ago

That's great. You will notice that the stable version of the library has a slightly different API for the CAN media layer. Please update your implementation accordingly, should be a no-brainer: https://github.com/UAVCAN/pyuavcan/commit/1b114ba00e653723f843c31b182526eea3c4bf36#diff-9ca7dfe6ff0e87b4555ef29093ef87d3c1540db8c5193ede9eb9746981cb3d5b

Also, testing is now managed via Nox instead of custom shell scripts:

nox -s test -k pythoncan

More info at https://pyuavcan.readthedocs.io/en/latest/pages/dev.html

Polarisru commented 3 years ago

Hello Pavel, test script is ready now. I could find a problem of malfunction but had to rebuild some features in the main python-can file, so I have to check if it still works :) I will do it tomorrow and hope we merge it finally.

pavel-kirienko commented 3 years ago

Great. Let me help you out here a bit by rebasing your changeset to master and addressing some minor issues.

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 37211579

Files with Coverage Reduction New Missed Lines %
/home/appveyor/projects/pyuavcan/pyuavcan/transport/can/media/pythoncan/_pythoncan.py 48 71.54%
<!-- Total: 48 -->
Totals Coverage Status
Change from base Build 37096973: -0.4%
Covered Lines: 11330
Relevant Lines: 11731
💛 - Coveralls
pavel-kirienko commented 3 years ago

@Polarisru I have slightly refactored the implementation, wrote some documentation, expanded the test suite a notch, and ran a basic manual test to validate the driver (using PCAN USB and Zubax Babel). Back to you now. If you could write extra docs and add examples that would be great because what we have now is a bit minimalistic.

Edit: compiled docs can be viewed at https://pyuavcan--123.org.readthedocs.build/en/123/ or locally via nox -s docs.

pavel-kirienko commented 3 years ago

Merging, as discussed in Telegram.