Introduce CAN Bus support

[rene]

CAN Bus support on EVE-OS

This PR introduces the support to the CAN Bus on EVE-OS.

In summary:

• New I/O device types introduced in EVE-API are defined in pillar
• A canbus pillar's library is introduced to control and setup CAN interfaces
• Support to CAN interfaces is added to pillar: It reads configuration from device model and setup Physical and/or Virtual CAN interfaces defined in the model
• CAN interface "passthrough" is implemented for KVM
• The corresponding documentation is provided
• The capability to list physical CAN interfaces is added to spec.sh script

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:

golangcilint: import 'github.com/vishvananda/netlink' is not allowed from list 'Main' (depguard)

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:

golangcilint: var-naming: don't use ALL_CAPS in Go names; use CamelCase (revive)
revive: don't use ALL_CAPS in Go names; use CamelCase https://revive.run/r#var-naming

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.51%. Comparing base (1cd8f6a) to head (daf627c).

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.
📢 Have feedback on the report? Share it here.

[christoph-zededa]

There are no tests?

[rene]

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....

[eriknordmark]

Regarding:
The following error is also present in several other source files:

golangcilint: import 'github.com/vishvananda/netlink' is not allowed from list 'Main' (depguard)
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.

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?

[eriknordmark]

LGTM but some of @christoph-zededa 's comments might not be have been considered and implemented. Kicking off tests while that happens.

[rene]

Regarding: The following error is also present in several other source files:

golangcilint: import 'github.com/vishvananda/netlink' is not allowed from list 'Main' (depguard) 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.

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?

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.

[eriknordmark]

@christoph-zededa can you mark those of your comments which @rene and no longer open as resolved.

Kicking off tests.

[christoph-zededa]

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....

There are two ways:

• mock the interface - which I think is not very useful as there is not much of a logic in the code here
• use https://netmodule-linux.readthedocs.io/en/latest/howto/can.html (and if the module does not exist, do t.Skip())

[christoph-zededa]

@christoph-zededa can you mark those of your comments which @rene and no longer open as resolved.

Kicking off tests.

I don't have a "Resolve" Button, but I wrote "resolved" for all resolved comments.

[milan-zededa]

Nice work! Added just few comments.

[rene]

Do we have other PRs related to this one? EVE-API?

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)

[rene]

Updates in the PR:

• Addressed latest comments.

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.

[eriknordmark]

@OhmSpectator thanks for your very thoughful comments.
We definitely want to think about what we can leverage particularly when it makes our lives easier and less complex by leveraging a community which is handling the complexity. Modem manager is an example of that.

For CANbus the need is much simpler - be able to configure the CAN interfaces.
Ideally that should be done using something like the netlink package, but as Renê said that isn't there yet. We should consider switching to using it once it gets more support (or contribute the missing pieces once it is out of beta).
The can-utils do not seem to focus on the configuration but instead have lots of other useful tools to test/debug and use canbus from scripts etc.

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.)

[eriknordmark]

Build failed with
#19 5.072 ERROR: https://dl-cdn.alpinelinux.org/alpine/v3.16/main: temporary error (try again later)
#19 5.072 WARNING: Ignoring https://dl-cdn.alpinelinux.org/alpine/v3.16/main: No such file or directory
#19 10.08 ERROR: https://dl-cdn.alpinelinux.org/alpine/v3.16/community: temporary error (try again later)
#19 10.08 WARNING: Ignoring https://dl-cdn.alpinelinux.org/alpine/v3.16/community: No such file or directory
so I restarted it.

[eriknordmark]

Re-run eden

[rene]

Re-run eden

ok @eriknordmark , in any case I will push an update for this PR... there is an small thing to fix....

[uncleDecart]

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)

[rene]

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....

[rene]

Updates in this PR:

• Rebase on top of master
• Addressed @uncleDecart 's comment
• Rewrote SetupCAN: Although @christoph-zededa 's proposal had reduced the complexity, Yetus was still complaining about the complexity of SetupCAN, it was giving false positive for duplicated lines, and the function was indeed still complex... so at the end I rewrote this function with the following improvements:
  • Validation of properties passed in order to setup CAN
  • Removal of maps to different functions, now just a few auxiliary functions are needed
  • Incremental setup, i.e., as soon as we have all properties for a given parameter, setup the parameter in the CAN interface. For instance, as soon as we have all BitTiming properties, setup this parameter in the CAN interface
• Just return at the first error

[eriknordmark]

Re-run eden

[eriknordmark]

Run again