Skip to content
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

RFC: drivers: stepper: API Specification Improvements #86052

Open
jbehrensnx opened this issue Feb 20, 2025 · 5 comments
Open

RFC: drivers: stepper: API Specification Improvements #86052

jbehrensnx opened this issue Feb 20, 2025 · 5 comments
Assignees
Labels
area: Stepper Enhancement Changes/Updates/Additions to existing features RFC Request For Comments: want input from the community

Comments

@jbehrensnx
Copy link
Contributor

Introduction

As discussed in PR 85658, here is the RFC about ambiguities concerning the expected behavior in the stepper api.

Problem description

While the the stepper api has made significant improvements concerning its specification, some questions remain. The purpose of this RFC is to discuss these and find answers. This will then be used to improve the documentation, and refine the tests from PR 85658, hopefully ensuring consistent behavior from all drivers that use the api.

Proposed changes

Things that I myself have noticed:

  • set_microstep_interval will cause some drivers to immediately change their their speed if they are already moving, while the current documentation implies that the new speed only takes effect once a movement command (move_to/move_by/run) has been called. I prefer this second interpretation and would like the documentation to be clarified in this regard.
  • set_microstep_interval 0 will fail with some drivers, which is in contrast to the old set_speed 0 which caused the motor to stop. As a result, currently the only way to stop a moving motor is to call disable followed by enable, which is not great. As a result, I would propose to either return set_microstep_interval 0 to its status as a stop command, or, preferably, to define a new dedicated stop command that stops the motor and keeps the coils energized.
  • regardless of the results of the previous issue, I would propose an emergency_stop command that immediately stops the motor and keeps the coils energized. The difference to the previous entry is mainly with acceleration based drivers, such as tmc 50XX and the step-dir acceleration driver I am currently developing. The regular stop command would result in them gradually decelerating, but for safety reasons it might be necessary to halt them immediately.
  • the enable/disable command is completely unspecified in its behavior. My recommended behavior would be (based on the drv8424 driver):
    • disable (enable off) causes movement to stop (including is_moving and changing position value), the motor coils to be de-energized and movement commands (move_to/move_by/run) to fail with an error code (drv8424 currently uses -ECANCELED). Setting microstep interval and resolution still works however.
    • enable causes the motor coils to be energized, but no movement occurs (even if the motor was moving before the disable command) until a movement command is called.

Dependencies

Changes to the api will require modification of the existing drivers. Their limited number means that this will be manageable.

Alternatives

Not improving the specification will lead to (and to a degree already has) divergent behavior of stepper api drivers, which is a significant problem.

@jbehrensnx jbehrensnx added the RFC Request For Comments: want input from the community label Feb 20, 2025
Copy link

Hi @jbehrensnx! We appreciate you submitting your first issue for our open-source project. 🌟

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. 🤖💙

@jbehrensnx
Copy link
Contributor Author

@jilaypandya
Copy link
Contributor

jilaypandya commented Feb 20, 2025

@jbehrensnx Thanks for creating this RFC.

I'll try to address the suggested changes sequentially :)


set_microstep_interval will cause some drivers to immediately change their their speed if they are already moving

True.

while the current documentation implies that the new speed only takes effect once a movement command (move_to/move_by/run) has been called. I prefer this second interpretation and would like the documentation to be clarified in this regard.

Not really sure if this is the case in the API documentation as of now.

If a user wants to change micro_step_interval while the stepper is moving, i think it's completely okay.


set_microstep_interval 0 will fail with some drivers which is in contrast to the old set_speed 0 which caused the motor to stop. As a result, currently the only way to stop a moving motor is to call disable followed by enable, which is not great. As a result, I would propose to either return set_microstep_interval 0 to its status as a stop command, or, preferably, to define a new dedicated stop command that stops the motor and keeps the coils energized.

regardless of the results of the previous issue, I would propose an emergency_stop command that immediately stops the motor and keeps the coils energized. The difference to the previous entry is mainly with acceleration based drivers, such as tmc 50XX and the step-dir acceleration driver I am currently developing. The regular stop command would result in them gradually decelerating, but for safety reasons it might be necessary to halt them immediately.

We can introduce a new dedicated stop(emergency_stop) command and keep the coils energized.


the enable/disable command is completely unspecified in its behavior. My recommended behavior would be (based on the drv8424 driver):
disable (enable off) causes movement to stop (including is_moving and changing position value), the motor coils to be de-energized and movement commands (move_to/move_by/run) to fail with an error code (drv8424 currently uses -ECANCELED). Setting microstep interval and resolution still works however.

I think this is already the case as of now, if you try to call any of the move commands with motor disabled, -ECANCELED will be the return value as per stepper api.

enable causes the motor coils to be energized, but no movement occurs (even if the motor was moving before the disable command) until a movement command is called.

This sounds fair :)


Todos:

@jilaypandya jilaypandya added the Enhancement Changes/Updates/Additions to existing features label Feb 20, 2025
@jilaypandya jilaypandya moved this to Todo in RFC Backlog Feb 21, 2025
@jbehrensnx
Copy link
Contributor Author

set_microstep_interval will cause some drivers to immediately change their their speed if they are already moving

True.

while the current documentation implies that the new speed only takes effect once a movement command (move_to/move_by/run) has been called. I prefer this second interpretation and would like the documentation to be clarified in this regard.

Not really sure if this is the case in the API documentation as of now.
If a user wants to change micro_step_interval while the stepper is moving, i think it's completely okay.

Just to clarify: your opinion is, that set_microstep_interval causing speed changes is the intended behavior? If yes than I would like it to be made explicit in the specification. If it should instead be implementation dependent, I would also like this to be made explicit and combined with adding an api call to determine which version is used by a given driver.
The point where this becomes relevant is, from my point of view, when you run the motor at a certain speed and then, while it is moving, want to move_by/move_to at a different speed. Not sure if it would be considered to be to contrived/niche.

the enable/disable command is completely unspecified in its behavior. My recommended behavior would be (based on the drv8424 driver):
disable (enable off) causes movement to stop (including is_moving and changing position value), the motor coils to be de-energized and movement commands (move_to/move_by/run) to fail with an error code (drv8424 currently uses -ECANCELED). Setting microstep interval and resolution still works however.

I think this is already the case as of now, if you try to call any of the move commands with motor disabled, -ECANCELED will be the return value as per stepper api.

I mainly mentioned it because the the two tmc drivers don't do this, but yes the specification mentions the -ECANCELED. I should probably given this as an example of not following the api not being caught without tests in the test pr instead.

  • Introduce a stop function in api

I would like to add both stop and emergency_stop as they have two different purposes, one to simply stop the motor during regular movement, and second to immediately stop the motor, regardless of the consequences. While these two functions would be the same for constant velocity drivers (gpio, counter + work timing source for step-dir), they would differ for acceleration based drivers like tmc50xx and the acceleration-counter timing source I am currently working on.

  • Update enable function with the specification : enable causes the motor coils to be energized, but no movement occurs (even if the motor was moving before the disable command) until a movement command is called.

I would like to also clarify what enable off does in the specification as well. While one could argue, that it is sort of specified in the movement commands, specifying it explicitly for the enable would make it clearer I think.

@jilaypandya
Copy link
Contributor

Just to clarify: your opinion is, that set_microstep_interval causing speed changes is the intended behavior? If yes than I would like it to be made explicit in the specification. If it should instead be implementation dependent, I would also like this to be made explicit and combined with adding an api call to determine which version is used by a given driver.

Set the time interval between steps in microseconds(correction to be introduced: nanoseconds). Setting a time interval between two consecutive steps affects the speed of the stepper is evident imo.

The point where this becomes relevant is, from my point of view, when you run the motor at a certain speed and then, while it is moving, want to move_by/move_to at a different speed. Not sure if it would be considered to be to contrived/niche.

so run(dev,direction) and move(dev,usteps), as the name suggests only execute run or move commands, and the concern of setting speed is with set_microstep_interval(dev, nsecs).
Just from the top of my head, wouldn't a ramp driver want to periodically update interval(speed) while ramping up/down. ?


I mainly mentioned it because the the two tmc drivers don't do this, but yes the specification mentions the -ECANCELED. I should probably given this as an example of not following the api not being caught without tests in the test pr instead.

Adding a todo for this


I would like to add both stop and emergency_stop as they have two different purposes, one to simply stop the motor during regular movement, and second to immediately stop the motor, regardless of the consequences. While these two functions would be the same for constant velocity drivers (gpio, counter + work timing source for step-dir), they would differ for acceleration based drivers like tmc50xx and the acceleration-counter timing source I am currently working on.

Took some inspiration from trinamic datasheet :)

enum stepper_stop_mode {
        SOFT_STOP,
        HARD_STOP,
}
stepper_stop(dev, stop_mode)

I would like to also clarify what enable off does in the specification as well. While one could argue, that it is sort of specified in the movement commands, specifying it explicitly for the enable would make it clearer I think.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Stepper Enhancement Changes/Updates/Additions to existing features RFC Request For Comments: want input from the community
Projects
Status: In Progress
Development

No branches or pull requests

3 participants