-
Notifications
You must be signed in to change notification settings - Fork 7.8k
drivers: stepper: introduce rudimentary trapezoidal ramp control #88564
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
drivers: stepper: introduce rudimentary trapezoidal ramp control #88564
Conversation
3eb28c1
to
d4a1f59
Compare
d4a1f59
to
161ed77
Compare
/** Ramp acceleration in micro-steps per second squared */ | ||
uint32_t acceleration; | ||
/** Ramp maximum velocity in micro-steps per second */ | ||
uint32_t max_velocity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is max_velocity
really needed? next step interval depends only on current interval and acceleration/deceleration values.
the actual limit will be given by the requested stepper interval
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to depict a trapezoidal ramp. I am also using max_velocity to check for valid_timing in move_by and move_to. But i'll check this again for sure if it is actually needed :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my experience, the speed set via set_microstep_interval
is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay we can use that function, but we definitely need to have a better name for it though. set_microstep_interval atleast imo sounds like this interval will take place with immediate effect. Probably something like set_target_step_interval
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the current micro-step interval implementation also requires changes to pre-decel-steps
, as it is dependent on the speed. My recommendation would be to use the current micro-step interval in combination with the deceleration value for this
03d7e6a
to
002919a
Compare
/** Ramp acceleration in micro-steps per second squared */ | ||
uint32_t acceleration; | ||
/** Ramp maximum velocity in micro-steps per second */ | ||
uint32_t max_velocity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my experience, the speed set via set_microstep_interval
is sufficient.
|
||
K_SPINLOCK(&data->lock) { | ||
data->ramp_common.ramp_profile.acceleration = ramp_profile->acceleration; | ||
data->ramp_common.ramp_profile.max_velocity = ramp_profile->max_velocity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need max_velocity
, you already have set_microstep_interval
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to find a consensus on how to rename that function, set_microstep_interval imo signifies more like a tick time, could be compared with for instance , can_set_bitrate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say, once this PR is merged, the CONFIG_STEPPER_RAMP can be deleted and all motors should support ramping by default.
|
||
if STEPPER_RAMP | ||
|
||
config STEPPER_RAMP_TRAPEZOIDAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer a dt-binding instead to allow for a per stepper-controller decision on using ramps or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. However, you would need one such variable for the source to be included, or is there a better way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a Kconfig extension for checking if a property is present on a compatible:
drivers/dma/Kconfig.mcux_edma:12: depends on $(dt_compat_any_has_prop,$(EDMA_COMPAT),$(REV_PROP),2)
but nothing like dt_any_of_driver_class_has_prop, which would be neat.
switch (ramp_data->current_ramp_state) { | ||
case STEPPER_RAMP_STATE_ACCELERATION: | ||
ramp_data->ramp_actual_position++; | ||
new_step_interval_in_ns = current_step_interval_in_ns - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cant always use the approximation. You will need to use the accurate algorithm using the sqrt for a number of steps after starting accelerating and a couple of steps before stopping when decelerating. This is because of the approximation error.
My PR 88342 already uses the same algorithm (through I forgot to mention the algorithm admittedly) for a step-dir implementation. Several decisions made during development for that are relevant for this PR as well, so it should be included in the discussion. |
002919a
to
2fb84c6
Compare
This is not related to the changes made in this PR, but could the functions |
f0f04fe
to
e5ab023
Compare
ca14aeb
to
bdb0a32
Compare
/** | ||
* @brief Stepper ramp profile | ||
*/ | ||
struct stepper_ramp_profile { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not have this in stepper.h
directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coz then ramp.h would need to include stepper.h and i wanted to keep the ramp.h as decoupled from the stepper.h as possible.
Need some feedback on this actually, how about instead of stepper_set_ramp_profile(ramp_profile)
, we do something like stepper_set_timing_data(accel, max_velocity, decel)
and then we put ramp_profile
in ramp.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done moved to stepper.h
|
||
if STEPPER_RAMP | ||
|
||
config STEPPER_RAMP_TRAPEZOIDAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a Kconfig extension for checking if a property is present on a compatible:
drivers/dma/Kconfig.mcux_edma:12: depends on $(dt_compat_any_has_prop,$(EDMA_COMPAT),$(REV_PROP),2)
but nothing like dt_any_of_driver_class_has_prop, which would be neat.
drivers/stepper/ramp/ramp.h
Outdated
*/ | ||
typedef void (*stepper_ramp_reset_ramp_runtime_data_t)(const struct stepper_ramp_config *config, | ||
struct stepper_ramp_common *ramp_common, | ||
const bool is_stepper_dir_changed, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it maybe make the API more stable when you move is_stepper_dir_changed
etc. into the runtime data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep that's correct :), Will do it in the next push.
bdb0a32
to
1014b4c
Compare
12d8d44
to
a4be436
Compare
d263d8e
to
314c9e9
Compare
@@ -129,6 +172,10 @@ static void update_direction_from_step_count(const struct device *dev) | |||
} else { | |||
LOG_ERR("Step count is zero"); | |||
} | |||
if (data->direction != direction) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Directly return this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
return false; | ||
#else | ||
if (data->delay_in_ns > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return this directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
314c9e9
to
0be7169
Compare
0be7169
to
402bc78
Compare
#ifdef CONFIG_STEPPER_RAMP | ||
if (data->ramp_common.ramp_runtime_data.current_ramp_state == | ||
STEPPER_RAMP_STATE_PRE_DECELERATION) { | ||
if (data->direction == STEPPER_DIRECTION_NEGATIVE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The non ramp part uses a if DIRECTION_POSITIVE else if DIRECTION_NEGATIVE
ordering and I would keep it consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks. done
switch (state) { | ||
case STEPPER_RAMP_STATE_ACCELERATION: | ||
new_step_interval = current_step_interval - | ||
(2 * current_step_interval) / (4 * ramp_position + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(2 * current_step_interval) / (4 * ramp_position + 1); | |
(2 * current_step_interval) / (4 * ramp_position + 3); |
Using this, you effectively add 0.5 to ramp position, calculating the new interval based on the middle of the ramp segment and not the start, improving acceleration behavior. Note that your deceleration calculation already does this.
case STEPPER_RAMP_STATE_PRE_DECELERATION: | ||
if ((current_step_interval_in_ns > | ||
ramp_data->ramp_stop_step_interval_threshold_in_ns)) { | ||
uint64_t start_velocity = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interval and not a velocity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
/** Ramp acceleration in micro-steps per second squared */ | ||
uint32_t acceleration; | ||
/** Ramp maximum velocity in micro-steps per second */ | ||
uint32_t max_velocity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the current micro-step interval implementation also requires changes to pre-decel-steps
, as it is dependent on the speed. My recommendation would be to use the current micro-step interval in combination with the deceleration value for this
402bc78
to
8d25028
Compare
Introduce rudimentary trapezoidal ramp control using following paper: https://www.boost.org/doc/libs/1_81_0/libs/safe_numerics/example/stepper-motor.pdf Signed-off-by: Jilay Pandya <[email protected]>
testing following criteria: - distance profile - first acceleration interval - next step intervals Signed-off-by: Andre Stefanov <[email protected]>
As a general thing, in my experience, you need picosecond resolution for the ramp calculations, which are then converted to nanoseconds for the stepper controllers (but saving the picosecond values for the next step). Not sure if your 0.676 to counteract approximation inaccuracies reduces accuracy so much that this no longer matters. Could could you maybe test if in an acceleration towards 50 or 200 steps/s in 1 sec actually takes 1 second? I remember during my development using this algorithm that I often got inaccuracies in the range of 20%, something that is not noticeable when using only the console without printing the times. |
8d25028
to
7723317
Compare
Trapezoidal Ramp Control for Stepper Motors:
The idea is to create a ramp lib that can be integrated in any driver, the step counting for the driver itself stays unaffected by integration a ramp lib
I have been using Shell to see how the ramping looks like.
Starting This PR to get some early feedback.
Integration in driver
References:
https://www.boost.org/doc/libs/1_81_0/libs/safe_numerics/example/stepper-motor.pdf
Equation 13 in the above mentioned paper can also be seen in this app note.
https://ww1.microchip.com/downloads/en/Appnotes/doc8017.pdf (Thanks @andre-stefanov :) )
Known open points:
stepper_run
as wellcopied isqrt
Update: 23.04.2025
Thanks @andre-stefanov for contributing unit-tests :)