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

Add interprocess virtual bus #644

Merged
merged 48 commits into from Dec 13, 2020
Merged

Add interprocess virtual bus #644

merged 48 commits into from Dec 13, 2020

Conversation

felixdivo
Copy link
Collaborator

@felixdivo felixdivo commented Jul 16, 2019

This PR adds a virtual interface, that allows multiple processes to exchange can data. This is done by wrapping the CAN frames in IPv6/4 packets and sending them to a multicast group. The wrapping is done using MessagePack as proposed by @windelbouwman. That theoretically allows processes running other software than this one to interoperate with python-can, though I doubt it will used as that much. The TTL of the IP packets is set to one by default, disallowing the packet to escape the localhost. It is configurable however, and would allow the communicating processes to run on different machines as well. However this has not been tested.

Things to discuss before this can be merged:

  • How shall it be called? virtual_interprocess, can_over_ip? Other suggestions? -> I would opt for can_over_ip.
  • How does this relate to the current virtual interface? Is it a replacement, or an addition? In principle, it is at feature-rich as the current virtual interface, but might be a bit slower and requires an external library. I would suggest that it is a addition. Shall msgpack be added as a general required package? -> added to the required ones as it is tiny and does not have any transitive dependencies
  • Do we want to provide the MessagePack wrapping of the CAN frames to other interfaces or the user as well? It might be useful beyond this interface at some point and is fairly general. -> Can still be done

Things for me to do

  • implement receive_own_messages -> raise warning if used for now
  • add tests
  • add docs/examples
  • register as an interface

Closes #530.

@windelbouwman
Copy link

This is nice, and will be handy! The multi-cast solution is handy, but multicast also is often beyond the grasping of many engineers. Maybe we could add a tcp connection based solution as well, with a central server?

@felixdivo
Copy link
Collaborator Author

felixdivo commented Jul 18, 2019

I do not like the central server, since it would not work out of the box by simply firing up an interface. I would propose giving some example IPv4 & IPv6 multicast addresses in the documentation so if someone needs more interfaces her/she can simply plug them in. That should be the simplest, right?

Currently I have the problem that the bus receives it's own messages, which might be desirable but should be allowed to be turned off I guess. I will first have to do some further reading I guess. And it seems like I can get rid of the separate sockets for sending and receiving; I thought that would solve the problem.

@windelbouwman
Copy link

Potential issues with multicast:

  • Firewall configuration, is this a special case?
  • Is multicast supported on all platforms?
  • Can communication over the internet, does this work with multicast? Is this a complex setup?

@christiansandberg
Copy link
Collaborator

I have made a server based interface here if you’re interested: https://github.com/christiansandberg/python-can-remote

@felixdivo
Copy link
Collaborator Author

felixdivo commented Jul 18, 2019

@windelbouwman Hm, but multicast is made exactly for this. 🎯 I know that there is always some hesitation when using it, but man ... It's a perfect fit from a networking perspective. Basically the use of multicast instead of unicast is not a big deal, it's like a dozen lines of additional code plus a different addressing scheme. But getting rid of a central node is really nice. I'll have a look at maybe combining multicast and unicast (and have a look at @christiansandberg's library).

EDIT: With combining I mean that the user may select a normal IP address or a multicast IP address (starting with ff00::/8), and the bus will adjust accordingly. But we would still need a master. I will test around a bit, but it might take a few days.

EDIT 2: It might take a few months. I'm quite busy at the moment.

@felixdivo felixdivo removed their assignment May 13, 2020
@mergify mergify bot requested a review from hardbyte November 16, 2020 00:19
@windelbouwman
Copy link

I made some comments across the PR, overall I believe this looks pretty good, and I'm happy with the work towards a networked virtualcan. Nice job! 👍

Repository owner deleted a comment from codecov bot Nov 22, 2020
@felixdivo
Copy link
Collaborator Author

I completed the documentation (If you don't have any more comments) and included this table:

Screenshot from 2020-11-22 13-48-44

I figured it is easier to read as an image than in source code.

@windelbouwman
Copy link

The table with comparison looks good!

@felixdivo
Copy link
Collaborator Author

felixdivo commented Nov 23, 2020

I think we should name it udp_multicast instead of multicast_ip as that is more descriptive of what it can achieve and what not.

What do you think? That'd be the last thing I'd change.

@codecov
Copy link

codecov bot commented Nov 25, 2020

Codecov Report

Merging #644 (c685ae0) into develop (f7a843e) will increase coverage by 0.76%.
The diff coverage is 85.33%.

@@             Coverage Diff             @@
##           develop     #644      +/-   ##
===========================================
+ Coverage    70.30%   71.06%   +0.76%     
===========================================
  Files           72       75       +3     
  Lines         7122     7351     +229     
===========================================
+ Hits          5007     5224     +217     
- Misses        2115     2127      +12     

@felixdivo
Copy link
Collaborator Author

@hardbyte There seems to be this error when the formatter in the cloud has to step in.

@felixdivo
Copy link
Collaborator Author

I renamed to bus to udp_multicast and added some more tests to improve coverage a bit.

@felixdivo
Copy link
Collaborator Author

Hm, I don't quite get why the black formatter fails on pull_request but succeds on push (as it should).

@felixdivo
Copy link
Collaborator Author

Else, this PR is ready for a final review.

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.

🤸🏼 nice! Everything looks good to merge from my perspective.

I've pushed a few minor tweaks to the docs - hopefully nothing controversial.

@felixdivo
Copy link
Collaborator Author

Thanks Brian!

And the changes are not controversial. ;)

@felixdivo felixdivo removed the request for review from christiansandberg December 13, 2020 15:19
@felixdivo
Copy link
Collaborator Author

Shall we merge (via squashing) or do we need another review? I thinks it's sufficient as is. @windelbouwman also had a look at it.

@hardbyte hardbyte merged commit 7ddea9e into develop Dec 13, 2020
@mergify mergify bot deleted the add-interprocess-virtual-bus branch December 13, 2020 20:10
@hardbyte
Copy link
Owner

Great effort @felixdivo. Let's push out another pre-release of 4.0 with this feature

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.

Interprocess virtual interface
4 participants