cpu/samd5x write can driver#19736
Conversation
firas-hamdi
requested review from
MichelRottleuthner,
aabadie,
benpicco,
dylad,
fjmolinas,
keestux,
leandrolanzieri and
smlng
as code owners
github-actions
bot
added
Area: boards
benpicco
left a comment
benpicco
left a comment
There was a problem hiding this comment.
Just some initial comments. I think you should just use the definitions from the vendor files when possible instead of re-defining them yourself.
|
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. |
firas-hamdi
force-pushed
the
cpu/samd5x-write-CAN-driver
branch
3 times, most recently
from
5bb6628 to
e25fcb6
Compare
firas-hamdi
force-pushed
the
cpu/samd5x-write-CAN-driver
branch
3 times, most recently
from
d2a1be0 to
2c27655
Compare
benpicco
added
the
CI: ready for build
| ifeq ($(BOARD), same54-xpro) | ||
| CFLAGS += "-DCANDEV_SAMD5X_DEFAULT_STD_FILTER_NUM=5" | ||
| CFLAGS += "-DCANDEV_SAMD5X_DEFAULT_EXT_FILTER_NUM=5" | ||
| endif |
There was a problem hiding this comment.
Why are the defaults only set that way for same54-xpro?
There can be other boards with that CAN peripheral
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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
force-pushed
the
cpu/samd5x-write-CAN-driver
branch
from
5fecc71 to
1525ea7
Compare
benpicco
removed
the
State: WIP
firas-hamdi
force-pushed
the
cpu/samd5x-write-CAN-driver
branch
2 times, most recently
from
04384ac to
e39c6c9
Compare
|
I will make a commits cleanup after the next round of reviewing |
|
please squash your last 4 commits where they belong and put 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 |
cpu/samd5x: load RX mailbox
firas-hamdi
force-pushed
the
cpu/samd5x-write-CAN-driver
branch
from
e39c6c9 to
491a1cd
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
benpicco
removed
the
CI: ready for build
|
🎉 |
6 participants
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.