diff --git a/drivers/stepper/allegro/a4979.c b/drivers/stepper/allegro/a4979.c index 644c77526d0..45f062270c1 100644 --- a/drivers/stepper/allegro/a4979.c +++ b/drivers/stepper/allegro/a4979.c @@ -24,7 +24,6 @@ struct a4979_config { struct a4979_data { const struct step_dir_stepper_common_data common; enum stepper_micro_step_resolution micro_step_res; - bool enabled; }; STEP_DIR_STEPPER_STRUCT_CHECK(struct a4979_config, struct a4979_data); @@ -54,7 +53,6 @@ static int a4979_stepper_enable(const struct device *dev) { int ret; const struct a4979_config *config = dev->config; - struct a4979_data *data = dev->data; bool has_enable_pin = config->en_pin.port != NULL; /* Check availability of enable pin, as it might be hardwired. */ @@ -69,8 +67,6 @@ static int a4979_stepper_enable(const struct device *dev) return ret; } - data->enabled = true; - return 0; } @@ -78,7 +74,6 @@ static int a4979_stepper_disable(const struct device *dev) { int ret; const struct a4979_config *config = dev->config; - struct a4979_data *data = dev->data; bool has_enable_pin = config->en_pin.port != NULL; /* Check availability of enable pin, as it might be hardwired. */ @@ -93,9 +88,6 @@ static int a4979_stepper_disable(const struct device *dev) return ret; } - config->common.timing_source->stop(dev); - data->enabled = false; - return 0; } @@ -153,42 +145,6 @@ static int a4979_stepper_get_micro_step_res(const struct device *dev, return 0; } -static int a4979_move_to(const struct device *dev, int32_t target) -{ - struct a4979_data *data = dev->data; - - if (!data->enabled) { - LOG_ERR("Failed to move to target position, device is not enabled"); - return -ECANCELED; - } - - return step_dir_stepper_common_move_to(dev, target); -} - -static int a4979_stepper_move_by(const struct device *dev, const int32_t micro_steps) -{ - struct a4979_data *data = dev->data; - - if (!data->enabled) { - LOG_ERR("Failed to move by delta, device is not enabled"); - return -ECANCELED; - } - - return step_dir_stepper_common_move_by(dev, micro_steps); -} - -static int a4979_run(const struct device *dev, enum stepper_direction direction) -{ - struct a4979_data *data = dev->data; - - if (!data->enabled) { - LOG_ERR("Failed to run stepper, device is not enabled"); - return -ECANCELED; - } - - return step_dir_stepper_common_run(dev, direction); -} - static int a4979_init(const struct device *dev) { const struct a4979_config *config = dev->config; @@ -269,13 +225,13 @@ static int a4979_init(const struct device *dev) static DEVICE_API(stepper, a4979_stepper_api) = { .enable = a4979_stepper_enable, .disable = a4979_stepper_disable, - .move_by = a4979_stepper_move_by, - .move_to = a4979_move_to, + .move_by = step_dir_stepper_common_move_by, + .move_to = step_dir_stepper_common_move_to, .is_moving = step_dir_stepper_common_is_moving, .set_reference_position = step_dir_stepper_common_set_reference_position, .get_actual_position = step_dir_stepper_common_get_actual_position, .set_microstep_interval = step_dir_stepper_common_set_microstep_interval, - .run = a4979_run, + .run = step_dir_stepper_common_run, .stop = step_dir_stepper_common_stop, .set_micro_step_res = a4979_stepper_set_micro_step_res, .get_micro_step_res = a4979_stepper_get_micro_step_res, diff --git a/drivers/stepper/gpio_stepper_controller.c b/drivers/stepper/gpio_stepper_controller.c index 331d2413ec1..560b8cd54be 100644 --- a/drivers/stepper/gpio_stepper_controller.c +++ b/drivers/stepper/gpio_stepper_controller.c @@ -24,6 +24,7 @@ static const uint8_t {0u, 0u, 1u, 1u}, {0u, 0u, 0u, 1u}, {1u, 0u, 0u, 1u}, {1u, 0u, 0u, 0u}}; struct gpio_stepper_config { + const struct gpio_dt_spec en_pin; const struct gpio_dt_spec *control_pins; bool invert_direction; }; @@ -39,7 +40,6 @@ struct gpio_stepper_data { int32_t actual_position; uint64_t delay_in_ns; int32_t step_count; - bool is_enabled; stepper_event_callback_t callback; void *event_cb_user_data; }; @@ -183,11 +183,6 @@ static int gpio_stepper_move_by(const struct device *dev, int32_t micro_steps) { struct gpio_stepper_data *data = dev->data; - if (!data->is_enabled) { - LOG_ERR("Stepper motor is not enabled"); - return -ECANCELED; - } - if (data->delay_in_ns == 0) { LOG_ERR("Step interval not set or invalid step interval set"); return -EINVAL; @@ -262,11 +257,6 @@ static int gpio_stepper_run(const struct device *dev, const enum stepper_directi { struct gpio_stepper_data *data = dev->data; - if (!data->is_enabled) { - LOG_ERR("Stepper motor is not enabled"); - return -ECANCELED; - } - K_SPINLOCK(&data->lock) { data->run_mode = STEPPER_RUN_MODE_VELOCITY; data->direction = direction; @@ -317,18 +307,16 @@ static int gpio_stepper_set_event_callback(const struct device *dev, static int gpio_stepper_enable(const struct device *dev) { + const struct gpio_stepper_config *config = dev->config; struct gpio_stepper_data *data = dev->data; int err; - if (data->is_enabled) { - LOG_WRN("Stepper motor is already enabled"); - return 0; - } - K_SPINLOCK(&data->lock) { - err = energize_coils(dev, true); - if (err == 0) { - data->is_enabled = true; + if (config->en_pin.port != NULL) { + err = gpio_pin_set_dt(&config->en_pin, 1); + } else { + LOG_DBG("No en_pin detected"); + err = -ENOTSUP; } } return err; @@ -336,14 +324,18 @@ static int gpio_stepper_enable(const struct device *dev) static int gpio_stepper_disable(const struct device *dev) { + const struct gpio_stepper_config *config = dev->config; struct gpio_stepper_data *data = dev->data; int err; K_SPINLOCK(&data->lock) { - (void)k_work_cancel_delayable(&data->stepper_dwork); - err = energize_coils(dev, false); - if (err == 0) { - data->is_enabled = false; + (void)energize_coils(dev, false); + if (config->en_pin.port != NULL) { + err = gpio_pin_set_dt(&config->en_pin, 0); + } else { + LOG_DBG("No en_pin detected, power stages will not be turned off if " + "stepper is in motion"); + err = -ENOTSUP; } } return err; diff --git a/drivers/stepper/ti/drv84xx.c b/drivers/stepper/ti/drv84xx.c index cc4a8651e8a..5e99964abea 100644 --- a/drivers/stepper/ti/drv84xx.c +++ b/drivers/stepper/ti/drv84xx.c @@ -49,7 +49,6 @@ struct drv84xx_pin_states { struct drv84xx_data { const struct step_dir_stepper_common_data common; const struct device *dev; - bool enabled; struct drv84xx_pin_states pin_states; enum stepper_micro_step_resolution ustep_res; struct gpio_callback fault_cb_data; @@ -220,8 +219,6 @@ static int drv84xx_enable(const struct device *dev) } } - data->enabled = true; - return ret; } @@ -255,10 +252,6 @@ static int drv84xx_disable(const struct device *dev) } } - config->common.timing_source->stop(dev); - - data->enabled = false; - return ret; } @@ -351,42 +344,6 @@ static int drv84xx_get_micro_step_res(const struct device *dev, return 0; } -static int drv84xx_move_to(const struct device *dev, int32_t target) -{ - struct drv84xx_data *data = dev->data; - - if (!data->enabled) { - LOG_ERR("Failed to move to target position, device is not enabled"); - return -ECANCELED; - } - - return step_dir_stepper_common_move_to(dev, target); -} - -static int drv84xx_move_by(const struct device *dev, int32_t steps) -{ - struct drv84xx_data *data = dev->data; - - if (!data->enabled) { - LOG_ERR("Failed to move by delta, device is not enabled"); - return -ECANCELED; - } - - return step_dir_stepper_common_move_by(dev, steps); -} - -static int drv84xx_run(const struct device *dev, enum stepper_direction direction) -{ - struct drv84xx_data *data = dev->data; - - if (!data->enabled) { - LOG_ERR("Failed to run stepper, device is not enabled"); - return -ECANCELED; - } - - return step_dir_stepper_common_run(dev, direction); -} - void fault_event(const struct device *dev, struct gpio_callback *cb, uint32_t pins) { struct drv84xx_data *data = CONTAINER_OF(cb, struct drv84xx_data, fault_cb_data); @@ -478,13 +435,13 @@ static int drv84xx_init(const struct device *dev) static DEVICE_API(stepper, drv84xx_stepper_api) = { .enable = drv84xx_enable, .disable = drv84xx_disable, - .move_by = drv84xx_move_by, - .move_to = drv84xx_move_to, + .move_by = step_dir_stepper_common_move_by, + .move_to = step_dir_stepper_common_move_to, .is_moving = step_dir_stepper_common_is_moving, .set_reference_position = step_dir_stepper_common_set_reference_position, .get_actual_position = step_dir_stepper_common_get_actual_position, .set_microstep_interval = step_dir_stepper_common_set_microstep_interval, - .run = drv84xx_run, + .run = step_dir_stepper_common_run, .stop = step_dir_stepper_common_stop, .set_micro_step_res = drv84xx_set_micro_step_res, .get_micro_step_res = drv84xx_get_micro_step_res, diff --git a/include/zephyr/drivers/stepper.h b/include/zephyr/drivers/stepper.h index fc76c17b442..64cf23c94e9 100644 --- a/include/zephyr/drivers/stepper.h +++ b/include/zephyr/drivers/stepper.h @@ -262,6 +262,7 @@ static inline int z_impl_stepper_enable(const struct device *dev) * * @param dev pointer to the stepper driver instance * + * @retval -ENOTSUP Disabling of driver is not supported. * @retval -EIO Error during Disabling * @retval 0 Success */ @@ -436,7 +437,6 @@ static inline int z_impl_stepper_set_microstep_interval(const struct device *dev * @param dev pointer to the stepper driver instance * @param micro_steps target micro-steps to be moved from the current position * - * @retval -ECANCELED If the stepper is disabled * @retval -EIO General input / output error * @retval 0 Success */ @@ -458,7 +458,6 @@ static inline int z_impl_stepper_move_by(const struct device *dev, const int32_t * @param dev pointer to the stepper driver instance * @param micro_steps target position to set in micro-steps * - * @retval -ECANCELED If the stepper is disabled * @retval -EIO General input / output error * @retval -ENOSYS If not implemented by device driver * @retval 0 Success @@ -485,7 +484,6 @@ static inline int z_impl_stepper_move_to(const struct device *dev, const int32_t * @param dev pointer to the stepper driver instance * @param direction The direction to set * - * @retval -ECANCELED If the stepper is disabled * @retval -EIO General input / output error * @retval -ENOSYS If not implemented by device driver * @retval 0 Success diff --git a/tests/drivers/stepper/drv84xx/api/src/main.c b/tests/drivers/stepper/drv84xx/api/src/main.c index 82a932dd2b0..04fe097730d 100644 --- a/tests/drivers/stepper/drv84xx/api/src/main.c +++ b/tests/drivers/stepper/drv84xx/api/src/main.c @@ -87,72 +87,6 @@ ZTEST_F(drv84xx_api, test_actual_position_set) zassert_equal(pos, 100u, "Actual position should be %u but is %u", 100u, pos); } -ZTEST_F(drv84xx_api, test_is_not_moving_when_disabled) -{ - int32_t steps = 100; - bool moving = true; - - (void)stepper_enable(fixture->dev); - (void)stepper_set_microstep_interval(fixture->dev, 20000000); - (void)stepper_move_by(fixture->dev, steps); - (void)stepper_disable(fixture->dev); - (void)stepper_is_moving(fixture->dev, &moving); - zassert_false(moving, "Driver should not be in state is_moving after being disabled"); -} - -ZTEST_F(drv84xx_api, test_position_not_updating_when_disabled) -{ - int32_t steps = 1000; - int32_t position_1 = 0; - int32_t position_2 = 0; - - (void)stepper_enable(fixture->dev); - (void)stepper_set_microstep_interval(fixture->dev, 20000000); - (void)stepper_move_by(fixture->dev, steps); - (void)stepper_disable(fixture->dev); - (void)stepper_get_actual_position(fixture->dev, &position_1); - k_msleep(100); - (void)stepper_get_actual_position(fixture->dev, &position_2); - zassert_equal(position_2, position_1, - "Actual position should not have changed from %d but is %d", position_1, - position_2); -} - -ZTEST_F(drv84xx_api, test_is_not_moving_when_reenabled_after_movement) -{ - int32_t steps = 1000; - bool moving = true; - - (void)stepper_enable(fixture->dev); - (void)stepper_set_microstep_interval(fixture->dev, 20000000); - (void)stepper_move_by(fixture->dev, steps); - (void)stepper_disable(fixture->dev); - (void)k_msleep(100); - (void)stepper_enable(fixture->dev); - (void)k_msleep(100); - (void)stepper_is_moving(fixture->dev, &moving); - zassert_false(moving, "Driver should not be in state is_moving after being reenabled"); -} -ZTEST_F(drv84xx_api, test_position_not_updating_when_reenabled_after_movement) -{ - int32_t steps = 1000; - int32_t position_1 = 0; - int32_t position_2 = 0; - - (void)stepper_enable(fixture->dev); - (void)stepper_set_microstep_interval(fixture->dev, 20000000); - (void)stepper_move_by(fixture->dev, steps); - (void)stepper_disable(fixture->dev); - (void)stepper_get_actual_position(fixture->dev, &position_1); - (void)k_msleep(100); - (void)stepper_enable(fixture->dev); - (void)k_msleep(100); - (void)stepper_get_actual_position(fixture->dev, &position_2); - zassert_equal(position_2, position_1, - "Actual position should not have changed from %d but is %d", position_1, - position_2); -} - ZTEST_F(drv84xx_api, test_move_to_positive_direction_movement) { int32_t pos = 50; @@ -247,23 +181,6 @@ ZTEST_F(drv84xx_api, test_move_to_is_moving_false_when_completed) zassert_false(moving, "Driver should not be in state is_moving after finishing"); } -ZTEST_F(drv84xx_api, test_move_to_no_movement_when_disabled) -{ - int32_t pos = 50; - int32_t curr_pos = 50; - int32_t ret = 0; - - (void)stepper_set_microstep_interval(fixture->dev, 20000000); - (void)stepper_disable(fixture->dev); - - ret = stepper_move_to(fixture->dev, pos); - zassert_equal(ret, -ECANCELED, "Move_to should fail with error code %d but returned %d", - -ECANCELED, ret); - (void)stepper_get_actual_position(fixture->dev, &curr_pos); - zassert_equal(curr_pos, 0, "Current position should not have changed from %d but is %d", 0, - curr_pos); -} - ZTEST_F(drv84xx_api, test_move_by_zero_steps_no_movement) { int32_t steps = 0; @@ -284,22 +201,6 @@ ZTEST_F(drv84xx_api, test_move_by_zero_steps_no_movement) zassert_equal(steps, 0, "Target position should be %d but is %d", 0, steps); } -ZTEST_F(drv84xx_api, test_move_by_zero_step_interval) -{ - int32_t steps = 100; - int32_t ret = 0; - int32_t pos = 100; - - (void)stepper_enable(fixture->dev); - (void)stepper_disable(fixture->dev); - ret = stepper_move_by(fixture->dev, steps); - - zassert_not_equal(ret, 0, "Command should fail with an error code, but returned 0"); - k_msleep(100); - (void)stepper_get_actual_position(fixture->dev, &pos); - zassert_equal(pos, 0, "Target position should not have changed from %d but is %d", 0, pos); -} - ZTEST_F(drv84xx_api, test_move_by_is_moving_true_while_moving) { int32_t steps = 50; @@ -334,23 +235,6 @@ ZTEST_F(drv84xx_api, test_move_by_is_moving_false_when_completed) zassert_false(moving, "Driver should not be in state is_moving after completion"); } -ZTEST_F(drv84xx_api, test_move_by_no_movement_when_disabled) -{ - int32_t steps = 100; - int32_t curr_pos = 100; - int32_t ret = 0; - - (void)stepper_set_microstep_interval(fixture->dev, 20000000); - (void)stepper_disable(fixture->dev); - - ret = stepper_move_by(fixture->dev, steps); - zassert_equal(ret, -ECANCELED, "Move_by should fail with error code %d but returned %d", - -ECANCELED, ret); - (void)stepper_get_actual_position(fixture->dev, &curr_pos); - zassert_equal(curr_pos, 0, "Current position should not have changed from %d but is %d", 0, - curr_pos); -} - ZTEST_F(drv84xx_api, test_run_positive_direction_correct_position) { uint64_t step_interval = 20000000; @@ -408,21 +292,4 @@ ZTEST_F(drv84xx_api, test_run_is_moving_true_when_step_interval_greater_zero) (void)stepper_disable(fixture->dev); } -ZTEST_F(drv84xx_api, test_run_no_movement_when_disabled) -{ - uint64_t step_interval = 20000000; - int32_t steps = 50; - int32_t ret = 0; - - (void)stepper_disable(fixture->dev); - (void)stepper_set_microstep_interval(fixture->dev, step_interval); - - ret = stepper_run(fixture->dev, STEPPER_DIRECTION_POSITIVE); - zassert_equal(ret, -ECANCELED, "Run should fail with error code %d but returned %d", - -ECANCELED, ret); - (void)stepper_get_actual_position(fixture->dev, &steps); - zassert_equal(steps, 0, "Current position should not have changed from %d but is %d", 0, - steps); -} - ZTEST_SUITE(drv84xx_api, NULL, drv84xx_api_setup, drv84xx_api_before, drv84xx_api_after, NULL);