-
Notifications
You must be signed in to change notification settings - Fork 159
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
Introduce CAN Bus support #3802
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3802 +/- ##
=======================================
Coverage 17.51% 17.51%
=======================================
Files 3 3
Lines 805 805
=======================================
Hits 141 141
Misses 629 629
Partials 35 35 ☔ View full report in Codecov by Sentry. |
There are no tests? |
Tests were made on real hardware with CAN controllers from devices we support. Unfortunately unit-tests are a bit trick here because real CAN controllers supports in general only a set of the parameters that can be set by the library introduced here. For non-supported parameters I've checked the result comparing with returns from iproute2, which were the same. One option is to create a dummy CAN driver kernel module that would accept all the parameters, so we could check the whole data route from netlink IPC to the driver itself... For now I can just compromise with the tests on real hardware, this introduction can unblock those who are needing CAN device access, and I can continue to working on unit tests.... |
Regarding: golangcilint: import 'github.com/vishvananda/netlink' is not allowed from list 'Main' (depguard) Seems like we need a config file per golangci/golangci-lint#3877 to allow imports other than standard packages. Do we not have a config file at all? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but some of @christoph-zededa 's comments might not be have been considered and implemented. Kicking off tests while that happens.
AFAIK we don't. It should be the reason, as you pointed out. I will take a look. I've addressed most of the comments and updated the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@christoph-zededa can you mark those of your comments which @rene and no longer open as resolved.
Kicking off tests.
There are two ways:
|
I don't have a "Resolve" Button, but I wrote "resolved" for all resolved comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Added just few comments.
All changes for EVE-API and Kernel were already merged. The initial changes for EVE-API are here: lf-edge/eve-api@5dda59a (they were updated by @christoph-zededa to add USB devices) |
Updates in the PR:
Big kudos to @christoph-zededa which proposed the changes now implemented in SetupCAN() in order to remove the switch/case for checking properties, reducing the complexity of the code. |
@OhmSpectator thanks for your very thoughful comments. For CANbus the need is much simpler - be able to configure the CAN interfaces. The general question about both how we leverage existing pieces but also how we plug them into EVE is something we should talk more explicitly about (as well as figuring out which existing pieces we can replace and/or refactor.) |
Build failed with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-run eden
ok @eriknordmark , in any case I will push an update for this PR... there is an small thing to fix.... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @rene for this!
I have one general non-critical comment regarding canbus.go module: it looks like you have a group of functions which take link *CAN as first parameter, which makes it feel like a class method. For example
func SetTermination(link *CAN, termination uint16) error;
turns into
func (link *CAN) SetTermination(termination uint16) error;
So you can write this:
link.SetTermination(canProps.Termination)
@uncleDecart , the reason to not use a receiver in those functions it's mainly because I followed the same approach as in the netlink's Go package functions.... my plan for the future is to merge canbus library into netlink and push it upstream.... |
Updates in this PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-run eden
Introduce CAN Bus library functions to handle CAN interfaces Signed-off-by: Renê de Souza Pinto <rene@renesp.com.br>
This commit introduces the CAN Bus support on EVE by providing the following changes: - Enable and parse the cbattr map of attributes from hardware models - Add the new types already present in eve-api: * IoUSBController * IoUSBDevice * IoCAN * IoVCAN * IoLCAN - Parse the configuration of CAN and/or Virtual CAN interfaces - Perform the setup of all CAN interfaces according to the configuration Signed-off-by: Renê de Souza Pinto <rene@renesp.com.br>
This commit adds the capability to "passthrough" host CAN interfaces to guests. The passthrough term is quoted because QEMU's implementation actually emulates a CAN device to the Guest, connecting it to the specificed CAN interface in the host. Any Logical CAN defined in the device hardware model can be connected to a Guest: { "ztype": "IO_TYPE_LCAN", "phylabel": "can0.1", "logicallabel": "can0.1", "assigngrp" : "can0.1", "phyaddrs" : { "ifname" : "can0" }, } The field 'ifname' is mandatory and corresponds to the host CAN interface to be accessed by the Guest. A name for the emulated device should be given in the phylabel and/or logicallabel fields. Having the former, priority over the latter. Note that this name corresponds only to the QEMU's configuration device name, it has no influence in the CAN interface's name on the Guest, which will be OS dependent. Signed-off-by: Renê de Souza Pinto <rene@renesp.com.br>
This commit adds the corresponding documentation regarding CAN Bus support on EVE-OS with detailed description and common use cases explained. Remove CAN mention from Network documentation since the proper information is now provided in a dedicated section. Signed-off-by: Renê de Souza Pinto <rene@renesp.com.br>
Add the capability to list physical CAN interfaces. Signed-off-by: Renê de Souza Pinto <rene@renesp.com.br>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run again
CAN Bus support on EVE-OS
This PR introduces the support to the CAN Bus on EVE-OS.
In summary:
The design document for this implementation can be accessed in the LF Edge's Wiki: https://wiki.lfedge.org/display/EVE/CAN+Bus+support
Summary of commits:
1d0b110 spec.sh: List physical CAN interfaces
d13e4af docs: Add CAN Bus documentation
4c9e53b kvm: Introduce CAN Bus passthrough
005287f pillar: Introduce CAN Bus support
a582768 pillar: Introduce CAN Bus library
Regarding Yetus errors:
Error 1
The following error is also present in several other source files:Running Yetus locally I found more than 2k incidences of this error throughout the source code. I couldn't find the root cause and how to solve it.
Error 2
Regarding the following error:I won't fix because I just followed the constants defined for the netlink interface in the Linux kernel and in exactly the same way as other similar constants are defined (all in capital letters) in the unix package from golang: https://github.com/Golangltd/leafltd/blob/282493799a89/src/golang.org/x/sys/unix/ztypes_linux_amd64.go#L331
Edit: Yetus error were fixed and/or properly ignored.