RFC: Change Stepper API to use step interval #83726
Conversation
zephyrbot
requested review from
bjarki-andreasen,
dipakgmx,
fabiobaltieri,
jilaypandya and
kartben
|
Did not include it yet, but the |
faxe1008
force-pushed
the
stepper_velocity
branch
7 times, most recently
from
5090c7c to
d09f9cb
Compare
|
The API Change and then relevant changes in shell, drivers, tests should be squashed in one commit. |
include/zephyr/drivers/stepper.h
Outdated
|
|
||
| static inline int z_impl_stepper_run(const struct device *dev, | ||
| const enum stepper_direction direction, | ||
| const uint32_t velocity) | ||
| const uint64_t step_interval_us) |
There was a problem hiding this comment.
I see now that passing step interval again over here is just redundant.
User would set_interval->enable->run
| const uint64_t step_interval_us) | |
| static inline int z_impl_stepper_run(const struct device *dev, | |
| const enum stepper_direction direction) |
There was a problem hiding this comment.
Yeah thats also true.
There was a problem hiding this comment.
But with this you can stop the motor - maybe thats worth keeping?
There was a problem hiding this comment.
moved the stopping of the motor to the set_step_interval function.
There was a problem hiding this comment.
i think we need to handle this in a different manner. Setting step_interval to zero in theory means that the motor spins at inf speed, hence setting step_interval to zero should not in theory stop the motor. If anything 0 is -EINVAL.
setting velocity to zero in drivers as of now does not power down the coils and is in hence some sort of hold mode. This could be done using stepper_hold(device, HOLD_MODE) or stepper_stop(device) where HOLD_MODE could even include stuff like passive_braking when it is supported by the driver.
faxe1008
force-pushed
the
stepper_velocity
branch
from
d09f9cb to
38f96d2
Compare
|
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 I would have had one commit for |
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. |
faxe1008
force-pushed
the
stepper_velocity
branch
from
38f96d2 to
7b61f11
Compare
| static inline uint32_t tmc5xxx_calculate_velocity_from_hz_to_fclk(uint64_t velocity_hz, | ||
| uint32_t clock_frequency) | ||
| static inline uint32_t | ||
| tmc5xxx_calculate_velocity_from_step_interval_to_fclk(uint64_t step_interval_us, |
There was a problem hiding this comment.
Nit: can the argument name be shortened in order to atleast have the function name and return type in a single line
There was a problem hiding this comment.
Needed to change the function name for that to work, done.
| @@ -20,10 +20,10 @@ typedef int (*stepper_timing_source_init)(const struct device *dev); | |||
| * @brief Update the stepper timing source. | |||
| * | |||
| * @param dev Pointer to the device structure. | |||
| * @param velocity Velocity in microsteps per second. | |||
| * @param step_interval_us The new step interval in microseconds. | |||
There was a problem hiding this comment.
Step interval in microseconds
include/zephyr/drivers/stepper.h
Outdated
| * @details If velocity > 0, motor shall be set into motion and run incessantly until and unless | ||
| * stalled or stopped using some other command, for instance, motor_enable(false). | ||
| * This function is non-blocking. | ||
| * @details The motor shall be set into motion and run incessantly until and |
There was a problem hiding this comment.
continuously instead of incessantly
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>
faxe1008
force-pushed
the
stepper_velocity
branch
from
7b61f11 to
8cadbdf
Compare
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