add SLCAN

[findlayfeng]

CAN over Serial / SLCAN is a can bus device driven on Serial

Added the merging of slcan with reference to the usbcan device. This merging has been tested simply on the stm32f103 series devices. Now it needs to test more devices

Devices that support both serial and can can add items in the configuration. You can directly submit and merge to this branch

No relevant documents have been written yet. For usage, please refer to the following content

// Enable the kernel

modprobe can
modprobe can-raw
modprobe slcan

// Create a device. Pay attention to the baud rate settings of the host and the host // slcand command comes from can-utils

slcand -o -s8 -S2000000 /dev/serial/by-id/xxx can0

// Enable the device

ip link set up dev can0

// Discover the device

~/klippy-env/bin/python ~/klipper/scripts/canbus_query.py can0

Reference Documents https://www.canusb.com/products/can232/ https://github.com/torvalds/linux/tree/master/drivers/net/can/slcan https://github.com/linux-can/can-utils/blob/master/slcand.c

[KevinOConnor]

Thanks. In general it seems fine to me. I have some questions and a few comments.

I am curious how you are using this support. What machines are you using it on? Is a uart speed of 2Mhz realistic?

In general the code seems fine, but I think it would be preferable if we could make some changes to the Kconfig definitions:

• Could we change CONFIG_SLCAN to something a little more verbose - perhaps CONFIG_SLCAN_CANBUS or CONFIG_SERIAL_CANBUS.
• I don't think we should change src/stm32/serial.c for new Kconfig settings. Instead, I think it would be preferable for the Kconfig files to define the appropriate symbols. So, for example, change the existing serial menu to STM32_DIRECT_SERIAL_UART4, add a new STM32_SLCAN_SERIAL_UART4, and then have Kconfig produce STM32_SERIAL_UART4 if either of those two settings are set. (As an example, see how the existing Kconfig produces one STM32_CANBUS_PB8_PB9 even though it can be set in different menus.)
• Same comment as above to have Kconfig produce just one CONFIG_SERIAL_BAUD setting.

Cheers, -Kevin

[github-actions[bot]]

Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html

There are some steps that you can take now:

1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards, ~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[findlayfeng]

When I was perfecting the driver code of slcan, I found that klipper's canbus implementation did not have a correct mechanism to handle packet timeouts. This problem is not a big problem when it is a can slave, because timeout means that there must be a bus error.

However, when it exists as a can bridge and there is no real client, this problem becomes very serious, because when the management packet is broadcast to the bridge, it will inevitably cause a timeout, but there may be no error here. At this time, the subsequent packets will be blocked. This is the case on stm32.

Do we need to implement error handling for the can interface? I think the usb can bridge also needs it.

[nefelim4ag]

If Klipper is configured as a CAN Bridge, it is required to have a slave device. https://www.klipper3d.org/CANBUS.html

So, the scenario where it is configured as a CAN bridge and there is no slave device is already not valid.

[KevinOConnor]

FWIW, the latest version of usb_canbus.c does implement a "discarding mode" if the canbus freezes - see src/generic/usb_canbus.c:try_canmsg_send().

-Kevin

[findlayfeng]

Currently this Pr is available

[Arksine]

It appears that one of the test configs needs to be updated. Specifically test/configs/stm32f103-serial.config.

--- a/test/configs/stm32f103-serial.config
+++ b/test/configs/stm32f103-serial.config
@@ -1,4 +1,4 @@
 # Base config file for STM32F1 ARM processor using serial communication
 CONFIG_MACH_STM32=y
 CONFIG_MACH_STM32F103=y
-CONFIG_STM32_SERIAL_USART1=y
+CONFIG_STM32_DIRECT_SERIAL_USART1=y

I think the above should work.

[findlayfeng]

What else do I need to do on this PR?

[Sofronio]

@findlayfeng I tried compiling on the MKS Skipr, and the following errors showed up:

src/generic/slcan.c: In function 'frame_dump':
src/generic/slcan.c:268:9: error: a label can only be part of a statement and a declaration is not a statement
         uint8_t index = f->cmd_data[0] - '0';
         ^~~~~~~
src/generic/slcan.c:287:9: error: a label can only be part of a statement and a declaration is not a statement
         struct canbus_msg msg;

It seems the compiler doesn't allow variable declarations right after a label (probably due to C89 rules). Moving those variable declarations to the top of the frame_dump() function fixed the issue and it compiled successfully.

[findlayfeng]

@findlayfeng I tried compiling on the MKS Skipr, and the following errors showed up:

src/generic/slcan.c: In function 'frame_dump':
src/generic/slcan.c:268:9: error: a label can only be part of a statement and a declaration is not a statement
         uint8_t index = f->cmd_data[0] - '0';
         ^~~~~~~
src/generic/slcan.c:287:9: error: a label can only be part of a statement and a declaration is not a statement
         struct canbus_msg msg;

It seems the compiler doesn't allow variable declarations right after a label (probably due to C89 rules). Moving those variable declarations to the top of the frame_dump() function fixed the issue and it compiled successfully.

Have you modified this Makefile?

https://github.com/findlayfeng/klipper/blob/293fb0a92349880aec94ad2d51cefeb0d67c81bb/Makefile#L33C3-L33C13