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

[wosym]

Contribution description

• Add the mcp2515 driver code from can mcp2515: spi can controller driver #6276
• Apply some minor fixes so it works with the current RIOT API (Because the driver was written > 4 years ago, and RIOT has obviously changed a bit since then) (Also ran uncrustify while I was at it)
• Add mcp2515 support to the candev app (see: Tests/candev: Initial version with native support #13360)

Testing procedure

• Adapt pinconfiguration in Makefile to match your own hardware (e.g. interrupt pin)
• Connect the board over USB with a linux laptop through means of a PCAN-USB cable (or something similar). Alternatively you can connect two boards to each other and receive the output of one on the other. This would remove the need for an expensive PCAN cable, but adds some complexity.
• Make sure libsocketcan and canutils are installed and running (see: https://github.com/RIOT-OS/RIOT/tree/master/tests/conn_can)
• Start candump on can0
• In the test-app shell: the send command will send a CAN message to can0. It should become visible in candump. Optionally you can pass some bytes (in decimal notation) as arguments (e.g. send 12 34 56). If no arguments are supplied, three default bytes will be sent.
• To test the receive part, we can send data from Linux with cansend (e.g. cansend can0 001#AABB) or with cangen (e.g. cangen can0 -v -n 1).
• In the test-app, the received bytes will be stored in a FIFO buffer and can be retrieved by calling receive. (an optional argument n can be passed to receive multiple can-messages). If there are no CAN-messages in the buffer when receive is called, it will block until a message is received.

Issues/PRs references

replaces #6276

[wosym]

@JannesVolkens Do you maybe have the hardware to test this?

[JannesVolkens]

@wosym I do.
I'm on vacation right now, but I'll get to it next week!

[JannesVolkens]

@wosym I tried to test this PR, but unfortunately I am unable to receive data. I’m using two mcp2515’s and also two nucleo-f207zg’s. I connected the mcp2515's to the boards accordingly and the two lines CANH and CANL are connected to a bus, which is terminated by resistors. Also I adapted the pin configuration for CS and INT (here I chose a empty pin on my board) in the makefile, even though I don’t know what to do with RESET. Furthermore I commented out line 26 in the makefile

USEMODULE += atmega_pcint0

(different cpu) because I don’t know which module to use here. Everything else stayed the same. The test is giving me send confirmations although I'm not getting any signals on my logic analyzer.

I probably overlooked something here. Can you give me some more information on how to configure the device to make it work? Or is there maybe something wrong with my setup?

[wosym]

Thanks for taking the time to test @JannesVolkens!
Admittedly, I have only tested it on AVR's myself (atmega328p and atmega1284p). Not on STM32 yet. (I have a nucleo board laying around here somewhere I'll try to free some time tomorrow to make a quick test with an STM32).

unfortunately I am unable to receive data.

What happens if you send multiple (> 4) messages in a row? Do you get an error? Usually, if the driver fails to transmit the message, it will keep it stored in mailboxes. After all mailboxes are filled, it should print this on screen.

I’m using two mcp2515’s and also two nucleo-f207zg’s. I connected the mcp2515's to the boards accordingly and the two lines CANH and CANL are connected to a bus, which is terminated by resistors.

I'm assuming you're also using a CAN-transceiver (such as the MCP2551), right? (I noticed that I didn't really make this clear in the testing-instructions of this PR. I'll fix that tomorrow)
Also: what value did you use as a terminating resistor?

I don’t know what to do with RESET

It is also mapped to a GPIO pin. You can set it in the Makefile, just like you did with CS and INT.
(Have to say, if I'm not mistaken, on a more recent board my company uses, they simply connected the RESET of the mcp to the reset of the AVR. So this should also work I think. )

Furthermore I commented out line 26 in the makefile

USEMODULE += atmega_pcint0

(different cpu) because I don’t know which module to use here.

I have no experience with external interrupts on an STM32 yet, but I can only imagine that it would also need the interrupts to be enabled. A quick scan through the datasheet tells me a few bits should be set in the NVIC.
RIOT will probably have this abstracted somehow, possible also with a USEMODULE, like the atmega's did. (Because it's pretty much the same operation... setting some bits in a register)
I'll try to do some more research tomorrow.

Or is there maybe something wrong with my setup?

It's not unlikely. Getting everything to work properly took me countless of hours of bug-hunting, and very often it turned out to be a hardware problem :) (not setting the terminating resistor is a classic, but you already mentioned that you did set it, so you're safe on this one ;) )

[JannesVolkens]

@wosym Sorry for the late response!

What happens if you send multiple (> 4) messages in a row? Do you get an error? Usually, if the driver fails to transmit the message, it will keep it stored in mailboxes. After all mailboxes are filled, it should print this on screen.

In this screenshot the MCP2515 was initialized and I tried sending four messages to the bus.

This second screenshot was taken not long after, but the printouts looked a bit different compared to the first one.

Both of the screenshots were taken with all debug messages turned on.

I'm assuming you're also using a CAN-transceiver (such as the MCP2551), right? (I noticed that I didn't really make this clear in the testing-instructions of this PR. I'll fix that tomorrow)
Also: what value did you use as a terminating resistor?

Yes I'm using a MPC2515 CAN module with a TJA1050 CAN-transceiver just like this one: https://www.makerfabs.com/can-module-mcp2515.html
My bus is terminated by 120 ohm resistors.

I have no experience with external interrupts on an STM32 yet, but I can only imagine that it would also need the interrupts to be enabled. A quick scan through the datasheet tells me a few bits should be set in the NVIC

I figured that the initialization of the INT pin is handled in:
int mcp2515_init(candev_mcp2515_t *dev, void(*irq_handler_cb)(void*)).
if I'm not mistaken.

During this process I had a logic analyzer connected and noticed that the MCP2515 kept trying to send the test message defined in
static int _send(int argc, char **argv)
over and over again but every single message was not acknowledged. This started with the first sending operation and continued even after I flashed my board (Changing the original message didn’t change the message that was being sent). The only way to stop the sending process was to cut the power. But I was able reproduce the same behaviour almost every single time. I was even able to receive the messages over a NCV7356 CAN-Transceiver but the sending process still continued.

Then I tried receiving messages over the MCP2515. I checked the reference manual for the MCP2515 and tried changing the mode of operation to the listen-only mode so that theoretically every single message is received (just to be sure). But unfortunately I received none of the messages I sent to the bus.

I really want to get it running but sadly I am not able to. Do you maybe have some more ideas?

[PeterKietzmann]

@JannesVolkens can you share the changes that you applied to the Makefile please

[wosym]

In this screenshot the MCP2515 was initialized and I tried sending four messages to the bus.
MCP2515_cmd

Usually this type of behaviour is because of a hardware problem: Faulty wiring, missing terminating resistor, OR interrupts not being received/handled properly.

This second screenshot was taken not long after, but the printouts looked a bit different compared to the first one.
MCP2515_cmdII

Mhmmm, this one looks strange. Is it possible that you reset the microcontroller, but not the MCP2515? Maybe some data was still in the mcp when the controller tried sending again?

Yes I'm using a MPC2515 CAN module with a TJA1050 CAN-transceiver just like this one: https://www.makerfabs.com/can-module-mcp2515.html
My bus is terminated by 120 ohm resistors.

I personally don't have any experience with this transceiver, but by the looks of it it works pretty much the same as the MCP2551. 120 ohm on both sides, close enough to the transceiver pins should work.

I figured that the initialization of the INT pin is handled in:
int mcp2515_init(candev_mcp2515_t *dev, void(*irq_handler_cb)(void*)).
if I'm not mistaken.

Correct. This function enables the interrupts. However, for AVR I needed an extra line in the Makefile. As far as I could tell, no such line is needed for STM32, but I can be wrong. Maybe some RIOT-STM32 expert can shed light on this :)

During this process I had a logic analyzer connected and noticed that the MCP2515 kept trying to send the test message defined in
static int _send(int argc, char **argv)
over and over again but every single message was not acknowledged. This started with the first sending operation and continued even after I flashed my board (Changing the original message didn’t change the message that was being sent). The only way to stop the sending process was to cut the power. But I was able reproduce the same behaviour almost every single time. I was even able to receive the messages over a NCV7356 CAN-Transceiver but the sending process still continued.

Then I tried receiving messages over the MCP2515. I checked the reference manual for the MCP2515 and tried changing the mode of operation to the listen-only mode so that theoretically every single message is received (just to be sure). But unfortunately I received none of the messages I sent to the bus.

I really want to get it running but sadly I am not able to. Do you maybe have some more ideas?

This really sounds like an issue with the interrupts. I think SPI communication between controller and MCP2515 works just fine, but once the mcp has finished sending the CAN-data, it needs to notify the controller of this so that it can clear its mailboxes. This is done by means of an interrupt.
If the interrupt is not received, the controller will not clear the mailboxes, if all mailboxes are full and you send more data, it will give you the "Failed to send CAN-message!" error. (This is why you don't get that message the first three times you send).

I have tried to clone your setup on a nucleo-board, and have not been able to send myself either. So indeed... Something still needs to be done for STM, but for now I don't really have an idea what that is.

I'll try to do some more experimenting in the coming days/weeks, but I can't say yet how much time I will have to invest in this. Corona is wreaking some havoc in my professional life :)

[PeterKietzmann]

@wosym thanks for your contribution and @JannesVolkens thanks for the testing efforts. This is just a very early review. Once we sorted out the issues with other platforms than AVRs, we can go into more detail, but some parts can already be cleaned up.

[JannesVolkens]

@wosym I cannot access the reset pin on the board that I use, because it is directly connected to VCC. Is the reset needed for this driver to work? And if yes does the int mcp2515_spi_reset(const candev_mcp2515_t *dev) function work exactly like the reset on the pin and can be used as a substitute?

[wosym]

@JannesVolkens I think that's the way I did it as well. If you look in the Makefile, there's a line TEST_MCP2515_RESET ?= GPIO_PIN\(PORT_B,2\). This is not the RESET-pin of the microcontroller, but simply an arbitrary chosen GPIO-pin that you connect to the RESET of the mcp2515.
If I'm not mistaken, it should also work if you connect the reset of the controller to the reset of the mcp (I see no reason why it shouldn't, and I think I've done so in the past).

But yes, I think a reset is required for the driver to work.

[PeterKietzmann]

This is not the RESET-pin of the microcontroller, but simply an arbitrary chosen GPIO-pin that you connect to the RESET of the mcp2515.

The mcp2515 breakout board does not expose the reset pin. Instead, the pin is wired to Vcc on the PCB

[wosym]

The mcp2515 breakout board does not expose the reset pin. Instead, the pin is wired to Vcc on the PCB

Aha! I didn't know that. I use discrete components myself. That complicates things a bit :D

However, I've been having another look at the code (the reset function in particular) and it seems like this reset-pin is never even used... The resetting happens in a different way, over SPI.

Maybe it'll work without defining this reset-pin? Because the way I currently see it (only by looking at the diff, don't have my laptop or hardware near me) I think there's a fair chance that it will work just fine. Can you test this, @JannesVolkens ? (I'll try testing it myself if I get the chance tomorrow)

[JannesVolkens]

The code compiles just fine without the definition of the reset pin. But I still get the same behaviour as described above.
Do you mean int mcp2515_spi_reset(const candev_mcp2515_t *dev)? This is also never used as far as i can tell.

[PeterKietzmann]

and it seems like this reset-pin is never even used

Isn't the reset pin used here which is at least called in the initialization here?

[wosym]

Yep, you are right @PeterKietzmann !
Like I said, I currently don't have my laptop near me so I was browsing through the diffs on this PR and ctrl+f gave me no hits on the reset pin. But I already thought it was weird that it wasn't used anywhere :)

It seems like there are two options: either connect the rst-pin to a GPIO and set it in the Makefile, or connect the reset of the mcp to the reset of the controller. I think both will work.

[PeterKietzmann]

It seems like there are two options: either connect the rst-pin to a GPIO and set it in the Makefile, or connect the reset of the mcp to the reset of the controller. I think both will work.

We will definitely try to modify our breakout board to make the reset pin available in order to test this PR. But we should not forget about option number three: explore an alternative reset mechanism so we don't rely on the reset pin. Our MCP2515 breakout board is a common piece of hardware which should be working out of the box with this driver .

[wosym]

I have ordered one of those breakout-boards myself a while ago, and expect it to arrive in a few days now. I'll test the driver once I have it.

If the breakoutboard really does not work out of the box, then it probably will need the RESET-byte written to it over SPI. This is what the mcp2515_spi_reset() is for.

[PeterKietzmann]

And if yes does the int mcp2515_spi_reset(const candev_mcp2515_t *dev) function work exactly like the reset on the pin and can be used as a substitute?

@JannesVolkens did you give it a try to replace the calls to mcp2515_reset() by mcp2515_spi_reset()?

[fjmolinas]

@wosym only https://github.com/RIOT-OS/RIOT/pull/13624/files#r495732584 missing from my side and we can move forward with this one.

[wosym]

If possible when all the comments are addressed I would like to see the PR tested in similar fashion as done here #13624 (comment), would you able to post some similar test results?

Since I think we're nearing the finish now, I already ran those tests again:

Sending a standard frame:

2020-09-30 21:39:38,342 # send
2020-09-30 21:39:38,347 # Inside mcp2515 send
2020-09-30 21:39:38,349 # sent usi_can_event: CANDEV_EVENT_ISR
2020-09-30 21:39:38,349 # Inside mcp2515 tx irq
2020-09-30 21:39:38,354 # _can_event: CANDEV_EVENT_TX_CONFIRMATION
result in candump:   can0  001   [3]  AB CD EF

Receiving a standard frame:

2020-09-30 21:42:02,785 # _can_event: CANDEV_EVENT_ISR
2020-09-30 21:42:02,791 # Inside mcp2515 rx irq, box=0
2020-09-30 21:42:02,792 # _can_event: CANDEV_EVENT_RX_INDICATION
2020-09-30 21:42:02,796 # 	id: 1 dlc: hx Data: 
receive
2020-09-30 21:42:05,407 # 	0xAA 0xBB  receive
2020-09-30 21:42:05,408 # Reading from Rxbuf...
2020-09-30 21:42:05,413 # id: 1 dlc: hx Data: 
2020-09-30 21:42:05,413 # 0xAA 0xBB 
same frame in candump:   can0  001   [2]  AA BB

Sending an extended frame:

2020-09-30 21:43:37,950 # send
2020-09-30 21:43:37,951 # Inside mcp2515 send
2020-09-30 21:43:37,956 # sent using_can_event: CANDEV_EVENT_ISR
2020-09-30 21:43:37,957 # Inside mcp2515 tx irq
2020-09-30 21:43:37,962 # _can_event: CANDEV_EVENT_TX_CONFIRMATION
result in candump:   can0  0FFFFFFF   [3]  AB CD EF

Receiving an extended frame:

2020-09-30 21:44:04,794 # _can_event: CANDEV_EVENT_ISR
2020-09-30 21:44:04,800 # Inside mcp2515 rx irq, box=0
2020-09-30 21:44:04,800 # _can_event: CANDEV_EVENT_RX_INDICATION
2020-09-30 21:44:04,806 # 	id: 8fffffff dlc: hx Data: 
receive
2020-09-30 21:44:09,078 # 	0xAA  receive
2020-09-30 21:44:09,079 # Reading from Rxbuf...
2020-09-30 21:44:09,084 # id: fffffff dlc: hx Data: 
2020-09-30 21:44:09,084 # 0xAA 
same frame in candump:   can0  0FFFFFFF   [1]  AA

(of course the last line of each code-box is not output from RIOT, but from candump running on Linux. It was sniffing the canbus during the test to verify all data being sent and received.)

[wosym]

I rebased on master to resolve the merge conflicts introduced in #14909 .
I did rework the way we select the drivers again though, to prevent things from getting too complex.
It's true that in the current form we make unnecessary steps, but I think this will make it much easier to use for someone who is not familiar with the can system in RIOT. I mean... I was even confused myself with how it was supposed to be used now.

Feel free to share your opinion on this though if you think there's another approach that works better without making it overcomplicated to the user!

[fjmolinas]

Feel free to share your opinion on this though if you think there's another approach that works better without making it overcomplicated to the user!

Lets not forget that this is a test application! Which means it should build for as many ARCH as possible,
the mcp2515 can be built for all platforms while periph_can can only be built for platforms with that module which are so far native and some stm32 boards. So that makes me think that maybe the best thing is actually to have two separate applications tests/periph_can and tests/driver_mcp2515, maybe configuration and code could be share between both. What do you think @wosym?

[fjmolinas]

This is not valid anymore.

[fjmolinas]

Suggested change

	Include the module for your desired driver in the Makefile. The application will automatically adapt its initialization procedure to the selected driver.
	Include the module for your desired driver in the Makefile. The application will automatically
	adapt its initialization procedure to the selected driver.

[fjmolinas]

Suggested change

	CAN_DRIVER ?= NATIVE
	CAN_DRIVER ?= PERIPH_CAN

[fjmolinas]

Suggested change

	ifeq ($(CAN_DRIVER), NATIVE)
	FEATURES_OPTIONAL += periph_can
	else ifeq ($(CAN_DRIVER), MCP2515)
	USEMODULE += mcp2515
	else ifeq ($(CAN_DRIVER), CAN_ALT)
	# other can modules can be defined here
	endif
	ifeq ($(CAN_DRIVER), PERIPH_CAN)
	FEATURES_REQUIRED += periph_can
	else ifeq ($(CAN_DRIVER), MCP2515)
	USEMODULE += mcp2515
	else ifeq ($(CAN_DRIVER), CAN_ALT)
	# other can modules can be defined here
	endif

[fjmolinas]

I think this would match better the current status.

[wosym]

I applied your suggestion, with the addition of an additional check on the used board. If the board is native, it will always default to periph_can, and not allow a different driver.
Why? Well... because before it was pretty clear that you had to use can_native on the native board. With the recent changes in the can structure it has (imo) become a little bit confusing that you have to use can_periph now for native.
So for users who are unfamiliar with can in RIOT, I think forcing this option on them will save them a lot of headaches I think :)

[wosym]

Feel free to share your opinion on this though if you think there's another approach that works better without making it overcomplicated to the user!

Lets not forget that this is a test application! Which means it should build for as many ARCH as possible,
the mcp2515 can be built for all platforms while periph_can can only be built for platforms with that module which are so far native and some stm32 boards. So that makes me think that maybe the best thing is actually to have two separate applications tests/periph_can and tests/driver_mcp2515, maybe configuration and code could be share between both. What do you think @wosym?

When I read your comment this morning I was very opposed to your suggestion. The original idea of this test application was to have a demo of the candev abstraction in RIOT that could be used with any of the available can drivers. Similar to tests/conn_can, but a lot less complex. It would work closer to the hardware, without the upper layers that make it a bit too complex for most situations.

But... two hours later I read you comment again, and I must admit... it would make sense in a way to split the application, and instead of making a single test for candev, we could indeed make a test for each of the can-devices (in this case there's only two: mcp2515 and can_periph, since apparently can_native is now deprecated and is also included in can_periph) that both communicate with their drivers through the candev abstraction.

This would add quite a bit of duplicate code, but then the tests would be more in line with the other tests in RIOT.

I'll try to split them as per your suggestion somewhere this week. I postpone resolving those last comments about the doc and Makefile for now, because those will need significant rewrites after the split anyways.

[fjmolinas]

Feel free to share your opinion on this though if you think there's another approach that works better without making it overcomplicated to the user!

Lets not forget that this is a test application! Which means it should build for as many ARCH as possible,
the mcp2515 can be built for all platforms while periph_can can only be built for platforms with that module which are so far native and some stm32 boards. So that makes me think that maybe the best thing is actually to have two separate applications tests/periph_can and tests/driver_mcp2515, maybe configuration and code could be share between both. What do you think @wosym?

When I read your comment this morning I was very opposed to your suggestion. The original idea of this test application was to have a demo of the candev abstraction in RIOT that could be used with any of the available can drivers. Similar to tests/conn_can, but a lot less complex. It would work closer to the hardware, without the upper layers that make it a bit too complex for most situations.

But... two hours later I read you comment again, and I must admit... it would make sense in a way to split the application, and instead of making a single test for candev, we could indeed make a test for each of the can-devices (in this case there's only two: mcp2515 and can_periph, since apparently can_native is now deprecated and is also included in can_periph) that both communicate with their drivers through the candev abstraction.

This would add quite a bit of duplicate code, but then the tests would be more in line with the other tests in RIOT.

I'll try to split them as per your suggestion somewhere this week. I postpone resolving those last comments about the doc and Makefile for now, because those will need significant rewrites after the split anyways.

@wosym since this PR has been dragging out for a while we could keep it somehow as is and split up as a future step?

[wosym]

@wosym since this PR has been dragging out for a while we could keep it somehow as is and split up as a future step?

Sure, I'm okay with that. The extra benefit of merging this one first, and then splitting in a separate PR is that everyone involved still has time to share their opinion about it. Because I can imagine some will be opposed to it.

I'll try to address those last comments this evening.

[fjmolinas]

@wosym since this PR has been dragging out for a while we could keep it somehow as is and split up as a future step?

Let me know what you prefer!

But if we keep it somehow at is we would still need it the application to be in a way where mcp2515 and periph_can get built for a number of arch. In the current state only stm32 and native would be built and only for periph_can. Something like the following could achieve that:

Option A

• Makefile

FEATURES_OPTIONAL += periph_can

• Makefile.board.dep

ifeq (,$(filter periph_can,$(FEATURES_USED)))
  USEMODULE += mcp2515
endif

It wouldn't have the configurability you want but it would allow us to get this PR in. The other option is have mcp2515 as the default, so:

Option B

CAN_DRIVER ?= MCP2515

ifeq ($(CAN_DRIVER), PERIPH_CAN)
  FEATURES_REQUIRED += periph_can
else ifeq ($(CAN_DRIVER), MCP2515)
  USEMODULE += mcp2515
else ifeq ($(CAN_DRIVER), CAN_ALT)
  # other can modules can be defined here
endif

Either of the two options is fine with me so pick which one you prefer (whats important for me is that the mcp2515 is pulled in with the default configuration so the CI builds it, periph_can is compile tested by conn_can)

And then as a second stage we could think of something similar to what is done for tests/driver_netdev_common.. something to iterate on.

[wosym]

@fjmolinas This is again the same chicken-and-egg question as before: what is it that this test is testing? The drivers? Or the candev abstraction?
Initially, the latter was intended. That's why I will opt for B, albeit with a slight change.

After the split, we can repurpose the test(s) and place the focus on the drivers, instead of the candev abstraction. I hope this is ok for you.

(Do note that we're essentially reverting the changes of the previous PR by doing this :) )

[fjmolinas]

ACK, @PeterKietzmann did the initial review of the code, I took over with the last details and @wosym provided test output for the most recent changes. Lets wait for murdock!

[fjmolinas]

@wosym hope you don't mind I pushed a change fixing a dependency issue for native, it was the only build failure, so I think this murdock run should be the last one... we will see in ~30min

[fjmolinas]

GO!

[fjmolinas]

Thanks for taking over this PR @wosym and for sticking through to the end, I'm sorry this took so long...

[wosym]

It was my pleasure @fjmolinas !
Big thanks to everyone who has helped get this feature written, reviewed, tested and merged! It was a good team effort :)