drivers: stepper: step_dir: fix 87698

During testing with teensy 4.0 it was discoverd that toggling
at high frequencies led to missed steps. As per the datasheets
of step-dir drivers an active edge needs to be maintained for
a certain timerperiod, for a step to be executed.

Instead of toggling pin twice instantaneously, the current fix
halves the step period and the step-pin is toggled on the timeout
of the respective timing source.

rework stepper stepper stop

set step pin to low if the driver does not support dual edge
and increment/decrement the actual position by one

Signed-off-by: Jilay Pandya <[email protected]>

Author: jilaypandya

drivers/stepper/step_dir/step_dir_stepper_common.c

@@ -8,28 +8,6 @@
 #include <zephyr/logging/log.h>
 LOG_MODULE_REGISTER(step_dir_stepper, CONFIG_STEPPER_LOG_LEVEL);
 
-static inline int step_dir_stepper_perform_step(const struct device *dev)
-{
-	const struct step_dir_stepper_common_config *config = dev->config;
-	int ret;
-
-	ret = gpio_pin_toggle_dt(&config->step_pin);
-	if (ret < 0) {
-		LOG_ERR("Failed to toggle step pin: %d", ret);
-		return ret;
-	}
-
-	if (!config->dual_edge) {
-		ret = gpio_pin_toggle_dt(&config->step_pin);
-		if (ret < 0) {
-			LOG_ERR("Failed to toggle step pin: %d", ret);
-			return ret;
-		}
-	}
-
-	return 0;
-}
-
 static inline int update_dir_pin(const struct device *dev)
 {
 	const struct step_dir_stepper_common_config *config = dev->config;
@@ -111,8 +89,38 @@ static void stepper_work_event_handler(struct k_work *work)
 }
 #endif /* CONFIG_STEPPER_STEP_DIR_GENERATE_ISR_SAFE_EVENTS */
 
-static void update_remaining_steps(struct step_dir_stepper_common_data *data)
+static int start_stepping(const struct device *dev)
 {
+	const struct step_dir_stepper_common_config *config = dev->config;
+	struct step_dir_stepper_common_data *data = dev->data;
+	int ret;
+
+	ret = config->timing_source->update(dev, config->dual_edge
+							 ? data->microstep_interval_ns
+							 : data->microstep_interval_ns / 2);
+	if (ret < 0) {
+		LOG_ERR("Failed to update timing source: %d", ret);
+		return ret;
+	}
+
+	ret = config->timing_source->start(dev);
+	if (ret < 0) {
+		LOG_ERR("Failed to start timing source: %d", ret);
+		return ret;
+	}
+
+	stepper_handle_timing_signal(dev);
+	return ret;
+}
+
+static void update_remaining_steps(const struct device *dev)
+{
+	const struct step_dir_stepper_common_config *config = dev->config;
+	struct step_dir_stepper_common_data *data = dev->data;
+
+	if (data->step_high && !config->dual_edge) {
+		return;
+	}
 	if (atomic_get(&data->step_count) > 0) {
 		atomic_dec(&data->step_count);
 	} else if (atomic_get(&data->step_count) < 0) {
@@ -138,7 +146,7 @@ static void position_mode_task(const struct device *dev)
 	struct step_dir_stepper_common_data *data = dev->data;
 	const struct step_dir_stepper_common_config *config = dev->config;
 
-	update_remaining_steps(dev->data);
+	update_remaining_steps(dev);
 
 	if (config->timing_source->needs_reschedule(dev) && atomic_get(&data->step_count) != 0) {
 		(void)config->timing_source->start(dev);
@@ -159,13 +167,24 @@ static void velocity_mode_task(const struct device *dev)
 
 void stepper_handle_timing_signal(const struct device *dev)
 {
+	const struct step_dir_stepper_common_config *config = dev->config;
 	struct step_dir_stepper_common_data *data = dev->data;
+	int ret;
 
-	(void)step_dir_stepper_perform_step(dev);
-	if (data->direction == STEPPER_DIRECTION_POSITIVE) {
-		atomic_inc(&data->actual_position);
-	} else {
-		atomic_dec(&data->actual_position);
+	atomic_val_t step_pin_status = atomic_xor(&data->step_high, 1) ^ 1;
+
+	ret = gpio_pin_set_dt(&config->step_pin, atomic_get(&step_pin_status));
+	if (ret < 0) {
+		LOG_ERR("Failed to set step pin: %d", ret);
+		return;
+	}
+
+	if (!atomic_get(&step_pin_status) || config->dual_edge) {
+		if (data->direction == STEPPER_DIRECTION_POSITIVE) {
+			atomic_inc(&data->actual_position);
+		} else {
+			atomic_dec(&data->actual_position);
+		}
 	}
 
 	switch (data->run_mode) {
@@ -218,7 +237,6 @@ int step_dir_stepper_common_init(const struct device *dev)
 		    CONFIG_STEPPER_STEP_DIR_EVENT_QUEUE_LEN);
 	k_work_init(&data->event_callback_work, stepper_work_event_handler);
 #endif /* CONFIG_STEPPER_STEP_DIR_GENERATE_ISR_SAFE_EVENTS */
-
 	return 0;
 }
 
@@ -247,8 +265,8 @@ int step_dir_stepper_common_move_by(const struct device *dev, const int32_t micr
 		if (ret < 0) {
 			K_SPINLOCK_BREAK;
 		}
-		config->timing_source->update(dev, data->microstep_interval_ns);
-		config->timing_source->start(dev);
+
+		ret = start_stepping(dev);
 	}
 
 	return ret;
@@ -265,9 +283,21 @@ int step_dir_stepper_common_set_microstep_interval(const struct device *dev,
 		return -EINVAL;
 	}
 
+	if (config->dual_edge && (microstep_interval_ns < config->step_width_ns)) {
+		LOG_ERR("Step interval too small for configured step width");
+		return -EINVAL;
+	}
+
+	if (microstep_interval_ns < 2 * config->step_width_ns) {
+		LOG_ERR("Step interval too small for configured step width");
+		return -EINVAL;
+	}
+
 	K_SPINLOCK(&data->lock) {
 		data->microstep_interval_ns = microstep_interval_ns;
-		config->timing_source->update(dev, microstep_interval_ns);
+		config->timing_source->update(dev, config->dual_edge
+							   ? data->microstep_interval_ns
+							   : data->microstep_interval_ns / 2);
 	}
 
 	return 0;
@@ -315,7 +345,6 @@ int step_dir_stepper_common_is_moving(const struct device *dev, bool *is_moving)
 int step_dir_stepper_common_run(const struct device *dev, const enum stepper_direction direction)
 {
 	struct step_dir_stepper_common_data *data = dev->data;
-	const struct step_dir_stepper_common_config *config = dev->config;
 	int ret;
 
 	K_SPINLOCK(&data->lock) {
@@ -325,8 +354,8 @@ int step_dir_stepper_common_run(const struct device *dev, const enum stepper_dir
 		if (ret < 0) {
 			K_SPINLOCK_BREAK;
 		}
-		config->timing_source->update(dev, data->microstep_interval_ns);
-		config->timing_source->start(dev);
+
+		ret = start_stepping(dev);
 	}
 
 	return ret;
@@ -335,6 +364,7 @@ int step_dir_stepper_common_run(const struct device *dev, const enum stepper_dir
 int step_dir_stepper_common_stop(const struct device *dev)
 {
 	const struct step_dir_stepper_common_config *config = dev->config;
+	struct step_dir_stepper_common_data *data = dev->data;
 	int ret;
 
 	ret = config->timing_source->stop(dev);
@@ -343,7 +373,18 @@ int step_dir_stepper_common_stop(const struct device *dev)
 		return ret;
 	}
 
+	if (!config->dual_edge && atomic_cas(&data->step_high, 1, 0)) {
+		gpio_pin_set_dt(&config->step_pin, 0);
+		/* If we are in the high state, we need to account for that step */
+		if (data->direction == STEPPER_DIRECTION_POSITIVE) {
+			atomic_inc(&data->actual_position);
+		} else {
+			atomic_dec(&data->actual_position);
+		}
+	}
+
 	stepper_trigger_callback(dev, STEPPER_EVENT_STOPPED);
+
 	return 0;
 }
 

drivers/stepper/step_dir/step_dir_stepper_common.h

@@ -29,6 +29,7 @@
 struct step_dir_stepper_common_config {
 	const struct gpio_dt_spec step_pin;
 	const struct gpio_dt_spec dir_pin;
+	uint32_t step_width_ns;
 	bool dual_edge;
 	const struct stepper_timing_source_api *timing_source;
 	const struct device *counter;
@@ -47,6 +48,7 @@ struct step_dir_stepper_common_config {
 		.step_pin = GPIO_DT_SPEC_GET(node_id, step_gpios),                                 \
 		.dir_pin = GPIO_DT_SPEC_GET(node_id, dir_gpios),                                   \
 		.dual_edge = DT_PROP_OR(node_id, dual_edge_step, false),                           \
+		.step_width_ns = DT_PROP(node_id, step_width_ns),                                  \
 		.counter = DEVICE_DT_GET_OR_NULL(DT_PHANDLE(node_id, counter)),                    \
 		.invert_direction = DT_PROP(node_id, invert_direction),                            \
 		.timing_source = COND_CODE_1(DT_NODE_HAS_PROP(node_id, counter),                   \
@@ -78,7 +80,7 @@ struct step_dir_stepper_common_data {
 	void *event_cb_user_data;
 
 	struct k_work_delayable stepper_dwork;
-
+	atomic_t step_high;
 #ifdef CONFIG_STEP_DIR_STEPPER_COUNTER_TIMING
 	struct counter_top_cfg counter_top_cfg;
 	bool counter_running;

dts/bindings/stepper/adi/adi,tmc2209.yaml

@@ -19,8 +19,12 @@ compatible: "adi,tmc2209"
 
 include:
   - name: stepper-controller.yaml
+  - name: step-dir-timing.yaml
 
 properties:
+  step-width-ns:
+    default: 100
+
   msx-gpios:
     type: phandle-array
     description: |

dts/bindings/stepper/allegro/allegro,a4979.yaml

@@ -25,8 +25,12 @@ compatible: "allegro,a4979"
 
 include:
   - name: stepper-controller.yaml
+  - name: step-dir-timing.yaml
 
 properties:
+  step-width-ns:
+    default: 1000
+
   m0-gpios:
     required: true
     type: phandle-array

dts/bindings/stepper/step-dir-timing.yaml

@@ -0,0 +1,10 @@
+# SPDX-FileCopyrightText: Copyright (c) 2025 Jilay Sandeep Pandya
+# SPDX-License-Identifier: Apache-2.0
+
+description: Step/Dir Control Signal Timing
+
+properties:
+  step-width-ns:
+    type: int
+    description: |
+      Minimum pulse width in nanoseconds for the step signal.

dts/bindings/stepper/ti/ti,drv84xx.yaml

@@ -31,8 +31,12 @@ compatible: "ti,drv84xx"
 
 include:
   - name: stepper-controller.yaml
+  - name: step-dir-timing.yaml
 
 properties:
+  step-width-ns:
+    default: 970
+
   fault-gpios:
     type: phandle-array
     description: Fault pin.

include/zephyr/drivers/stepper.h

@@ -354,7 +354,9 @@ static inline int z_impl_stepper_set_reference_position(const struct device *dev
 }
 
 /**
- * @brief Get the actual a.k.a reference position of the stepper
+ * @brief Get the actual step count for a given stepper.
+ * @note This function does not guarantee that the returned position is the exact current
+ * position. For precise positioning, encoders should be used in addition to the stepper driver.
  *
  * @param dev pointer to the stepper driver instance
  * @param value The actual position to get in micro-steps
@@ -432,7 +434,7 @@ static inline int z_impl_stepper_set_microstep_interval(const struct device *dev
 /**
  * @brief Set the micro-steps to be moved from the current position i.e. relative movement
  *
- * @details The stepper will move by the given number of micro-steps from the current position.
+ * @note The stepper will move by the given number of micro-steps from the current position.
  * This function is non-blocking.
  *
  * @param dev pointer to the stepper driver instance
@@ -453,7 +455,7 @@ static inline int z_impl_stepper_move_by(const struct device *dev, const int32_t
 /**
  * @brief Set the absolute target position of the stepper
  *
- * @details The stepper will move to the given micro-steps position from the reference position.
+ * @note The stepper will move to the given micro-steps position from the reference position.
  * This function is non-blocking.
  *
  * @param dev pointer to the stepper driver instance
@@ -478,7 +480,7 @@ static inline int z_impl_stepper_move_to(const struct device *dev, const int32_t
 /**
  * @brief Run the stepper with a given step interval in a given direction
  *
- * @details The stepper shall be set into motion and run continuously until
+ * @note The stepper shall be set into motion and run continuously until
  * stalled or stopped using some other command, for instance, stepper_stop(). This
  * function is non-blocking.
  *
@@ -504,7 +506,7 @@ static inline int z_impl_stepper_run(const struct device *dev,
 
 /**
  * @brief Stop the stepper
- * @details Cancel all active movements.
+ * @note Cancel all active movements.
  *
  * @param dev pointer to the stepper driver instance
  *

tests/drivers/stepper/stepper_api/boards/native_sim_adi_tmc2209.overlay

@@ -21,5 +21,6 @@
 		en-gpios = <&gpio2 1 0>;
 		msx-gpios = <&gpio3 0 0>, <&gpio4 1 0>;
 		counter = <&counter0>;
+		dual-edge-step;
 	};
 };

tests/drivers/stepper/stepper_api/boards/native_sim_adi_tmc2209_work_q.overlay

@@ -20,5 +20,6 @@
 		step-gpios = <&gpio1 1 0>;
 		en-gpios = <&gpio2 1 0>;
 		msx-gpios = <&gpio3 0 0>, <&gpio4 1 0>;
+		dual-edge-step;
 	};
 };