can: add proper checks for ifnum validity

[vincent-d]

Contribution description

Improve error checking of user-facing CAN APIs.

Most functions were using asserts, but in some cases it might not be a
programmatic error to pass an invalid ifnum. This makes sure the code
does not crash by testing it at runtime and returning an error.

[wosym]

That try_get_msg function looked very familiar. Took me a while to remember, but now I do!
It's the function I removed in #13270. You even commented on the removal, on which I motivated the removal.
I don't really object to adding it again (it won't break anything), but... as far as I can tell the motivation for removal I gave back then still stands. It is a redundant check which complicates the code a bit.

The other checks all make sense. ACK on those.

---

Possibly unrelated, but I tried to test the changes on native (before trying it on a nucleo board), and I wasn't able to get it working.
make all term BOARD=native gives There are unsatisfied feature requirements: periph_can.

When adding CONTINUE_ON_EXPECTED_ERRORS=1 it shows the error fatal error: can_params.h: No such file or directory 20 | #include "can_params.h"

It's entirely possible that I'm doing something wrong here (or there's something wrong with my system), but can you confirm that this test should be able to run on native without any changes? (And if yes: any clue what I'm doing wrong here? Haven't really used conn_can myself yet)

[vincent-d]

@wosym thanks for the review.

I removed the try_get_msg thing, it was a leftover when I cherry-picked to commit from our branch which was based on an old RIOT.

For the native issue. It seems master was also broken (probably for quite some time, btw). The native CAN driver was not properly ported to the periph_can approach. So I added another commit to this PR to fix this.

[wosym]

The new commits you added look a lot scarier than the initial one.
For example, the entire auto_init_can_linux file was removed?
Could you explain the motivations behind this?
is can_init now generic for all can initializations? (not just native)
If it is, then this might have a significant impact on how #13624 needs to work.

Anyways, I tested the changes by running the candev app (before and after this patch), and it seems to function the same.

[vincent-d]

can_init() is the init function for CAN periph, provided by cpu/board. It has been introduced in #6178 for stm32, then #11989 adapted esp32 to follow the API. Unfortunately, we forgot to adapt the native board at that time (my fault). Those changes fix this and change the native driver so it's using the periph API it is supposed to implement.

IMO, it should not impact your work on #13624 since mcp2515 is not a periph, but an external driver.

[wosym]

I tested the conn_can with this PR today. two remarks:

On native, init 0 results in a segmentation fault on my system. However, when I do can send 0 1 12 (without doing the init first), everything seems to work like it's supposed to. This might be worth investigating?

On nucleo-f207zg it complains at compile time about not having periph_can. I assume this is because it is missing in the KConfig for that board (but I guess that is out of scope for this PR).

[vincent-d]

Finally found some time to check.

init 0 is supposed to initialize a CAN transceiver. But there is none with native board. There is a robustness issue with the init shell command of the conn_can test, but I think this is out of scope for this PR.

[wosym]

Code looks good to me and makes sense.

ACK

(Murdock still has some complaints though)

[fjmolinas]

@vincent-d there are some issue murdock seems to have flaged out;

• missing dependency, please add the following in cpu/stm32/Makefile.dep

ifneq (,$(filter periph_can,$(FEATURES_USED)))
  FEATURES_REQUIRED += periph_gpio
  FEATURES_REQUIRED += periph_gpio_irq
endif

• HAVE_CAN_T and HAVE_CAN_CONF_T must be defined for candev_linux and they are wrongly placed for esp32. I'm a bit confused about how this is structured, module wise this is the layout I infer:

                                can/candev.h
                                    |
                                    |
                    --------------------------------
                    |                               |
                    |                               |
                    |                               |
                periph/can.h                others(e.g. mcp2515)
                    |
                    |
    ----------------------------
    |                          |
    |                          |
candev_linux.c            periph/can.c

Header wise you have these HAVE_CAN_T and HAVE_CAN_CONF_T flags do define custom
configurations, but these need to be accounted for in periph_cpu.h so either move those flags and the structs definitions there or include the headers there (e.g.: for native include candev_linux.h in periph_cpu.h). The problem with this is that periph_cpu.h would include unrelated periph headers.

This is just a suggestion there might be better way to layout this. You could do something like what is done for sock, and have a can_types.h header (instead of can_linux.h and can_esp.h). And then in periph/can.h have something like:

#ifndef HAVE_CAN_T
/**
 * @brief   CAN device descriptor identifier
 */
typedef candev_t can_t;
#else
include "can_types.h"
#endif

#ifndef HAVE_CAN_CONF_T
/**
 * @brief   CAN configuration identifier
 */
typedef int can_conf_t;
#else
include "can_types.h"
#endif

• can_init is now redefined for candev_linux and can be removed form the candev_linux.h header

[vincent-d]

@fjmolinas I tried to clean up a bit. First I renamed candev_linux.c to periph/can.c so it's now clear that the Linux version is a native periph.

On the side of the headers, I modified both native and esp32 so they are done the same way stm32 did: the specific header (candev_linux.h or can_esp.h) is included from periph_cpu.h if MODULE_PERIPH_CAN is built.

Let me know if it's good enough with you. 

[fjmolinas]

Let me know if it's good enough with you.

Looks good! lets see if murdock is happy now!

[fjmolinas]

@vincent-d this should fix the last remaining issues:

diff --git a/cpu/native/include/can_params.h b/cpu/native/include/can_params.h
index 61e42117f3..8c7f37c391 100644
--- a/cpu/native/include/can_params.h
+++ b/cpu/native/include/can_params.h
@@ -29,7 +29,7 @@ extern "C" {
 /**
  * @brief Default parameters (device names)
  */
-static candev_params_t candev_params[] = {
+static const candev_params_t candev_params[] = {
     { .name = "can0", },
     { .name = "can1", },
 };
diff --git a/sys/auto_init/can/auto_init_periph_can.c b/sys/auto_init/can/auto_init_periph_can.c
index 0c2ffb6697..8fa49fbdd1 100644
--- a/sys/auto_init/can/auto_init_periph_can.c
+++ b/sys/auto_init/can/auto_init_periph_can.c
@@ -16,6 +16,7 @@
  * @}
  */
 
+#include "periph/can.h"
 #include "can/device.h"
 #include "can_params.h"

On a side note what is the difference between these two headers, or more specifically I'm a bit lost regarding the header hierarchy

• can/candev.h
• can/device.h

[vincent-d]

I guess this part might be refactored, at least the naming. But can/candev.h defines a stack-independent driver API, where can/device.h is the stack-specific definitions.

[fjmolinas]

@vincent-d murdock is failing to report to status... but its all green but for static tests, please squash!

[fjmolinas]

Untested ACK, trusting @vincent-d testing.

[wosym]

Untested ACK, trusting @vincent-d testing.

@fjmolinas I had tested the initial version, but after that a lot more changes were added to this PR.
One of those changes is that the candev app now uses PERIPH_CAN by default, instead of CAN_NATIVE.
I tested it now, and everything seems to work like before, but could you (@vincent-d ) give some explanation what exactly are the differences between the current implementation and the previous one?
Does this mean that this will allow us to flash the same code on to, let's say a nucleo board with CAN, and use the can peripheral of that board out of the box? (If yes, that's a very neat change!)
If not, what exactly are the differences?
And should CAN_NATIVE be considered deprecated from now on?

In any case: I must say that the code looks a lot cleaner with this change, and I think it also solves a potential future problem that had been discussed a few weeks ago. Kudos to you! :)