Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Smoother motion by fixing calculating trapezoids and ISR stepping. #27013
Smoother motion by fixing calculating trapezoids and ISR stepping. #27013
Changes from 6 commits
549a4f4
1f3f416
b9c8e24
30ece3d
89d7e17
2fb1a2b
81686a3
4b41cd8
2519e73
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Well, the new naming did highlight this: I think it should be without the +1 but I'm not sure.
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.
We'll have to analyze the intent of this line and look at its history. Previously it wanted to match
accelerate_until - 1
which would be the second-to-last step of acceleration. Assuming that we want no change in behavior,and assuming that.accelerate_before
is now one more thanaccelerate_until
, the actual correction would bestep_events_completed + 2 == accelerate_before
Again, we need to look closer at the original intent of the line, because, for one thing,
step_events_completed
is not incremented by 1, but byevents_to_do
which may or may not always line up correctly to match the exact indexaccelerate_before - n
. Once we fully grok the intent of this line we can simply replace it with whatever best fits the updated code.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.
Let's see if @descipher has anything to add….
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.
Well it's been quite some time since my head was deep inside this code! "first block->accelerate_until step_events_completed" is the key element, since the first block starts at 0 we will have value 1 after the ISR does the count increment, thus the < accelerate_until naming usage which is not the same meaning as accelerate_before, it only looks that way when viewing it as less than accelerate_until at the moment of comparison. However this comparison moment is the actual accelerate_until PIT and not before.
In regards to the +1 == if I recall correctly the reason for + 1 stems from an odd step count and the PIT when the laser would need to be turned off or incr/decr . It worked out that the counts were shifted toward the tail of the trap so the counts were consistently short on the decel side of the trap. This can be viewed by turning on debug where it will give you the actual step counts. It becomes obvious with debug on and the simulator is your friend for that ;) The laser power will be inc'd and dec'd closer to ideal with the +1. Keep in mind its not uniform in a real world print or laser burn, but in tests from a dead start to full stop the ramps should be equal and that's when this shifted count behavior was first observed. If there is a change in the counts/shaping now it should be re-evaluated again.
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.
My worry is that with some moves it will never be that
step_events_completed + 1 == accelerate_before
during theblock_phase_isr
because AFAIK this code is not guaranteed to be executed after every step, but only when some new "block phase" is "needed." And, as mentioned,step_events_completed
is not incremented by 1, but byevents_to_do
.The best thing to do would be to test this in the simulator, not with laser enabled, but just with that comparison being done at that same point in the code to see how it matches up and whether we are in fact guaranteed to be able to scrutinize every step index. If we can, then the very last index of acceleration will be
accelerate_before - 1
and we can apply that correction.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.
As a reminder, it is possible to log signals in the simulator and export them to files that can be viewed in PulseView. This allows you to analyze stepper pulses without requiring a physical logic analyzer setup. I find that it provides a much faster feedback loop than working with actual hardware.
It won't work for hardware PWM signals like you would have for a laser, but for any software-timed pulses it works well.
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.
PR Sim compiling result:
Took a look at the simulation outputs and captured a logic vcd of (X|Y)_STEP_PIN it looks reasonable however there is still the count shifting towards the tail of the trap where we can observe the resulting faster decel step rate.
This is the X_STEP_PIN Y_STEP_PIN output point and the zoom is the X axis.
Here the start pulse interval is 3.187ms and the stop is 2.27ms. This is a dead stop to dead stop thus we should have equal sides on the trap. We have a significantly harder decel due to the shorter decel count calc.
@thinkyhead We can use ( + 1 >=) to ensure we set the laser power on a missed compare. The intent of this code is to set the power to the current_block planner calculated "Cruise" laser power value. Since this code only happens on the cruise else section it should have no serious negative impact with the addition of >. If we end up fixing the counts then the +1 can be removed. So why use == to save compute cycles, if we use > it would increase the compute load by unnecessarily reapplying the power 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.
Since there are
LASER_POWER_TRAP
specific issues I've created #27031 to address them. The issues addressed in a way that's independent of this change.Now to address how this change affects laser: I looked over how
LASER_POWER_TRAP
usesac/decelerate_steps
and (with the #27031 fixes applied too) the resulting ramp up/down is smoother than before. Here's a comparison at the end of a deceleration when the laser is going to 0% power.Before:
(just a note that my steppers are set to edge stepping)
After:
I've also looked at the acceleration->cruise transition. I'd say the difference is not significant / not above the noise in the pwm/logic analyzer capture: Last step 75.4 -> 80% versus 76.7% -> 80%.
@descipher You are right about the absolute ideal being equal intervals. And that even with this change the first step is slower than the last step, or stated another way: the first step is at exactly
initial_rate
but the last step isn't yet atfinal_rate
. Two answers:ac/deceleration_steps
computation has to work for all the following types of segments: full acceleration, full deceleration, acceleration->deceleration, acceleration->cruise->deceleration (5 cases). And say we do that, and get the first step at exactlyinitial_rate
and the last at exactlyfinal_rate
(=initial_rate
of next segment). Now the full acceleration->full acceleration and the full deceleration->full deceleration transitions become jerky because whenfinal_rate
andinitial_rate
of the next segment are equal that means there's no acceleration happening between segments, i.e. we go full accel, 0 accel, full accel. Special casing the 5 cases into 5 cases transitions is just too much. Also the float->discrete steps forces us to accept an error somewhere. To avoid this complexity explosion the way this code is structured is to accept an error of ~1/2 of the ideal acceleration. This half error manifests as not hitting thefinal_rate
exactly.