Skip to content

drivers: stepper: introduce stepper_drv api to facilitate implementing motion controllers as device drivers #91979

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jilaypandya
Copy link
Member

@jilaypandya jilaypandya commented Jun 21, 2025

Update 29.06.25

Commit 1: Introduces stepper_drv api for stepper driver drivers
Commit 2: Split stepper_drv related functions from stepper_api and introduces stepper_idx in all motion_control functions
Commit 3: Minor bugfix in stepper_api test suite
commit 4: Add stepper index selection functionality in shell


This PR aims to facilitate implementation of stepper motion controllers as device drivers. Hence the low-level step-dir drivers now shall implement a stepper_drv api, effectively also signifying what the IC actually supports i.e enable, step/dir and in some cases fault detection

  • Stepper API as all of us know it, implements motion control functions
  • Stepper Drv API is responsible for the configuration of stepper_drv drivers.
Drivers Stepper Driver (stepper_drv API) Stepper Motion Control Driver ( stepper API)
adi,tmc22xx x
allegro,a4979 x
ti,drv84xx x
zephyr,gpio-stepper(h-bridge based driver) x
adi,tmc50xx x x
adi,tmc51xx x x
zephyr,stepper-motion-controller (CPU Based) x

Stepper Driver Subsystem shall now have two APIs

stepper driver api (stepper_drv) stepper motion control api (stepper)
enable / disable impl
microstep_resolution impl
step/dir impl
move_to/move_by/run/stop impl
set_reference_position impl
*is_moving impl
get_actual_position impl
set_reference_position impl

Code Snippet:

#define X_AXIS  0
#define Y_AXIS  1

stepper_move_to(tmc50xx_motion_controller, X_AXIS, 200);
stepper_stop(tmc50xx_motion_controller, Y_AXIS);
stepper_drv_disable(tmc50xx_stepper_driver_x_axis@0);
stepper_drv_disable(tmc50xx_stepper_driver_y_axis@1);

spi {
    
      /* Implements stepper motion control api */
     tmc50xx_motion_controller {
        compatible = "adi,tmc50xx"

        /* Implements stepper driver api */
        tmc50xx_stepper_driver_x_axis@0 {
        };

        /* Implements stepper driver api */
        tmc50xx_stepper_driver_y_axis@1 {
        };
    };
};

Side effects:

  1. A lot of common code is refactored out and the stepper step-dir or h-bridge based drivers only do what the ic supports. i.e. enable/disable, set/get ustep resolution, step/dir.

  2. Drv84xx handles fault events as of now, which is good. However, handling of fault event has to be refactored out of stepper_api, as mentioned in the RFC Split Stepper Motion Controller <-> Stepper Driver #89786 .
    Introduce set_fault_event_callback, remove fault_event from the current stepper_event enum and rename stepper_event_callback to stepper_motion_control_event_callback.

@jilaypandya jilaypandya force-pushed the feature/introduce-step-dir-functions branch from 0429ffb to 5c9c369 Compare June 21, 2025 22:35
@jilaypandya jilaypandya changed the title wip drivers: stepper: introduce step-dir functions in stepper api to facilitate implementing motion controllers as device drivers Jun 21, 2025
@jilaypandya jilaypandya force-pushed the feature/introduce-step-dir-functions branch 9 times, most recently from 676d5fd to 1d51595 Compare June 22, 2025 12:25
Copy link
Collaborator

@faxe1008 faxe1008 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small little pass, will do some in depth review later on :^)


static int tmc22xx_stepper_enable(const struct device *dev)
{
const struct tmc22xx_config *config = dev->config;

LOG_DBG("Enabling Stepper motor controller %s", dev->name);
return gpio_pin_set_dt(&config->enable_pin, 1);
return gpio_pin_set_dt(&config->enable_pin, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally you would set this to 1 and change the actual output to be active low via devicetree

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense :) Thanks :^)

Ping @andre-stefanov

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes this is exactly what i came up with in my implementation in the end. It is a bit inconvenient because the user of e.g. tmc2209 has to know this inverted EN logic during DTS definition instead of just saying "i define the pins, the driver knows which level has to be used for enable/disable". But unfortunately i could not provide gpio port/pin references without output level flag in DTS so i left it as faxe is proposing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if you do #define STEPPER_GPIO_ENABLE_LEVEL DT_PROP(instance_nr, enable_level)
Then enable_level may be defined in .yaml as an integer or boolean as a required entry.
Then one can do: return gpio_pin_set_dt(&config->enable_pin, STEPPER_GPIO_ENABLE_LEVEL);.
I may be wrong - just my 2 cents here.

}

static int tmc22xx_stepper_disable(const struct device *dev)
{
const struct tmc22xx_config *config = dev->config;

LOG_DBG("Disabling Stepper motor controller %s", dev->name);
return gpio_pin_set_dt(&config->enable_pin, 0);
return gpio_pin_set_dt(&config->enable_pin, 1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -121,7 +120,6 @@ static int a4979_stepper_set_micro_step_res(const struct device *dev,
case STEPPER_MICRO_STEP_4:
m0_value = 0;
m1_value = 1;
break;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this was an accident

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep :)

const struct gpio_stepper_config *config = dev->config;

LOG_DBG("Initializing %s gpio_stepper with %d pin", dev->name, NUM_CONTROL_PINS);
for (uint8_t n_pin = 0; n_pin < NUM_CONTROL_PINS; n_pin++) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking readyness via gpio_ready_dt

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@jilaypandya jilaypandya force-pushed the feature/introduce-step-dir-functions branch 2 times, most recently from 9ebf11f to d3c27b5 Compare June 22, 2025 13:17
@jilaypandya jilaypandya added this to the v4.3.0 milestone Jun 22, 2025
@jilaypandya jilaypandya force-pushed the feature/introduce-step-dir-functions branch 2 times, most recently from 3b94e61 to 703d030 Compare June 22, 2025 14:16
#include <zephyr/sys/__assert.h>

#include <zephyr/logging/log.h>
LOG_MODULE_REGISTER(gpio_stepper_motor_controller, CONFIG_STEPPER_LOG_LEVEL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: gpio_stepper_motor_controller, just a bit too long :)

@jilaypandya jilaypandya force-pushed the feature/introduce-step-dir-functions branch 4 times, most recently from 4a60ace to e4b6c2d Compare June 22, 2025 15:31
@jilaypandya jilaypandya force-pushed the feature/introduce-step-dir-functions branch 4 times, most recently from 84392f5 to 060b707 Compare June 22, 2025 17:35
@jilaypandya jilaypandya force-pushed the feature/introduce-step-dir-functions branch from 060b707 to e2dd143 Compare June 22, 2025 19:16
@@ -1,18 +1,18 @@
# Copyright (c) 2024 Fabian Blatz <fabianblatz@gmail.com>
# SPDX-License-Identifier: Apache-2.0

config STEPPER_$(module)_GENERATE_ISR_SAFE_EVENTS
config $(module)_GENERATE_ISR_SAFE_EVENTS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be dependent on step-dir stepper types? I mean, you would'nt need this in a hardware based controller

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, you are right, this has to be moved to step_dir_common. Thanks for pointing it out :)

Copy link
Member Author

@jilaypandya jilaypandya Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just thought about it again as to why i had moved this to motion controller, this event mechanism is getting used in zephyr,stepper-motion-controller and is independent of the stepper-drv, since motion-controller will know when the steps are completed and will trigger the events.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you meant that a tempate is not rquired anymore? I've moved it to the Kconfig.zephyr_stepper_motion_controller.

Copy link
Member

@dipakgmx dipakgmx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments from first glance. Else, it seems good

@jilaypandya jilaypandya force-pushed the feature/introduce-step-dir-functions branch from 3c796d8 to e500d53 Compare July 1, 2025 17:24
@jilaypandya jilaypandya linked an issue Jul 2, 2025 that may be closed by this pull request
@jilaypandya jilaypandya requested a review from dipakgmx July 2, 2025 20:43
@jilaypandya jilaypandya force-pushed the feature/introduce-step-dir-functions branch 2 times, most recently from 3ce317f to 6e4b29d Compare July 2, 2025 20:54
@jilaypandya jilaypandya force-pushed the feature/introduce-step-dir-functions branch 4 times, most recently from 7d822fc to 61247b8 Compare July 3, 2025 18:32
}

if (!k_is_in_isr()) {
data->callback(dev, MAX_SUPPORTED_STEPPER, event, data->event_cb_user_data);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAX_SUPPORTED_STEPPER has value 1 while the actual stepper idx used is probably 0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

/* Run the callback */
if (data->callback != NULL) {
/* using MAX_SUPPORTED_STEPPER as currently only one stepper is supported */
data->callback(data->dev, MAX_SUPPORTED_STEPPER, event,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAX_SUPPORTED_STEPPER has value 1 while the actual stepper idx used is probably 0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

K_SPINLOCK(&data->lock) {
data->callback = cb;
}
data->event_cb_user_data = user_data;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could lead to a race condition. it has to be inside of the spinlock otherwise if an ISR or another thread occurs between those two assignments, it would use the new callback but old user data

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed it.

struct stepper_motion_control_data *data = dev->data;

K_SPINLOCK(&data->lock) {
data->actual_position = position;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data->reference_position is unused while z_stepper_motion_control_set_reference_position is changing data->actual_position

Copy link
Member Author

@jilaypandya jilaypandya Jul 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. dropped reference position

@jilaypandya jilaypandya force-pushed the feature/introduce-step-dir-functions branch 2 times, most recently from 6d81404 to 8b3c64f Compare July 5, 2025 08:29
introducing stepper_drv api which is to be implemented by
the stepper drivers

Add fault handling in drv84xx using a fault cb mechanism

With the introduction of the stepper_drv api, the goal is
to achieve better separation of concerns where motion
controllers are responsible for generating step pulses
whereaas stepper drivers do are responsible for stepping,
enabling, setting microstep resolution

Signed-off-by: Jilay Pandya <jilay.pandya@outlook.com>
- Drop stepper_drv functions from stepper_api
- Refactor Stepper Motion Controller
- Refactor Shell

Signed-off-by: Jilay Pandya <jilay.pandya@outlook.com>
@jilaypandya jilaypandya force-pushed the feature/introduce-step-dir-functions branch from 8b3c64f to 0295fbb Compare July 5, 2025 08:56
Copy link

sonarqubecloud bot commented Jul 5, 2025

Introduce check for asserting if the given stepper index
is less than max supported steppers by the controller

Signed-off-by: Jilay Pandya <jilay.pandya@outlook.com>
Add stepper index  selection functionality in stepper shell to select
different steppers for the same stepper motion controller

Signed-off-by: Jilay Pandya <jilay.pandya@outlook.com>
@jilaypandya jilaypandya force-pushed the feature/introduce-step-dir-functions branch from 2c4d801 to 45bdc7e Compare July 5, 2025 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split Stepper Motion Controller <-> Stepper Driver drivers: stepper: Handling of Multiple Step-Dir Implementations
8 participants