Skip to content

Conversation

@felixdivo
Copy link
Collaborator

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

krixkrixand 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
@felixdivofelixdivo mentioned this pull request Feb 10, 2018
This was referenced Feb 11, 2018
@felixdivo
Copy link
CollaboratorAuthor

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

@hardbytehardbyte added this to the 2.2 Release milestone Feb 12, 2018
hardbyte
hardbyte previously requested changes Feb 17, 2018
Copy link
Owner

@hardbytehardbyte 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
CollaboratorAuthor

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.

@felixdivofelixdivo self-assigned this Feb 18, 2018
@felixdivo
Copy link
CollaboratorAuthor

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
CollaboratorAuthor

felixdivo commented Feb 23, 2018

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

@felixdivo
Copy link
CollaboratorAuthor

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

@felixdivo
Copy link
CollaboratorAuthor

felixdivo commented Mar 19, 2018

This still needs to be done:

Can anyone help to close this ancient issue?

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

@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
CollaboratorAuthor

@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

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
CollaboratorAuthor

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

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
CollaboratorAuthor

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

@acolomb
Copy link
Contributor

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
CollaboratorAuthor

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

@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
CollaboratorAuthor

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

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

"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
CollaboratorAuthor

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.

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

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

@felixdivofelixdivo mentioned this pull request Feb 21, 2019
@felixdivofelixdivo removed this from the 3.2 Release milestone Apr 6, 2019
Sign up for freeto 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

@felixdivo@hardbyte@krixkrix@acolomb@rusoku@christiansandberg