Skip to content
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

Move cruise LASER_POWER_TRAP code to cruise block. #27031

Merged
merged 2 commits into from
May 17, 2024

Conversation

mh-dm
Copy link
Contributor

@mh-dm mh-dm commented Apr 29, 2024

Move cruise LASER_POWER_TRAP code into stepper cruise block. Small cleanup that fixes some potential issues:

  • when events_to_do is not 1 (like if MULTISTEPPING_LIMIT is not 1) then the laser cruise update may not get run at all
  • when we go straight from acceleration into deceleration then both the LASER_POWER_TRAP deceleration and cruise blocks run, which is not intentional

Only 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):
laser-before

This change comes as a result of discussions on #27013

@tombrazier
Copy link
Contributor

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.

@descipher
Copy link
Contributor

descipher commented May 10, 2024

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!

@mh-dm
Copy link
Contributor Author

mh-dm commented May 10, 2024

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.

Pre-existing problem but a very good catch. I've forced an always on 4x multistepping with some local chages and got this capture:
laser_multi_broken

I've implemented the suggested * steps_per_isr and there's no more sudden jump in laser power:
laser_multi_after

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.

@tombrazier
Copy link
Contributor

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.

@sjasonsmith
Copy link
Contributor

@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?

@tombrazier
Copy link
Contributor

It stands on its own.

@tombrazier
Copy link
Contributor

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.

@mh-dm
Copy link
Contributor Author

mh-dm commented May 16, 2024

If there are no more concerns, can this be merged to unblock #27013?

@mh-dm
Copy link
Contributor Author

mh-dm commented May 16, 2024

I mistakenly thought at least someone in this thread had bugfix merge privileges so looks like I need to call on @thinkyhead.

@thinkyhead thinkyhead merged commit 383e6f4 into MarlinFirmware:bugfix-2.1.x May 17, 2024
62 checks passed
@mh-dm mh-dm deleted the laser-cruise branch May 17, 2024 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants