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

can: add a CAN stack for RIOT #5793

Merged
merged 9 commits into from
Jun 26, 2017
Merged

can: add a CAN stack for RIOT #5793

merged 9 commits into from
Jun 26, 2017

Conversation

vincent-d
Copy link
Member

@vincent-d vincent-d commented Aug 31, 2016

Hi,

This PR propose a CAN stack for RIOT.

It includes a device driver interface (drivers/include/can/candev.h) and a reference driver for native cpu and board (under Linux only using socketCAN).

The architecture of the device driver is inspired by netdev2, with a thread per device communicating with messages with the upper layer threads.

A basic conn API is also provided for user applications, for raw CAN and isotp.

This PR implements only the data link layer (raw CAN) and provide support for simultaneous reception from multiple user threads.

A common area is used to allocate CAN packets (frame + stack data) (implementation in sys/can/pkt.c) and the routing of the messages to the user threads is done by the "router" part (sys/can/router.c).

We are still working on 2 others drivers for stm32 internal CAN controller and MCP2515 (SPI CAN controller) and on a common interface to manage CAN transceivers.

Still to do:

  • add unit tests (ongoing)
  • power management and wake up from the CAN bus
  • improve bittiming setting
  • implementation of ISO-TP

You can test under Linux by setting up virtual CAN interface:

sudo modprobe vcan
sudo ip link add vcan0 type vcan
sudo ip link add vcan1 type vcan
sudo ip link set vcan0 up
sudo ip link set vcan1 up

Then you can run one the test app (tests/conn_cann) as usual:

make all
make term

Sending on can0 from RIOT, frame id 0x100, data 0x11 0x22:

can send 0 0x100 0x11 0x22

Receiving on can0 from RIOT, with thread 0, frame id 0x101:

can recv 0 0 0x101

Sending from Linux (with canutils), frame id 0x100, data 0x11 0x22:

cansend vcan0 100#1122

Receiving from Linux (any frame)

candump vcan0

Comments are welcommed :)

@OlegHahm OlegHahm added PR-award-nominee Deprecated. Will be removed soon. Type: new feature The issue requests / The PR implemements a new feature for RIOT Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Aug 31, 2016
@toonst
Copy link
Member

toonst commented Aug 31, 2016

Great to see it's here!

@miri64
Copy link
Member

miri64 commented Aug 31, 2016

Since googling for CAN just gives me a bunch of pictures of beverage containers I would appreciate some information about this stack ;-)

@kYc0o
Copy link
Contributor

kYc0o commented Aug 31, 2016

Try this one ;)

@miri64
Copy link
Member

miri64 commented Aug 31, 2016

thanks :-)

const struct candev_driver *driver; /**< ptr to that driver's interface. */
candev_event_cb_t event_callback; /**< callback for device events */
void *isr_arg; /**< argument to pass on isr event */
kernel_pid_t pid; /**< device thread pid */
Copy link
Contributor

Choose a reason for hiding this comment

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

is this necessary?

netdev2 has been designed to not force usage of a thread per device. maybe that architecture can be re-used here?

Copy link
Member Author

Choose a reason for hiding this comment

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

if we want the candev to be completely thread-agnostic, we can remove that from this struct, then we would need to add it into a struct in the can/device.h file.

I'll propose something.

@lebrush
Copy link
Member

lebrush commented Sep 1, 2016

Awesome!! I'll have a look at the code whenever I've a chance.

In case you are still working on a stm32 driver, I've a working version I wrote time ago for a previous RIOT version. Have a look here for the driver and here for the necessary board defines configuration.

@miri64
Copy link
Member

miri64 commented Sep 1, 2016

Apart from get_filter, set_filter, and abort (and the filter member) the driver struct doesn't look too dissimilar to netdev2_driver_t. Can we model the three functions as options (NETOPT_FILTER and NETOPT_ABORT)? I'm unsure about the filter member, but putting it into the function struct (which ideally would be const so we can benefit from compiler optimizations) seems odd to me anyway. So can we move it to the candev_t struct? Then it would be possible to model them just as netdev2s.

@vincent-d
Copy link
Member Author

A few words about CAN and filters.

With CAN you don't address nodes but frames, then every frame is broadcasted in the bus. Every CAN controller provide hardware filtering, to allow a node to receive only the frames it wants to receive.

The nb_filter member was here to set the number of hardware filters available in the controller. It's not used at the moment, and I'm not sure anymore if it makes sense anyway.

set_filter and remove_filter are then used to set and remove a hardware filter in the controller. The reason why we chose not using the generic set is that the set_filter is supposed to return an id associated to the filer we could use later on when receiving a frame. Knowing which filter was triggered might help to route the frame to the right user even if it's not used at the moment. Then remove_filter was build symetrically to remove a filter, and otherwise we would need an option like NETOPT_REMOVE_FILTER.

Another difference between candev_driver_t and netdev2_driver_t is that we remove the recv function. Frame reception is then achieved through the event. The received frame is passed as parameter.

Which leads to another difference, there is an additional parameter in the event function, which is used when sending, to return the handle of the sent frame and then be able to know wich frame has been sent correctly (event CANDEV_EVENT_TX_CONFIRMATION) or has timeouted (event CANDEV_EVENT_TIMEOUT_TX_CONF), or when receiving, to return the received frame as I previously wrote.

I'm wondering whether merging candev_driver_t and netdev2_drivert_t would actually make sense. I'm pretty convinced that this CAN stack could share some code with the network stack, but on the low level, a CAN controller will never be able to plugged to an IP network stack, and and radio module will never be connected to a CAN bus, so which benefit could we find in merging the both driver api ? I'm not against the idea, but I have trouble seeing the advantages.

@miri64
Copy link
Member

miri64 commented Sep 2, 2016

Okay. Thanks for the explanation :)

candev_linux_t *dev = (candev_linux_t *)candev;

mutex_init(&dev->rx_mutex);
mutex_init(&dev->rx_mutex);
Copy link
Member

@lebrush lebrush Sep 2, 2016

Choose a reason for hiding this comment

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

the mutex was already initialized in the line above, maybe here should be tx_mutex instead

Copy link
Member Author

Choose a reason for hiding this comment

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

both mutex are actually useless, I will remove them

@vincent-d
Copy link
Member Author

squashed and rebased

@aabadie aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 21, 2017
@aabadie
Copy link
Contributor

aabadie commented Jun 21, 2017

Needs squashing label was still set. I removed it and restarted Murdock. I guess I'll merge when everything is green (and after lunch :) )

@vincent-d
Copy link
Member Author

I guess I'll merge when everything is green

We need libsocketcan to build on native, RIOT-OS/riotdocker#31 is ready but needs review.

@aabadie
Copy link
Contributor

aabadie commented Jun 21, 2017

Ah yes, I forgot this...

@aabadie
Copy link
Contributor

aabadie commented Jun 23, 2017

@smlng @kaspar030 can you merge RIOT-OS/riotdocker#31 or can I do it myself ? Does the CI workers need special update afterwards or is it automatic once merged ?

@aabadie aabadie modified the milestone: Release 2017.07 Jun 23, 2017
@emmanuelsearch emmanuelsearch 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 Jun 26, 2017
@emmanuelsearch
Copy link
Member

emmanuelsearch commented Jun 26, 2017

@vincent-d CI still complains.

@kaspar030
Copy link
Contributor

CI docker container is rebuilding as we speak.

@kaspar030 kaspar030 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 Jun 26, 2017
@vincent-d
Copy link
Member Author

All green, finally! Thanks @kaspar030

@aabadie
Copy link
Contributor

aabadie commented Jun 26, 2017

Let's merge this one then ! GO!

@aabadie aabadie merged commit 0672319 into RIOT-OS:master Jun 26, 2017
@aabadie
Copy link
Contributor

aabadie commented Jun 26, 2017

Thanks for your patience @vincent-d :)

@aabadie
Copy link
Contributor

aabadie commented Jun 26, 2017

Now it's time to work on #6178, #6927 and #6925 for CAN hardware support. Those PRs need to be rebased.

@vincent-d vincent-d deleted the pr/can_stack branch June 26, 2017 14:17
@vincent-d
Copy link
Member Author

Thanks all!

Now it's time to work on #6178, #6927 and #6925 for CAN hardware support.

And #6276 ;)

@emmanuelsearch
Copy link
Member

@vincent-d congrats!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: major The PR changes a significant part of the code base. It should be reviewed carefully PR-award-nominee Deprecated. Will be removed soon. Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.