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

Tests/candev: Initial version with native support #13360

Merged
merged 2 commits into from
Mar 11, 2020
Merged

Tests/candev: Initial version with native support #13360

merged 2 commits into from
Mar 11, 2020

Conversation

wosym
Copy link
Member

@wosym wosym commented Feb 13, 2020

Contribution description

Adds a candev test app that can be used to control can-devices through the candev-abstraction, but without using the can-DLL.

Currently, only native is supported (because we thought it made sense to make a PR with only native first, so the main concepts can be tested, reviewed and commented on), but code that adds the MCP2515 driver (see #6276) to this test-app has already been written and tested, and will be made into a PR soon, along with the driver-code of this old PR.

Testing procedure

  • Make sure libsocketcan and canutils are installed and running (see: https://github.com/RIOT-OS/RIOT/tree/master/tests/conn_can)
  • Start candump on vcan0
  • In the test-app shell: the send command will send a CAN message to vcan0. It should become visible in candump. Optionally you can pass some bytes (in decimal notation) as arguments (e.g. send 12 34 56). If no arguments are supplied, three default bytes will be sent.
  • To test the receive part, we can send data from Linux with cansend (e.g. cansend vcan0 001#AABB) or with cangen (e.g. cangen vcan0 -v -n 1).
  • In the test-app, the received bytes will be stored in a FIFO buffer and can be retrieved by calling receive. (an optional argument n can be passed to receive multiple can-messages). If there are no CAN-messages in the buffer when receive is called, it will block until a message is received.

Issues/PRs references

Depends on #13342 --> bugfix is required in order for this candev app to work.

@wosym wosym changed the title Tests/candev native4 Tests/candev: Initial version with native support Feb 13, 2020
@benpicco benpicco added Area: tests Area: tests and testing framework Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Feb 13, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Feb 21, 2020
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 22, 2020
@wosym wosym force-pushed the tests/candev_native4 branch from aa2d271 to 909a52a Compare February 24, 2020 12:48
@wosym wosym force-pushed the tests/candev_native4 branch from 909a52a to eaef186 Compare February 24, 2020 12:52
@wosym
Copy link
Member Author

wosym commented Feb 24, 2020

Now that #13342 is merged, we can continue with this one.
I rebased on master so that we have the patch included, and can have this one working. (I hope I didn't screw up here)

I noticed that in the meantime some changes to Makefile.dep were made in #13089.
In our PR, we removed auto_init_can from the dependencies for can and added it to the Makefile for conn_can (because we can use can without having the auto_init enable a DLL). However it appears the method of fjolminas was a bit cleaner. For now, I simply removed the line again because I wasn't sure what the cleanest solution was, but feel free to make other suggestions!

For some reason, Github made a bunch of people code-owner for my new additions (did I cause this? Or is this normal?) It automatically assigned you all as reviewer now.

@kaspar030 kaspar030 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 24, 2020
@kaspar030
Copy link
Contributor

Pausing for #13456

@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 24, 2020
@wosym wosym force-pushed the tests/candev_native4 branch 3 times, most recently from fd8a076 to 154bab5 Compare February 25, 2020 13:04
@benpicco
Copy link
Contributor

Another rebase is needed.

@wosym wosym force-pushed the tests/candev_native4 branch from 154bab5 to c6c2723 Compare February 26, 2020 07:17
@wosym
Copy link
Member Author

wosym commented Feb 26, 2020

@smlng Care to share your thoughts? This app should be something similar to what you tried to make a few years ago if I'm not mistaken.

@wosym wosym mentioned this pull request Mar 3, 2020
Merged
@JannesVolkens
Copy link
Contributor

I've tested this PR and was able to send and receive CAN frames through your test-app. Both standard and extended frame format with different dlc lengths are working as described.

@tcschmidt
Copy link
Member

@wosym @vincent-d what are the plans for #6276? Should we take up testing or do you plan to obsolete in a new PR?

@wosym
Copy link
Member Author

wosym commented Mar 6, 2020

@tcschmidt the reason this PR is here, is because I wanted to get vincent-d's old PR finally merged.
I rebased those old commits, and did some minor fixes (because the way RIOT works has changed in the years since that PR was made).
After this candev-native PR is merged, I will open a PR with the mcp commits. (I'm waiting because I don't want to make too much of a mess. It will need some more rebasing to get everything in a clean and logical order.)
If you want a sneak peak to the MCP2515 PR, you can find it here: toonst#8
(But ignore the dependencies I mentioned there, because the way I'm PR'ing it to the RIOT repo is different than the PR's I made in toonst's repo for code-review)

@JannesVolkens
Copy link
Contributor

JannesVolkens commented Mar 6, 2020 •
edited
Loading

I've tested this PR and was able to send and receive CAN frames through your test-app. Both standard and extended frame format with different dlc lengths are working as described.

I've encountered some problems when receiving extended CAN frames. When the ID is larger than a specific amount, the received ID differs for the _can_event_callback() and the _receive().
DEBUG was enabled in this screenshot.

Screenshot from 2020-03-06 15-12-38

@JannesVolkens
Copy link
Contributor

@wosym I've tested again and now everything is working correctly. 👍

@benpicco benpicco added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Mar 9, 2020
benpicco
benpicco reviewed Mar 9, 2020
View reviewed changes
tests/candev/README.md Outdated Show resolved Hide resolved
benpicco
benpicco reviewed Mar 9, 2020
View reviewed changes
tests/candev/main.c Outdated Show resolved Hide resolved
benpicco
benpicco reviewed Mar 9, 2020
View reviewed changes
tests/candev/main.c Outdated Show resolved Hide resolved
benpicco
benpicco reviewed Mar 9, 2020
View reviewed changes
tests/candev/main.c Outdated Show resolved Hide resolved
benpicco
benpicco reviewed Mar 9, 2020
View reviewed changes
tests/candev/main.c Outdated Show resolved Hide resolved
benpicco
benpicco reviewed Mar 9, 2020
View reviewed changes
tests/candev/main.c Outdated Show resolved Hide resolved
smlng
smlng reviewed Mar 9, 2020
View reviewed changes
tests/candev/main.c Outdated Show resolved Hide resolved
smlng
smlng reviewed Mar 9, 2020
View reviewed changes
tests/candev/main.c Outdated Show resolved Hide resolved
smlng
smlng reviewed Mar 9, 2020
View reviewed changes
tests/candev/main.c Outdated Show resolved Hide resolved
smlng
smlng reviewed Mar 9, 2020
View reviewed changes
tests/candev/main.c Outdated Show resolved Hide resolved
@wosym wosym force-pushed the tests/candev_native4 branch from f975b3e to 3186e71 Compare March 10, 2020 08:47
benpicco
benpicco reviewed Mar 10, 2020
View reviewed changes
tests/candev/main.c Outdated Show resolved Hide resolved
@wosym wosym force-pushed the tests/candev_native4 branch from 3186e71 to c485317 Compare March 10, 2020 09:07
benpicco
benpicco reviewed Mar 10, 2020
View reviewed changes
tests/candev/main.c Outdated Show resolved Hide resolved
@wosym wosym force-pushed the tests/candev_native4 branch from c485317 to 8eaa65a Compare March 10, 2020 16:58
benpicco
benpicco reviewed Mar 10, 2020
View reviewed changes
tests/candev/Makefile.ci Outdated Show resolved Hide resolved
@benpicco
Copy link
Contributor

The comments have all been addressed, please squash.

wosym added 2 commits March 10, 2020 18:12
@wosym
tests/conn_can: make auto_init_can explicit
9b829c9 
in other apps we might not want to automatically select the auto_init
@wosym
test/candev: add candev test app with native support
a7880ab 
This test app bypasses candll and uses the candev abstraction directly.
This has the limitation that you can only use one can driver and one can
device, which is in most use cases sufficient.
@wosym wosym force-pushed the tests/candev_native4 branch from 8eaa65a to a7880ab Compare March 10, 2020 17:14
@wosym
Copy link
Member Author

wosym commented Mar 10, 2020

@benpicco Done!

benpicco
benpicco approved these changes Mar 11, 2020
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Code looks good, tested by @JannesVolkens

@benpicco benpicco merged commit bb025e4 into RIOT-OS:master Mar 11, 2020
@wosym wosym mentioned this pull request Mar 12, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smlng smlng smlng left review comments

@benpicco benpicco benpicco approved these changes

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

@DipSwitch DipSwitch Awaiting requested review from DipSwitch

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

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

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

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss

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

Assignees
No one assigned
Labels
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 Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Release 2020.04
Development

Successfully merging this pull request may close these issues.
8 participants
@wosym @kaspar030 @benpicco @JannesVolkens @tcschmidt @vincent-d @smlng @leandrolanzieri