Add 'tool' module

[Cirromulus]

This PR adds a extras/tool module to control PWM-tools like lasers or spindles via M3-M5.
Also adds a feature of a safety timer and a default shutdown value to increase safety when handling with lasers.

As long as longer commands are not split into smaller segments, the "keep going"-message is not sent in time.
This is because register_lookahead_callback only appends the function to the last move.
I did not find a more suitable way to insert this resend callback.

The timeout is still usable, but needs to be configured to always be longer than the longest running command.
This shows especially with arc moves.

With adding M3-M5, this also increases the compatibility with existing CAM-tools.

This adds a step towards #133. The only missing link left would be a fast path for PWM-Messages, e.g. over the stepcompress system.

[KevinOConnor]

Interesting. As high-level feedback, I'm not really sure what this module does that's different from the output_pin.py module. Is there something that is done here, that couldn't be done with macros and SET_PIN?

If it's the max_duration stuff, could we add that to output_pin instead? Also, we should be able to implement the max_duration checks using reactor callbacks so that the user does not need to worry about limiting command duration.

Also, I think the module name "tool" is too generic. If we do need a new module I think we should come up with a more specific name.

Thanks,
-Kevin

[Cirromulus]

If it's the max_duration stuff, could we add that to output_pin instead?

I did not want to touch existing codebase, but this is probably the most "klipper way", so I agree.

Also, we should be able to implement the max_duration checks using reactor callbacks

Ok, then I need to dig deeper into the reactor callbacks. Until now, I only found a way to add a command to the queue, effectively waiting for longer commands.

If we do need a new module I think we should come up with a more specific name.

As you said, it is probably best SET_PIN gets the max_duration and shutdown_value, while implementing G3-5 simply in Macros.
Are one-liners anyway.

[Cirromulus]

we should be able to implement the max_duration checks using reactor callbacks so that the user does not need to worry about limiting command duration.

This is now implemented. The callback now calculates the exact needed time derived from last_print_time.
Updates are not sent during homing (drip mode) however. This should be OK, because there is no sense in having a laser/drill/... running during homing, anyway.

[Cirromulus]

Squashed commits.

[lenne0815]

Cirro you are an absolute beast, thank you so much for your work ! Could you tell me how it roughly performs ? I can do 200mm/s at 300 DPI with batched gcode via lightburn / cohesion, i guess this outperforms it by a rather large margin ?

[Cirromulus]

I did not test it to the limit.
I am currently lasering at .1mm resolution (~250DPI) and 30-40 mm/s.
This is mainly because I have a weak laser, but also the missing proportionality is starting to show up on high speeds. Makes up for nice effects sometimes, though.
The top switching speed will be board-dependent. I have only an ATMega2560 and my gantry shakes at 100mm/s already*, so I wont go higher. Laser switching was accurate, though.

*: I use my printer with a custom head-swapping mechanism. So it will be primarily a 3D-Printer that also can laser engrave, but not the other way around.

[lenne0815]

Thanks for the heads up ! My pure co2 laser can easily sustain 2k mms so im in for some speed testing :D Im gonna test it as soon as everything is merged and my pi arrived ! I believe this is a rather big deal for co2 machines because until now everyone was capped around 200 mms with the fastest non dsp controllers, dsp controllers only start from ~500 euros onward.

[lenne0815]

Just one more thought; for slower lasers it would be important to scale pwm according to the actual speed of the machine, could you implement that as well ?

[Cirromulus]

Just one more thought; for slower lasers it would be important to scale pwm according to the actual speed of the machine, could you implement that as well ?

We'll see that when this PR is merged.
The Roadmap for that is documented in #3338 (comment)
So right now, only my fork (not in this PR) is capable of raster engraving.
This needs a revamp which @KevinOConnor might do, but probably only after this PR .

[KevinOConnor]

Thanks. Unfortunately, it's not valid to call toolhead.register_lookahead_callback() without holding the gcode mutex. That is, it can't be called directly from a reactor callback. You probably want to do something like print_time = self.mcu_pwm.get_mcu().estimated_print_time(eventtime) instead - though the time will need to be padded by 100ms (to allow for transmission time to the mcu).

-Kevin

[Cirromulus]

Is it allowed to just set the pin directly, as it is now?
100ms time padding is now explicitly in the "relax_margin" which is supposed to give the scheduler some time.

[KevinOConnor]

The toolhead.get_last_move_time() and related toolhead.register_lookahead_callback() always return a time in the future, so that it is possible to schedule events at that time. A call to mcu.estimated_print_time(systime) returns the time of "now" on the micro-controller - if one were to schedule an event at that time, it would always result in a "timer too close" as it takes time to transmit that schedule to the micro-controller.

I'm not sure what the goal of the "relax_margin" is. You only need the PIN_MIN_TIME - any additional time is probably not a good thing as it limits how response future updates are.

-Kevin

[Cirromulus]

the "relax margin" was supposed to reduce stress between "now" and the scheduled time the pin has to update (reducing the possibility of too close timers).
But if you say that this just removes responsiveness, it can be replaced for just the PIN_MIN_TIME, instead of 10% timeout + PIN_MIN_TIME.

[KevinOConnor]

As an additional minor comment, could we change the "safety_timeout" parameter - maybe something like "host_verification_time"? I'm a little leery of implying that the setting will somehow make something "safe". Also, the parameter should have a reasonable minval - maybe .500 .

-Kevin

[Cirromulus]

That is no problem.
In the newest commit, it may be either zero or at least 1.25*PIN_MIN_TIME, so that the scheduling later (0.8 * safety_timeout - PIN_MIN_TIME) does not evaluate to the past.
I agree that reasonably values under 0.5s create unnecessary effort on the machine, so maybe the lower limit is just 0.5s .

[Cirromulus]

I suggest host_acknowledge_timeout, because verification sounds a bit ambiguous (to me, at least).

[Cirromulus]

@KevinOConnor
Do you have any more comments or requests?

[KevinOConnor]

Thanks. In general it looks fine to me, but I did notice some minor things.

-Kevin

[Cirromulus]

Is this mention ok for you, or shall this be kept out until the PR is actually started?

[KevinOConnor]

Thanks. I committed this along with a couple of minor changes on top (commit 861144d and d02c80e).

-Kevin