New pwm_tool module that supports mcu queing of pwm updates #6369
Conversation
There was a problem hiding this comment.
Overall, I like this MR.
It is concise in the stepcompress system and adds the pwm_tool as a klipper-extra which is nice (although that adds some redundancy to output_pin.py).
Real-world tests are still outstanding, but might get to this by this or the following weekend.
klippy/toolhead.py
Outdated
| m.flush_moves(mq_time) | ||
| else: | ||
| # Move queue will get flushed via regular toolhead flushing | ||
| self.last_kin_move_time = max(self.last_kin_move_time, mq_time) |
There was a problem hiding this comment.
Might use note_kinematic_activity(mq_time)
| ffi_main, ffi_lib = chelper.get_ffi() | ||
| self._stepqueue = ffi_main.gc(ffi_lib.stepcompress_alloc(self._oid), | ||
| ffi_lib.stepcompress_free) | ||
| self._mcu.register_stepqueue(self._stepqueue) |
There was a problem hiding this comment.
Minor cosmetic: It is not really a step-queue.
In object-oriented C++ I would have suggested a general buffer management queue (e.g. command_queue), which the stepcompress system would inherit the queue from and add the compression part (i.e. step-dir-filter etc.)
Perhaps I am thinking too general, but the reason for this is that this might be a possible location for a hypothetical proportionality feature (i.e. S-Parameter in G0 in RepRap documentation).
After a stepcompress-flush, a toolhead might insert pwm_tool messages into the queue according to actual speed / requested speed and a gamma-curve.
But we might "cross the bridge once we get there".
There was a problem hiding this comment.
Earlier this year I spent some time reworking the trapq, stepcompress, and steppersync code. It's difficult to get all that code correct however, so I haven't gotten it to the point that it is ready to merge. Largely, the pwm_tool code is largely orthogonal, so likely best not to introduce additional dependencies to it.
| SET_PIN PIN=test_pwm_tool VALUE=0.5 | ||
| SET_PIN PIN=test_pwm_tool VALUE=0.5 | ||
| SET_PIN PIN=test_pwm_tool VALUE=0.25 | ||
| SET_PIN PIN=test_pwm_tool VALUE=1 |
There was a problem hiding this comment.
Other things worthy to test:
maximum_mcu_durationby activating the pin to some non-default value and starting a toolhead move that will take longer than this duration.- small rasterized image with some speed and reasonable resolution to test a "real" world example (perhaps the Klipper3D logo)
If wanted, I can provide these tests as I used something similar (but manually).
klippy/toolhead.py
Outdated
| @@ -543,6 +543,14 @@ def register_lookahead_callback(self, callback): | |||
| last_move.timing_callbacks.append(callback) | |||
| def note_kinematic_activity(self, kin_time): | |||
| self.last_kin_move_time = max(self.last_kin_move_time, kin_time) | |||
| def note_move_queue_activity(self, mq_time): | |||
| if self.special_queuing_state == "Flushed": | |||
There was a problem hiding this comment.
During homing and force move, the special_queueing_state is set to "Drip".
While it is not recommended to have some pwm-tool in its non-default state during homing, it might still happen (e.g. queued GCODE).
If it is not flushed in this mode, it will still have an active PWM-Tool during homing, but will fail either because of maximum_mcu_duration or, after homing, due to the delayed schedule (timer_too_close).
At least, it happened to me with only update_move_time. I am not sure whether updating the last_kin_move_time in "Drip"-mode will lead to updates, because _update_move_time(kin_time) did not.
KevinOConnor
force-pushed
the
work-pwmtool-20231013
branch
from
ed8f2f4 to
bf07692
Compare
|
Thanks for the detailed review. I made some changes and rebased this branch. Upon further thought, I'm not confident that the The Klipper code attempts to plan out stepper movement two seconds in advance of when it is needed. That helps ensure that the micro-controllers have the necessary movement information fully buffered so that it can maintain precise timing. As part of this, the host prioritizes the filling of the move_queue and the serial bandwidth up to two seconds into the future. Unfortunately, the There is also a related problem that an appended pwm update with a deadline just 100ms in the future does not necessarily wake up the host flushing mechanism in time. (This is likely the reason you saw errors when in "Drip" mode.) I'm not sure what a good way to address this issue is. Some possibilities:
I'm not sure. -Kevin |
Currently, number 2 is most appealing to me. This, however, holds only if Klippy's fail-mode is fail silent (which I believe it is).
This is probably the reason for it. I had a hunch, and thought of making a version of your suggestion Nr. 3, but I did not finish this idea because it felt hacky and added a lot of special-case code. |
KevinOConnor
force-pushed
the
work-pwmtool-20231013
branch
from
bf07692 to
e6a75ed
Compare
|
FYI, I rebased this branch again.
It's possible, but the issue with that approach is that it likely requires mcu code changes. That's a pain to deploy and may risk regressions. A user could probably "hack" this today by declaring a dummy pin (or led or something innocuous) as an output_pin with its maximum_mcu_duration. Then as long as the user commanded that pin on during print start and off during print end, then they should get general host/mcu disconnect protection. In general, I'd probably prefer approach 3. It's more work and likely requires some notable refactoring, but seems like less of a hack. In any case, I suspect only option 1 is practical if we want to merge prior to the next Klipper release. https://klipper.discourse.group/t/upcoming-v0-12-0-release/10948
I'm not sure what you mean by "fail silent". -Kevin |
This is correct. With this big of a user basis, I see the pain of deployment. I still find a timeout / keepalive system more elegant than having a separate "non-move" queue with a higher priority, but this discussion is probably more suited in the discourse development channel.
As my Howto suggests a separate enable-pin for power supply to the Laser / Tool, this would still fit well. The non-high-throughput pin can have the safety timeout, so that the PWM-pin does not need it. I suggest adding this to the
Fail silent means that if something fails in klipper host, it does not continue to send (possibly bogus) messages to the MCU, but does not send anything at all. |
KevinOConnor
force-pushed
the
work-pwmtool-20231013
branch
from
e6a75ed to
41dd3e6
Compare
|
Okay, thanks. I've rebased this branch to just include the basic It would help if others with actual pwm controlled tools can confirm that the PR here works on that hardware and provides some utility. -Kevin |
|
I will test it with my laser setup tomorrow. I still suggest adapting the example configuration to include the (regular) power supply pin: diff --git a/config/sample-pwm-tool.cfg b/config/sample-pwm-tool.cfg
index 48986061..e4cbd45e 100644
--- a/config/sample-pwm-tool.cfg
+++ b/config/sample-pwm-tool.cfg
@@ -3,46 +3,46 @@
# See docs/Using_PWM_Tools.md for a more detailed description.
[pwm_tool TOOL]
-pin: !ar9 # use your fan's pin number
+pin: !ar9 # use your pin number connected to PWM input
hardware_pwm: True
cycle_time: 0.001
shutdown_value: 0
-maximum_mcu_duration: 5
-# Default: 0 (disabled)
-# Amount of time in which the host has to acknowledge
-# a non-shutdown output value.
-# Suggested value is around 5 seconds.
-# Use a value that does not burn up your stock.
-# Please note that during homing, your tool
-# needs to be in default speed.
+
+[output_pin TOOL_ENABLE]
+pin: !ar10 # use the pin number connected to power supply of Tool
+shutdown_value: 0
+maximum_mcu_duration: 2
[gcode_macro M3]
gcode:
+ SET_PIN PIN=TOOL_ENABLE VALUE=1
{% set S = params.S|default(0.0)|float %}
SET_PIN PIN=TOOL VALUE={S / 255.0}
[gcode_macro M4]
gcode:
+ SET_PIN PIN=TOOL_ENABLE VALUE=1
{% set S = params.S|default(0.0)|float %}
SET_PIN PIN=TOOL VALUE={S / 255.0}
[gcode_macro M5]
gcode:
SET_PIN PIN=TOOL VALUE=0
+ SET_PIN PIN=TOOL_ENABLE VALUE=0
# Optional: LCD Menu Control
[menu __main __control __toolonoff]
type: input
-enable: {'pwm_tool TOOL' in printer}
-name: Fan: {'ON ' if menu.input else 'OFF'}
+enable: {'output_pin TOOL_ENABLE' in printer}
+name: Tool: {'ON ' if menu.input else 'OFF'}
input: {printer['output_pin TOOL_ENABLE'].value}
input_min: 0
input_max: 1
input_step: 1
gcode:
- M3 S{255 if menu.input else 0}
+ SET_PIN PIN=TOOL_ENABLE VALUE={1 if menu.input else 0}
[menu __main __control __toolspeed]
type: input
diff --git a/docs/Using_PWM_Tools.md b/docs/Using_PWM_Tools.md
index 108ae37a..98e9f91c 100644
--- a/docs/Using_PWM_Tools.md
+++ b/docs/Using_PWM_Tools.md
@@ -21,12 +21,14 @@ _fully on_ for the time it takes the MCU to start up again.
For good measure, it is recommended to _always_ wear appropriate
laser-goggles of the right wavelength if the laser is powered;
and to disconnect the laser when it is not needed.
-Also, you should configure a safety timeout,
+An additional safety layer would be powering your tool through the hotend-heater pin.
+In the configuration, the additional `[output_pin]` would be activated by `M3/M4` and disabled by `M5`.
+In this pin you should configure a safety timeout,
so that when your host or MCU encounters an error, the tool will stop.
For an example configuration, see [config/sample-pwm-tool.cfg](/config/sample-pwm-tool.cfg).
-## Commands
+## Example Commands
`M3/M4 S<value>` : Set PWM duty-cycle. Values between 0 and 255.
`M5` : Stop PWM output to shutdown value.
@@ -36,26 +38,29 @@ For an example configuration, see [config/sample-pwm-tool.cfg](/config/sample-pw
If you use Laserweb, a working configuration would be:
GCODE START:
- M5 ; Disable Laser
G21 ; Set units to mm
G90 ; Absolute positioning
G0 Z0 F7000 ; Set Non-Cutting speed
GCODE END:
M5 ; Disable Laser
- G91 ; relative
- G0 Z+20 F4000 ;
- G90 ; absolute
GCODE HOMING:
M5 ; Disable Laser
G28 ; Home all axis
TOOL ON:
- M3 $INTENSITY
+ M3
TOOL OFF:
- M5 ; Disable Laser
+ M3 S0 ; No output, but powered
LASER INTENSITY:
- S
+ M3 S
+
+ PWM MIN S VALUE:
+ 0
+
+ PWM MAX S VALUE:
+ 255 |
|
Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html There are some steps that you can take now:
Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available. Best regards, PS: I'm just an automated script, not a human being. |
|
Hi @Cirromulus and @KevinOConnor, First of all thanks for your work on that upgrade! I've been trying to use it with my setup, but I'm having some issue Internal error on command:"G1" when trying to run Gcode from laserburn or snapmaker Below 2 klippy logs taken right after the crash happens, if it helps to review the feature. I have a laser and can try to run some test if you want, to review the feature (not only related to my issue) so it can eventually be integrated with the main branch. I can't get the laser working right now, but the printing setup works fine. I've created 2 config file, printer.cfg and laser.cfg, so I deactivate the laser config when printing normally. |
|
The relevant portion of your log is the following: This does not seem to be the clock value, but the "setvalue" |
|
Just tried with software PWM (set hardware_pwm = False), and I got the same issue I'm using Biqu CB1 instead of Rpi by the way, not sure it could have an effect? |
KevinOConnor
force-pushed
the
work-pwmtool-20231013
branch
from
41dd3e6 to
b2a9308
Compare
|
Thanks. That looks like a bug in the new pwm_tool.py code. I have updated the code and rebased this branch. Hopefully this issue is now fixed. -Kevin |
|
I have successfully tested the newest version (b2a9308) to work with my GT2560 Mainboard. |
|
looks like it works for me as well with the new code Thanks! |
KevinOConnor
force-pushed
the
work-pwmtool-20231013
branch
from
b2a9308 to
6a2f2ae
Compare
…e space Signed-off-by: Kevin O'Connor <[email protected]>
The output_pin module is only capable of updating an output pin at most once every 100ms. Add a new pwm_tool module that is capable of queuing updates in the micro-controller and thus allowing for much higher update rates. Signed-off-by: Kevin O'Connor <[email protected]>
KevinOConnor
force-pushed
the
work-pwmtool-20231013
branch
from
6a2f2ae to
29b7550
Compare
|
Thanks. I committed this branch. @Cirromulus - if you'd like a different config example, feel free to send it along. I'd like to try to get to better "trapq flushing" in the next month or so, which should hopefully allow us to reintroduce the "maximum duration" check. -Kevin |
|
Hi everyone, I'm trying to set up a Melzi 1.1.4 board (8bit atmega1284p) to take advantage of this new feature. I don't really have the knowledge to understand where the problem is, so if there's need for any other information or test I will try to help as soon as possible. Thank you for this awesome firmware and for this feature |
|
This is happening a good bit with my laser as well when sending commands manually, but it works great in the context of a fully printed gcode file. I'm not sure if the console commands are queued differently or this is an expected part of the max duration issue mentioned in the previous post… |
|
@willpuckett thank you for your feedback, later today I'll try to run a job instead of manual commands and see what happens |
|
Hi everyone, As @willpuckett suggested (thank you!) running a gcode file do not give any error also for me.
Sorry for all the questions and thank you in advance |
This is a defect in the new pwm_tool code. Issuing a command when the printer is otherwise idle does not flush out updates. (In contrast, if the printer is in an "active" state, it should flush the updates.) This is similar to the problem identified earlier with maximum_mcu_duration. Unfortunately, there does not seem to be a simple fix to this issue. I'll have to think about a solution to this issue. -Kevin |
|
MKS Robin Nano Mini v3 is not working with Master, before i used the repo from Cirromulus and the same config has not worked. currently pwm.cfg, commented parts not included: [pwm_tool LASER] with Cirromulus, i declared it as output_pin and everything has worked as expected. I dont need enable the tool it get power from the mainline of print head. i have a TevoUp Hydra 2 in 1 |
In my branch, I check for this case (idle) based on the |
|
Hi everyone, Today I managed to make my laser module work. It turns out that the PWM input my module need is at 5V instead of 12V, so after setting a different pin the laser started to work. Thanks to you all, if you need to run some tests count me in |
Alas, calling -Kevin EDIT: To be clear, your branch also calls |
|
I believe commit #6410 should fix the "Timer too close" errors reported here. -Kevin |
|
FYI, I have created a new development branch at https://github.com/KevinOConnor/klipper-dev/tree/work-pwmflush-20231130 with initial support for That development branch is based on the code from #6410. It's tricky covering all the "corner cases" during flushing. However, I'm hopeful the above development branch is correct. If all goes well with #6410 then I'll create a PR for the above development branch. -Kevin |
This is an alternative implementation for #4128. The goal is to support fine grained control of spindles, lasers, and similar tools that are controlled by PWM input. The current
output_pinmodule requires that each pin update have a minimum duration of 100ms, which is often not sufficient for these tools. This PR introduces a newpwm_toolmodule capable of queuing PWM updates in the micro-controller and is therefore capable of much higher update rates.This differs from #4128 in the implementation. I'm leery that some of the low-level changes in #4128 may introduce a regression. This PR makes only a few changes outside of the new pwm_tool.py code. That is, the implementation here is mostly isolated to the new module and is therefore unlikely to introduce a regression to any users not using this module.
Unfortunately, I do not have a good setup to test this module. Feedback is appreciated.
@Cirromulus - FYI.
-Kevin