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

sync channel for fast move_queue population #4128

Closed
wants to merge 4 commits into from

Conversation

Cirromulus
Copy link
Contributor

@Cirromulus Cirromulus commented Mar 31, 2021

This PR adds the possibility to update SW/HW PWM faster than the original implementation (0.100s).
This would close #133, enabling laser raster engraving.
While it will probably not reach the stepper step-times (which are compressed), I suppose it may come close to some multiples of cycle_time in bursts, depending on the serial bandwidth to the MCU.
Early testers report around 400-800 updates per second, peaks higher (as the queue is buffered).

Known Issues

  • Code seems to work on some different hardware setups. More testers appreciated!

If you want to try this out, you may follow this small HowTo.

How it works

This PR adds a "sync_channel" (name may be subject to change) to the steppersync/stepcompress system.
Basically, it allows a command to be inserted into the mcu's move_queue while respecting the watermark and timing.
This enables much tighter packing of pin-update commands.
To ensure the steppersync queues are flushed regularly even when the toolhead is not generating any movement,
a combination of note_kinematic_activity(), update_move_time() and check_stall() is used.

Other Notes

PWM Frequency: I suggest to not mean too well with the tool frequency. Choose the lowest frequency that still has a pulse time of a small multiple of your highest resolution. E.g. Resolution of 0.1mm at a speed of 50mm/s I recommend 1kHz (cycle_time: 0.001)

Hardware-PWM may be less calculation intense on the MCU, but the MCU will choose in "best effort" a frequency that is near the desired frequency.
The maximum pin update-rate is this chosen frequency.
Software-PWM will be less precise, but may offer more flexibility at odd frequencies.
So if you plan to have an update rate of 1000 updates / s, choose at least a frequency of 1kHz, thus a cycle_time of 0.001.

Unaddressed features, Roadmap

  1. This PR wraps the "ht-mode" (high throughput) at the command wrapper, reducing the impact on MCU_pwm code.
    In the state of this PR, one pin can be put in ht-mode. In long term (another PR), I plan to rework the stepcompress system, dividing the two functionalities (1. compressing steps, 2. making sure the move queue is populated and ordered), so that the PWM updates don't need to "piggy back" on kinematics-stepper update flushes. This would enable even faster pinupdates and, most importantly, any number of ht-mode pins.

  2. This PR only adds the possibility to update a pin much more frequently.
    For laser applications, a proportionality feature is useful. This would dim the laser during acceleration and deceleration phase proportional to its actual speed (see also S-Parameter in G0 in RepRap documentation), just like it does with the extruder E command.

@Arti4ever
Copy link

Hi @Cirromulus,
I am very interested in using laser engraver with Klipper, so I gave a try to your PR.
Unfortunately, your code do not work for me, Klipper crash when executing homing with error : " Exception in flush_handler".
Logs for this error : klippy_G28.log

I also tested just using M3 S5 command, but there is also an error : "MCU 'mcu' shutdown: Timer too close"
Logs for this error : klippy_M3.log

I hope this may help you to fix theses errors. Or maybe advise me if I am doing something wrong ?

klippy/mcu.py Outdated
self._set_cmd.send([self._oid, clock, on_ticks],
minclock=minclock, reqclock=clock)
# queue_pwm_out oid=%c clock=%u value=%hu
data = (self._set_cmd, self._oid, clock & 0xFFFFFFFF, v)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be wrong on non-8-bit boards

@Cirromulus
Copy link
Contributor Author

Thanks for testing!
I will look into that soon.
You are using a 32bit board, do you?
I never tested it with one, so maybe the manual cap on the scheduled time is wrong (mcu.py)
Just for kicks, does it behave the same when using hardware_pwm: true ?

@Arti4ever
Copy link

Arti4ever commented Apr 10, 2021

Yes I am using a 32bits board.
Using hardware pwm do not work for me. It raise a config error.

@Arti4ever
Copy link

I finally tried with hardware PWM (support for LPC176x was added 2 weeks ago, so my mcu was not up to date, that's why it did not work at first...), but I have the same behavior.

@KevinOConnor
Copy link
Collaborator

KevinOConnor commented Apr 14, 2021

Interesting. I agree this functionality would be good to add to Klipper.

At a high-level, the code doesn't look correct to me - at a minimum, the set_pwm() command has to be able to work from a background thread, which means it isn't valid for that function to call the toolhead code. (The heater code will change PWM settings directly from the background thread used to process temperature reports.) Alas, I haven't had time to look into this, so I don't have any implementation advice.

-Kevin

@Cirromulus
Copy link
Contributor Author

Hi Kevin, thanks for your first feedback.
I will look into that (I believe I initially produced the same condition in the pin safety timeout PR).
So basically, it is not safe to call ....send([self._oid, clock, v], minclock=minclock, reqclock=clock) inside the GCode-code?
I suspect implementing a similar style as the command_queue would be more elegant and probably allow for concurrency.

@KevinOConnor
Copy link
Collaborator

So basically, it is not safe to call ....send([self._oid, clock, v], minclock=minclock, reqclock=clock)

I'm not sure about the above. My comments were referring to the self._th.note_synchronous_command(print_time) call from set_pwm() - it's not valid to call toolhead methods from a background thread.

-Kevin

klippy/mcu.py Outdated Show resolved Hide resolved
@Cirromulus
Copy link
Contributor Author

@Arti4ever If you want to try it again, it may be better now.
Toolhead code is now called by the reactor.

@TazerReloaded
What PWM frequency do you use? The Code limits a maximum update frequency to the pwm frequency.

@TazerReloaded
Copy link

I wrote this config:

[output_pin intensity]
pin: P2.0
pwm: true
cycle_time: 0.001
scale: 255
value: 0
shutdown_value: 0
[gcode_macro M3]
default_parameter_S: 255
gcode:
  SET_PIN PIN=intensity VALUE={S}
[gcode_macro M5]
gcode:
  SET_PIN PIN=intensity VALUE=0

So currently I'm using 1kHz software PWM i guess, which is a bit slow considering 50mm/s at a resolution of 0.1 already means 500 pin updates per second. How fast can/should I go with software PWM?
I also tried hardware_pwm: True, but that did not change the frequency (checked with scope), despite having the newest (as of yesterday) firmware built from your branch on the SKR. Do I have to specify a frequency for hardware PWM to work? Pin 2.0 seems to be PWM capable according to the chip manual.
I also noticed that my laser seems to respond to low analog voltages. I have a 470 Ohm resistor on the PWM line to protect the MCU from over current (already killed one with a short to VCC due to bad wiring), which means the pull-up on the laser board always pulls the PWM line to around 0.2V when the duty cycle is 0, and that makes the laser shimmer a bit. I use that like a feature because that low intensity can be used to align the laser with the zero point, and when not in use the 12V power to the laser is cut via a relay. I don't think it affects the PWM signal.

I also pulled your latest changes, will continue testing next weekend. Awesome work so far, already works great, thank you!

@Arti4ever
Copy link

It but it still doesn't work (for me at least) when I try do to a homing. the error is not the same though : " Failed to home x: Timeout during homing"
Same config on the master branch works great.
Here my log : klippy.log

@TazerReloaded
Copy link

Also noticed an issue while trying to print today: Whenever I enable a heater, Klipper shuts down with a "Timer too close" error after a few seconds. Same config on master works fine. Klippy log attached.
klippy.log

@Cirromulus
Copy link
Contributor Author

How fast can/should I go with software PWM?

That depends on the computing power of your board. I suggest testing higher freqencies and measuring the jitter with a oscilloscope.

I also tried hardware_pwm: True, but that did not change the frequency

Then, the hardware-pwm was able to be configured to your desired frequency.
With cycle_time: 0.001 you specified a frequency of 1kHz (1/cycle_time). My board can't go faster, but yours might.

Whenever I enable a heater, Klipper shuts down with a "Timer too close" error after a few seconds.

I can reproduce that. I suspect that my way of toolhead.note_synchronous_command() is susceptible to concurrently flushing the move queues (in the past).
This is because I somehow rely on the move commands in the steppersync system to flush regurlarly.
This way, pwm-commands do not starve the stepper-queues. This introduces a lot of flushing and concurrency issues, however.
I will need some time to analyze the previously-used command_queues handled by the serial (slow) path.

@Cirromulus
Copy link
Contributor Author

@Arti4ever I see that you use a BL-Touch as well. Does it even stow?
I had this problem during development as well, but it was solved within the last commits.

I like the fact that you also can cut the power supply to the Laser.
But it is possible, that this breaks the setup (two different pin-updates immediately after each other... did not test it yet!)
Klipper will try to update these pins exactly at the same time, I suppose.

@TazerReloaded
Copy link

TazerReloaded commented Apr 22, 2021

The relay for laser power is attached to the Pi and controlled via Moonraker power commands, so I don't think that affects Klipper in any way. I also control the printers 12/24V PSU and the enclosure lights that way. I usually leave the room before powering the laser, don't want to take any risks, and if anything goes wrong, I can cut the power to everything remotely.
I can't think of an use case where pin changes have to happen exactly simultaneously, maybe if someone builds a needle printer running Klipper, but from an user perspective I'd expect it to be possible to control multiple pins without issues.

@TazerReloaded
Copy link

I tested the output frequency today, and on my hardware (SKR 1.4), I get a nice clean 10.8kHz square wave with the following settings:

hardware_pwm: true
cycle_time: 0.0001

It is a little bit faster than the expected frequency, but that could be caused by the hardware PWM clock scaling. Edges look very clean and even values like 0.005 (0.5%) are represented accurately.
I also installed a new laser, which responds very precisely to those PWM values. I'm cutting 4mm plywood, so no high speeds, but stock Klipper is still way too slow, leaving ugly marks when moving the laser before shutting it off. Your PR on the other hand works perfectly, no timing issues so far. If it wouldn't render heaters unusable at the moment I would already use it in my everyday firmware.

@Cirromulus
Copy link
Contributor Author

Nice hearing that!
Unfortunately, getting the heaters to work requires partly a new concept.
Currently no free time for that milestone 🐱‍🐉

At least, one can check out the different branch, restart klipper and go on lasing.
Before pwm_queue was not implemented in the mcu firmware, one would have to re-flash the printer with every switch 🤕

@TazerReloaded
Copy link

Unfortunately, the time of re-flashing is coming back sooner or later. The master branch has already altered the communication protocol, and introduced a few fixes for my setup. I flashed a DFU loader onto my board, so at least I don't have to fiddle the memory card out of the electronics enclosure every time. According to GitHub here are no conflicts so far, so a patch would be possible, but I expect it to be only a matter of a few weeks until conflicts arise.
I'd like to help, but I'm also short on free time, and I doubt that I can come up with a better solution without investing at least a few days into understanding the inner mechanisms of Klipper.
Anyways, thank you for all the effort so far, it has some edges, but it's a working solution!

@Cirromulus
Copy link
Contributor Author

According to GitHub here are no conflicts so far, so a patch would be possible, but I expect it to be only a matter of a few weeks until conflicts arise.

You are correct.

I doubt that I can come up with a better solution without investing at least a few days into understanding the inner mechanisms of Klipper.

That is my problem right now. I need a free weekend to uncouple the sync_channel from the move queues.
Right now, if any pin-update flushes the move queue (or the mq itself), a different pin (e.g. from a background task) will queue its update, but will not be flushed till the next round, which may or may not be too late for the update to happen.

@Alexdad76
Copy link

"Timer too close" error after a few seconds after the command M3 (1-255) . Same config on master works fine. Klippy log attached.
SKR 1.1
klippy.log

@Alexdad76
Copy link

According to GitHub here are no conflicts so far, so a patch would be possible, but I expect it to be only a matter of a few weeks until conflicts arise.

You are correct.

I doubt that I can come up with a better solution without investing at least a few days into understanding the inner mechanisms of Klipper.

That is my problem right now. I need a free weekend to uncouple the sync_channel from the move queues.
Right now, if any pin-update flushes the move queue (or the mq itself), a different pin (e.g. from a background task) will queue its update, but will not be flushed till the next round, which may or may not be too late for the update to happen.

I want to clarify. Does you need more information? #4128 (comment)

@Cirromulus
Copy link
Contributor Author

Your log points to an error in the message flush system. This relates to the strong coupling between stepper moves and the pin output.

Line 870: Sent 99 5414.972272 5414.972272 14: seq: 10, queue_pwm_out oid=4 clock=1735086958 value=100
Line 971: Receive: 99 5414.973191 5414.972272 12: seq: 11, shutdown clock=1745260113 static_string_id=Timer too close

This indicates that the second queue_pwm_out was sent out far too late (10.173.155 clock cycles, or around 0,1 seconds).
Do you have maximum_mcu_duration turned on? If configured, I suggest either turning it off or increasing the value for testing purposes.

@Alexdad76
Copy link

maximum_mcu_duration in config turned on:
maximum_mcu_duration = 5

When trying to turn off an error occurs:
maximum_mcu_duration = 0
Error: Option 'maximum_mcu_duration' in section 'output_pin TOOL' must have minimum of 0.5

When trying to increase more than 5, an error occurs
maximum_mcu_duration = 15
Error: Option 'maximum_mcu_duration' in section 'output_pin TOOL' must have maximum of 5.0

@Cirromulus
Copy link
Contributor Author

Yeah, this one is a bit tricky. To turn it off, you have to delete the line. Then it will default to zero.

@Alexdad76
Copy link

Yeah, this one is a bit tricky. To turn it off, you have to delete the line. Then it will default to zero.

HOORAY! Klipper stopped shutdown! The idle passage without a laser was successful. The LED on the output of the fan worked correctly.I will try real burning. Thank you so much for the hint!

@Cirromulus
Copy link
Contributor Author

No Problem. Once this system gets more stable, you should activate it again, though. For safety ;)

@Alexdad76
Copy link

Alexdad76 commented May 15, 2021

No Problem. Once this system gets more stable, you should activate it again, though. For safety ;)

Everything works fine:
Test80-80
Laser: NEJE Module 7W
Software: https://laserweb.yurl.ch/
Mode: "Laser Raster"
Laser Diameter: 0,1
Speed: 80 mm/s, Power: 0-80%

  1. Heard strange knocks over each square.
  2. There are some artifacts - lines about 6 and 7 - the laser was not turned off (maybe the G-code bug).

By the way, LaserWeb does not create a G-code if in the section Settings\GCODE"TOOL ON", use M3$Intensity. It works only simply M3

Update:
Strange, now the code is generated and with M3 $INTENSITY

Just in case I attach klippy.log
klippy80_80.log

@Cirromulus
Copy link
Contributor Author

@bk227865

I cannot change the CYCLE_TIME via the SET_PIN command, even for non high throughput pins. Trying on a Ender 3 S1. If i select a different CYCLE_TIME then the command seems to lock up and i have to reset to stop the PWM. Further SET_PIN commands do nothing anymore, even those without the CYCLE_TIME component. I recompiled the firmware to be on the same level as the PR. klippy.log moonraker.log

Hi, I could reproduce your problem; it had something to do with the wrong calculation of the guard time between beep and cycle_time change.
Could you test it again?
By the way: cycle_time changes are not queued, this means that you can't change the frequency as fast as the actual value (even in high throughput mode).
So in your beep-macros it will default to the "normal" operation.

@KevinOConnor
Copy link
Collaborator

FYI, I suspect the fixes in #5975 will improve the results of this code.

-Kevin

@Cirromulus
Copy link
Contributor Author

Thanks, I will rebase and test it on my system. Perhaps it has something to do with this issue where a user gets not-flushed elements after a CANCEL_PRINT and a timer-too-close when it moves again.

@Cirromulus
Copy link
Contributor Author

Rebased. Users report that this indeed fixes the CANCEL_PRINT issue.

@Rneil-ca
Copy link

Rneil-ca commented Feb 9, 2023

Hi Cirromulus,

Just wanted to say thank you for creating this fork. I have it successfully working on a skr v1.4 board with a Chinese laser with a 5v pwm input. Just got it working tonight and so far it is working great. Found your fork when I wasn't able to get my laser to burn properly with the standard version of Klipper.

Thanks again for all the work you have put into this.

Ryan

@Rneil-ca
Copy link

Any updates on this being merged into the main Klipper branch? I have been using this for months and it works great.

Thanks,
ryan

@naikymen
Copy link

Same here! It does require a bit of merging work though.

I merged the branch yesterday, but some functions have changed names in upstream Klipper since this PR.

I don't understand that part of the code, so It's a bit beyond my capability to update it for now.

@Cirromulus
Copy link
Contributor Author

@Rneil-ca nice to hear. Do you use it for both printing and lasing purposes?

@naikymen Do you need a specific feature of the klipper master branch? The fork works well for me, and I grow tired of the continuous rebasing.

@naikymen
Copy link

naikymen commented Aug 2, 2023

Hi @Cirromulus, thanks for the reply.

Yes but sort of... I need to use this fork, aimed at multipurpose CNC, which I keep in sync with upstream klipper.

I keep it synced because I like the fixes and new features across the complete stack (i.e. MR and Mainsail). I do not plan to open more PRs here, as they are a lot of work and hard to get merged (from my perspective).

I grow tired of the continuous rebasing.

Makes sense. I was still expecting that your work be merged to master, but if this is not the case then I'd humbly request one more rebase.

Some folks and I have been discussing klipper for more general CNC over here, and yours is the only branch implementing laser support that I'm aware of.

I'd gladly support the rebase with a small donation if any of this makes sense to you.

Thanks for your work!

@Rneil-ca
Copy link

Rneil-ca commented Aug 3, 2023

@Rneil-ca nice to hear. Do you use it for both printing and lasing purposes?

This machine does both CNC and Laser, I use normal Klipper on my 3d printers. So far this has been working great, so no need anything now. Would be nice if this was permanently merged into the main branch so we can stay up to date, but not sure if they are willing to do that.

Thanks again for all your work on this!

Ryan

@Cirromulus
Copy link
Contributor Author

Rebased against current master.

Some folks and I have been discussing klipper for more general CNC over here, and yours is the only branch implementing laser support that I'm aware of.

Also cool to hear. I followed the thread a bit. Hopefully someone contributes a 'XY-only' kinematic for the folks without Z axis.

I'd gladly support the rebase with a small donation if any of this makes sense to you.

Thank you very much, but I want to give back to the FOSS-community when I find the time :)

@Cirromulus
Copy link
Contributor Author

@Rneil-ca

This machine does both CNC and Laser, I use normal Klipper on my 3d printers.

The thing @KevinOConnor is most concerned about is that nothing breaks normal 3d printing operations.
So you could help us with using the fork on your actual printers as well, and then report back whether you encounter any problems or not (my preferred feedback 😄).

@ssendev
Copy link

ssendev commented Sep 7, 2023

It's been some time but when I tested it printing worked fine but it crashed KlipperScreen which was the reason i didn't leave it installed permanently.

@naikymen
Copy link

naikymen commented Sep 7, 2023

Also cool to hear. I followed the thread a bit. Hopefully someone contributes a 'XY-only' kinematic for the folks without Z axis.

I think this is working already, there is a config draft over here: https://gitlab.com/pipettin-bot/forks/firmware/klipper-stack/-/blob/pipetting/printer_data/config/cnc_shield_xy/printer.cfg#L19

We're making a new machine so I haven't had much time to test these features.

Thanks a lot for rebasing, I've merged the branch without conflicts. 🥳

Over here: https://github.com/naikymen/klipper-for-cnc/tree/develop-pwm_sync_channel-merge

@Rneil-ca
Copy link

Rneil-ca commented Sep 8, 2023

The thing @KevinOConnor is most concerned about is that nothing breaks normal 3d printing operations. So you could help us with using the fork on your actual printers as well, and then report back whether you encounter any problems or not (my preferred feedback 😄).

Thanks for the rebase, will install the latest on both of my printers and my CNC and will let you know if there are any issues.

Thanks again for all your work on this.

Ryan

@HWiese1980
Copy link

I've been running this change for about a year now on a Raspi Zero 2 and BTT Octopus without any issues. Printing works like a charm, as usual, just like using the laser engraving feature with a laser attached to a fan port. Never encountered any problems.

Please merge!

@KevinOConnor
Copy link
Collaborator

The functionality in this PR should now be on the main branch (with the merge of PRs #6369, #6410, #6420).

-Kevin

@KevinOConnor KevinOConnor added the resolved Issue is thought to now be fixed label Dec 16, 2023
Copy link

This ticket is being closed because the underlying issue is now thought to be resolved.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

@github-actions github-actions bot closed this Dec 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolved Issue is thought to now be fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SET_PIN not usable for laser engraving