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

cpu/samd5x write can driver #19736

Merged
merged 10 commits into from
Mar 28, 2024

Conversation

firas-hamdi
Copy link
Contributor

@firas-hamdi firas-hamdi commented Jun 14, 2023

Contribution description

This PR provides the samd5x CPU definition with the CAN controller driver.
The driver supports the initialization of the CAN controller, the send/receive operations set/remove filters and the test mode that sets the CAN controller in loop back (listens to the bus as well as the own transmitted messages)

Testing procedure

The PR provides also an application to test the driver using the same54-xpro board.
The CAN controller should receive only the messages having a CAN ID matching the filters.
The bus can be monitored using a PCAN-USB adapter or a logic analyser.

@firas-hamdi firas-hamdi changed the title Cpu/samd5x write can driver cpu/samd5x write can driver Jun 14, 2023
@github-actions github-actions bot added Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: sys Area: System Area: tests Area: tests and testing framework Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Jun 14, 2023
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.

Just some initial comments. I think you should just use the definitions from the vendor files when possible instead of re-defining them yourself.

cpu/samd5x/include/can_params.h Outdated Show resolved Hide resolved
cpu/samd5x/include/candev_samd5x.h Outdated Show resolved Hide resolved
cpu/samd5x/include/candev_samd5x.h Outdated Show resolved Hide resolved
cpu/samd5x/include/candev_samd5x.h Outdated Show resolved Hide resolved
cpu/samd5x/include/candev_samd5x.h Outdated Show resolved Hide resolved
cpu/samd5x/periph/can.c Outdated Show resolved Hide resolved
sys/include/can/can.h Outdated Show resolved Hide resolved
@benpicco benpicco requested a review from wosym June 14, 2023 13:49
@wosym
Copy link
Member

wosym commented Jun 14, 2023

It's been ages since I worked with CAN and RIOT also has changed quite a bit since I last used it, so I probably won't be able to add much.

One question I did have after skimming through the code is: what's up with the separate candev test? The original test (now found in tests/drivers/candev) was meant to be entirely agnostic to the device used. Just specify the CAN device you're using, flash to target, et voila! It works and does the same as it would on another device. This way, it's possible to have several different MCU's with different CAN devices all running the same example while still being able to communicate with each other. I wonder what the reason is for creating a separate test that appears to be doing more or less the same. I would suggest editing the existing test with some preprocessor code to select your same5 driver and change as little as possible aside from that.

sys/include/can/can.h Outdated Show resolved Hide resolved
sys/include/can/can.h Outdated Show resolved Hide resolved
sys/include/can/can.h Outdated Show resolved Hide resolved
@firas-hamdi firas-hamdi force-pushed the cpu/samd5x-write-CAN-driver branch 3 times, most recently from 5bb6628 to e25fcb6 Compare June 20, 2023 16:48
@firas-hamdi firas-hamdi requested review from wosym and benpicco June 21, 2023 09:57
@firas-hamdi firas-hamdi force-pushed the cpu/samd5x-write-CAN-driver branch 3 times, most recently from d2a1be0 to 2c27655 Compare June 21, 2023 20:14
@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 Jul 7, 2023
Comment on lines 10 to 17
ifeq ($(BOARD), same54-xpro)
CFLAGS += "-DCANDEV_SAMD5X_DEFAULT_STD_FILTER_NUM=5"
CFLAGS += "-DCANDEV_SAMD5X_DEFAULT_EXT_FILTER_NUM=5"
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are the defaults only set that way for same54-xpro?
There can be other boards with that CAN peripheral

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 saw in boards definition that it's the only board using the SAMD/E5x cpu, so I limited these defaults to this board. But you're right, I can leave that out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benpicco I added a new variable here to leave these compilation flags just for the SAME54 family (as the only supporting CAN controllers)

@firas-hamdi
Copy link
Contributor Author

I will make a commits cleanup after the next round of reviewing

@kfessel
Copy link
Contributor

kfessel commented Mar 26, 2024

please squash your last 4 commits where they belong and put

add CAN RX mailbox feature

some where before them (the commit title needs area of work)

other than that this is good to go

if you like to you may rebase on master ( not required

@firas-hamdi firas-hamdi force-pushed the cpu/samd5x-write-CAN-driver branch from e39c6c9 to 491a1cd Compare March 26, 2024 14:18
Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

looks good 🎉

@benpicco benpicco enabled auto-merge March 26, 2024 14:57
@benpicco benpicco added this pull request to the merge queue Mar 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 26, 2024
@benpicco benpicco enabled auto-merge March 26, 2024 23:16
@benpicco benpicco added this pull request to the merge queue Mar 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 27, 2024
@benpicco benpicco enabled auto-merge March 27, 2024 13:53
@benpicco benpicco disabled auto-merge March 27, 2024 16:31
@benpicco benpicco enabled auto-merge March 27, 2024 16:32
@benpicco benpicco 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 Mar 27, 2024
@benpicco benpicco added this pull request to the merge queue Mar 28, 2024
Merged via the queue into RIOT-OS:master with commit 333207d Mar 28, 2024
25 checks passed
@kfessel
Copy link
Contributor

kfessel commented Mar 28, 2024

🎉

@MrKevinWeiss MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024
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: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: sys Area: System Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants