8000 Improve USB2CAN interface by felixdivo · Pull Request #511 · hardbyte/python-can · GitHub
[go: up one dir, main page]
Skip to content

Improve USB2CAN interface#511

Merged
hardbyte merged 15 commits intodevelopfrom
usb2can-improvements-from-canal
Mar 4, 2019
Merged

Improve USB2CAN interface#511
hardbyte merged 15 commits intodevelopfrom
usb2can-improvements-from-canal

Conversation

@felixdivo
Copy link
Collaborator
@felixdivo felixdivo commented Feb 21, 2019

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 felixdivo added this to the 3.2 Release milestone Feb 21, 2019
@felixdivo felixdivo self-assigned this Feb 21, 2019
@felixdivo
Copy link
Collaborator Author
felixdivo commented Feb 21, 2019

cc @acolomb @rusoku @krixkrix

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

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

@acolomb
Copy link
Contributor
acolomb commented Feb 22, 2019

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
Copy link
rusoku commented Feb 22, 2019

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

@acolomb
Copy link
Contributor
acolomb commented Feb 22, 2019 •
edited
Loading

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
Copy link
Contributor
acolomb commented Feb 22, 2019 •
edited
Loading

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
acolomb suggested changes Feb 22, 2019
View reviewed changes
Copy link
Contributor
@acolomb acolomb left a comment

Choose a reason for hiding this comment

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

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

@acolomb
Copy link
Contributor
acolomb commented Feb 22, 2019

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
Copy link
Collaborator Author
felixdivo commented Feb 22, 2019

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
Copy link
codecov bot commented Feb 22, 2019 •
edited
Loading

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
Copy link
Collaborator Author
felixdivo commented Feb 22, 2019

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
Copy link
Collaborator Author
felixdivo commented Feb 24, 2019

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
acolomb suggested changes Feb 25, 2019
View reviewed changes
Copy link
Contributor
@acolomb acolomb left a comment

Choose a reason for hiding this comment

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

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
acolomb suggested changes Feb 25, 2019
View reviewed changes
Copy link
Contributor
@acolomb acolomb left a comment

Choose a reason for hiding this comment

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

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

acolomb
acolomb reviewed Feb 25, 2019
View reviewed changes
@acolomb
Copy link
Contributor
acolomb commented Feb 25, 2019

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.

@felixdivo felixdivo mentioned this pull request Feb 26, 2019
Closed
14 tasks
acolomb
acolomb suggested changes Feb 27, 2019
View reviewed changes
Copy link
Contributor
@acolomb acolomb left a comment

Choose a reason for hiding this comment

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

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

Looks good except for a few nitpicks ;-)

@acolomb
Copy link
Contributor
acolomb commented Feb 27, 2019

@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
Copy link
Collaborator Author
felixdivo commented Feb 27, 2019

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

@acolomb
Copy link
Contributor
acolomb commented Feb 28, 2019

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

@felixdivo
Copy link
Collaborator Author
felixdivo commented Feb 28, 2019 •
edited
Loading

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

I thinks it's done.

hardbyte
hardbyte approved these changes Mar 4, 2019
View reviewed changes
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've had a quick look through the changes and all looks good to me.

Thanks @felixdivo and @acolomb

@hardbyte hardbyte merged commit 4be2233 into develop Mar 4, 2019
@hardbyte hardbyte deleted the usb2can-improvements-from-canal branch March 4, 2019 03:31
This was referenced Mar 10, 2019
Closed
Merged
@felixdivo felixdivo mentioned this pull request May 10, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hardbyte hardbyte hardbyte approved these changes

@christiansandberg christiansandberg Awaiting requested review from christiansandberg

+1 more reviewer

@acolomb acolomb acolomb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@felixdivo felixdivo

Labels

backend:usb2can enhancement

Projects

None yet

Milestone

3.2 Release

Development

Successfully merging this pull request may close these issues.

4 participants

@felixdivo @acolomb @rusoku @hardbyte
0