drivers: stepper: seperate stepper_stop from stepper_disable

[jilaypandya]

stepper_stop() shall be used be used explicitly to cancel stepper step counting.
stepper_disable() is responsible for turning off power stages.

Attached along with is the trace from tmc50xx stepper motion controller + driver.
It shows that disabling the stepper driver does not implicitly stop the motion control part.

uart:~$ stepper run tmc_stepper@0 positive 
uart:~$ stepper get_actual_position tmc_stepper@0 
Actual Position: -6512577
uart:~$ stepper disable tmc_stepper@0 
uart:~$ stepper get_actual_position tmc_stepper@0
Actual Position: -6030449
uart:~$ stepper enable tmc_stepper@0 
uart:~$ stepper get_actual_position tmc_stepper@0
Actual Position: -5278472
uart:~$ stepper get_actual_position tmc_stepper@0
Actual Position: -4574999
uart:~$ stepper disable tmc_stepper@0
uart:~$ stepper get_actual_position tmc_stepper@0
Actual Position: -4282534
uart:~$ stepper get_actual_position tmc_stepper@0
Actual Position: -4112518

Originally stepper_stop() was missing in the stepper api and hence while disabling the motor was stopped implicitly by stopping counter or work.
However, there is a dedicated stop() function now,
hence enable<->disable should be concerned with turning power stages on and off
while move<->stop should be concerned with setting the stepper in motion and stopping it

[jbehrensnx]

Counterpoint: drv84xx, allegro a4979, gpio_stepper and the tmc2130 (currently in PR) all stop on disable.

This is also more intuitive, that a device stops all actions on a disable instead of some components continuing to run.

Only the tmc50xx, tmc51xx and tmc22xx drivers do not stop on disable.

At this point it would be better to discuss the expected api behavior on disable.
I personally prefer that the disable function also stops the stepper movement. This is, as already mentioned, more intuitive, as disable intuitively means that all functionality is off, including step signal generation.
There is also another thing to consider, ramping. Once ramping is implemented, the stop function will ramp down, decelerating towards stop. But an emergency stop functionality (hard stop, no deceleration) would still be needed, and the disable-stop would fulfill this quite well without adding additional api functions.

EDIT: My preference would be for tmc22xx to get the standard step-dir stop on disable, and for the tmc50xx/51xx (which by the way have no stop function at all) to decelerate towards 0 on stop and immediately halt on disable.

[jilaypandya]

Counterpoint: drv84xx, allegro a4979, gpio_stepper and the tmc2130 (currently in PR) all stop on disable.

This is also more intuitive, that a device stops all actions on a disable instead of some components continuing to run.

Only the tmc50xx, tmc51xx and tmc22xx drivers do not stop on disable.

They actually don't "stop" on disable, stopping is a motion control concern, disabling turns of the power stages, and this will help in separating the two apis in the future. If a user wants to stop with disable, then stepper_stop() and stepper_disable().

At this point it would be better to discuss the expected api behavior on disable. I personally prefer that the disable function also stops the stepper movement. This is, as already mentioned, more intuitive, as disable intuitively means that all functionality is off, including step signal generation.

step signal generation is not a concern of a step-dir stepper driver, it's a concern of motion control part. If you want to stop generating step signals than stepper_stop() is there.

Initially there was no stepper_stop function in the api and that's why we stopped the stepper using stepper_disable, this was done as a part of this RFC #86052.
But now we have a dedicated function, so user should use stepper_stop() for stopping and then stepper_disable() for disabling the power stages.

The reason I am taking 5xxx as a benchmark since it's the only driver upstream which actually is a motion_control + stepper_driver. Went through the data sheet what disable is actually supposed to, it just says that power stages shall be turned off.

Moreover, dedicated motion controllers like the TMC429 do not have an en_pin, which shows its actually the concern of stepper driver.

[dipakgmx]

There is also another thing to consider, ramping. Once ramping is implemented, the stop function will ramp down, decelerating towards stop. But an emergency stop functionality (hard stop, no deceleration) would still be needed, and the disable-stop would fulfill this quite well without adding additional api functions.

How is a disabling power stages going to achieve emergency stop?

And as far as the stop is concerned, we have discussed this already.

[jilaypandya]

Drivers DRV84XX, A4979 and gpis-stepper-controller are updated and test cases have been also updated accordingly

[jbehrensnx]

If you want to remove stopping from enable/disable, I would rename the functions to clarify that they only affect power stages. A general enable/disables sounds like a device on/off (which is what I meant with stop being included was intuitive) where as enable_power_stages or something similar would clarify what you are enabling/disabling and remove confusion.

Concerning the drv84xx tests, instead of removing them, it might be an idea to change at least some of them so that they check that movement actually continues, as this would be the new expected behavior that the drivers should have.
Also, I took another attempt at putting the drv84xx into the stepper_api tests (as you mentioned in the test generalization PR that is now closed), but ran into some strange timeout issues (best as I can describe it) in native sim (they ran fine on actual hardware), so I have put that on hold for now.

[dipakgmx]

If you want to remove stopping from enable/disable, I would rename the functions to clarify that they only affect power stages. A general enable/disables sounds like a device on/off (which is what I meant with stop being included was intuitive) where as enable_power_stages or something similar would clarify what you are enabling/disabling and remove confusion.

Could you tell me what your exact use case with stepper motors is. I am not able to follow what you are trying to recommend here. What else would enable/disable for a stepper driver mean otherwise?

[jbehrensnx]

Basically an on/off switch. We are after all not dealing with stepper motors, but with (physical) stepper drivers. The drv8424 is an physical stepper driver, thus its software driver is an stepper driver driver. In this context enable/disable easily means enabling/disabling the drv8424, which has internal logic and settings. The tmc5xxx series is even more advanced. Enabling/disabling this kind of stepper driver (technically stepper controller, but that is a topic for the stepper api rework PRs) is significantly different from just enabling/disabling the power stages. Hence renaming the functions.

[jilaypandya]

where as enable_power_stages or something similar would clarify what you are enabling/disabling and remove confusion.

I have corrected the documentation in an appropriate manner imo.

Also, I took another attempt at putting the drv84xx into the stepper_api tests (as you mentioned in the test generalization PR that is now closed), but ran into some strange timeout issues (best as I can describe it) in native sim (they ran fine on actual hardware), so I have put that on hold for now.

Okay perfect, good to know that you are on it. We can directly implement those tests than in stepper_api test suite.

Thanks for your feedback :)

[jbehrensnx]

I have corrected the documentation in an appropriate manner imo.

While the documentation is indeed corrected, I fell that a rename would clarify this better and avoid confusion. As mentioned, a general enable/disable implies more than what it actually does.

[jilaypandya]

While the documentation is indeed corrected, I fell that a rename would clarify this better and avoid confusion. As mentioned, a general enable/disable implies more than what it actually does.

Hey, enable/disable seems to have a standard meaning when it comes to stepper drivers, below are the excerpts from trinamic & ti data sheet.

DRV_ENN 29 I Enable input for motor drivers. The power stage becomes switched
off (all motor outputs floating) when this pin becomes driven to a
high level. Tie to GND for normal operation.

from chopconf register.
%0000: Driver disable, all bridges off

ti. drv8424

ENABLE 25 20 I Input
Logic low to disable device outputs; logic high to enable; internal
pullup to DVDD. Also determines the type of OCP and OTSD
response.

[andre-stefanov]

Counterpoint: drv84xx, allegro a4979, gpio_stepper and the tmc2130 (currently in PR) all stop on disable.

Each stepper would automatically physically stop on disable because the coils get disengaged.

This is also more intuitive, that a device stops all actions on a disable instead of some components continuing to run.

I would disagree here because there is no real intuitive way here. Depending on the application it is possible that some components should keep running for whatever reason (e.g. to track "lost" steps or redirect them to another device/component).

At this point it would be better to discuss the expected api behavior on disable. I personally prefer that the disable function also stops the stepper movement. This is, as already mentioned, more intuitive, as disable intuitively means that all functionality is off, including step signal generation. There is also another thing to consider, ramping. Once ramping is implemented, the stop function will ramp down, decelerating towards stop. But an emergency stop functionality (hard stop, no deceleration) would still be needed, and the disable-stop would fulfill this quite well without adding additional api functions.

Since disabling a stepper is pretty much the same as pulling the plug (power supply) of this single component, i would expect everything else to keep running unless I tell the system not to do so (e.g. by calling emergency stop before disable). Application which use disable for an emergency stop with complete loss of any torque it would be harmful to wait for some motion related operations. Other applications would want to decelerate before disabling (e.g. for homing applications where you reach an endstop and want to gracefully stop your heavy rig after that following by a power off). Each application will have its own requirements and interpretations but they all have in common that disable is just a rudimentary powering off of the stepper. Some maybe do not even have any motion controller in between and they control the stepping manually.

EDIT: My preference would be for tmc22xx to get the standard step-dir stop on disable, and for the tmc50xx/51xx (which by the way have no stop function at all) to decelerate towards 0 on stop and immediately halt on disable.

This strongly contradicts the idea of an intuitive and consistent API and also reduces the freedom of the application to decide what it actually wants before turning off the hardware. Since "disable" is the software equivalent of pulling the plug, the only thing that should happen for all drivers out there is to disable all the coils completely. This is the only common logic which will be understood by API users regardless of the used stepper drivers or application requirements.

[andre-stefanov]

I have corrected the documentation in an appropriate manner imo.

While the documentation is indeed corrected, I fell that a rename would clarify this better and avoid confusion. As mentioned, a general enable/disable implies more than what it actually does.

Since the drivers i have seen before call this feature "enable"/"disable" and even have the EN pin for that in most cases, i would strongly recommend to stick with this naming because this would match the vendor documentation and also make it easy to predict the actual behavior behind these functions for API users. Introducing new names would automatically lead to the need of reading the documentation just to realizing that it is the commonly named "enable"/"disable" hardware feature.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud