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

Added support for PCAN on Mac #365

Merged
merged 17 commits into from
Aug 9, 2018
Merged

Added support for PCAN on Mac #365

merged 17 commits into from
Aug 9, 2018

Conversation

Lauszus
Copy link
Collaborator

@Lauszus Lauszus commented Jul 18, 2018

I had to change the ID from c_uint to c_ulong, as it caused a "Segmentation fault: 11"

Driver can be found here: http://www.mac-can.com

I had to change the ID from u_uint to c_ulong, as it caused a "Segmentation fault: 11"

Driver can be found here: http://www.mac-can.com
@Lauszus
Copy link
Collaborator Author

Lauszus commented Jul 18, 2018

FYI I have been using it with my small CAN viewer terminal application: https://github.com/Lauszus/python_can_viewer.

Copy link
Collaborator

@felixdivo felixdivo left a comment

Choose a reason for hiding this comment

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

Great! This should also be documented in /doc/iterfaces/pcan.rst before merging.

@felixdivo
Copy link
Collaborator

Do you work more often with the PEAK systems and have time to address #56 as well? It is an old issue and nobody seems to pick it up.

@codecov
Copy link

codecov bot commented Jul 18, 2018

Codecov Report

Merging #365 into develop will increase coverage by 0.03%.
The diff coverage is 58.33%.

@@             Coverage Diff             @@
##           develop     #365      +/-   ##
===========================================
+ Coverage    59.31%   59.34%   +0.03%     
===========================================
  Files           55       55              
  Lines         4242     4243       +1     
===========================================
+ Hits          2516     2518       +2     
+ Misses        1726     1725       -1

@Lauszus
Copy link
Collaborator Author

Lauszus commented Jul 18, 2018

@felixdivo just updated the PCAN documentation. Also made PCAN_USBBUS1 the default channel.

@@ -88,9 +89,11 @@ def __init__(self, channel, state=BusState.ACTIVE, *args, **kwargs):

"""
if not channel:
Copy link
Collaborator

@felixdivo felixdivo Jul 18, 2018

Choose a reason for hiding this comment

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

You could just put channel = 'PCAN_USBBUS1' in the formal parameter list of __init__() and then remove lines 91-94. The same goes for bitrate as well AFAIK. We would have to re-add it in the super().__init__() call at the end though. Example code.

@felixdivo
Copy link
Collaborator

Thanks for the additions! Do you know what devices are supported by the backend? That would make the docs quite complete for now. 😄

@Lauszus
Copy link
Collaborator Author

Lauszus commented Jul 18, 2018

@felixdivo I have added the suggested changes. I have only tested it with PCAN-USB, as that is the only one I have available.

@Lauszus
Copy link
Collaborator Author

Lauszus commented Jul 18, 2018

Btw I have tested it on Mac and Windows. Will test on Linux asap.

@felixdivo
Copy link
Collaborator

Nice, thank you!

@Lauszus
Copy link
Collaborator Author

Lauszus commented Jul 18, 2018

I have updated the documentation, as the PCAN adapters are actually supported by the kernel >= 3.4.

@felixdivo
Copy link
Collaborator

Good addition, although the new section would better fit in the socketcan section.

@Lauszus
Copy link
Collaborator Author

Lauszus commented Jul 24, 2018

@felixdivo please see: eb9640f. I have added this as well, so it will use the default channel for the interface if channel is None.

I will fix the documentation in the coming days (I'm on vacation atm).

@Lauszus
Copy link
Collaborator Author

Lauszus commented Jul 24, 2018

@felixdivo may I suggest you add a codecov.yml file were you set the threshold, so codecov does not fail for very small thresholds. You can check out this for for an example: https://github.com/Lauszus/python_can_viewer/blob/master/codecov.yml. This also allow you to make the codecov comment way smaller: Lauszus/python_can_viewer#1 (comment).

@felixdivo
Copy link
Collaborator

See #372

@Lauszus
Copy link
Collaborator Author

Lauszus commented Jul 24, 2018

I have updated the documentation in 5a4868e.

@felixdivo
Copy link
Collaborator

Yes, looks good now! Shall I merge it like that?

@christiansandberg
Copy link
Collaborator

christiansandberg commented Jul 24, 2018

The basic.py file comes from Peak and we try to keep it up to date. So if we make our own modifications then we need to make clear comments in the code why it has been modified. Otherwise someone will update it to a newer version in the future and the changes will be lost since they will think that the previous version was probably wrong.

@Lauszus
Copy link
Collaborator Author

Lauszus commented Jul 24, 2018

@christiansandberg fixed in 958f57c.

@felixdivo
Copy link
Collaborator

We could also notify PEAK of the fix.

@Lauszus
Copy link
Collaborator Author

Lauszus commented Jul 25, 2018

It's part of the Mac PCAN basic driver, so they must be aware of it.

@felixdivo
Copy link
Collaborator

Shall I merge it our do you still have some pending changes?

@Lauszus
Copy link
Collaborator Author

Lauszus commented Jul 29, 2018

435ac1b fixes a bug, as the millis_overflow value was not used when calculating the timestamp. This has not been tested, but I will be able to test it in a week or two.

@Lauszus
Copy link
Collaborator Author

Lauszus commented Aug 9, 2018

I just tested this on both Mac and Windows and it should be good to merge :)

@felixdivo felixdivo merged commit 7b3bb42 into hardbyte:develop Aug 9, 2018
@Lauszus Lauszus deleted the pcan_mac branch December 18, 2018 09:29
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.

None yet

4 participants