can mcp2515: spi can controller driver by vincent-d · Pull Request #6276 · RIOT-OS/RIOT

vincent-d · 2016-12-30T09:53:34Z

This driver is compliant with the candev interface and this PR is based on #5793 .

It has been tested on our own hardware.

The mcp2515 needs an external clock which can be provided wither by a crystal or by the host MCU.

A can_conf_mcp2515.h file is needed and must contains at least the following:

#include "candev_mcp2515.h"

const static candev_mcp2515_conf_t can_config_candev_mcp2515[] = {
    {
        .spi = SPI_1,
        .cs_pin = xxx,
        .rst_pin = xxx,
        .int_pin = xxx,
    }
};

#define CANDEV_MCP2515_NUMOF (sizeof(can_config_candev_mcp2515) / sizeof(candev_mcp2515_conf_t))

#define CANDEV_MCP2515_CLOCK 20000000U

LudwigKnuepfer · 2016-12-30T12:23:57Z

If someone from Berlin has time to review and test but needs hardware: feel free to contact me.

vincent-d · 2017-02-06T13:08:24Z

Rebased and adapted with the new autoinit process from #5793, and the new SPI interface!

LudwigKnuepfer · 2017-02-07T18:48:48Z

Since no review has happened so far, why don't you squash commits before anyone starts to review?

vincent-d · 2017-02-08T09:07:54Z

mcp2515 part is squashed

This driver implements the candev interface

tcschmidt · 2018-06-01T23:16:39Z

@LudwigKnuepfer if you have the hardware and mind testing yourself, why don't you ask @mfrey for assistance?

tcschmidt · 2018-06-11T10:15:24Z

@LudwigKnuepfer @A-Paul ping?

A-Paul · 2018-06-15T15:48:53Z

@aabadie , @lebrush and @haukepetersen. Is there a chance to review this PR?
I can do some reviewing based on code conventions but I have no insight into CAN. Since you have already been involved in #5793 you might be more effective here.

lebrush

I can't judge if it works but codewise looks neat and clean. I added some comments and questions regarding code and style (so no approve or refuse from my side).

lebrush · 2018-06-18T09:08:10Z

drivers/include/candev_mcp2515.h

+    /** masks list */
+    uint32_t masks[MCP2515_RX_MAILBOXES];
+    /** filters list */
+    canid_t filter_ids[MCP2515_RX_MAILBOXES][MCP2515_FILTERS_MB1];


There are two filter_ids too much, which will never be used. The construction MCP2515_FILTERS_MB0 * mailbox_index + filter_pos is used to interface with the mcp2515, I think the same construction could be used to access memory and two positions could be saved.

lebrush · 2018-06-18T09:08:40Z

drivers/mcp2515/candev_mcp2515.c

+
+static inline int _max_filters(int mailbox)
+{
+    return mailbox == 0 ? MCP2515_FILTERS_MB0 : MCP2515_FILTERS_MB1;


parenthesis. I've been quite a bit off-line, maybe this is accepted now :-)

lebrush · 2018-06-18T09:08:54Z

drivers/mcp2515/candev_mcp2515.c

+
+void candev_mcp2515_init(candev_mcp2515_t *dev, const candev_mcp2515_conf_t *conf)
+{
+    memset(dev, 0, sizeof(*dev));


~~dev not checked against NULL~~

i guess it's assumed that it will never be NULL. I remember in some pieces of RIOT's code the doxygen claims something like @pre (dev != NULL)` to make the user aware (and then point to a usage bug and not a code issue)

lebrush · 2018-06-18T09:09:39Z

drivers/mcp2515/candev_mcp2515.c

+    memcpy(&dev->candev.bittiming, &timing, sizeof(timing));
+    /* configure filters to be closed */
+    for (int mailbox = 0; mailbox < MCP2515_RX_MAILBOXES; mailbox++) {
+        dev->masks[mailbox] = 0;


I think these loops are not necessary. You already set all dev struct to 0 on the first line of the method.

lebrush · 2018-06-18T09:13:38Z

drivers/mcp2515/candev_mcp2515.c

+        }
+    }
+    res = mcp2515_set_mode(dev, MODE_NORMAL);
+    return res;


maybe you can remove res and simply return mcp2515_set_mode(...)

lebrush · 2018-06-18T10:38:55Z

drivers/mcp2515/mcp2515.c

+    int res = mcp2515_spi_read(dev, MCP2515_CANSTAT, &mode, 1);
+    if (res == 0) {
+        //TODO: error handling of extra information in mode result
+        result = (enum mcp2515_mode) mode & MCP2515_CANSTAT_OPMOD_MASK;


maybe add parenthesis

lebrush · 2018-06-18T10:40:17Z

drivers/mcp2515/mcp2515.c

+
+    mcp2515_spi_bitmod(dev, MCP2515_CANCTRL, MCP2515_CANCTRL_REQOP_MASK, (uint8_t) mode);
+    int cnt = 0;
+    while (cur_mode != mode && cnt++ < 2) {


magic number 2

lebrush · 2018-06-18T10:45:31Z

drivers/mcp2515/mcp2515_defines.h

+#define CAN_IRQ_PORTREN P1REN
+#define CAN_IRQ_PORTIES P1IES
+#define CAN_IRQ_PORTIE P1IE
+#define CAN_IRQ_PORTIFG P1IFG


Are these defines board/cpu specific?

lebrush · 2018-06-18T10:47:33Z

drivers/Makefile.dep

    FEATURES_REQUIRED += periph_gpio
 endif

+ifneq (,$(filter ltc4150,$(USEMODULE)))


does this belong here?

lebrush · 2018-06-18T10:52:05Z

drivers/mcp2515/candev_mcp2515.c

+
+    DEBUG("Inside mcp2515 set opt=%d\n", opt);
+    switch (opt) {
+    case CANOPT_BITTIMING:       /**< bit timing parameter */


why doxygen comment?

A-Paul · 2018-06-21T19:06:29Z

@vincent-d, I apologize for the delay. Hopefully you're still interested to get this driver into RIOT.

I made an approach to come as close as possible to run the test with a real device (although I havn't one yet). Both, with a checked out pure branch and rebased onto master. But build failed.

I also have no clue what to do when the shell will be running. And "tests/driver_mcp2515/README.md" is a litlle bit terse.

A-Paul

Compiling the test application faiies.
In the initial comment you mention creating a "can_conf_mcp2515.h". Where should this be placed?
There seems to be not location in the code where this file is referenced.
It defines a variable of type candev_mcp2515_conf_t, same as in the "main.c" where the unrsolved sysbol are. Why is this defined twice?

A-Paul · 2018-06-21T19:16:14Z

tests/driver_mcp2515/main.c

+
+static const candev_mcp2515_conf_t conf = {
+    .spi = SPI_1,
+    .cs_pin = CAN2_SS_PIN,


Causes a compile error as undeclared. Should it be replaced because names have changed or defined by the user?

A-Paul · 2018-06-21T19:17:47Z

tests/driver_mcp2515/main.c

+static const candev_mcp2515_conf_t conf = {
+    .spi = SPI_1,
+    .cs_pin = CAN2_SS_PIN,
+    .rst_pin = CAN2_RST_PIN,


Same as in line 46.

A-Paul · 2018-06-21T19:17:54Z

tests/driver_mcp2515/main.c

+    .spi = SPI_1,
+    .cs_pin = CAN2_SS_PIN,
+    .rst_pin = CAN2_RST_PIN,
+    .int_pin = CAN2_INT_PIN,


Same as in line 46.

A-Paul · 2018-06-21T19:19:41Z

tests/driver_mcp2515/main.c

+
+
+static const shell_command_t shell_commands[] = {
+    { "send", "send some data", _send },


_send causes a compile error as undeclared.

A-Paul · 2018-06-21T19:20:13Z

tests/driver_mcp2515/main.c

+
+static const shell_command_t shell_commands[] = {
+    { "send", "send some data", _send },
+    { "receive", "receive some data", _receive },


_receive causes a compile error as undeclared.

vincent-d · 2018-06-21T20:35:11Z

I'm gonna need some time to get back into this, it's been quite a long time ;) Thanks for taking a look at it anyway.

We actually don't use it in our product anymore, but I still think this driver is useful as this device is widely used.

If I remember correctly we wanted to rework or remove this test app, as it actually should be tested through the stack.

I can't promise I'll work on this soon as it's not anymore on top of my priority list.

smlng · 2018-08-08T11:28:38Z

@vincent-d I have the hardware and tried to test it but couldn't make it work. I connected it to a CAN bus where I'm certain that CAN messages are send on it from another (non-RIOT) device, but the receive IRQ and its handler on the MCP2515 never fired.

Could be SPI related though, but the SPI is working - I tested with another device driver which worked fine.

A-Paul · 2018-08-14T05:12:13Z

@smlng what board are you using and which code changes have you made?

smlng · 2018-08-14T06:23:15Z

I'm using a Bluepill, I just made a quick test no in-depth debugging, my changes are here https://github.com/smlng/RIOT/tree/testing/mcp2515

tcschmidt · 2018-10-30T19:20:31Z

@smlng @A-Paul could you document the state of the review with corresponding labels?

stale · 2019-08-10T04:08:10Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

miri64 · 2020-06-23T11:44:45Z

Closing this PR in favor of #13624.

LudwigKnuepfer added Area: drivers Area: Device drivers Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Dec 30, 2016

OlegHahm added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable State: waiting for other PR State: The PR requires another PR to be merged first labels Jan 3, 2017

OlegHahm assigned A-Paul Jan 3, 2017

vincent-d force-pushed the pr/mcp2515 branch from a332047 to 91643eb Compare January 23, 2017 08:58

vincent-d force-pushed the pr/mcp2515 branch 2 times, most recently from 5f0c589 to b6f79e2 Compare February 6, 2017 13:02

vincent-d force-pushed the pr/mcp2515 branch from b6f79e2 to 46078f8 Compare February 8, 2017 09:06

vincent-d force-pushed the pr/mcp2515 branch from 46078f8 to 9a714e0 Compare February 8, 2017 12:40

vincent-d force-pushed the pr/mcp2515 branch 3 times, most recently from c02f87c to fe54ff0 Compare March 1, 2017 12:33

vincent-d force-pushed the pr/mcp2515 branch from fe54ff0 to 7395a8f Compare March 3, 2017 15:52

vincent-d force-pushed the pr/mcp2515 branch 2 times, most recently from d5a8273 to ce1892a Compare March 23, 2017 14:45

vincent-d force-pushed the pr/mcp2515 branch 2 times, most recently from 1a9d198 to 6b7dc4a Compare April 13, 2017 08:15

vincent-d force-pushed the pr/mcp2515 branch from 46d3e74 to 20adca4 Compare May 19, 2017 13:48

vincent-d force-pushed the pr/mcp2515 branch from 20adca4 to 0049897 Compare June 16, 2017 12:45

aabadie modified the milestone: Release 2017.07 Jun 26, 2017

vincent-d force-pushed the pr/mcp2515 branch from 0049897 to 275f3ba Compare June 26, 2017 14:17

vincent-d mentioned this pull request Jun 26, 2017

can: add a CAN stack for RIOT #5793

Merged

Vincent Dupont added 2 commits June 26, 2017 16:19

mcp2515: add a driver for the mcp2515 SPI CAN controller

e8b5b00

This driver implements the candev interface

mcp2515: add a (partial) test app

cd1ee0f

vincent-d removed this from the Release 2018.04 milestone Apr 11, 2018

A-Paul requested review from aabadie, haukepetersen and lebrush June 15, 2018 15:41

lebrush reviewed Jun 18, 2018

View reviewed changes

A-Paul suggested changes Jun 21, 2018

View reviewed changes

vincent-d removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 21, 2018

miri64 added CI: needs squashing and removed CI: needs squashing labels Oct 5, 2018

miri64 removed the CI: needs squashing label Oct 12, 2018

A-Paul added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Nov 5, 2018

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019

smlng added State: don't stale State: Tell state-bot to ignore this issue and removed State: stale State: The issue / PR has no activity for >185 days labels Aug 12, 2019

wosym mentioned this pull request Jan 31, 2020

Drivers/mcp2515 toonst/RIOT#7

Closed

wosym mentioned this pull request Feb 13, 2020

Tests/candev: Initial version with native support #13360

Merged

wosym mentioned this pull request Mar 12, 2020

tests/candev: add mcp2515 + driver/mcp2515: add driver #13624

Merged

miri64 closed this Jun 23, 2020



		static const shell_command_t shell_commands[] = {
		{ "send", "send some data", _send },

Conversation

vincent-d commented Dec 30, 2016

Uh oh!

LudwigKnuepfer commented Dec 30, 2016

Uh oh!

vincent-d commented Feb 6, 2017

Uh oh!

LudwigKnuepfer commented Feb 7, 2017

Uh oh!

vincent-d commented Feb 8, 2017

Uh oh!

tcschmidt commented Jun 1, 2018

Uh oh!

tcschmidt commented Jun 11, 2018

Uh oh!

A-Paul commented Jun 15, 2018

Uh oh!

lebrush left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

A-Paul commented Jun 21, 2018

Uh oh!

A-Paul left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincent-d commented Jun 21, 2018

Uh oh!

smlng commented Aug 8, 2018

Uh oh!

A-Paul commented Aug 14, 2018

Uh oh!

smlng commented Aug 14, 2018

Uh oh!

tcschmidt commented Oct 30, 2018

Uh oh!

stale bot commented Aug 10, 2019

Uh oh!

miri64 commented Jun 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

lebrush left a comment •

edited

Loading