Closed mh-dm closed 1 day ago
I think the problems with MULTISTEPPING_LIMIT
and laser also extend to the accel and decel ramp LASER_POWER_TRAP
code blocks. In those blocks the laser power is increased by current_block->laser.trap_ramp_entry_incr
or decreased by current_block->laser.trap_ramp_exit_decr
for every call to block_phase_isr()
. These values should be multiplied by steps_per_isr
.
Another problem is that the laser power ramps are linear with respect to step count whereas the speed is proportional to the square root of the step count. This is because speed is increasing linearly with time but the earlier steps in the acceleration ramp take longer than the later steps and mutatis mutandis for deceleration.
Did a review pass on this one, I think it is Ok, Instead of having the laser trap calc'd based on it's own step tracking for nominal you are moving it to the assumed cruise (nominal) rate if else position. I do not see any negative impacts and there is one less compare to do. This is of course with the multi step adjustment :) +1 Tom!
I think the problems with
MULTISTEPPING_LIMIT
and laser also extend to the accel and decel rampLASER_POWER_TRAP
code blocks. In those blocks the laser power is increased bycurrent_block->laser.trap_ramp_entry_incr
or decreased bycurrent_block->laser.trap_ramp_exit_decr
for every call toblock_phase_isr()
. These values should be multiplied bysteps_per_isr
.
Pre-existing problem but a very good catch. I've forced an always on 4x multistepping with some local chages and got this capture:
I've implemented the suggested * steps_per_isr
and there's no more sudden jump in laser power:
Another problem is that the laser power ramps are linear with respect to step count whereas the speed is proportional to the square root of the step count. This is because speed is increasing linearly with time but the earlier steps in the acceleration ramp take longer than the later steps and mutatis mutandis for deceleration.
Good point. However, it's a more complex pre-existing issue so I'm not going to attempt a fix in this PR.
Good point. However, it's a more complex pre-existing issue so I'm not going to attempt a fix in this PR.
I support that. For reference, I think a solution that adds a multiple of interval
would work. But to do it properly would need good testing by someone with a laser cutter. In the meantime, it would be good to get the PR merged.
@mh-dm / @tombrazier does this change stand on its own, or does it need to be consumed at the same time as the other planner changes in flight?
It stands on its own.
However it will conflict with the much discussed variable rename. But that's easy to resolve as this PR simply removes the one reference to accelerate_until
.
If there are no more concerns, can this be merged to unblock #27013?
I mistakenly thought at least someone in this thread had bugfix merge privileges so looks like I need to call on @thinkyhead.
Move cruise
LASER_POWER_TRAP
code into stepper cruise block. Small cleanup that fixes some potential issues:events_to_do
is not 1 (like ifMULTISTEPPING_LIMIT
is not 1) then the laser cruise update may not get run at allLASER_POWER_TRAP
deceleration and cruise blocks run, which is not intentionalOnly affects code using
LASER_POWER_TRAP
.Lightly tested by hooking up a logic analyzer to the laser pwm pin. The ramp up/cruise/down look ok and quite similar as to before this change. Here's just an example of what I've looked at (snapshot of acceleration->cruise at 80% power):
This change comes as a result of discussions on #27013