resonance_tester: Added a new sweeping_vibrations resonance test method

[dmbutyugin]

This adds a new resonance test method that can help if a user has some mechanical problems with the printer. The new method is not enabled by default and can be activated via a config option.

Signed-off-by: Dmitry Butyugin dmbutyugin@google.com

[dmbutyugin]

For a discussion, see https://klipper.discourse.group/t/a-bit-different-resonance-test/17227

Notably, this PR also adds a fix to the toolhead junction calculation between the moves. The problem can be demonstrated on the following GCode sequence:

G0 X0 Y0 Z10 F6000
SET_VELOCITY_LIMIT ACCEL=3000
G0 X50 F6000
SET_VELOCITY_LIMIT ACCEL=0.1
G0 X100 F6000
SET_VELOCITY_LIMIT ACCEL=3000
G0 X150 F6000

The expectation: the toolhead moves along a straight line from X=0,Y=0 to X=150,Y=0 with regular acceleration and deceleration of 3000 mm/sec^2 at X=0 and X=150 with no slow-downs in the middle. In practice it slows down around the move from X=50 to X=100. While in normal circumstances this behavior cannot be reasonably triggered, the new test actually is susceptible to this issue.

[KevinOConnor]

Interesting, thanks.

I'm not sure I understand the changes to toolhead.py. Can you elaborate on what they do and why they are needed? As a general observation, it seems a little odd to alter the main kinematics code to facilitate an infrequently run test case.

For what it is worth, if we think the existing test case had some errors (cases where it was prone to reporting incorrect values), and if we think the new test case is an improvement (more likely to report accurate results), then at first glance it would seem like we should replace the existing test with the new test. That is, document that the test has changed, set it as the default, and document that the old test is deprecated. I may be missing something though.

Thanks again, -Kevin

[Sineos]

I have tested the new test method and it works well for me. I still think the old test has its merits because it allows you to check for binding axes. From a pure resonance testing perspective, this might be considered a shortcoming, but for overall printer quality, I find it immensely useful.

[dmbutyugin]

@KevinOConnor

On the toolhead changes: there are two changes actually. One is commit 466af6624b954ea1a80cffc3210b745517bf94c1 which fixes what I consider a bug in the junction calculation. It's just that nobody complained about it because regular users are not likely to hit that bug. But as I wrote in the comment, the following GCode

G0 X0 Y0 Z10 F6000
SET_VELOCITY_LIMIT ACCEL=3000 MINIMUM_CRUISE_RATIO=0.0
G0 X50 F6000
SET_VELOCITY_LIMIT ACCEL=0.1
G0 X100 F6000
SET_VELOCITY_LIMIT ACCEL=3000 MINIMUM_CRUISE_RATIO=0.5
G0 X150 F6000
G4 P2000

at the current Klipper mainline head eeb2678ec2faa72af57444657b1b4f2589b77d65 generates the following moves (obtained with motan): and with just 466af6624b954ea1a80cffc3210b745517bf94c1 cherry-picked on top we get: I hope you'd agree that the current mainline behavior is technically incorrect and, while it is not a critical bug, there is no reason not to fix it. I noticed this behavior some time ago, but only sending a fix now when I actually need it. But it can totally be committed separately from this PR.

Then, on the test itself. FWIW, my personal opinion is that the new test is better in every aspect that the previous test. So my intention is to eventually replace the existing test with the new one. However, the responses from people were not very numerous, like maybe a dozen people tested it besides me, and many of them had issues with the previous method, so for them it was a strict improvement. But I think we have not enough evidence of using the new test by people who did not have issues with the previous method either. And by integrating the new test into the mainline Klipper I hope that the barrier to the new test adoption will considerably reduce (just adding a new config option vs installing a custom branch or copying a few files over existing installation). I'm open to feedback to change the default choice however to be the new test method, but my current plan is to first add the new test as a new option, then change the default choice to the new test, then eventually remove the old test altogether.

On the implementation side of things, the test performs a convolution of a sweeping motion with low-amplitude acceleration with the regular test as it was before, so it looks like this:

Using this combined acceleration profile helps making sure that the toolhead is not moving around with more or less constant velocity, which would be prone to hitting resonance frequencies of the motor and such. However, it complicates the motion control, as the regular Klipper move control operates on positions, and making sure that the velocity and acceleration profiles are as expected is no longer very trivial. Hence you can see that I introduced a new option to the toolhead to enqueue a move with a given max_junction_v2 upper limit, which is a cleaner and more reliable solution than fiddling around with 'fake' tiny moves at the Klipper computation tolerance. So while max_junction_v2 is not used anywhere else besides resonance tester code, I hope this is an acceptable addition, considering the alternatives.

@Sineos I get it that the current test may occasionally be useful for detecting problems with mechanics. However, I think it is not particularly reliable in that regard, and so I'm unsure whether it is worth keeping around. FWIW, binding axis (or rather, an axis with high stiction) is not always worth fixing. For example, I think it was a frequent occurrence on Creality K1 (or maybe another similar printer), that X axis would be a bit difficult to move due to rod bearing being very tight on tolerances, affecting the resonance test. But doing a bit of grinding/milling just introduces play and therefore people should not be doing that, even though it is improving the old resonance test results.

[Sineos]

However, I think it is not particularly reliable in that regard, and so I'm unsure whether it is worth keeping around.

I have already troubleshooted 3 printers that would show this effect (one being my own) and seen 3 to 4 reports in various places on the www. Granted, all were linear rails and I do not know how general this may be. From my experience, it is an added value and even if it is inherent in the design (K1), it is still something worth checking. Maybe a compromise would be a setting to turn the superimposed movement on/off (default to on)? Might even add value as it would allow direct comparison.

[dmbutyugin]

Maybe a compromise would be a setting to turn the superimposed movement on/off (default to on)?

Maybe when deprecating the old method I can make the new one handle the case of motion_period equal to 0, which would give the old behavior.

[dmbutyugin]

Actually, I decided to add sweeping_period: 0 support right away. As such, @KevinOConnor if you prefer to switch to the new method right away with this PR, I can do that and just add a notice to Config_Changes.md that the test method has been updated and to restore the old behavior, if absolutely necessary, the users can add sweeping_period: 0 option to [resonance_tester] section.

[KevinOConnor]

Thanks for the additional information. Sorry for the delay in responding. In general it looks good to me.

I hope you'd agree that the current mainline behavior is technically incorrect and, while it is not a critical bug, there is no reason not to fix it.

Makes sense.

Hence you can see that I introduced a new option to the toolhead to enqueue a move with a given max_junction_v2 upper limit, which is a cleaner and more reliable solution than fiddling around with 'fake' tiny moves at the Klipper computation tolerance.

I understand. I'm not against it, but I'd like to take a few days to think if there might be an alternate implementation that avoids altering toolhead.move().

On the implementation side of things, the test performs a convolution of a sweeping motion with low-amplitude acceleration with the regular test as it was before

Nice!

Actually, I decided to add sweeping_period: 0 support right away. As such, @KevinOConnor if you prefer to switch to the new method right away with this PR, I can do that and just add a notice to Config_Changes.md that the test method has been updated and to restore the old behavior,

I don't have a strong preference. I suspect you wont get many more testers until you actually change the default though. (Which is fine if you want to do this in a stages.)

FWIW, though, seems odd to introduce a new method: vibrations config setting just to deprecate it later.

Cheers, -Kevin

[dmbutyugin]

I understand. I'm not against it, but I'd like to take a few days to think if there might be an alternate implementation that avoids altering toolhead.move().

Sure. Please let me know when you decide on something.

I don't have a strong preference. I suspect you wont get many more testers until you actually change the default though. (Which is fine if you want to do this in a stages.)

FWIW, though, seems odd to introduce a new method: vibrations config setting just to deprecate it later.

OK, fair enough. Given that the user will be able to revert back to the old behavior using sweeping_period: 0, it might make sense to switch to the new method right away and avoid introducing config options that will be deprecated right away. And this may be a good opportunity to also change default values of the parameters - specifically accel_per_hz. The new test typically induces stronger vibrations (supposedly due to the fact that now it is easier for the test to excite the vibrations), and so I'm thinking about reducing this parameter from the current 75 mm/sec to 60 mm/sec to compensate for that.

So, if I don't hear any objections in the next few days, I'll go ahead with this plan: remove method option and make the new test the default, reduce default value for accel_per_hz to 60 mm/sec, update the documentation accordingly, and add a notice to docs/Config_Changes.md about the change of the default resonance test method and how the old one can be restored.

[dmbutyugin]

Following the discussion here, I went ahead and modified the code to remove method configuration option and make the new test a default.

[KevinOConnor]

Thanks. In general it looks good. I have a few minor comments/suggestions:

1. Could we move "toolhead: Fixed junction deviation calculation for straight segments" to a separate PR? I'm concerned in may trade one unusual "corner case" for another one. Specifically, it'll close a loop-hole where we may pessimize speeds if acceleration changes midway through a straight line movement, but I fear it opens a loop-hole where a slicer could emit an arc with a series of infinitesimal moves such that each of those moves appears to be "close to straight" which then results in the arc proceeding with no slow-down at all. So, probably best to discuss and analyze the ramifications of this separate from the resonance testing work (it doesn't seem they are related).
2. Instead of changing toolhead.move() to include a new max_junction_v2 parameter, could we introduce a new toolhead.limit_junction_speed() function that finds the last move on the lookahead queue and calls its move.limit_junction()?
3. Are you looking for me to squash and merge this, or rebase and merge?

Thanks again, -Kevin

[dmbutyugin]

Thank you Kevin. Per your suggestion in (1), I opened a separate #6747. Once that hopefully gets merged, I'll do as you suggest in (2) and rebase this PR. Then I think we can squash-merge this PR as a single commit (re (3)).