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.

Sign up for GitHub

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

Jump to bottom

native/periph_can: usability improvements and documentation updates #17949

Merged
merged 5 commits into from
Apr 25, 2022
Merged

native/periph_can: usability improvements and documentation updates #17949

merged 5 commits into from
Apr 25, 2022

Conversation

fjmolinas
Copy link
Contributor

@fjmolinas fjmolinas commented Apr 14, 2022 •
edited
Loading

Contribution description

This PR wants to make the vcan setup usage for native easier. It does so by:

  • 2a5879a: combining setup README for virtual can interfaces while adding details on how to connect two can interface instead of relying on candump/cansend and others.
  • 3143a99: add PERIPH_CAN_FLAGS to specify the can interface to be used, similar to how PORT is used with tap*, but using an explicitly distinct variable for this.

I've also removed the apt-get instructions for installing the 32bit libsocketcan instructions since it's not packages in recent distro versions and with #17533 its used through a pkg by default.

Testing procedure

Follow the README.native.can.md instructions to se tup two native can interfaces, and send messages between them as well as from the Linux host.

Issues/PRs references

It depends on #17533 to not have to re-adapt the README.

@fjmolinas fjmolinas added the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 14, 2022
@github-actions github-actions bot added Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports Area: tests Area: tests and testing framework Platform: native Platform: This PR/issue effects the native platform labels Apr 14, 2022
@aabadie aabadie changed the title native/perih_can: usability improovements native/periph_can: usability improvements Apr 14, 2022
fjmolinas added a commit to fjmolinas/RIOT that referenced this pull request Apr 14, 2022
@fjmolinas
MERGECOMMIT: native/periph_can: usability improvements RIOT-OS#17949
dbcf433
fjmolinas added a commit to thingsat/RIOT that referenced this pull request Apr 15, 2022
@fjmolinas
MERGECOMMIT: native/periph_can: usability improvements RIOT-OS#17949
3de29db
fjmolinas added a commit to thingsat/RIOT that referenced this pull request Apr 15, 2022
@fjmolinas
MERGECOMMIT: native/periph_can: usability improvements RIOT-OS#17949
efd1cbc
@fjmolinas fjmolinas force-pushed the pr_native_can_improovements branch from c91d499 to 91e4c34 Compare April 20, 2022 12:40
@fjmolinas fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Apr 20, 2022
@github-actions github-actions bot removed Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports labels Apr 20, 2022
@fjmolinas fjmolinas requested a review from vincent-d April 21, 2022 09:35
@fjmolinas
Copy link
Contributor Author

Maybe @benpicco or @DanielLockau-MLPA might be interested as well since they seem to use CAN through Dose.

@benpicco
Copy link
Contributor

DOSE doesn't use CAN, but @firas-hamdi has been working with CAN lately

@fjmolinas fjmolinas changed the title native/periph_can: usability improvements native/periph_can: usability improvements and documentation updates Apr 22, 2022
aabadie
aabadie reviewed Apr 22, 2022
View reviewed changes
boards/native/Makefile.include Outdated
Comment on lines 109 to 110
PERIPH_CAN_FLAGS = --can $(VCAN_IFACE)
TERMFLAGS += $(PERIPH_CAN_FLAGS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not:

Suggested change
PERIPH_CAN_FLAGS = --can $(VCAN_IFACE)
TERMFLAGS += $(PERIPH_CAN_FLAGS)
TERMFLAGS += --can $(VCAN_IFACE)

?

aabadie
aabadie reviewed Apr 22, 2022
View reviewed changes
cpu/native/include/can_params.h
@@ -30,8 +30,12 @@ extern "C" {
* @brief Default parameters (device names)
*/
static const candev_params_t candev_params[] = {
#if CAN_DLL_NUMOF >= 1
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand how the CAN_DLL_NUMOF is supposed to be set and how it integrate with the native can flags (is it possible to have multiple calls to --can ?).
Where is it documented btw ? Maybe we should make it more explicit that it's limited to 2 (and why 2 btw) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did some archeology search and found:

#5793 (comment)

Where @vincent-d mentions wanting multiple CAN devices for testing. This still allows but sets by default only one can device, AFAIK from the original PR #5793 there is no mention of a testing procedure for this. your testing was on the same interface #5793 (comment). I did add some more documentation on this though.

(is it possible to have multiple calls to --can ?)

Yes, I added more documentation on this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did some archeology search and found:

#5793 (comment)

Where @vincent-d mentions wanting multiple CAN devices for testing. This still allows but sets by default only one can device, AFAIK from the original PR #5793 there is no mention of a testing procedure for this. your testing was on the same interface #5793 (comment). I did add some more documentation on this though.

(is it possible to have multiple calls to --can ?)

Yes, I added more documentation on this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, things are clearer now. Just found a typo in the native related README. Please squash!

aabadie
aabadie reviewed Apr 25, 2022
View reviewed changes
tests/candev/README.native.can.md Outdated
interfaces will not be connected by default but look like if they where on
different Buses.

Too configure which `vcan` interface each native CAN device should use then
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Too configure which `vcan` interface each native CAN device should use then
To configure which `vcan` interface each native CAN device should use then

@fjmolinas fjmolinas force-pushed the pr_native_can_improovements branch from 86e4931 to 667454b Compare April 25, 2022 10:48
@fjmolinas
Copy link
Contributor Author

squashed

aabadie
aabadie approved these changes Apr 25, 2022
View reviewed changes
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

ACK

@aabadie aabadie enabled auto-merge April 25, 2022 10:52
@aabadie aabadie merged commit 9821e55 into RIOT-OS:master Apr 25, 2022
@fjmolinas
Copy link
Contributor Author

Thanks @aabadie!

@fjmolinas fjmolinas deleted the pr_native_can_improovements branch April 25, 2022 16:12
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabadie aabadie aabadie approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@jia200x jia200x Awaiting requested review from jia200x

@cgundogan cgundogan Awaiting requested review from cgundogan

@smlng smlng Awaiting requested review from smlng

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@vincent-d vincent-d Awaiting requested review from vincent-d

Assignees
No one assigned
Labels
Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: native Platform: This PR/issue effects the native platform
Projects
None yet
Milestone
Release 2022.07
Development

Successfully merging this pull request may close these issues.
4 participants
@fjmolinas @benpicco @aabadie @chrysn