Merge pull request #14909 from OTAkeys/pr/conn_can_clean_up

can: add proper checks for ifnum validity

cpu/esp32/include/periph_cpu.h

@@ -563,6 +563,10 @@ typedef struct {
 #define UART_NUMOF_MAX  (3)
 /** @} */
 
+#ifdef MODULE_PERIPH_CAN
+#include "can_esp.h"
+#endif
+
 #ifdef __cplusplus
 }
 #endif

cpu/native/Kconfig

@@ -51,6 +51,7 @@ config NATIVE_OS_DARWIN
 
 config NATIVE_OS_LINUX
     bool
+    select HAS_PERIPH_CAN
     select HAS_PERIPH_GPIO
     select HAS_PERIPH_GPIO_IRQ
     select HAS_PERIPH_SPI

cpu/native/Makefile.features

@@ -22,4 +22,6 @@ ifeq ($(OS),Linux)
   FEATURES_PROVIDED += periph_spi
   # Hardware GPIO access is only available on Linux hosts
   FEATURES_PROVIDED += periph_gpio periph_gpio_irq
+  # CAN is only supported on Linux through socketCAN
+  FEATURES_PROVIDED += periph_can
 endif

cpu/native/include/can_params.h

@@ -16,8 +16,8 @@
  * @author      Vincent Dupont <vincent@otakeys.com>
  */
 
-#ifndef CANDEV_LINUX_PARAMS_H
-#define CANDEV_LINUX_PARAMS_H
+#ifndef CAN_PARAMS_H
+#define CAN_PARAMS_H
 
 #include "candev_linux.h"
 #include "can/device.h"
@@ -29,7 +29,7 @@ extern "C" {
 /**
  * @brief Default parameters (device names)
  */
-static candev_params_t candev_linux_params[] = {
+static const candev_params_t candev_params[] = {
     { .name = "can0", },
     { .name = "can1", },
 };
@@ -38,5 +38,5 @@ static candev_params_t candev_linux_params[] = {
 }
 #endif
 
-#endif /* CANDEV_LINUX_PARAMS_H */
+#endif /* CAN_PARAMS_H */
 /** @} */

cpu/native/include/candev_linux.h

@@ -42,10 +42,13 @@ extern "C" {
 /**
  * Linux candev configuration
  */
-typedef struct candev_linux_conf {
+typedef struct candev_conf {
     /** local interface name */
     char interface_name[CAN_MAX_SIZE_INTERFACE_NAME + 1];
-} candev_linux_conf_t;
+} can_conf_t;
+
+/** CAN device configuration type can_conf_t is redefined by native CAN */
+#define HAVE_CAN_CONF_T
 
 #ifndef CANDEV_LINUX_MAX_FILTERS_RX
 /**
@@ -74,25 +77,18 @@ typedef struct candev_linux_conf {
 typedef struct candev_linux {
     candev_t candev;                  /**< candev base structure */
     int sock;                         /**< local socket id */
-    const candev_linux_conf_t *conf;  /**< device configuration */
+    const can_conf_t *conf;  /**< device configuration */
     /** filter list */
     struct can_filter filters[CANDEV_LINUX_MAX_FILTERS_RX];
-} candev_linux_t;
+} can_t;
 
-/**
- * @brief Device specific initialization function
- *
- * @param[inout] dev   the device to initialize
- * @param[in] conf     the device configuration
- *
- * @return 0 on success
- */
-int candev_linux_init(candev_linux_t *dev, const candev_linux_conf_t *conf);
+/** CAN device type can_t is redefined by native CAN */
+#define HAVE_CAN_T
 
 /**
  * @brief Array containing socketCAN device names
  */
-extern candev_linux_conf_t candev_linux_conf[CAN_DLL_NUMOF];
+extern can_conf_t candev_conf[CAN_DLL_NUMOF];
 
 #endif /* defined(__linux__) */
 

cpu/native/include/periph_cpu.h

@@ -156,6 +156,10 @@ typedef enum {
 #endif
 /** @} */
 
+#ifdef MODULE_PERIPH_CAN
+#include "candev_linux.h"
+#endif
+
 #ifdef __cplusplus
 }
 #endif

cpu/native/periph/can.c

@@ -53,7 +53,7 @@ static int _remove_filter(candev_t *candev, const struct can_filter *filter);
 static int _power_up(candev_t *candev);
 static int _power_down(candev_t *candev);
 
-static int _set_bittiming(candev_linux_t *dev, struct can_bittiming *bittiming);
+static int _set_bittiming(can_t *dev, struct can_bittiming *bittiming);
 
 static const candev_driver_t candev_linux_driver = {
     .send = _send,
@@ -69,7 +69,7 @@ static const candev_driver_t candev_linux_driver = {
 static candev_event_t _can_error_to_can_evt(struct can_frame can_frame_err);
 static void _callback_can_sigio(int sock, void *arg);
 
-candev_linux_conf_t candev_linux_conf[CAN_DLL_NUMOF] = {
+can_conf_t candev_conf[CAN_DLL_NUMOF] = {
 #if CAN_DLL_NUMOF >= 1
    {
         .interface_name = "vcan0",
@@ -82,9 +82,9 @@ candev_linux_conf_t candev_linux_conf[CAN_DLL_NUMOF] = {
 #endif
 };
 
-int candev_linux_init(candev_linux_t *dev, const candev_linux_conf_t *conf)
+int can_init(can_t *dev, const can_conf_t *conf)
 {
-    memset(dev, 0, sizeof(candev_linux_t));
+    memset(dev, 0, sizeof(can_t));
     dev->candev.driver = &candev_linux_driver;
     dev->conf = conf;
     dev->candev.bittiming.bitrate = CANDEV_LINUX_DEFAULT_BITRATE;
@@ -129,7 +129,7 @@ static candev_event_t _can_error_to_can_evt(struct can_frame can_frame_err)
 static void _callback_can_sigio(int sockfd, void *arg)
 {
     (void) sockfd;
-    candev_linux_t *dev = (candev_linux_t *) arg;
+    can_t *dev = (can_t *) arg;
 
     if (dev->candev.event_callback) {
         dev->candev.event_callback(&dev->candev, CANDEV_EVENT_ISR, NULL);
@@ -149,7 +149,7 @@ static int _init(candev_t *candev)
     int ret;
 
     DEBUG("Will start linux CAN init\n");
-    candev_linux_t *dev = (candev_linux_t *)candev;
+    can_t *dev = (can_t *)candev;
 
     if ((strlen(dev->conf->interface_name) == 0)
             || (strlen(dev->conf->interface_name) > CAN_MAX_SIZE_INTERFACE_NAME)) {
@@ -212,7 +212,7 @@ static int _init(candev_t *candev)
 static int _send(candev_t *candev, const struct can_frame *frame)
 {
     int nbytes;
-    candev_linux_t *dev = (candev_linux_t *)candev;
+    can_t *dev = (can_t *)candev;
 
     nbytes = real_write(dev->sock, frame, sizeof(struct can_frame));
 
@@ -232,7 +232,7 @@ static void _isr(candev_t *candev)
 {
     int nbytes;
     struct can_frame rcv_frame;
-    candev_linux_t *dev = (candev_linux_t *)candev;
+    can_t *dev = (can_t *)candev;
 
     if (dev == NULL) {
         return;
@@ -272,7 +272,7 @@ static void _isr(candev_t *candev)
 
 }
 
-static int _set_bittiming(candev_linux_t *dev, struct can_bittiming *bittiming)
+static int _set_bittiming(can_t *dev, struct can_bittiming *bittiming)
 {
     int res;
 
@@ -300,7 +300,7 @@ static int _set_bittiming(candev_linux_t *dev, struct can_bittiming *bittiming)
 
 static int _set(candev_t *candev, canopt_t opt, void *value, size_t value_len)
 {
-    candev_linux_t *dev = (candev_linux_t *) candev;
+    can_t *dev = (can_t *) candev;
     int res = 0;
 
     switch (opt) {
@@ -349,7 +349,7 @@ static int _set(candev_t *candev, canopt_t opt, void *value, size_t value_len)
 
 static int _get(candev_t *candev, canopt_t opt, void *value, size_t max_len)
 {
-    candev_linux_t *dev = (candev_linux_t *) candev;
+    can_t *dev = (can_t *) candev;
     int res = 0;
 
     switch (opt) {
@@ -456,7 +456,7 @@ static int _get(candev_t *candev, canopt_t opt, void *value, size_t max_len)
 
 static int _set_filter(candev_t *candev, const struct can_filter *filter)
 {
-    candev_linux_t *dev = (candev_linux_t *)candev;
+    can_t *dev = (can_t *)candev;
 
     if (filter == NULL) {
         DEBUG("candev_native: _set_filter: error filter NULL\n");
@@ -500,7 +500,7 @@ static int _set_filter(candev_t *candev, const struct can_filter *filter)
 
 static int _remove_filter(candev_t *candev, const struct can_filter *filter)
 {
-    candev_linux_t *dev = (candev_linux_t *)candev;
+    can_t *dev = (can_t *)candev;
 
     if (filter == NULL) {
         DEBUG("candev_native: _remove_filter: error filter NULL\n");

cpu/native/startup.c

@@ -515,7 +515,7 @@ __attribute__((constructor)) static void startup(int argc, char **argv, char **e
                     usage_exit(EXIT_FAILURE);
                 }
                 optarg++;
-                strncpy(candev_linux_conf[i].interface_name, optarg,
+                strncpy(candev_conf[i].interface_name, optarg,
                         CAN_MAX_SIZE_INTERFACE_NAME);
                 }
                 break;

cpu/stm32/Makefile.dep

@@ -20,4 +20,9 @@ ifneq (,$(filter stm32_eth,$(USEMODULE)))
   USEMODULE += xtimer
 endif
 
+ifneq (,$(filter periph_can,$(FEATURES_USED)))
+  FEATURES_REQUIRED += periph_gpio
+  FEATURES_REQUIRED += periph_gpio_irq
+endif
+
 include $(RIOTCPU)/cortexm_common/Makefile.dep

sys/auto_init/can/auto_init_can_linux.c

@@ -1,53 +0,0 @@
-/*
- * Copyright (C) 2016 OTA keys S.A.
- *
- * This file is subject to the terms and conditions of the GNU Lesser
- * General Public License v2.1. See the file LICENSE in the top level
- * directory for more details.
- */
-
-/**
- * @ingroup     sys_auto_init
- * @{
- * @file
- * @brief       initializes native can device
- *
- * @author      Vincent Dupont <vincent@otakeys.com>
- * @}
- */
-
-#include "can/device.h"
-#include "candev_linux_params.h"
-
-#define CANDEV_LINUX_NUMOF ARRAY_SIZE(candev_linux_params)
-
-#ifndef CANDEV_LINUX_STACKSIZE
-#define CANDEV_LINUX_STACKSIZE (THREAD_STACKSIZE_DEFAULT + THREAD_EXTRA_STACKSIZE_PRINTF)
-#endif
-
-#ifndef CANDEV_LINUX_BASE_PRIORITY
-#define CANDEV_LINUX_BASE_PRIORITY (THREAD_PRIORITY_MAIN - CANDEV_LINUX_NUMOF - 2)
-#endif
-
-static candev_dev_t candev_dev_linux[CANDEV_LINUX_NUMOF];
-static char _can_linux_stacks[CANDEV_LINUX_NUMOF][CANDEV_LINUX_STACKSIZE];
-static candev_linux_t candev_linux[CANDEV_LINUX_NUMOF];
-
-void auto_init_can_native(void) {
-
-    for (size_t i = 0; i < CANDEV_LINUX_NUMOF; i++) {
-        candev_linux_init(&candev_linux[i], &candev_linux_conf[i]);
-        candev_dev_linux[i].dev = (candev_t *)&candev_linux[i];
-        candev_dev_linux[i].name = candev_linux_params[i].name;
-#ifdef MODULE_CAN_TRX
-        candev_dev_linux[i].trx = candev_linux_params[i].trx;
-#endif
-#ifdef MODULE_CAN_PM
-        candev_dev_linux[i].rx_inactivity_timeout = candev_linux_params[i].rx_inactivity_timeout;
-        candev_dev_linux[i].tx_wakeup_timeout = candev_linux_params[i].tx_wakeup_timeout;
-#endif
-
-        can_device_init(_can_linux_stacks[i], CANDEV_LINUX_STACKSIZE, CANDEV_LINUX_BASE_PRIORITY + i,
-                        candev_linux_params[i].name, &candev_dev_linux[i]);
-    }
-}

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"
 

sys/auto_init/can/init.c

@@ -49,11 +49,6 @@ void auto_init_candev(void)
     isotp_init(isotp_stack, ISOTP_STACK_SIZE, ISOTP_PRIORITY, "isotp");
 #endif
 
-#ifdef MODULE_CAN_LINUX
-    extern void auto_init_can_native(void);
-    auto_init_can_native();
-#endif
-
 #ifdef MODULE_PERIPH_CAN
     extern void auto_init_periph_can(void);
     auto_init_periph_can();

sys/can/conn/raw.c

@@ -42,7 +42,11 @@ int conn_can_raw_create(conn_can_raw_t *conn, struct can_filter *filter, size_t
                         int ifnum, int flags)
 {
     assert(conn != NULL);
-    assert(ifnum < CAN_DLL_NUMOF);
+    if (ifnum < 0 || ifnum >= CAN_DLL_NUMOF) {
+        memset(conn, 0, sizeof (*conn));
+        conn->ifnum = -1;
+        return -ENODEV;
+    }
 
     DEBUG("conn_can_raw_create: create conn=%p, ifnum=%d flags=%d\n", (void *)conn, ifnum, flags);
 
@@ -121,7 +125,11 @@ static void _tx_conf_timeout(void *arg)
 int conn_can_raw_send(conn_can_raw_t *conn, const struct can_frame *frame, int flags)
 {
     assert(conn != NULL);
-    assert(conn->ifnum < CAN_DLL_NUMOF);
+
+    if (conn->ifnum < 0 || conn->ifnum >= CAN_DLL_NUMOF) {
+        return -ENODEV;
+    }
+
     assert((conn->flags & CONN_CAN_RECVONLY) == 0);
     assert(frame != NULL);
 
@@ -201,7 +209,11 @@ static void _rx_timeout(void *arg)
 int conn_can_raw_recv(conn_can_raw_t *conn, struct can_frame *frame, uint32_t timeout)
 {
     assert(conn != NULL);
-    assert(conn->ifnum < CAN_DLL_NUMOF);
+
+    if (conn->ifnum < 0 || conn->ifnum >= CAN_DLL_NUMOF) {
+        return -ENODEV;
+    }
+
     assert(frame != NULL);
 
     xtimer_t timer;
@@ -256,7 +268,10 @@ int conn_can_raw_recv(conn_can_raw_t *conn, struct can_frame *frame, uint32_t ti
 int conn_can_raw_close(conn_can_raw_t *conn)
 {
     assert(conn != NULL);
-    assert(conn->ifnum < CAN_DLL_NUMOF);
+
+    if (conn->ifnum < 0 || conn->ifnum >= CAN_DLL_NUMOF) {
+        return -ENODEV;
+    }
 
     DEBUG("conn_can_raw_close: conn=%p\n", (void *)conn);
 

sys/can/dll.c

@@ -470,7 +470,9 @@ int raw_can_power_up(int ifnum)
 
 int raw_can_set_bitrate(int ifnum, uint32_t bitrate, uint32_t sample_point)
 {
-    assert(ifnum < candev_nb);
+    if (ifnum < 0 || ifnum >= candev_nb) {
+        return -1;
+    }
 
     int res = 0;
     int ret;

tests/candev/Makefile

@@ -1,20 +1,16 @@
 include ../Makefile.tests_common
 
-# the test currently only works with can_linux, so on "native"
-BOARD_WHITELIST := native
-
 USEMODULE += shell
 USEMODULE += can
 USEMODULE += isrpipe
 
+FEATURES_OPTIONAL += periph_can
+
 # define the CAN driver you want to use here
-CAN_DRIVER ?= CAN_NATIVE
+CAN_DRIVER ?= CAN_EXT
 
-ifeq ($(CAN_DRIVER), PERIPH_CAN)
-# periph_can modules/variables go here
-
-else ifeq ($(CAN_DRIVER), CAN_NATIVE)
-# can_native modules/variables go here
+ifeq ($(CAN_DRIVER), CAN_EXT)
+# external CAN driver modules/variables go here
 
 endif
 

tests/candev/main.c

@@ -30,11 +30,12 @@
 #include "shell.h"
 #include "can/device.h"
 
-#if IS_USED(MODULE_CAN_LINUX)
+#if IS_USED(MODULE_PERIPH_CAN)
 
-#include <candev_linux.h>
+#include "periph/can.h"
+#include "can_params.h"
 
-static candev_linux_t linux_dev;
+static can_t periph_dev;
 
 #else
 /* add other candev drivers here */
@@ -192,10 +193,10 @@ int main(void)
     puts("candev test application\n");
 
     isrpipe_init(&rxbuf, (uint8_t *)rx_ringbuf, sizeof(rx_ringbuf));
-#if IS_USED(MODULE_CAN_LINUX)
-    puts("Initializing Linux Can device");
-    candev_linux_init( &linux_dev, &(candev_linux_conf[0]));    /* vcan0 */
-    candev = (candev_t *)&linux_dev;
+#if IS_USED(MODULE_PERIPH_CAN)
+    puts("Initializing CAN periph device");
+    can_init(&periph_dev, &(candev_conf[0]));    /* vcan0 on native */
+    candev = (candev_t *)&periph_dev;
 #else
     /* add initialization for other candev drivers here */
 #endif