Improve USB2CAN interface

[felixdivo]

This basically replaces #245 and instead of creating a new interface, applies the changes to usb2can.

New features:

• slightly better error handling
• multiple serial devices can be found
• support for the _detect_available_configs() API

[felixdivo]

cc @acolomb @rusoku @krixkrix

[felixdivo]

Could someone test this? I do not have the required hardware.

[acolomb]

I will try today, but can currently only offer a test with the adapter connected. My hardware for actual bus communications with the python-can based software is out of reach.

[rusoku]

If anyone wants I can send one TouCAN converter for free for testing.

[acolomb]

I didn't have time to dig much deeper yet, but initial testing has resulted in the following exception:

Traceback (most recent call last):
  File "\canopen\canopen\network.py", line 110, in connect
    self.bus = can.interface.Bus(*args, **kwargs)
  File "\can\can\interface.py", line 127, in __new__
    return cls(channel, *args, **config)
  File "\can\can\interfaces\usb2can\usb2canInterface.py", line 115, in __init__
    self.handle = self.can.open(connector.encode('utf-8'), flags)
  File "\can\can\interfaces\usb2can\usb2canabstractionlayer.py", line 85, in open
    configuration = configuration.encode('ascii', 'ignore')
AttributeError: 'bytes' object has no attribute 'encode'

[acolomb]

So Usb2canInterface.__init__() deliberately encodes the connection string as UTF-8, which yields a byte array. Usb2CanAbstractionLayer.open() tries to transcode it to ASCII, but that fails because it is already a byte array instead of a string.

So unless someone passes in a non-ASCII channel or serial argument, this double conversion will be a no-op anyway. I don't see the need (or possiblity) for non-ASCII serial numbers, so it is probably best to convert to ASCII in the wrapper class constructor and just use the resulting byte array unmodified in open().

[acolomb]

Seems like the configuration string handling is a little messy overall. Too many layers passing down their arguments to each other.

[acolomb]

I also noticed that the connection usually fails on first try. The second attempt to open the interface succeeds, but I haven't tested whether the transmitted objects are actually seen on the CAN bus.

[felixdivo]

The code for selecting the serial number now looks as follows:

        # get the serial number of the device
        if "serial" in kwargs:
            device_id = kwargs["serial"]
        else:
            device_id = channel

        # search for a serial number if the device_id is None or empty
        if not device_id:
            devices = find_serial_devices()
            if not devices:
                raise CanError("could not automatically find any serial device")
            device_id = devices[0]

I adapted the documentation accordingly.

[codecov]

Codecov Report

Merging #511 into develop will decrease coverage by 0.06%.
The diff coverage is 18.57%.

@@             Coverage Diff             @@
##           develop     #511      +/-   ##
===========================================
- Coverage    64.52%   64.46%   -0.07%     
===========================================
  Files           63       63              
  Lines         5647     5651       +4     
===========================================
- Hits          3644     3643       -1     
- Misses        2003     2008       +5

[felixdivo]

Seems like the configuration string handling is a little messy overall. Too many layers passing down their arguments to each other.

Removing the trivial function format_connection_string() made it quite a bit simpler to read. And I added documentation on what types open() expects.

[felixdivo]

I also noticed that the connection usually fails on first try. The second attempt to open the interface succeeds, but I haven't tested whether the transmitted objects are actually seen on the CAN bus.

@acolomb Could you find out why this happens?

[acolomb]

I tested this rather quickly, and the return value of CanalOpen() was 1681, which I think is a valid handle, but triggered an exception. Good job on making those more verbose, btw.

[acolomb]

This is actually an improvement to the auto-detection logic, and I think we are getting close.

[acolomb]

Sorry if my comments are a little chaotic, I'm still getting used to GitHub's review interface.

Just added one more comment to your first commit in this PR, but I don't know where it shows up now.

[acolomb]

I found the cause for the truncated serial numbers, see comments in code.

Looks good except for a few nitpicks ;-)

[acolomb]

@felixdivo I hope you are getting notification e-mails for my inline code comments, because even I cannot find all of them inside the PR view on GitHub. Sorry again for the confusion...

[felixdivo]

Yep, I'm getting notifications. ;) And they could be even messier, trust me ...

[acolomb]

So I think the only thing missing now is the item.Dependent.strip('"')[-8:] change. Tested successfully here.

[felixdivo]

Could one of you {@hardbyte, @christiansandberg} please review & merge this?

I thinks it's done.

[hardbyte]

I've had a quick look through the changes and all looks good to me.

Thanks @felixdivo and @acolomb