search

OpenCyphal / pycyphal

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

Implement gs_usb interface #212

Closed chemicstry closed 2 years ago

chemicstry commented 2 years ago

Another attempt on supporting Canable adapters by implementing gs_usb interface after #210 failed.

This is currently blocked on upstream changes: https://github.com/hardbyte/python-can/pull/1270

I was not sure how to go about implementing hardware loopback frames. Specifying boolean for each interface seemed too verbose. Instead, I made it switch to hardware loopback as soon as it receives a TX frame.

This seems to be working fine with yakut monitor/call/subscribe on windows and canable adapter with candleLight firmware.

pavel-kirienko commented 2 years ago

Okay. Please tag me when unblocked.

chemicstry commented 2 years ago

One test fails here, but it's because python-can uses the MSG_DONTROUTE flag to detect loopback, unlike MSG_CONFIRM in pyuavcan's socketcan implementation. This makes all virtual can frames to be loopback (is_rx false), because they originate from the same machine. Fixing this in python-can could break someone's else code, depending on the use case. So IMO, the best way is to either disable the loopback assert or implement an exception for virtual socket can.

Also opened an issue on python-can: https://github.com/hardbyte/python-can/issues/1274

pavel-kirienko commented 2 years ago

So IMO, the best way is to either disable the loopback assert or implement an exception for virtual socket can.

This is not just an assertion check, this is a unit test. Removing the test will make it pass but break the production code.

chemicstry commented 2 years ago

So IMO, the best way is to either disable the loopback assert or implement an exception for virtual socket can.

This is not just an assertion check, this is a unit test. Removing the test will make it pass but break the production code.

I know, I know. I might not have been clear enough. What I was trying to say is that this issue is more of a unit test quirk and only affects virtual can sockets, it would not affect a real socketcan adapter.

But anyway, let's wait if this is resolved at the python-can repo.

pavel-kirienko commented 2 years ago

@chemicstry any update here? do we still want this open?

chemicstry commented 2 years ago

There was a big discussion in python-can repository and TLDR of it is that rx/echo/loopback/confirm/etc terminology is completely mixed up and I don't think I personally can resolve it in the python-can library. Properly fixing it would be a breaking change and I'm not sure if anyone is up to it.

An alternative without touching python-can is to conditionally enable hardware loopback just for select interfaces and I think that is the way to go. Unfortunatelly, my PR for the required gs_usb interface fixes is still hanging in python-can, I'll try pinging.

chemicstry commented 2 years ago

I rebased the PR and made hardware loopback to be explicitly specified per interface instead of auto detection. This should fix the failing socketcan tests. I'm not sure about other interfaces, but I believe their loopback support can be enabled in the future if someone confirms that it's working.

But this still has to wait for the new python-can release 😕

chemicstry commented 2 years ago

Updated docs and while at it also realized that gs_usb supports hardware monotonic timestamps. Added that as well.

pavel-kirienko commented 2 years ago

But this still has to wait for the new python-can release

@chemicstry do we have any eta?

chemicstry commented 2 years ago

But this still has to wait for the new python-can release

@chemicstry do we have any eta?

I don't know, probably not very soon, python-can is really short on maintainers.

Although if this was merged it wouldn't cause any issues unless gs_usb interface was selected, then you would get interface not found error. But you could still use it if you installed python-can from git.

Since this contains more relevant changes for other interfaces (PythonCANMediaOptions), maybe adding a note to gs_usb docs that it requires latest python-can from git would be fine for now?

EDIT: sorry, it probably wouldn't work, because python-can v4.0.0 has gs_usb interface but it's missing enumeration by index and loopback support, so you would get some weird error attempting to use gs_usb interface

pavel-kirienko commented 2 years ago

Indeed, that sounds reasonable.

On Wed, Jun 22, 2022, 19:54 Jurgis @.***> wrote:

But this still has to wait for the new python-can release

@chemicstry https://github.com/chemicstry do we have any eta?

I don't know, probably not very soon, python-can is really short on maintainers.

Although if this was merged it wouldn't cause any issues unless gs_usb interface was selected, then you would get interface not found error. But you could still use it if you installed python-can from git.

Since this contains more relevant changes for other interfaces ( PythonCANMediaOptions), maybe adding a note to gs_usb docs that it requires latest python-can from git would be fine for now?

— Reply to this email directly, view it on GitHub https://github.com/OpenCyphal/pycyphal/pull/212#issuecomment-1163378948, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZFIZBF74RJOV6WYSWIME3VQNALZANCNFSM5PZAAAYA . You are receiving this because you commented.Message ID: @.***>

chemicstry commented 2 years ago

Added the note.

This is the error, when you attempt to use gs_usb with python-can v4.0.0:

InvalidMediaConfigurationError: Interface error: GsUsbBus.__init__() missing 2 required positional arguments: 'bus' and 'address'.
Note: gs_usb currently requires unreleased python-can version from git.