drivers: step_dir: refactor stepper_handle_timing_signal

1. Reduce the spinlock scope in stepper_handle_timing_signal
2. perform a step each time the timing signal is called
3. Increment/Decrement actual_position and steps using atomics

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

Author: jilaypandya

drivers/stepper/step_dir/step_dir_stepper_common.c

@@ -11,7 +11,6 @@ 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;
-	struct step_dir_stepper_common_data *data = dev->data;
 	int ret;
 
 	ret = gpio_pin_toggle_dt(&config->step_pin);
@@ -28,12 +27,6 @@ static inline int step_dir_stepper_perform_step(const struct device *dev)
 		}
 	}
 
-	if (data->direction == STEPPER_DIRECTION_POSITIVE) {
-		data->actual_position++;
-	} else {
-		data->actual_position--;
-	}
-
 	return 0;
 }
 
@@ -120,25 +113,20 @@ static void stepper_work_event_handler(struct k_work *work)
 
 static void update_remaining_steps(struct step_dir_stepper_common_data *data)
 {
-	const struct step_dir_stepper_common_config *config = data->dev->config;
-
-	if (data->step_count > 0) {
-		data->step_count--;
-	} else if (data->step_count < 0) {
-		data->step_count++;
-	} else {
-		stepper_trigger_callback(data->dev, STEPPER_EVENT_STEPS_COMPLETED);
-		config->timing_source->stop(data->dev);
+	if (atomic_get(&data->step_count) > 0) {
+		atomic_dec(&data->step_count);
+	} else if (atomic_get(&data->step_count) < 0) {
+		atomic_inc(&data->step_count);
 	}
 }
 
 static void update_direction_from_step_count(const struct device *dev)
 {
 	struct step_dir_stepper_common_data *data = dev->data;
 
-	if (data->step_count > 0) {
+	if (atomic_get(&data->step_count) > 0) {
 		data->direction = STEPPER_DIRECTION_POSITIVE;
-	} else if (data->step_count < 0) {
+	} else if (atomic_get(&data->step_count) < 0) {
 		data->direction = STEPPER_DIRECTION_NEGATIVE;
 	} else {
 		LOG_ERR("Step count is zero");
@@ -150,23 +138,20 @@ 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;
 
-	if (data->step_count) {
-		(void)step_dir_stepper_perform_step(dev);
-	}
+	update_remaining_steps(dev->data);
 
-	if (config->timing_source->needs_reschedule(dev) && data->step_count != 0) {
+	if (config->timing_source->needs_reschedule(dev) && atomic_get(&data->step_count) != 0) {
 		(void)config->timing_source->start(dev);
+	} else if (atomic_get(&data->step_count) == 0) {
+		stepper_trigger_callback(data->dev, STEPPER_EVENT_STEPS_COMPLETED);
+		config->timing_source->stop(data->dev);
 	}
-
-	update_remaining_steps(dev->data);
 }
 
 static void velocity_mode_task(const struct device *dev)
 {
 	const struct step_dir_stepper_common_config *config = dev->config;
 
-	(void)step_dir_stepper_perform_step(dev);
-
 	if (config->timing_source->needs_reschedule(dev)) {
 		(void)config->timing_source->start(dev);
 	}
@@ -176,18 +161,23 @@ void stepper_handle_timing_signal(const struct device *dev)
 {
 	struct step_dir_stepper_common_data *data = dev->data;
 
-	K_SPINLOCK(&data->lock) {
-		switch (data->run_mode) {
-		case STEPPER_RUN_MODE_POSITION:
-			position_mode_task(dev);
-			break;
-		case STEPPER_RUN_MODE_VELOCITY:
-			velocity_mode_task(dev);
-			break;
-		default:
-			LOG_WRN("Unsupported run mode: %d", data->run_mode);
-			break;
-		}
+	(void)step_dir_stepper_perform_step(dev);
+	if (data->direction == STEPPER_DIRECTION_POSITIVE) {
+		atomic_inc(&data->actual_position);
+	} else {
+		atomic_dec(&data->actual_position);
+	}
+
+	switch (data->run_mode) {
+	case STEPPER_RUN_MODE_POSITION:
+		position_mode_task(dev);
+		break;
+	case STEPPER_RUN_MODE_VELOCITY:
+		velocity_mode_task(dev);
+		break;
+	default:
+		LOG_WRN("Unsupported run mode: %d", data->run_mode);
+		break;
 	}
 }
 
@@ -251,7 +241,7 @@ int step_dir_stepper_common_move_by(const struct device *dev, const int32_t micr
 
 	K_SPINLOCK(&data->lock) {
 		data->run_mode = STEPPER_RUN_MODE_POSITION;
-		data->step_count = micro_steps;
+		atomic_set(&data->step_count, micro_steps);
 		update_direction_from_step_count(dev);
 		ret = update_dir_pin(dev);
 		if (ret < 0) {
@@ -265,7 +255,7 @@ int step_dir_stepper_common_move_by(const struct device *dev, const int32_t micr
 }
 
 int step_dir_stepper_common_set_microstep_interval(const struct device *dev,
-					      const uint64_t microstep_interval_ns)
+						   const uint64_t microstep_interval_ns)
 {
 	struct step_dir_stepper_common_data *data = dev->data;
 	const struct step_dir_stepper_common_config *config = dev->config;
@@ -298,9 +288,7 @@ int step_dir_stepper_common_get_actual_position(const struct device *dev, int32_
 {
 	struct step_dir_stepper_common_data *data = dev->data;
 
-	K_SPINLOCK(&data->lock) {
-		*value = data->actual_position;
-	}
+	*value = atomic_get(&data->actual_position);
 
 	return 0;
 }
@@ -311,9 +299,7 @@ int step_dir_stepper_common_move_to(const struct device *dev, const int32_t valu
 	int32_t steps_to_move;
 
 	/* Calculate the relative movement required */
-	K_SPINLOCK(&data->lock) {
-		steps_to_move = value - data->actual_position;
-	}
+	steps_to_move = value - atomic_get(&data->actual_position);
 
 	return step_dir_stepper_common_move_by(dev, steps_to_move);
 }

drivers/stepper/step_dir/step_dir_stepper_common.h

@@ -71,9 +71,9 @@ struct step_dir_stepper_common_data {
 	struct k_spinlock lock;
 	enum stepper_direction direction;
 	enum stepper_run_mode run_mode;
-	int32_t actual_position;
 	uint64_t microstep_interval_ns;
-	int32_t step_count;
+	atomic_t actual_position;
+	atomic_t step_count;
 	stepper_event_callback_t callback;
 	void *event_cb_user_data;