drivers: stepper: step_dir: Adjust toggeling speed of step pin #88282
Conversation
github-actions
bot
requested review from
bjarki-andreasen,
dipakgmx,
fabiobaltieri,
jbehrensnx and
jilaypandya
| @@ -74,6 +74,7 @@ struct step_dir_stepper_common_data { | |||
| int32_t actual_position; | |||
| uint64_t microstep_interval_ns; | |||
| int32_t step_count; | |||
| uint8_t toggle_count; | |||
There was a problem hiding this comment.
Use a bool that is flipped instead, same memory and you don't rely on integer overflow to work as you expect.
There was a problem hiding this comment.
Yeah had that originally, but I thought the naming of a uint8_t toggle_count is more readable then somthing like step_performed. Changed it. If you have a better name suggestion feel free :^)
There was a problem hiding this comment.
For the acceleration step-dir i am currently using step_pin_low.
Changes the timing source to perform the stepping in half of the given frequency to space out the toggeling of the step pin more. This fixes misstepping for configurations where the speed of successive toggeling exceeds what the stepper driver can handle. Fixes zephyrproject-rtos#87698. Signed-off-by: Fabian Blatz <[email protected]>
faxe1008
force-pushed
the
fix_step_dir_toggle_speed
branch
from
9eda3d9 to
443fa53
Compare
| if (config->dual_edge) { | ||
| return K_NSEC(data->microstep_interval_ns); | ||
| } else { | ||
| return K_NSEC(data->microstep_interval_ns / 2); |
There was a problem hiding this comment.
| return K_NSEC(data->microstep_interval_ns / 2); | |
| return K_NSEC(data->microstep_interval_ns >> 1); |
| if (config->dual_edge) { | ||
| data->counter_top_cfg.ticks = DIV_ROUND_UP(toggle_freq, NSEC_PER_SEC); | ||
| } else { | ||
| data->counter_top_cfg.ticks = DIV_ROUND_UP(toggle_freq / 2, NSEC_PER_SEC); |
There was a problem hiding this comment.
| data->counter_top_cfg.ticks = DIV_ROUND_UP(toggle_freq / 2, NSEC_PER_SEC); | |
| data->counter_top_cfg.ticks = DIV_ROUND_UP(toggle_freq >> 1, NSEC_PER_SEC); |
There was a problem hiding this comment.
I would go with NSEC_PER_SEC * 2 instead, as it makes easier to understand what is happening.
|
Hmm fails, the step count is only half of what it is supposed to be. |
This issue occurs with all step-dir drivers (look at the stepper_api test suite). Can you check if it works on real hardware? There are some strict limitations on the native_sim counter that could possibly be the source of the issue. |
|
@faxe1008, Do you think we should introduce something like |
|
@jilaypandya have not gotten around working on this PR yet. Regarding your question: it does not hurt but I think since there is no convenient way for us to default the value to the one specified in a drivers datasheet this means the user has to set it. If speed really is a concern the user probably calculates the step interval from the speed anyways and he should know best what value to pick. On the other hand this would make hardware-depedant limits move to devicetree, in case the user choses to use a new stepper IC. Hmm I think it makes sense to have this :^) |
Changes the timing source to perform the stepping in half of the given frequency to space out the toggeling of the step pin more. This fixes misstepping for configurations where the speed of successive toggeling exceeds what the stepper driver can handle.
Fixes #87698.