-
Notifications
You must be signed in to change notification settings - Fork 7.5k
drivers: stepper: introduce step-dir 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
base: main
Are you sure you want to change the base?
Conversation
0429ffb
to
5c9c369
Compare
676d5fd
to
1d51595
Compare
There was a problem hiding this 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 :^)
drivers/stepper/adi_tmc/tmc22xx.c
Outdated
|
||
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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
drivers/stepper/adi_tmc/tmc22xx.c
Outdated
} | ||
|
||
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
drivers/stepper/allegro/a4979.c
Outdated
@@ -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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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++) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
9ebf11f
to
d3c27b5
Compare
3b94e61
to
703d030
Compare
#include <zephyr/sys/__assert.h> | ||
|
||
#include <zephyr/logging/log.h> | ||
LOG_MODULE_REGISTER(gpio_stepper_motor_controller, CONFIG_STEPPER_LOG_LEVEL); |
There was a problem hiding this comment.
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 :)
4a60ace
to
e4b6c2d
Compare
84392f5
to
060b707
Compare
drivers/stepper/stepper_motion_controllers/zephyr_stepper_motion_controller.c
Outdated
Show resolved
Hide resolved
drivers/stepper/stepper_motion_controllers/zephyr_stepper_motion_controller.c
Outdated
Show resolved
Hide resolved
060b707
to
e2dd143
Compare
6b2aadc
to
804ed2e
Compare
c9641a7
to
9b36dbc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly minor stuff from me. Overall I am pretty happy with it and I think for an interim solution it is fine.
There are some aspects of the final intended architecture that I feel are worth discussing, but I also feel that they would be beyond the scope of this PR.
The one exception to this is the fault event, as I feel that motion-controllers should be able to raise fault events as well.
Personally, I would prefer if the stepper_motion_controller
got renamed to indicate that it is for step-dir drivers, but that is not a hard requirement for me.
de5b453
to
e41a6df
Compare
Hey Everyone, here is the wip implementation of splitted stepper_api. stepper_api implements motion_control functions whereas step_dir_api implements stepper_driver functions. MotionControl functions look highly similar to the ones now from the very first stepper_api #26221. The tmc50xx sample shows how the combined use of the two apis would look like. Let me know of your opinions :) |
introduce step_dir api which is to be implemented by the step_dir stepper drivers Add fault handling in drv84xx using a fault cb mechanism With the introduction of the step-dir api,different motion controllers to consume various step-dir drivers allowing for further upstream as well as downstream stepper motion controllers to be implemented Signed-off-by: Jilay Pandya <[email protected]>
e41a6df
to
1710929
Compare
|
This PR aims to facilitate implementation of stepper motion controllers as device drivers. Hence the low-level step-dir drivers now shall implement a step-dir-driver-api, effectively also signifying what the IC actually supports i.e enable, step-dir and in some cases fault detection
Notes:
This is an intermediate step. Final Goal is having two APIs as follows,
Teaser:
Side effects:
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.
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.