RFC: Change Stepper API to use step interval #83726

faxe1008 · 2025-01-09T08:06:48Z

As discussed in the original RFC issue: Change the stepper API to accept the interval to wait at a minimum in between executing consequtive steps. This allows for a more granular control then the previous interface.

Resolves #83591

faxe1008 · 2025-01-09T08:09:46Z

Did not include it yet, but the stepper.h could be a macro STEP_INTERVAL_US_FROM_VELOCITY to ease the transition for people already using the old API.

jilaypandya · 2025-01-09T08:19:58Z

The API Change and then relevant changes in shell, drivers, tests should be squashed in one commit.

include/zephyr/drivers/stepper.h

faxe1008 · 2025-01-09T15:15:08Z

About the squashing - don't know whats the proper ettiquette for it. Did not do that many treewide changes as of now.

jilaypandya · 2025-01-09T15:29:22Z

About the squashing - don't know whats the proper ettiquette for it. Did not do that many treewide changes as of now.

when we recently shifted to DEVICE_API, we introduced that change in all the drivers in a single commit, I believe its easier to track. The way it's done right now is like this that we introduce a change and then do the correction in drivers, tests, shell successively in respective commits.

I would have had one commit for stepper_set_step_interval, one commit for stepper_runand then one migration-guide commit.

fabiobaltieri · 2025-01-09T15:34:09Z

About the squashing - don't know whats the proper ettiquette for it. Did not do that many treewide changes as of now.

You should really squash the API change with the code update to use the new API, basically no commit should break the build. Doc update can be on its own. In this case it does make it harder to review but that's how it should be done.

drivers/stepper/adi_tmc/adi_tmc5xxx_common.h

drivers/stepper/step_dir/step_dir_stepper_timing_source.h

include/zephyr/drivers/stepper.h

drivers/stepper/adi_tmc/adi_tmc5041_stepper_controller.c

jilaypandya · 2025-01-10T09:48:05Z

drivers/stepper/adi_tmc/adi_tmc5xxx_common.h

 {
 	__ASSERT_NO_MSG(clock_frequency);
+	__ASSERT_NO_MSG(step_interval_us);
+
+	uint32_t velocity_hz = 1e6 / step_interval_us;


Max possible speed here is then limited 1MHz, which is not much since 256 usteps * 200 steps/rev = 51200 usteps/rev, so over here we will be limiting the speed to somewhere around 2rps. At this point i can only see that we need to allow both the interfaces.

Maybe use nanoseconds instead of microseconds?

Also 1e6/51200 is about 20rps, or am I missing something?

No that's correct, although 20 rps is still i would say on the slow speed spectrum.

Let's do it this way since step_interval serves majority of the drivers, let's keep it, i think microseconds timebase is good enough.

I think going in nanoseconds timebase would just be to accomodate drivers like tmc5041, or?

For drivers like tmc5041, this function shall return -ENOTSUP, as there is also a way to set speed via ramp as can be seen over here

zephyr/include/zephyr/drivers/stepper/stepper_trinamic.h

Line 162 in 5faf471

int tmc5041_stepper_set_ramp(const struct device *dev,

.

There are also 0.9° steppers which are also very common especially for projects requiring precise positions. This would half the RPM additionally. But on the other hand, the use case where you step at 1/256 uSteps and want to achieve very fast speeds at the same time are very rare. In this case dynamic microstepping can be used, where you change the uStep resolution for slow and fast velocities can be used (this would be responsibility of the application code, since it would introduce to much complexity into the stepper driver in zephyr). This is what we are doing in OpenAstroTech. We switch to something like 1/32 for fast slew movements and then back to e.g. 1/128 for slow and precise tracking rotations.

In general tmc2209 needs a STEP pin input timing of at least 100ns. Disregarding other factors this would result in maximal frequency of 10MHz.

so i suppose we need to go to nanoseconds timebase.

For controllers like tmc5041 with internal ramp control we cannot ascertain step_interval due to acceleration/deceleration, we can more or less only set_max_velocity, also the reason why this API was designed like this.

stepper_set_step_interval(const struct device*, uint64_t step_interval_nsec) it is :-)

For drivers like tmc5041, this function shall return -ENOTSUP, as there is also a way to set speed via ramp as can be seen over here

How would that condition look like? The issue is if you want to be faster then 1 step_interval_us but since its a uint64_t you can not attempt to go faster, right?

Sorry it is -ENOSYS, i mean that the tmc5041 driver shall not implement stepper_set_step_interval, ofcourse with uint64_t tmc5041 could go faster, what i was trying to say that when i set a step interval, i expect that every step shall be exactly that interval apart, which cannot be ascertained with drivers like tmc5041 which have internal ramp motion controller which is reponsible for stepping intervals.

Hence the stepper_set_step_interval shall not be supported by the tmc5041 driver.

Hmm test does fail now because the counter_us_to_ticks returns a uint32_t which is not enough to hold 50 microsteps per second in microsecond interval (stepper_set_step_interval(dev, 20000000) which will call counter_us_to_ticks with 20000000000).

@jilaypandya @andre-stefanov I fixed this inside the counter timing source calculation, can you also verify that this is correct?

include/zephyr/drivers/stepper.h

doc/hardware/peripherals/stepper.rst

jilaypandya · 2025-01-12T18:47:31Z

drivers/stepper/adi_tmc/adi_tmc5041_stepper_controller.c

@@ -348,15 +348,21 @@ static int tmc5041_stepper_move_by(const struct device *dev, const int32_t micro
 	return 0;
 }

-static int tmc5041_stepper_set_max_velocity(const struct device *dev, uint32_t velocity)
+static int tmc5041_stepper_set_step_interval(const struct device *dev, uint64_t step_interval_ns)


tmc5041 can contextually only set velocities and because of its internal ramp controller, the step intervals vary depending on which part of ramp you are on.

The term max_velocity kind of also pointed towards that the api was not really ensuring that the stepper would move with that velocity, was more like a best effort scenario.

I like the step_interval approach since it allows more granular control, however, does not really apply to these kind of drivers with internal ramp control. Hence this function cannot be supported by the tmc5041 driver.

Would like to hear from @dipakgmx about this as well.

jilaypandya · 2025-01-12T19:09:53Z

doc/releases/migration-guide-4.1.rst

  * Renamed the ``stepper_move`` function to :c:func:`stepper_move_by`.
  * Renamed the ``stepper_set_target_position`` function to :c:func:`stepper_move_to`.
+  * Renamed the ``stepper_set_max_velocity`` function to :c:func:`stepper_set_step_interval`.
+    The function now takes the step interval in nanoseconds. This allows for a more precise control.
+  * Changed the :c:func:`stepper_run` to take the step interval in microseconds instead of velocity.


Suggested change

* Changed the :c:func:`stepper_run` to take the step interval in microseconds instead of velocity.

* Deprecating setting max velocity via :c:func:`stepper_run` .

jilaypandya · 2025-01-13T17:54:15Z

drivers/stepper/step_dir/step_dir_stepper_counter_timing.c

-	data->counter_top_cfg.ticks =
-		DIV_ROUND_UP(counter_us_to_ticks(config->counter, USEC_PER_SEC), velocity);
+	data->counter_top_cfg.ticks = DIV_ROUND_UP(
+		counter_get_frequency(config->counter) * step_interval_ns, NSEC_PER_SEC);


This conversion looks good to me. @Irockasingranite @jbehrensnx can you give this a look as well.

Looks fine to me, its essentially what counter_us_to_ticks actually does behind the scenes, just with ns instead of us

jilaypandya · 2025-01-13T17:55:10Z

drivers/stepper/step_dir/step_dir_stepper_counter_timing.c

@@ -17,18 +17,18 @@ static void step_counter_top_interrupt(const struct device *dev, void *user_data
 	stepper_handle_timing_signal(data->dev);
 }

-int step_counter_timing_source_update(const struct device *dev, const uint32_t velocity)
+int step_counter_timing_source_update(const struct device *dev, const uint64_t step_interval_ns)


let's use microstep in order to avoid ambiguity.

Suggested change

int step_counter_timing_source_update(const struct device *dev, const uint64_t step_interval_ns)

int step_counter_timing_source_update(const struct device *dev, const uint64_t microstep_interval_ns)

jilaypandya · 2025-01-13T17:55:30Z

drivers/stepper/step_dir/step_dir_stepper_common.h

@@ -70,7 +70,7 @@ struct step_dir_stepper_common_data {
 	enum stepper_direction direction;
 	enum stepper_run_mode run_mode;
 	int32_t actual_position;
-	uint32_t max_velocity;
+	uint64_t step_interval_ns;


Suggested change

uint64_t step_interval_ns;

uint64_t microstep_interval_ns;

jilaypandya · 2025-01-13T17:56:58Z

include/zephyr/drivers/stepper.h

 */
-typedef int (*stepper_set_max_velocity_t)(const struct device *dev,
-					  const uint32_t micro_steps_per_second);
+typedef int (*stepper_set_step_interval_t)(const struct device *dev,


sorry that this was not discovered in one of the earlier reviews.

Suggested change

typedef int (*stepper_set_step_interval_t)(const struct device *dev,

typedef int (*stepper_set_microstep_interval_t)(const struct device *dev,

jilaypandya

Just this one open point about tmc5041 :)

jilaypandya · 2025-01-14T17:06:43Z

drivers/stepper/adi_tmc/adi_tmc5041_stepper_controller.c

+	}
+
+	velocity_fclk = tmc5xxx_calculate_velocity_fclk_from_step_interval(microstep_interval_ns,
+									   clock_frequency);

 	err = tmc5041_write(config->controller, TMC5041_VMAX(config->index), velocity_fclk);


Can we undo the changes to the tmc5041 driver for now. It has an internal ramp controller which takes care of setting step intervals. As can be seen over here converting a step_interval to vmax is kinda not really correct. VMAX is a ramp parameter.

Undid the changes - the header common method is now unused, but I did not remove it just yet.

jilaypandya · 2025-01-15T07:38:44Z

drivers/stepper/adi_tmc/adi_tmc5041_stepper_controller.c

@@ -348,24 +348,6 @@ static int tmc5041_stepper_move_by(const struct device *dev, const int32_t micro
 	return 0;
 }

-static int tmc5041_stepper_set_max_velocity(const struct device *dev, uint32_t velocity)


Let's not remove this function for now, I'll create a PR right after this PR is merged exposing this function from the trinamic api. This function is quite relevant for the use-cases where you do not want to set the entire ramp and just want to cut the speed by a certain fraction.

Change the stepper API to instead of changing the stepper speed based on the velocity in microsteps per second to use the delay in usec between successive steps. Also remove the velocity from the `stepper_run` function as typical API usage is enable -> set step interval -> run. Signed-off-by: Fabian Blatz <fabianblatz@gmail.com>

Migrates the peripherals documentation to the new step interval based APi instead of the velocity one. Signed-off-by: Fabian Blatz <fabianblatz@gmail.com>

Adds a two bullet points explaining the change of the velocity based API towards an step interval one. Signed-off-by: Fabian Blatz <fabianblatz@gmail.com>

bjarki-andreasen

Looks good :)

zephyrbot added Release Notes To be mentioned in the release notes area: Stepper labels Jan 9, 2025

zephyrbot requested review from bjarki-andreasen, dipakgmx, fabiobaltieri, jilaypandya and kartben January 9, 2025 08:07

zephyrbot assigned jilaypandya Jan 9, 2025

faxe1008 force-pushed the stepper_velocity branch 7 times, most recently from 5090c7c to d09f9cb Compare January 9, 2025 08:19

jilaypandya reviewed Jan 9, 2025

View reviewed changes

include/zephyr/drivers/stepper.h Outdated Show resolved Hide resolved

faxe1008 force-pushed the stepper_velocity branch from d09f9cb to 38f96d2 Compare January 9, 2025 13:36

fabiobaltieri previously approved these changes Jan 9, 2025

View reviewed changes

faxe1008 dismissed fabiobaltieri’s stale review via 7b61f11 January 9, 2025 15:43

faxe1008 force-pushed the stepper_velocity branch from 38f96d2 to 7b61f11 Compare January 9, 2025 15:43

fabiobaltieri previously approved these changes Jan 9, 2025

View reviewed changes

jilaypandya reviewed Jan 9, 2025

View reviewed changes

drivers/stepper/adi_tmc/adi_tmc5xxx_common.h Outdated Show resolved Hide resolved

drivers/stepper/step_dir/step_dir_stepper_timing_source.h Outdated Show resolved Hide resolved

include/zephyr/drivers/stepper.h Outdated Show resolved Hide resolved

faxe1008 dismissed fabiobaltieri’s stale review via 8cadbdf January 9, 2025 16:03

faxe1008 force-pushed the stepper_velocity branch 2 times, most recently from 8cadbdf to facdd26 Compare January 10, 2025 08:35

jilaypandya reviewed Jan 10, 2025

View reviewed changes

faxe1008 force-pushed the stepper_velocity branch from facdd26 to f194f0b Compare January 10, 2025 12:38

jilaypandya reviewed Jan 10, 2025

View reviewed changes

include/zephyr/drivers/stepper.h Show resolved Hide resolved

faxe1008 force-pushed the stepper_velocity branch from f194f0b to d5c2833 Compare January 12, 2025 18:36

jilaypandya reviewed Jan 12, 2025

View reviewed changes

faxe1008 force-pushed the stepper_velocity branch from d5c2833 to 5a9daa2 Compare January 12, 2025 19:07

jilaypandya reviewed Jan 12, 2025

View reviewed changes

faxe1008 force-pushed the stepper_velocity branch 2 times, most recently from 5e9305c to 7400931 Compare January 13, 2025 15:11

jilaypandya reviewed Jan 13, 2025

View reviewed changes

faxe1008 force-pushed the stepper_velocity branch 4 times, most recently from e3bba2b to 8b413bf Compare January 14, 2025 09:40

jilaypandya reviewed Jan 14, 2025

View reviewed changes

faxe1008 force-pushed the stepper_velocity branch 2 times, most recently from c79b6e3 to f6c2c94 Compare January 15, 2025 07:27

jilaypandya reviewed Jan 15, 2025

View reviewed changes

faxe1008 added 2 commits January 15, 2025 08:46

doc: hardware: peripherals: stepper: Migrate to step interval API

bb8b89a

Migrates the peripherals documentation to the new step interval based APi instead of the velocity one. Signed-off-by: Fabian Blatz <fabianblatz@gmail.com>

faxe1008 force-pushed the stepper_velocity branch from f6c2c94 to 408ccd2 Compare January 15, 2025 07:46

doc: migration-guide-4.1: Add section about step interval based API

c42e944

Adds a two bullet points explaining the change of the velocity based API towards an step interval one. Signed-off-by: Fabian Blatz <fabianblatz@gmail.com>

faxe1008 force-pushed the stepper_velocity branch from 408ccd2 to c42e944 Compare January 15, 2025 07:48

jilaypandya approved these changes Jan 15, 2025

View reviewed changes

bjarki-andreasen approved these changes Jan 15, 2025

View reviewed changes

kartben merged commit 7537a14 into zephyrproject-rtos:main Jan 15, 2025
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Change Stepper API to use step interval #83726

RFC: Change Stepper API to use step interval #83726

faxe1008 commented Jan 9, 2025

faxe1008 commented Jan 9, 2025

jilaypandya commented Jan 9, 2025

faxe1008 commented Jan 9, 2025

jilaypandya commented Jan 9, 2025

fabiobaltieri commented Jan 9, 2025

jilaypandya Jan 10, 2025 •

edited

Loading

faxe1008 Jan 10, 2025

faxe1008 Jan 10, 2025

jilaypandya Jan 10, 2025 •

edited

Loading

andre-stefanov Jan 10, 2025

jilaypandya Jan 10, 2025 •

edited

Loading

faxe1008 Jan 10, 2025

jilaypandya Jan 10, 2025

faxe1008 Jan 13, 2025

faxe1008 Jan 13, 2025

jilaypandya Jan 12, 2025

jilaypandya Jan 12, 2025

faxe1008 Jan 12, 2025

jilaypandya Jan 13, 2025

jbehrensnx Jan 14, 2025

jilaypandya Jan 13, 2025

faxe1008 Jan 14, 2025

jilaypandya Jan 13, 2025

faxe1008 Jan 14, 2025

jilaypandya Jan 13, 2025

faxe1008 Jan 14, 2025

jilaypandya left a comment

jilaypandya Jan 14, 2025

faxe1008 Jan 15, 2025

jilaypandya Jan 15, 2025

bjarki-andreasen left a comment

	* Changed the :c:func:`stepper_run` to take the step interval in microseconds instead of velocity.
	* Deprecating setting max velocity via :c:func:`stepper_run` .

	int step_counter_timing_source_update(const struct device *dev, const uint64_t step_interval_ns)
	int step_counter_timing_source_update(const struct device *dev, const uint64_t microstep_interval_ns)

	typedef int (stepper_set_step_interval_t)(const struct device dev,
	typedef int (stepper_set_microstep_interval_t)(const struct device dev,

RFC: Change Stepper API to use step interval #83726

RFC: Change Stepper API to use step interval #83726

Conversation

faxe1008 commented Jan 9, 2025

faxe1008 commented Jan 9, 2025

jilaypandya commented Jan 9, 2025

faxe1008 commented Jan 9, 2025

jilaypandya commented Jan 9, 2025

fabiobaltieri commented Jan 9, 2025

jilaypandya Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jilaypandya Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jilaypandya Jan 10, 2025 • edited Loading

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

jilaypandya 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

bjarki-andreasen left a comment

Choose a reason for hiding this comment

jilaypandya Jan 10, 2025 •

edited

Loading

jilaypandya Jan 10, 2025 •

edited

Loading

jilaypandya Jan 10, 2025 •

edited

Loading