8000 Adds canal interface by felixdivo · Pull Request #245 · hardbyte/python-can · GitHub
[go: up one dir, main page]

Skip to content

Adds canal interface#245

Closed
felixdivo wants to merge 27 commits intohardbyte:developfrom
felixdivo:develop
Closed

Adds canal interface#245
felixdivo wants to merge 27 commits intohardbyte:developfrom
felixdivo:develop

Conversation

@felixdivo
Copy link
Collaborator

This adds the changes that were requested in #161. That older PR can now be closed.

krixkrix and others added 5 commits February 11, 2018 00:28
 * almost just a copy of the usb2can module which is badly named
 * the usb2can module is for now left unchanged but should be changed to use the new CANAL interface
 * TODO serial_selector update...
 * autodetect device ID failed
 * CanalOpen failed
@felixdivo
Copy link
Collaborator Author

The failing test is the problematic test/simplecyclic_test.py, see #243. It is not related to this PR.

@hardbyte hardbyte added this to the 2.2 Release milestone Feb 12, 2018
Copy link
Owner
@hardbyte hardbyte left a comment

Choose a reason for hiding this comment

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

I'm unlikely to merge this without a page added to the documentation.

It wouldn't need to cover much - links to the hardware vendor, mention and link to the required libraries. Any comments on compatibility, limitations etc.

@felixdivo
Copy link
Collaborator Author

All requested changes are valid. ;) I will address them as soon as I can, but that will have to wait for one or too weeks probably, since I am very busy right now.

@felixdivo felixdivo self-assigned this Feb 18, 2018
@felixdivo
Copy link
Collaborator Author
felixdivo commented Feb 22, 2018

I'm unlikely to merge this without a page added to the documentation.

It wouldn't need to cover much - links to the hardware vendor, mention and link to the required libraries.

Any comments on compatibility, limitations etc.

I would like someone else to do that since I do not really get Sphinx and do not know the hardware behind the interface at all. Can someone else pick that up?

@felixdivo
Copy link
Collaborator Author
felixdivo commented Feb 23, 2018

Does it need to be registered in can/interface.py :: BACKENDS as well?

@felixdivo
Copy link
Collaborator Author

@krixkrix Could you write some docs? That would be really nice.

@felixdivo
Copy link
Collaborator Author
felixdivo commented Mar 19, 2018

This still needs to be done:

Can anyone help to close this ancient issue?

@felixdivo felixdivo self-assigned this Sep 7, 2018
@hardbyte hardbyte changed the title introducing canal interface: add the changes that ware requested in #161 Adds canal interface Sep 8, 2018
@felixdivo
Copy link
Collaborator Author

@krixkrix Just mentioning you because I'm not sure if you else get notified by new comments. See above for my comment on docs & tests.

@felixdivo
Copy link
Collaborator Author

@krixkrix Have you already found time to look at this? As version 3.0 is now released, I want to get this working early in this "iteration" so we can finally release this into 3.1.

@acolomb
Copy link
Contributor
acolomb commented Feb 11, 2019

What is the difference between this backend and the USB2CAN one? I do have hardware for the latter and it also uses an abstraction layer DLL named CANAL. If it's in any way related, I could do some testing.

Would be good to have at least some kind of documentation, to answer questions like this ;-)

@felixdivo
Copy link
Collaborator Author

This might be the case, yeah. I actually have no idea of the required DLLs, I only copied PR #161 and applied some general patches. Also, I found this through the issue #465: https://github.com/krumboeck/usb2can_canal.

If this is redundant, we can close this PR, sure. Apparently, no one had interest to solve this, so that might explain it.

@acolomb
Copy link
Contributor
acolomb commented Feb 12, 2019

So the real question is why did @krixkrix make a copy of the usb2can backend? I don't get it from the comment, but didn't check the source code either.

@felixdivo Would you mind diffing the two backends against each other, to figure out the main differences?

@felixdivo
Copy link
Collaborator Author

Yeah, I could do that in a few days/weeks. I'm quite busy right now.

@acolomb
Copy link
Contributor
acolomb commented Feb 13, 2019

According to the VSCP documentation:

For Python developers python-can is a good tool. Unfortunately the CANAL interface is named USB2CAN but it is there.

Apparently the CANAL API is implemented by a bunch of drivers there, not only USB2CAN. But I wouldn't bet on more people using this interface backend if it were named CANAL, especially since probably no one ever tested with other CANAL implementation DLLs.

@felixdivo
Copy link
Collaborator Author

But I wouldn't bet on more people using this interface backend if it were named CANAL.

I wouldn't either.

@acolomb
Copy link
Contributor
acolomb commented Feb 19, 2019

@felixdivo I've started a comparison of the backends, see this Gist.

Besides renaming the files and classes, there are some differences in error handling (exceptions) and fewer comments about the backend being targeted to Windows mainly. In some places, I get the impression that some general python-can changes, like msg.is_extended_id vs. msg.id_type have not been incorporated properly into the current PR state.

My knowledge of python-can internals is very limited though, so I won't try to judge which backend is "better". Some obvious documentation and cosmetic changes could be ported to usb2can easily, though.

@felixdivo
Copy link
Collaborator Author
felixdivo commented Feb 19, 2019

I didn't know Github can display .diff files that nicely. ^^

Yeah, there are some changes that should be ported to usb2can to replace this interface.

I can do it this weekend, but I have a question about serial_selector.py's find_serial(): Is serialMatcher = "PID_6001" a sensible default, or is "ED" better? I don't know how they usually look like, so ...

@acolomb
Copy link
Contributor
acolomb commented Feb 19, 2019

The USB2CAN adapters from 8devices apparently always have a serial number starting with 'ED' prefix. I don't know what the resulting strings from the Windows API query will look like, but it seems to slice out the last 8 bytes trying to get the serial number. On the other hand, the USB product ID PID_6001 probably matches an FTDI UART interface chip on the adapter, which will result in pretty much any USB developer gadget without an own vendor id, plus the ones in the linked query.

Since the method only returns the first match, the latter might cause problems. It's a rather fragile approach either way. Looking for 'ED' will probably only work for adapters sold by Eight Devices, not @rusoku's TouCAN, for example.

Since _detect_available_configs() is never called with an argument, it's also not easy to override the selection substring.

I'd probably go with the more flexible approach from canal/, but keep the default prefix at 'ED' until someone complains.

@rusoku
Copy link
rusoku commented Feb 20, 2019

"ED" prefix comes from my old company edevices.lt ;-)
usb2can USB PID is 1234 and not 6001.
BTW TouCAN converter has different init string format like: 0 ; 12345678; 1000.
https://www.rusoku.com/storage/app/media/CANAL%20Library%20User%20Guide.pdf
Added symbol before serial number which means device type: 0 - TouCAN, TouCAN Marine, TouCAN micro, 1 - TouCAN duo, 2 - TouCAN FD etc...
filter/mask values now moved from init string to its own extended functions.
All rusoku devices will have 8 symbols serial number.

@felixdivo
Copy link
Collaborator Author

Thanks for all the input. ;)

Maybe the Win32 API can even reveal the serial numbers directly? Does anyone know the API well enough for that? I couldn't find it via the first search results ...

BTW TouCAN converter has different init string format like: 0 ; 12345678; 1000

Then we could simply look for the last regex group with (\d+) and take that as the serial number. Is it safe to assume that it's always in decimal notation?

Added symbol before serial number which means device type

Do we need to strip the device type number from the serial ID or is it okay to leave it in there? Because how can we detect when to strip it and when not?

I'd probably go with the more flexible approach from canal/, but keep the default prefix at 'ED' until someone complains.

Sounds good, combined with variable length IDs.

@felixdivo felixdivo added this to the 3.2 Release milestone Feb 20, 2019
@rusoku
Copy link
rusoku commented Feb 20, 2019

BTW TouCAN converter has different init string format like: 0 ; 12345678; 1000

Then we could simply look for the last regex group with (\d+) and take that as the serial number. Is it safe to assume that it's always in decimal notation?

Added symbol before serial number which means device type

Do we need to strip the device type number from the serial ID or is it okay to leave it in there? Because how can we detect when to strip it and when not?

For TouCAN devices init string always consists of 3 blocks: device_id ; serial number ; speed.
Or for custom speed : device_id; serial_number ; 0 ; tseg1; tseg2; sjw; brp
serial number is always 8 decimal characters

@felixdivo felixdivo closed this Feb 21, 2019
@felixdivo felixdivo removed this from the 3.2 Release milestone Apr 6, 2019
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.

6 participants

0