-
Notifications
You must be signed in to change notification settings - Fork 7.6k
drivers: stepper: STM Timer Based Step-Dir Implementation #87800
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?
drivers: stepper: STM Timer Based Step-Dir Implementation #87800
Conversation
@jilaypandya @faxe1008 |
.sleep_pin = GPIO_DT_SPEC_INST_GET_OR(inst, sleep_gpios, {0}), \ | ||
.en_pin = GPIO_DT_SPEC_INST_GET_OR(inst, en_gpios, {0}), \ | ||
.m0_pin = GPIO_DT_SPEC_INST_GET(inst, m0_gpios), \ | ||
.m1_pin = GPIO_DT_SPEC_INST_GET(inst, m1_gpios), \ | ||
}; \ | ||
\ | ||
static struct drv8424_data drv8424_data_##inst = { \ | ||
.common = STEP_DIR_STEPPER_DT_INST_COMMON_DATA_INIT(inst), \ | ||
.common = STEP_DIR_STEPPER_DT_INST_STM_TIMER_DATA_INIT(inst), \ |
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.
The driver then supports only STM timer?
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.
Correct. That's its limitation. For different timers (nxp, nordic, ...) different drivers need to be developed. One of the goals of this step-dir implementation is to raise awareness that you can use stepper drivers this way and to create a blueprint for this class of step-dir implementations to show how they can be implemented.
Added an additional step-dir implementation using stm-timers. Signed-off-by: Jan Behrens <[email protected]>
Added, for demonstration purposes, the stm-timer based step-dir implementation to the drv8424 stepper driver.
This is done only for the Nucleo F767zi boards. Consequently all other drv8424 test are currently broken. This is not an issue, as this commit is intended to be droped before merge. Signed-off-by: Jan Behrens <[email protected]>
b2c43e1
to
deb09ab
Compare
Made a lot of small (and some larger changes). Primarily focused on bugfixes, but some new features like automatic timer prescaler calculation were introduced as well. I am at this point primarily interested in feedback on the algorithm itself. The advantage of this step-dir approach, vendor locked as it may be, is speed. On a Nucleo WB55RG the maximum step frequency with the counter timing-source is about 20 kHz, higher and the step signal becomes unstable. At this point the processor is basically servicing ISRs the whole time, which makes this very inefficient. With the st-timer approach, I was easily able to surpass the drv8424s maximum Frequency of 500 kHz, through that had its own issue, as described below. There is currently an issue at very high step frequencies. The stepper will take additional steps before stopping, taking 101 instead of 100 steps for example. The position is still tracked correctly however, returning a position of 101 in that example. The effect gets worse the higher the frequency and is caused by hardware and software delays between the step generator generating the last step and the timer being stopped. The starting threshold is board specific and on the Nucleo WB55RG lies somewhere between 60-80 kHz step frequency. The real world effect of this issue should however be fairly low, as the use of these high frequencies also usually also includes acceleration and deceleration (at this point in time at application level), at which point the frequency before stopping is low enough that this issue would not occur. And in addition, the position is still tracked correctly, allowing move_to to prevent cumulative errors. I also used the drv8424 api tests to test the step-dir implementation in addition to manual tests with the shell. I am removing the draft status in the hopes of increasing engagement, as I have so far gotten little feedback. |
|
The step/dir would be common for all stepper drivers which use the step/dir paradigm, this is completely common is it not? The step generation is vendor specific sure, but a stepper driver like the drv8424 surely does not care how the steps are signals are generated, just like a sensor would not care exactly how an I2C transaction is performed. I thought we had aligned on this with the implementation of the common step_dir and timing sources? I would way prefer we update the common step_dir "API" to support both gpio "bit banging" step dir generation and vendor specific step/dir generation, and then update the drivers to use this more flexible abstration :) Simple example: instead of having both
and
have
like we do for other abstract implementations :) |
(&step_work_timing_source_api)), \ | ||
} | ||
(&step_work_timing_source_api)), \ | ||
} |
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.
unrelated changes :)
struct counter_top_cfg cfg_count; | ||
uint32_t counter_gen_base_freq; | ||
|
||
#ifdef CONFIG_STEPPER_STEP_DIR_GENERATE_ISR_SAFE_EVENTS |
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 have been thinking about this since quite sometime, should we transfer this concern to the application code and reduce a bit of overhead on the driver side?
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.
Which concern exactly? If you mean that on the api side we always assume that the event callbacks are ISR safe, I am fine with that.
If I understood you correctly, that is my plan as well. It is just that this part is not ready yet. I was waiting on how the stepper-api rework went, as that would have solved that issue a different way. With that not coming for a while a different solution is needed and my idea goes along the same way as yours. But I simply hadn't had time to work on it yet. And in the meantime, I can can collect and implement feedback on the already existing stm-timer code, as the proposed step-dir changes would not lead to massive changes. |
ok, makes sense :) |
include: | ||
- name: pinctrl-device.yaml | ||
|
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.
include: | |
- name: pinctrl-device.yaml | |
include: pinctrl-device.yaml |
@@ -9,7 +9,8 @@ | |||
#include <zephyr/drivers/stepper.h> | |||
#include <zephyr/drivers/gpio.h> | |||
#include <zephyr/drivers/stepper/stepper_drv8424.h> | |||
#include "../step_dir/step_dir_stepper_common.h" | |||
// #include "../step_dir/step_dir_stepper_common.h" | |||
#include "../step_dir/step_dir_stepper_stm_timer.h" |
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 think no relative paths shall be used. Can't find any rules from zephyr side - but I've not seen any relative include paths so far.
Ping @adrienbruant, i think you also tried something similar in the same direction sometime back. #35778 (comment) Using an extended API seems to be the correct approach. |
Sorry I don't work on the project that got me involved with steppers anymore, so I haven't followed the latest developments, and my work was from before the stepper API so I can only give general feedback. As I understand it, this PR uses two chained timers in primary secondary mode. I had considered it but instead decided to use the repeat counter (
which saved one counter. The difference was that my stepper API only moved by full steps. An idea I had but never got around to implement was to precalculate acceleration profiles and use DMA to load the new value in the counter. I imagine this could be necessary at higher pulse rate. This would be in the scope of a motion planner but maybe it needs provisioning at the driver level to connect the various events. |
I considered the repeat counter as well, but I found 3 issues that prevented me from using it (through they might not have been relevant to your api):
I know that these things were not relevant for you, but I want others to know the reasoning for using 2 timers instead of one.
This is something I thought about as well, but I wanted to star without it, as DMA under Zephyr has its share of pitfalls and I wanted to do something simpler to start. That said, depending on how the project I am developing this for goes, DMA based acceleration might come later. EDIT: I did some further investigation into combining repetition counter and and one-pulse mode. It is possible to create a stepper driver from this, but it has its own set of limitations and pitfalls. It might be worthwhile to create such a driver, but in terms of capabilities it would have some serious differences from this driver. Still, I think there would be several use cases for it, but I don't plan to develop it myself. |
This PR includes a step-dir implementation using stm timers - making it vendor and board specific, but requiring neither the workqueue nor interrupts for the step signal generation.
Its goal is to showcase an example of how to implement something like this and to support an RFC about changes on how multiple step-dir implementations should be handled.