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

periph_can,candev: socketcan pkg for native, candev test cleanup #17533

Merged

Conversation

DanielLockau-MLPA
Copy link
Contributor

Contribution description

  • add libsocketcan as a package to resolve its reduced availability (Ubuntu dropped it for i386 builds)
  • allow switching between OS provided and package provided libsocketcan for cpu/native
  • make tests/candev more readable

Testing procedure

On Ubuntu 20.04 with libsocketcan2:amd64 available, libsocketcan-dev available, libsocketcan2:i386 unavailable.

In tests/candev:

  • make -j BOARD=native CAN_DRIVER=PERIPH_CAN should fail
  • make -j BOARD=native CAN_DRIVER=PERIPH_CAN USEPKG=libsocketcan should build
  • sudo modprobe vcan; sudo ip link add dev vcan0 type vcan; sudo ip link set up dev vcan0 should create a virtual can device
  • install can-utils for the OS
  • run the native build, run candump vcan0 in another terminal
  • run receive in the native build's terminal
  • run cansend vcan0 1234567f#123456 in another terminal -> both candump and the native build's terminal should show similar output
  • run send 1 2 3 4 in the native build's terminal -> candump should show the data

Issues/PRs references

None.

@github-actions github-actions bot added 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 Jan 18, 2022
@DanielLockau-MLPA DanielLockau-MLPA changed the title Fix/20220113 test candev periph_can,candev: socketcan pkg for native, candev test cleanup Jan 19, 2022
@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 Jan 26, 2022
@chrysn chrysn added 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 labels Jan 26, 2022
@@ -9,11 +9,11 @@ USEMODULE += isrpipe
CAN_DRIVER ?= MCP2515

Copy link
Contributor

@benpicco benpicco Jan 27, 2022

Choose a reason for hiding this comment

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

Can't we

USEPKG += libsocketcan

here to make sure this is compile tested by CI?

Copy link
Contributor

Choose a reason for hiding this comment

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

By adding the following in a Makefile.board.dep, that should be possible:

ifeq (native,$(BOARD))
  USEPKG += libsocketcan
endif

Copy link
Contributor

Choose a reason for hiding this comment

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

Or just in the Makefile.dep of native cpu if the periph_can feature is used => that would be my preferred solution

@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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 14, 2022
@fjmolinas
Copy link
Contributor

fjmolinas commented Apr 14, 2022

@DanielLockau-MLPA could you apply the following patch?

diff --git a/boards/native/Makefile.dep b/boards/native/Makefile.dep
index f0e996ae56..dedea2cc63 100644
--- a/boards/native/Makefile.dep
+++ b/boards/native/Makefile.dep
@@ -9,6 +9,7 @@ ifneq (,$(filter mtd,$(USEMODULE)))
 endif

 ifneq (,$(filter periph_can,$(FEATURES_USED)))
+  USEPKG += libsocketcan
   ifeq ($(OS),Linux)
     CFLAGS += -DCAN_DLL_NUMOF=2
   endif
diff --git a/tests/candev/Makefile b/tests/candev/Makefile
index 7f956d7406..b323ce96e0 100644
--- a/tests/candev/Makefile
+++ b/tests/candev/Makefile
@@ -1,21 +1,7 @@
-BOARD ?= nucleo-f767zi
-
 include ../Makefile.tests_common

 USEMODULE += shell
 USEMODULE += can
 USEMODULE += isrpipe

-
-# define the CAN driver you want to use here
-CAN_DRIVER ?= MCP2515
-
-ifeq ($(CAN_DRIVER), PERIPH_CAN)
-  FEATURES_REQUIRED += periph_can
-else ifeq ($(CAN_DRIVER), MCP2515)
-  USEMODULE += mcp2515
-else ifeq ($(CAN_DRIVER), CAN_ALT)
-  # other can modules can be defined here
-endif
-
 include $(RIOTBASE)/Makefile.include
diff --git a/tests/candev/Makefile.board.dep b/tests/candev/Makefile.board.dep
new file mode 100644
index 0000000000..3ccc5453ed
--- /dev/null
+++ b/tests/candev/Makefile.board.dep
@@ -0,0 +1,14 @@
+ifneq (,$(filter native,$(BOARD)))
+  CAN_DRIVER ?= PERIPH_CAN
+endif
+
+# define the CAN driver you want to use here
+CAN_DRIVER ?= MCP2515
+
+ifeq ($(CAN_DRIVER), PERIPH_CAN)
+  FEATURES_REQUIRED += periph_can
+else ifeq ($(CAN_DRIVER), MCP2515)
+  USEMODULE += mcp2515
+else ifeq ($(CAN_DRIVER), CAN_ALT)
+  # other can modules can be defined here
+endif

I tested that it works, I'll ACK right away:

Initializing CAN periph device
> receive
receive
Reading from Rxbuf...
id: 1234567f dlc: 3 data: 0x12 0x34 0x56
> send 1 2 3 4
send 1 2 3 4
> ^C
native: exiting

@DanielLockau-MLPA
Copy link
Contributor Author

DanielLockau-MLPA commented Apr 20, 2022

@DanielLockau-MLPA could you apply the following patch?

@fjmolinas I tried it locally but it seems to refer to a rebased version of this branch to a recent version of RIOT/master. Your patch touches code which was added by @kfessel to upstream but not by me. I will still rebase & apply as this part should only concern the CI, if anything.

Ubuntu dropped i386 support for socketcan already a while ago.
@fjmolinas
Copy link
Contributor

@DanielLockau-MLPA could you apply the following patch?

@fjmolinas I tried it locally but it seems to refer to a rebased version of this branch to a recent version of RIOT/master. Your patch touches code which was added by @kfessel to upstream but not by me. I will still rebase & apply as this part should only concern the CI, if anything.

Since I was not able to push, I opened a PR with your changes rebased #17967, ci seems to be passing so far, if its green maybe we can just get that one in to avoid another CI run? I don't really mind either way, I opened the PR just to get this in faster :)

@github-actions github-actions bot added the Area: boards Area: Board ports label Apr 20, 2022
@DanielLockau-MLPA
Copy link
Contributor Author

Since I was not able to push, I opened a PR with your changes rebased #17967, ci seems to be passing so far, if its green maybe we can just get that one in to avoid another CI run? I don't really mind either way, I opened the PR just to get this in faster :)

Damn. I just did the push. I don't mind as well. You can just go with your PR. Seems cleaner to me as well.

@fjmolinas
Copy link
Contributor

Since I was not able to push, I opened a PR with your changes rebased #17967, ci seems to be passing so far, if its green maybe we can just get that one in to avoid another CI run? I don't really mind either way, I opened the PR just to get this in faster :)

Damn. I just did the push. I don't mind as well. You can just go with your PR. Seems cleaner to me as well.

Let's see what happens faster, this PR finishing building or another maintainer taking a look at #17967, maybe @benpicco could proxy-ACK that one.

@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 20, 2022
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

ACK, tested, and murdock is building this now! ACK

@fjmolinas fjmolinas merged commit 85c76fb into RIOT-OS:master Apr 20, 2022
@fjmolinas
Copy link
Contributor

Thanks @DanielLockau-MLPA!

@benpicco benpicco deleted the fix/20220113__test_candev branch April 20, 2022 12:49
@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
Labels
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 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
Development

Successfully merging this pull request may close these issues.

5 participants