Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement gs_usb interface #212

Merged
merged 12 commits into from
Jun 23, 2022
Merged

Conversation

chemicstry
Copy link
Contributor

@chemicstry chemicstry commented Mar 3, 2022

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

This is currently blocked on upstream changes: hardbyte/python-can#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
Copy link
Member

Okay. Please tag me when unblocked.

@chemicstry
Copy link
Contributor Author

chemicstry commented Mar 4, 2022

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: hardbyte/python-can#1274

@pavel-kirienko
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

@chemicstry
Copy link
Contributor Author

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
Copy link
Contributor Author

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 😕

Copy link
Member

@pavel-kirienko pavel-kirienko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add your remark regarding the possibility of enabling the hardware loopback for the other interfaces to the class docstring?

@chemicstry
Copy link
Contributor Author

chemicstry commented Jun 21, 2022

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

@pavel-kirienko
Copy link
Member

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

@chemicstry do we have any eta?

@chemicstry
Copy link
Contributor Author

chemicstry commented Jun 22, 2022

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
Copy link
Member

pavel-kirienko commented Jun 22, 2022 via email

@chemicstry
Copy link
Contributor Author

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.

@pavel-kirienko pavel-kirienko merged commit 99d767b into OpenCyphal:master Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants