[RFC] drivers: stepper: initial support for stepper motors #26221

gmarull · 2020-06-16T18:03:53Z

A proposal to introduce a new API to control stepper motors. The API allows to control motor enablement, execute movements (sync and async) as well as set the speed. It has been designed to allow multiple stepper motors per controller.

The proposed implementation is for STM32 microcontrollers (tested on H7 only, but can be easily adjusted for all families). It uses two timers in master/slave mode (one to generate the steps and the second to keep track of the steps). It has been tested using a DRV8825 stepper driver. Note that while driver supports more than a single motor, only one can be active at the same time.

Example DT entry:

stepperctl0: {
	compatible = "st,stm32-stepperctl";

	#address-cells = <1>;
	#size-cells = <0>;
	label = "STEPPERCTL_0";
	master-timer = <&timers1>;
	slave-timer = <&timers2>;
        itr = <DT_STM32_TIMERS_TIM1_TIM2_ITR>;

	stepper0: stepper@2 {
		reg = <2>;
		microsteps = <1>;
		enable-gpios = <&gpioe 15 GPIO_ACTIVE_LOW>;
		direction-gpios = <&gpiod 8 GPIO_ACTIVE_HIGH>;
	};

	stepper1: stepper@1 {
		reg = <1>;
		microsteps = <8>;
		enable-gpios = <&gpioe 13 GPIO_ACTIVE_LOW>;
		direction-gpios = <&gpioe 12 GPIO_ACTIVE_HIGH>;
	};
};

Note that if this RFC is accepted I'll add documentation, examples and tests.

dcpleung · 2020-06-16T18:15:08Z

Q: what are the differences between sync and async movement? (like under what circumstances they are being used)

gmarull · 2020-06-16T18:17:41Z

@dcpleung sync: waits until movement is completed, async: you are notified (useful e.g. to change the speed while moving)

dcpleung

API additions look good.

ABOSTM

Thanks @gmarull for this PullRequest.

drivers/stepper/stepper_stm32.c

ABOSTM · 2020-07-03T10:06:55Z

drivers/stepper/stepper_stm32.c

+	 * yield to a value that does not fit into the register. In such case an
+	 * iterative computation is performed by changing psc.
+	 *
+	 * In order to prioritize runtime speed, the current psc value is used.


There is a smart and efficient way to compute prescaler:
if 32b timer psc=0 will match all cases with best precision
if 16b timer: psc = (f_tim / speed) >> 16
Note the same kind of formula could be apply to 32b with ... >> 32 , but is is useless due to uint32_t variable type usage

I have updated calculations, can you check? Really good suggestion, thanks.

drivers/stepper/stepper_stm32.c

ABOSTM · 2020-07-03T12:19:39Z

drivers/stepper/stepper_stm32.c

+ * @param slave Slave timer.
+ * @param itr Where ITR will be stored.
+ */
+static int get_slave_timer_itr(TIM_TypeDef *master, TIM_TypeDef *slave,


I discussed with @erwango about this function:

First of all, those ITR number mapping are different from one STM32 serie to another. So it would be better to defined it in files dedicated to a serie.

Also we think we could save some flash, save some processing time and simplify stm32 serie variants by handling this differently, ... thanks to DeviceTree.

Our proposal would be:

Create some defines in includes/dt-bindings/timers/stm32h7_timers.h
One file for each serie supporting stepper motor.
(File name is '.../timers...' and not stepper has this ITR may be used for other purpose)
Example of defines:

/* <MasterTIM>_<SlaveTIM>_ITR */ #define TIM15_TIM1_ITR LL_TIM_TS_ITR0

add in file 'st,stm32-stepperctl.yaml ' a property 'itr' besides 'master-timer' and 'slave-timer'
Possibles value would be one of the above defines.
Then in device tree one should add itr value besides master-timer and slave-timer instances

Sounds reasonable, and at the same time these settings become available for other drivers if needed.

Added an initial version for H7. Can you check if this is what you guys expected? cc: @erwango

drivers/stepper/stepper_stm32.c

ABOSTM · 2020-07-03T14:00:56Z

drivers/stepper/stepper_stm32.c

+static uint32_t get_timer_clock(struct device *clk,
+				const struct stm32_pclken *pclken)
+{
+	int err;
+	uint32_t bus_clk, apb_psc, tim_clk;
+
+	err = clock_control_get_rate(clk, (clock_control_subsys_t *)pclken,
+				     &bus_clk);
+	__ASSERT_NO_MSG(err == 0);
+
+#if defined(CONFIG_SOC_SERIES_STM32H7X)
+	if (pclken->bus == STM32_CLOCK_BUS_APB1) {
+		apb_psc = CONFIG_CLOCK_STM32_D2PPRE1;
+	} else {
+		apb_psc = CONFIG_CLOCK_STM32_D2PPRE2;
+	}
+
+	/*
+	 * Depending on pre-scaler selection (TIMPRE), timer clock frequency
+	 * is defined as follows:
+	 *
+	 * - TIMPRE=0: If the APB prescaler (PPRE1, PPRE2) is configured to a
+	 *   division factor of 1 then the timer clock equals to APB bus clock.
+	 *   Otherwise the timer clock is set to twice the frequency of APB bus
+	 *   clock.
+	 * - TIMPRE=1: If the APB prescaler (PPRE1, PPRE2) is configured to a
+	 *   division factor of 1, 2 or 4, then the timer clock equals to HCLK.
+	 *   Otherwise, the timer clock frequencies are set to four times to
+	 *   the frequency of the APB domain.
+	 */
+	if (LL_RCC_GetTIMPrescaler() == LL_RCC_TIM_PRESCALER_TWICE) {
+		if (apb_psc == 1U) {
+			tim_clk = bus_clk;
+		} else {
+			tim_clk = bus_clk * 2U;
+		}
+	} else {
+		if (apb_psc == 1U || apb_psc == 2U || apb_psc == 4U) {
+			tim_clk = SystemCoreClock;
+		} else {
+			tim_clk = bus_clk * 4U;
+		}
+	}
+#else
+	if (pclken->bus == STM32_CLOCK_BUS_APB1) {
+		apb_psc = CONFIG_CLOCK_STM32_APB1_PRESCALER;
+	}
+#if !defined(CONFIG_SOC_SERIES_STM32F0X) && !defined(CONFIG_SOC_SERIES_STM32G0X)
+	else {
+		apb_psc = CONFIG_CLOCK_STM32_APB2_PRESCALER;
+	}
+#endif
+
+	/*
+	 * If the APB prescaler equals 1, the timer clock frequencies
+	 * are set to the same frequency as that of the APB domain.
+	 * Otherwise, they are set to twice (×2) the frequency of the
+	 * APB domain.
+	 */
+	if (apb_psc == 1U) {
+		tim_clk = bus_clk;
+	} else {
+		tim_clk = bus_clk * 2U;
+	}
+#endif
+
+	return tim_clk;
+}


This function get_timer_clock() doesn't match all STM32 series cases.
For example some stm32f4 socs or stm32f7 socs have also x1 and x4 cases.
You can have a look at Arduino implementation which cover most cases:
https://github.com/stm32duino/Arduino_Core_STM32/blob/73f76d6028f7171f6273aa3a52cb0a4117f92ff8/cores/arduino/HardwareTimer.cpp#L1253

Thanks, I'll take a look. PWM driver also needs a fix then, as this code is the same one.

drivers/stepper/stepper_stm32.c

tbursztyka · 2020-07-14T05:38:39Z

drivers/stepper/Kconfig

+# Copyright (c) 2020 Teslabs Engineering S.L.
+# SPDX-License-Identifier: Apache-2.0
+
+menuconfig STEPPER


Could you add [EXPERIMENTAL] ?

It's a really good API addition, but once it will be in master (hopefully for 2.4), users might have some improvements, changes etc... Could be a feature for position reference (homing) or else.

Having it specifically set "experimental" will avoid to go through a tedious process of changing a stable API

good point, marked as experimental. thanks!

The API lifecycle doesn't require this, though perhaps it's something that could be added @carlescufi

APIs start out as experimental and don't become stable until certain conditions are met including being available for two development releases. A first step that seems to have been missed is adding this to the API project so it can be triaged and tracked. I've done that.

Indeed, I am following the legacy rule. Which imo, could be kept as well. It's more obvious to users that the API is not stable if it's marked experimental in Kconfig.

Yes, I will make a note to add this to the guide, thanks @pabigot.

Also to discuss this (Kconfig marking of EXPERIMENTAL) in API meeting. It looks like a lot of Zephyr subsystems are doing this for features within a subsystem, but possibly not for the entire API. (Except for a couple like LoRa and Modem which aren't even listed in the API overview.)

ABOSTM · 2020-07-15T12:28:24Z

drivers/stepper/stepper_stm32.c

+	 * psc = ((f_tim / speed) / (arr + 1)) - 1
+	 *     = ((f_tim / speed) / 65536) - 1
+	 *     = ((f_tim / speed) >> 16) - 1.


No need of -1:
Prescaler value (not the register) is equal to (f_tim/speed) +1.
+1 correspond to the fact prescaler cannot be null, it is greater or equal to 1.
Now if we talk about register PSC = Prescaler value - 1 // (register can be null)
= (f_tim/speed) +1 -1
= (f_tim/speed)
I also suggest to remove the 1st equation which is wrong: psc doesn't depends on 'arr' variable.
(eventually it depends on arr_max = 65535)

psc = ((f_tim / speed) / (arr + 1)) - 1

Suggested change

* psc = ((f_tim / speed) / (arr + 1)) - 1

* = ((f_tim / speed) / 65536) - 1

* = ((f_tim / speed) >> 16) - 1.

* psc = ((f_tim / speed) / 65536)

* = ((f_tim / speed) >> 16)

Note that these calculations compute the register value (hence the -1). I can subtract the value when calling the LL prescaler set function if calculations become more clear.

Yes I notice that. But As I said, prescaler value is ((f_tim/speed) +1), then psc (register) is psc= Prescaler_value - 1
so psc = (f_tim/speed) +1 -1 = f_tim/speed.
So I confirm -1 should be removed for the register.
Imagine (f_tim/speed) is smaller than 65536, then due to entire division round down psc = -1 this is not good for register value !!!

Note that this calculation is not performed if arr fits into the 16-bit register, so psc = -1 is not possible (see https://github.com/zephyrproject-rtos/zephyr/pull/26221/files/b5615ec8eff0d67ed296a42e2e319ad185dbe992#diff-0321881e45a0440f0ae270f34324a9c6R377). In the docs arr and psc refer to actual register values, maybe I should clarify that.

ABOSTM · 2020-07-15T12:40:30Z

drivers/stepper/stepper_stm32.c

+		psc = ((data->f_tim / speed) >> 16U) - 1U;
+		arr = UINT16_MAX;


See comment above about prescaler.
Also 'arr' , computation is not that simple:
You can take an example (data->f_tim / speed) = 200 000 = 0x 3 0D40
Prescaler value is 4 (psc register = 3)
and ARR is (200 000 /prescalerValue) - 1 = 49 000 = 0xC34F

Suggested change

psc = ((data->f_tim / speed) >> 16U) - 1U;

arr = UINT16_MAX;

psc = (data->f_tim / speed) >> 16U;

arr = ((data->f_tim / speed) / (psc + 1U)) - 1U ;

I understand this way: we first have f_tim / speed = K = (arr + 1) * (psc + 1). K is constant. Either arr or psc can be fixed. If I fix arr to its maximum value, 0xFFFF, then psc is determined by psc + 1 = K / (0xFFFF + 1) = K >> 16. The advantage I see on fixing arr to its maximum value is to always have the maximum PWM resolution (not critical here, though). Am I missing something?

Your proposal give better dutycycle resolution (due to lower prescaler) but at a cost of inaccuracy on period (potentially very important) .
Lets take the worst example with your method.
K = 0x1 FFFF
You fixed arr= 0xFFFF.
then psc = K / (0xFFFF +1) -1 = 0
But reverting the computation: with your method psc = 0 and arr = 0xFFFF
then (arr + 1) * (psc + 1) = 0x1000
Delta compare to expected K value DeltaK = 0x1 FFFF - 0x1000 = 0xFFFF
This is almost 50% error !!!
With your method, maximum inaccuracy on period is equal to 0xFFFF, because (0XFFFF+ 1) * (psc + 1) will take discrete values 0x10000; 0x20000; 0x30000 ... depending psc.
And there is still issue when K smaller than 0x10000 leading to psc =-1

With my method, I get psc =1 and arr = 0xFFFE
then (arr + 1) * (psc + 1) = 0xFFFF * 2 = 0x1FFFE
Error on period is 1 .
With my method, max inaccuracy on period is equal to prescaler (if prescaler value is N, meaning N tick of tim input, then I may miss a maximum of N tick.)

Thanks @ABOSTM for the details. In this case period accuracy is the most important factor, so I see the point. I will fix the calculations, thanks again!

include/dt-bindings/timers/stm32_timers_h7.h

ABOSTM · 2020-07-15T21:02:04Z

Edit: new proposed formula:

/* ITR selection may be composed of non adjacent bits :
 * TIM_SMCR_TS Bits 21, 20, 6, 5, 4 TS[4:0]: Trigger selection
 * (bit 20 and 21 are not available on all STM32 series)
 */
#define DT_STM32_TIMERS_ITR(n) ( ((n & 0x18U) << 20U) + (n & 0x7U) << 4U))

Timer internal trigger (ITR) settings have been added to DT bindings. Only H7 series for now. Signed-off-by: Gerard Marull-Paretas <gerard@teslabs.com>

A proposal to introduce a new API to control stepper motors. The API allows to control motor enablement, execute movements (sync and async) as well as control the speed. It has been designed to allow multiple stepper motors per controller. The proposed implementation is for STM32 microcontrollers (tested on H7 only, but can be easily adjusted for all families). It uses two timers in master/slave mode (one to generate the steps and the second to keep track of the steps). It has been tested using a DRV8825 driver (step and direction). Note that while driver supports more than a single motor, only one can be active at the same time. Signed-off-by: Gerard Marull-Paretas <gerard@teslabs.com>

ABOSTM · 2020-07-16T11:30:04Z

@ABOSTM can you check latest changes wrt trigger selection in DT?

Yes, LGTM for DT trigger selection.

pabigot

A couple changes along with comments/questions about the API.

pabigot · 2020-07-20T13:52:56Z

include/drivers/stepper.h

+/**
+ * @file
+ *
+ * @brief Public APIs for the stepper controller drivers.


This API needs to be added to the table in doc/reference/overview.rst as Experimental.`

pabigot · 2020-07-20T13:55:17Z

dts/bindings/stepper/st,stm32-stepperctl.yaml

+
+compatible: "st,stm32-stepperctl"
+
+include: base.yaml


How much of this could be part of a generic stepper control devicetree node, and how much is specific to the STM32 peripherals that support this function? I'm specifically concerned about microsteps and direction-gpios, as well as itr, all of which aren't described adequately for somebody not working on STM32 to provide an equivalent function.

I'm not keen on adding a new subsystem API for something that can't be used by multiple vendors.

pabigot · 2020-07-20T14:01:52Z

include/drivers/stepper.h

+
+static inline int z_impl_stepper_stop(struct device *dev, uint32_t channel)
+{
+	const struct stepper_driver_api *api =


Put all the declarations outside the #ifdef, and replace the body of this function with something like:

int ret = -ENOTSUP; #ifdef CONFIG_STEPPER_ASYNC const struct stepper_driver_api *api = (const struct stepper_driver_api *)dev->driver_api; if (api->stop) { ret = api->stop(dev, channel); } #endif return ret;

pabigot · 2020-07-20T14:03:21Z

include/drivers/stepper.h

+
+#ifdef CONFIG_STEPPER_ASYNC
+/**
+ * @brief Move stepper N steps (asynchronous API).


Since this API function is optional, we should also clarify whether drivers are required to implement it if it's enabled.

Part of the API discussion of this PR should include whether this should be optional, or required.

include/drivers/stepper.h

pabigot · 2020-07-20T14:13:06Z

drivers/stepper/stepper_context.h

+	int status;
+
+#ifdef CONFIG_STEPPER_ASYNC
+	struct k_poll_signal *signal;


Can we eliminate the bool and use a non-null signal as a sign that the call is asynchronous?

pabigot · 2020-07-20T14:14:53Z

drivers/stepper/stepper_context.h

+	k_sem_take(&ctx->lock, K_FOREVER);
+
+#ifdef CONFIG_STEPPER_ASYNC
+	ctx->asynchronous = asynchronous;


I guess this could be OK, but it'd be messy if asynchronous was true while signal == NULL.

pabigot · 2020-07-20T14:21:26Z

drivers/stepper/stepper_context.h

+extern "C" {
+#endif
+
+struct stepper_context {


This foo_context header replicates a pattern that we know is wrong. It supports asynchronous by adding a blocking async method, which is confusing. It also uses a semaphore for coordination while a mutex may be more appropriate (not withstanding #25678).

I don't know that there's an alternative right now, but it might be wise to not provide an async API until we have a newer interface that addresses these concerns.

pabigot · 2020-07-20T14:23:26Z

drivers/stepper/stepper_stm32.c

+	/** Pin. */
+	gpio_pin_t pin;
+	/** Flags. */
+	gpio_flags_t flags;


gpio_dt_flags_t. It's smaller and would pack with gpio_pin_t.

pabigot · 2020-07-20T14:24:20Z

drivers/stepper/stepper_stm32.c

+	return dev->config_info;
+}
+
+static inline int32_t get_motor_index(const struct stepper_stm32_config *config,


use ssize_t as the return type.

carlescufi · 2020-07-21T16:42:03Z

API meeting 21st July:

In general the API looks reasonable, with some caveats:

The "channel" parameter needs to be clarified and discussed. Does it make sense in the context of the tree hierarchy? Instead an option would be to have the actual motors as DT nodes and then those refer to the timers that they share. In that case we would have multiple device structures, one per motor. Right now the controller is the node with each motor being defined inside it.
Do we want to provide an API to control a single motor, or a generic one that can encapsulate stepper drivers that support multiple motors connected at a time?
set_enabled(): this holds the torque, as opposed to running free, when not moving.

galak · 2020-07-21T16:42:54Z

Add a link to stepper motor control chips:

https://www.trinamic.com/fileadmin/assets/Products/ICs_Documents/TMC5072_datasheet_Rev1.23.pdf
https://www.nxp.com/products/power-management/motor-and-solenoid-drivers/bldc-h-bridge-stepper:PWRHBRIDGESTEPMTR

github-actions · 2020-09-20T01:03:50Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

gmarull · 2020-11-06T23:29:26Z

Closing as I have no bandwidth to work on necessary changes. May re-open in the future if there is interest.

KenthJohan · 2021-01-15T20:32:17Z

Any news on this? I used Zephyr once to control TMC2130 using PWM for speed and GPIO for direction, but it also has I2C and so I ended up using Arduino to configure it with I2C because driver exists in Arduino environment. It would be nice to have driver support for these chips. Some configuration types might be unique to certain chips so a stepper driver with entity attribute value system to set arbitrary configurations could be useful. I think that drivers would spark interest for 3D printers to use Zephyr.

gmarull added the RFC Request For Comments: want input from the community label Jun 16, 2020

gmarull requested review from carlescufi, erwango, galak, MaureenHelm and nashif as code owners June 16, 2020 18:03

carlescufi requested review from dcpleung and pabigot June 16, 2020 18:04

zephyrbot added area: Devicetree area: API Changes to public APIs labels Jun 16, 2020

carlescufi requested review from mnkp and henrikbrixandersen June 16, 2020 18:13

dcpleung approved these changes Jun 17, 2020

View reviewed changes

github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR area: Device Tree area: Devicetree Binding PR modifies or adds a Device Tree binding and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jun 30, 2020

galak removed the area: Device Tree label Jul 1, 2020

github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jul 1, 2020

github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jul 2, 2020

ABOSTM requested changes Jul 3, 2020

View reviewed changes

github-actions bot added the platform: STM32 ST Micro STM32 label Jul 7, 2020

tbursztyka reviewed Jul 14, 2020

View reviewed changes

tbursztyka approved these changes Jul 14, 2020

View reviewed changes

gmarull removed the has-conflicts Issue/PR has conflicts with another issue/PR label Jul 14, 2020

ABOSTM requested changes Jul 15, 2020

View reviewed changes

gmarull added 2 commits July 16, 2020 11:47

dts: bindings: add stm32 h7 bindings for timers ITR

f16b347

Timer internal trigger (ITR) settings have been added to DT bindings. Only H7 series for now. Signed-off-by: Gerard Marull-Paretas <gerard@teslabs.com>

pabigot requested changes Jul 20, 2020

View reviewed changes

github-actions bot added the Stale label Sep 20, 2020

gmarull removed the Stale label Sep 23, 2020

gmarull closed this Nov 6, 2020

gmarull mentioned this pull request Feb 7, 2024

Feature/introduce stepper motor api utilizing a "motor" bus #68548

Closed


		compatible: "st,stm32-stepperctl"

		include: base.yaml

[RFC] drivers: stepper: initial support for stepper motors #26221

[RFC] drivers: stepper: initial support for stepper motors #26221

Conversation

gmarull commented Jun 16, 2020 • edited Loading

dcpleung commented Jun 16, 2020

gmarull commented Jun 16, 2020

dcpleung left a comment

Choose a reason for hiding this comment

ABOSTM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ABOSTM Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ABOSTM commented Jul 15, 2020

ABOSTM commented Jul 16, 2020

pabigot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlescufi commented Jul 21, 2020

galak commented Jul 21, 2020 • edited Loading

github-actions bot commented Sep 20, 2020

gmarull commented Nov 6, 2020

KenthJohan commented Jan 15, 2021

gmarull commented Jun 16, 2020 •

edited

Loading

ABOSTM Jul 15, 2020 •

edited

Loading

galak commented Jul 21, 2020 •

edited

Loading