drivers: can: CANBUS driver for native_posix board #37124

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.

Sign up for GitHub

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

Jump to bottom

Closed

pllee4-weston wants to merge 4 commits into zephyrproject-rtos:main from westonrobot:patch-upstream-main-can_native_posix

pllee4-weston commented Jul 22, 2021 •

edited

Loading

Rebased jukkar's work for canbus for native_posix board to latest main branch

jukkar#21
#14196

@jukkar it would be great if you can help to review as well

jukkar and others added 4 commits

July 22, 2021 08:55


          drivers: can: Add support for native_posix board

713cc0b

This initial version supports SocketCAN. The driver passes
CANBUS data between Linux vcan virtual CAN driver and Zephyr.

You also need to use can-setup.sh script from the net-tools
project in order to make communication work with Linux vcan
driver.

Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>


          samples: net: sockets: can: Add native_posix board support

2d0cb8b

Add native_posix board support to socket-can sample application.

Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>


          rebase: rebased jukkar:canbus-driver-for-native_posix to tag zephyr-v…

cdcc8be

…2.6.0

Update file name canbus_native_posix_xx.x to can_native_posix_xx.x to
keep consistent with can driver for other platforms.

Update implementation to work with the current state of zephyr and
update samples/net/sockets/can/boards/native_posix.conf.

Replace deprecated DEVICE_AND_API_INIT() with DEVICE_DT_INST_DEFINE()
and add can_1 and can_2 in native_posix.dts.

Signed-off-by: Ruixiang Du <ruixiang.du@westonrobot.com>


          drivers: can: replace canbus_np_context with socket_can_context

b1cf9c6

Move struct socket_can_context to socket_can_context.h to ensure
struct socket_can_context can be included without replying on
socket_can_generic.h.

Change rx thread of canbus to be preemptive to avoid can frame loss.

Update naming of _ENABLE to _ENABLED to follow convention in
Kconfig.native_posix.

Signed-off-by: Pin Loon Lee <pinloon.lee@westonrobot.com>

pllee4-weston requested review from aescolar, alexanderwachter, daor-oti, pfalcon, rlubos and tbursztyka as code owners

July 22, 2021 00:58

github-actions bot added area: Boards area: CAN area: Devicetree area: native port area: Networking area: Samples labels

pllee4-weston changed the title ~~CANBUS driver for native_posix board~~ drivers: can: CANBUS driver for native_posix board

cfriedt assigned rlubos

Contributor

rlubos commented Jul 26, 2021

@cfriedt I'm not a CAN maintainer, I think it should rather be assigned to @alexanderwachter.

rlubos removed their assignment

cfriedt assigned alexanderwachter

gmarull reviewed

View reviewed changes

drivers/can/CMakeLists.txt

    
              zephyr_sources_ifdef(CONFIG_CAN_SHELL        can_shell.c)

              zephyr_sources_ifdef(CONFIG_CAN_NET          can_net.c)

              if(CONFIG_CAN_NATIVE_POSIX)

Member

gmarull Jul 26, 2021

Just a quick comment: socket CAN is only available on Linux, so I think a name other than "native_posix" should be used. The build should only be available on Linux hosts actually (maybe CMAKE_HOST_SYSTEM_NAME can be useful here?)

gmarull reviewed

View reviewed changes

drivers/can/Kconfig.native_posix

    
              config CAN_NATIVE_POSIX_INTERFACE_1_NAME

              	string "CANBUS interface 1 name on Linux side"

              	depends on CAN_NATIVE_POSIX_INTERFACE_1_ENABLED

              	default "vcan0"

Member

gmarull Jul 26, 2021

DeviceTree can likely be used for that

Member

alexanderwachter commented Jul 26, 2021

SocketCAN was written mainly maintained by @jukkar. I'm not very familiar with SocketCAN, but I can comment on the API later this week.

henrikbrixandersen self-requested a review

August 27, 2021 21:42

pfalcon requested a review from jukkar

September 14, 2021 21:04

Contributor

pfalcon commented Sep 14, 2021

@pllee4-weston: This now has a git conflict, please rebase.

Contributor

pfalcon commented Sep 14, 2021

@alexanderwachter on Jul 26:

I'm not very familiar with SocketCAN, but I can comment on the API later this week.

It still would be nice to get a review from you, as you're the CAN guy ;-).

alexanderwachter requested changes

View reviewed changes

drivers/can/can_native_posix.c

    
              #define DT_DRV_COMPAT zephyr_native_posix_can

              #define LOG_MODULE_NAME canbus_posix

Member

alexanderwachter Sep 15, 2021

These can go to the registration in line 20

drivers/can/can_native_posix.c

    
              			}

              		}

              		k_sleep(K_MSEC(10));

Member

alexanderwachter Sep 15, 2021

Why is there a sleep? Do you need to poll for the interface up?

drivers/can/can_native_posix.c

    
              	int ret = -ENODEV;

              	ARG_UNUSED(timeout);

              	ARG_UNUSED(callback_isr);

Member

alexanderwachter Sep 15, 2021

I would rather return an error in case someone pass a callback.
Since callbacks are not supported, this driver may not work with other CAN subsys like ISO-TP or canopen.

drivers/can/can_native_posix.c

    
              	return ret < 0 ? ret : 0;

              }

              static int canbus_np_attach_isr(const struct device *dev, can_rx_callback_t isr,

Member

alexanderwachter Sep 15, 2021

How do you want to receive any frame if this function is empty?
Seems that you only want to support socketCAN. If so, please remove the z_can API

drivers/can/can_native_posix.c

    
              	ctx->dev_fd = canbus_np_iface_open(ctx->if_name);

              	if (ctx->dev_fd < 0) {

              		LOG_ERR("Cannot open %s (%d)", ctx->if_name, ctx->dev_fd);

Member

alexanderwachter Sep 15, 2021

Return an error?

drivers/can/can_native_posix_adapt.c

    
              #define LOG_LEVEL CONFIG_CAN_LOG_LEVEL

              #include <logging/log.h>

              LOG_MODULE_REGISTER(LOG_MODULE_NAME);

Member

alexanderwachter Sep 15, 2021

Same as above, move the name and level as arguments to this macro

drivers/can/can_native_posix_adapt.c

Comment on lines +65 to +67

    
              		ret = -errno;

              		close(fd);

              		return ret;

Member

alexanderwachter Sep 15, 2021

Suggested change

      
            		ret = -errno;
          
            		close(fd);
          
            		return ret;
          
            		close(fd);
          
            		return -errno;

Member

alexanderwachter commented Sep 15, 2021

Since this PR only supports socketCAN, why don't you use a socket with PF_CAN directly instead?

github-actions bot commented Nov 15, 2021

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

github-actions bot added the Stale label

github-actions bot closed this

martinjaeger mentioned this pull request

drivers: can: implement native Linux SocketCAN driver #49894

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

gmarull gmarull left review comments

alexanderwachter alexanderwachter requested changes

aescolar Awaiting requested review from aescolar

daor-oti Awaiting requested review from daor-oti

pfalcon Awaiting requested review from pfalcon

rlubos Awaiting requested review from rlubos

tbursztyka Awaiting requested review from tbursztyka

henrikbrixandersen Awaiting requested review from henrikbrixandersen

jukkar Awaiting requested review from jukkar

Labels

area: Boards area: CAN area: Devicetree area: native port area: Networking area: Samples Stale