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 stm32: add a driver for STM32 bxCAN peripheral #6178

Merged
merged 4 commits into from
Apr 8, 2019

Conversation

vincent-d
Copy link
Member

This driver is compliant with the candev interface. It has been tested
with STM32F0 and STM32F2 only at this time but should be compliant with
other STM32Fx devices.

This is based on #5793.

@OlegHahm OlegHahm added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: drivers Area: Device drivers State: waiting for other PR State: The PR requires another PR to be merged first labels Dec 8, 2016
@vincent-d vincent-d force-pushed the pr/can_stm32 branch 2 times, most recently from 81d1f98 to 6d9394d Compare December 15, 2016 11:04
@vincent-d
Copy link
Member Author

Rebased on latest #5793.

@vincent-d vincent-d force-pushed the pr/can_stm32 branch 2 times, most recently from 0c3a99d to e0b7c9e Compare December 30, 2016 09:43
@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jan 14, 2017
@vincent-d vincent-d force-pushed the pr/can_stm32 branch 2 times, most recently from 0b72376 to df1dce3 Compare February 6, 2017 13:02
@vincent-d
Copy link
Member Author

Rebased and adapted with the new autoinit process from #5793.

@vincent-d
Copy link
Member Author

add support for 3 CAN devices (stm32f413), it includes #6565

@vincent-d vincent-d force-pushed the pr/can_stm32 branch 2 times, most recently from cdfb528 to 557cac8 Compare February 8, 2017 12:40
@vincent-d vincent-d force-pushed the pr/can_stm32 branch 4 times, most recently from b67eabf to 6fb32f9 Compare March 1, 2017 11:15
@vincent-d vincent-d force-pushed the pr/can_stm32 branch 2 times, most recently from d35c765 to 3ff7184 Compare March 23, 2017 14:44
@vincent-d vincent-d force-pushed the pr/can_stm32 branch 2 times, most recently from 8dacbac to c3501fc Compare April 13, 2017 08:14
@vincent-d vincent-d force-pushed the pr/can_stm32 branch 2 times, most recently from dd5cff8 to fd87d3f Compare May 11, 2017 11:52
@aabadie aabadie added this to the Release 2017.07 milestone Jun 23, 2017
@smlng
Copy link
Member

smlng commented Mar 25, 2019

Hi @vincent-d we tested again with your fix for STM F1 and it looks a bit better but also weird in other places. We have an oscilloscope capable of understanding CAN and when we use the nucleo-F1 and send something the osci can see the correct data and stuff but also errors stating "Missing ACK". However connecting 2 Nucleo F1s we only see error frames, also when using a CAN Tx/Rx to CAN High/Low translation module we only see error frames. So it seems that the sender is somehow detecting an error right from the start, maybe because the RX line is hold up (or down) incorrectly.

@MrKevinWeiss had similar problems when using I2C on the F1 because of missing pull ups (or downs) on the signal lines, maybe that's the problem here, too.

@MrKevinWeiss what did you do or try to make the I2C work on the F1?

@MrKevinWeiss
Copy link
Contributor

@MrKevinWeiss what did you do or try to make the I2C work on the F1?

There were two things I had to do, one was a problem with alternate pin function, I had to enable the alt pin remap clock then specify the i2c pins to use the alternate function pin. The second thing was I updated some documentation because you cannot have pullups on alternate function pins. I had to physically add them when testing.

@vincent-d
Copy link
Member Author

First, you can't connect two CAN controllers directly, you need CAN transceivers in between, as in this picture:

    +----------------+        +-----------------+                +-----------------+        +----------------+
    |                |   TX   |                 |    CAN High    |                 |   TX   |                |
    | CAN Controller +--------> CAN Transceiver +----------------+ CAN Transceiver <--------+ CAN Controller |
    |                |        |                 |                |                 |        |                |
    |     STM32      <--------+                 +----------------+                 +-------->     STM32      |
    |                |   RX   |                 |    CAN Low     |                 |   RX   |                |
    +----------------+        +-----------------+                +-----------------+        +----------------+

Most of the transceivers have a low power mode where they can only pass received signals but not emit signals, so you have to ensure they are in normal mode.

The bus also needs to be terminated by a 120 ohm reistor at both ends of the bus (between CAN high and CAN low), see https://en.wikipedia.org/wiki/CAN_bus#/media/File:CAN_ISO11898-2_Network.png .

Missing ACK means that no node on the bus ACK'ed the frame (after the transmission, all nodes on the bus should write an ACK bit on the bus).

@smlng
Copy link
Member

smlng commented Mar 28, 2019

@vincent-d thanks for the advice, we made some more tests with a proper setup and were able to send and receive to/from another (non-RIOT) device.

@smlng smlng added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Mar 28, 2019
Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

the code is not a beauty (especially all the #ifdefs are annoying) but it works, so there is room for improvement. Anyways, we should get this merged first ... see comments, and then also squash please!

Makefile.dep Outdated
@@ -634,6 +634,10 @@ ifneq (,$(filter puf_sram,$(USEMODULE)))
FEATURES_REQUIRED += puf_sram
endif

ifneq (,$(filter can_stm32,$(USEMODULE)))
USEMODULE += xtimer
Copy link
Member

Choose a reason for hiding this comment

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

why? I didn't find any usage of timer, or did I miss something?

Copy link
Member Author

Choose a reason for hiding this comment

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

probably left-over, I'll remove

@@ -8,7 +8,15 @@ BOARD_INSUFFICIENT_MEMORY := arduino-duemilanove arduino-mega2560 arduino-uno \
nucleo-l053r8 stm32f0discovery telosb \
waspmote-pro wsn430-v1_3b wsn430-v1_4 z1

CFLAGS += -DLOG_LEVEL=LOG_ALL
BOARDS_WITH_STM32_CANDEV := nucleo-f072rb \
Copy link
Member

Choose a reason for hiding this comment

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

what's that for? actually it would be better to have an ifeq or something matching these boards and then add USEMODULE = periph_can bc otherwise CAN is not pulled in. At least I had to specify the USEMODULE explicitly for my nucleo-f103rb, otherwise there was no can device available

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a left-over, I'll remove.

The boards supporting CAN should have a FEATURES_PROVIDED += periph_can in their Makefile.features to pull it in (nucleo-f413zh has it, we can expand to other boards too)

@smlng smlng added the Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines label Mar 28, 2019
This driver is compliant with the candev interface. It has been tested
with STM32F0 and STM32F2 and STM32F413 ONLY at this time but should be
compliant with other STM32Fx devices
@vincent-d
Copy link
Member Author

@smlng thanks so much for the review and the test. I know there is room for clean-up and improvements here, but having this merged would be the first step!

Squashed!

@smlng
Copy link
Member

smlng commented Mar 29, 2019

I would agree to merge, however Murdock does not: some documentation issues, but also there is now an ESP32 providing periph_can... I'm not sure if that is compatible with this or what adaption is needed. Maybe @gschorcht can also have a look and shed some light?!

@vincent-d
Copy link
Member Author

Indeed, ESP32 needs adaptation...I can take a look, but I'll need help to test it.

@gschorcht
Copy link
Contributor

@sming @vincent-d I will take a look later today to this PR and how it is related to ESP. I'm out of office at the moment.

@gschorcht
Copy link
Contributor

@vincent-d
ESP32 integrates a CAN DLL and PHY signaling sublayer controller which is compatible with the NXP SJA1000 CAN controller. The esp_can module implements a CAN driver as documented in CAN device driver interface for the ESP32 CAN controller which is used by CAN Data Link Layer.

You also implement the STM32 CAN interface according to CAN device driver interface. But instead defining an architecture specific auto initialization function, you also define a new low level peripheral interface periph/can with auto initialization functionality. Great idea 😄

The compilation error occurs since the board definition of esp32-olimex-evb enables feature periph_can why can_params.h is tried to be included in sys/auto_init/can/auto_init_periph_can.c which doesn't exist in ESP32 CAN implementation 😟

Two options to solve the problem:

  1. We change esp32-olimex-evb board definition as follows

    - FEATURES_PROVIDED += periph_can
    + FEATURES_PROVIDED += esp_can
  2. We adapt esp_can according to your new interface.

Maybe, the easiest way for the moment might be to follow option 1 and once your PR is merged I can try to realize option 2 as separate PR.

@smlng
Copy link
Member

smlng commented Mar 30, 2019

Maybe, the easiest way for the moment might be to follow option 1 and once your PR is merged I can try to realize option 2 as separate PR.

yep, I was thinking the same, @vincent-d please add a commit commenting out periph_can on esp32 also add comment on why. After getting this here merged, we can work on (re)activating it.

@smlng
Copy link
Member

smlng commented Apr 3, 2019

please squash

@smlng
Copy link
Member

smlng commented Apr 8, 2019

ACK & GO

@smlng smlng merged commit 4dd09ea into RIOT-OS:master Apr 8, 2019
@gschorcht
Copy link
Contributor

@vincent-d I'm going to change esp_can_auto_init to auto_init_periph_can and was wondering whether following code isn't obsolete and should be removed.

#ifdef MODULE_CAN_STM32
extern void auto_init_can_stm32(void);
auto_init_can_stm32();
#endif
}

@vincent-d
Copy link
Member Author

@gschorcht indeed, it seems like leftovers that slipped through...This is dead code since can_stm32 isn't a module anymore.

Feel free to remove.

And thanks for working on CAN ;) Let me know if I can help to review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.