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 [email protected]

[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 466af66 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 eeb2678 generates the following moves (obtained with motan):

and with just 466af66 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)).

[dmbutyugin]

@KevinOConnor,

While I'm at it, I tried to implement (2), and now I remembered why I did not do that in the first place. Now I tried to make it along these lines, and the code occasionally fails with the following error:

Internal error on command:"TEST_RESONANCES"
Traceback (most recent call last):
  File "/home/pi/klipper/klippy/gcode.py", line 211, in _process_commands
    handler(gcmd)
  File "/home/pi/klipper/klippy/gcode.py", line 137, in <lambda>
    func = lambda params: origfunc(self._get_extended_params(params))
  File "/home/pi/klipper/klippy/extras/resonance_tester.py", line 337, in cmd_TEST_RESONANCES
    data = self._run_test(
  File "/home/pi/klipper/klippy/extras/resonance_tester.py", line 265, in _run_test
    self.executor.run_test(test_seq, axis, gcmd)
  File "/home/pi/klipper/klippy/extras/resonance_tester.py", line 172, in run_test
    toolhead.move([nX, nY, Z, E], max(abs_v, abs_last_v))
  File "/home/pi/klipper/klippy/toolhead.py", line 477, in move
    self.lookahead.add_move(move)
  File "/home/pi/klipper/klippy/toolhead.py", line 191, in add_move
    self.flush(lazy=True)
  File "/home/pi/klipper/klippy/toolhead.py", line 180, in flush
    self.toolhead._process_moves(queue[:flush_count])
  File "/home/pi/klipper/klippy/toolhead.py", line 351, in _process_moves
    move.accel_t, move.cruise_t, move.decel_t,
AttributeError: 'Move' object has no attribute 'accel_t'
Transition to shutdown state: Internal error on command:"TEST_RESONANCES"

So, the problem at hand is that the following sequence does work as expected:

+                toolhead.move([nX, nY, Z, E], max(abs_v, abs_last_v))
+                toolhead.limit_last_junction_speed(abs_last_v)

My understanding is that the problem is that move(...) method of Toolhead calls

        self.lookahead.add_move(move)

which, first of all, calls calc_junction(...) for the newly enqueed move, and then, depending on the state of the queue, may issue a partial flush of the queue, which processes the queue based on junction limits and starting velocities computed so far. Then, subsequent call to limit_last_junction_speed(...) can mess up internal consistency of the queue and basically put the last move of the queue into an invalid state. In contrast, the code I initially wrote does the following in the move method of Toolhead:

+        if max_junction_v2 is not None:
+            move.limit_junction(max_junction_v2)
         self.lookahead.add_move(move)

which ensures that all junction limits are applied and are in effect before calc_junction is called, and even if subsequently the queue will be partially flushed, correct junction limits are applied. With all that said, unfortunately I do not see how to implement a separate function to limit the junction velocity for the last enqueued move without substantial rewrite of the enqueuing logic. It would be possible to implement a function to limit an upcoming junction velocity, however it would require tracking the next junction speed limit in the Toolhead class, something like

@@ -220,6 +220,7 @@ class ToolHead:
         # Velocity and acceleration control
         self.max_velocity = config.getfloat('max_velocity', above=0.)
         self.max_accel = config.getfloat('max_accel', above=0.)
+        self.max_junction_v2 = None
         min_cruise_ratio = 0.5
         if config.getfloat('minimum_cruise_ratio', None) is None:
             req_accel_to_decel = config.getfloat('max_accel_to_decel', None,
@@ -464,7 +465,11 @@ class ToolHead:
         self.commanded_pos[:] = newpos
         self.kin.set_position(newpos, homing_axes)
         self.printer.send_event("toolhead:set_position")
-    def move(self, newpos, speed, max_junction_v2=None):
+    def limit_next_junction_speed(self, speed):
+        speed2 = speed**2
+        if self.max_junction_v2 is None or self.max_junction_v2 > speed2:
+            self.max_junction_v2 = speed2
+    def move(self, newpos, speed):
         move = Move(self, self.commanded_pos, newpos, speed)
         if not move.move_d:
             return
@@ -472,8 +477,9 @@ class ToolHead:
             self.kin.check_move(move)
         if move.axes_d[3]:
             self.extruder.check_move(move)
-        if max_junction_v2 is not None:
-            move.limit_junction(max_junction_v2)
+        if self.max_junction_v2 is not None:
+            move.limit_junction(self.max_junction_v2)
+            self.max_junction_v2 = None
         self.commanded_pos[:] = move.end_pos
         self.lookahead.add_move(move)
         if self.print_time > self.need_check_pause:

which is hardly any better (however if you prefer to have it this way, please let me know).

Edit: updated the last patch slightly.

[dmbutyugin]

Following the merge of #6747, I rebased this PR on top of the current mainline HEAD.

[KevinOConnor]

While I'm at it, I tried to implement (2), and now I remembered why I did not do that in the first place.

I was thinking something like the following (untested):

--- a/klippy/toolhead.py
+++ b/klippy/toolhead.py
@@ -47,6 +47,7 @@ class Move:
         self.delta_v2 = 2.0 * move_d * self.accel
         self.max_smoothed_v2 = 0.
         self.smooth_delta_v2 = 2.0 * move_d * toolhead.max_accel_to_decel
+        self.next_junction_v2 = 999999999.9
     def limit_speed(self, speed, accel):
         speed2 = speed**2
         if speed2 < self.max_cruise_v2:
@@ -55,6 +56,8 @@ class Move:
         self.accel = min(self.accel, accel)
         self.delta_v2 = 2.0 * self.move_d * self.accel
         self.smooth_delta_v2 = min(self.smooth_delta_v2, self.delta_v2)
+    def limit_next_junction_speed(self, speed):
+        self.next_junction_v2 = speed*speed
     def move_error(self, msg="Move out of range"):
         ep = self.end_pos
         m = "%s: %.3f %.3f %.3f [%.3f]" % (msg, ep[0], ep[1], ep[2], ep[3])
@@ -64,9 +67,9 @@ class Move:
             return
         # Allow extruder to calculate its maximum junction
         extruder_v2 = self.toolhead.extruder.calc_junction(prev_move, self)
-        max_start_v2 = min(
-            extruder_v2, self.max_cruise_v2, prev_move.max_cruise_v2,
-            prev_move.max_start_v2 + prev_move.delta_v2)
+        max_start_v2 = min(extruder_v2, self.max_cruise_v2,
+                           prev_move.max_cruise_v2, prev_move.next_junction_v2,
+                           prev_move.max_start_v2 + prev_move.delta_v2)
         # Find max velocity using "approximated centripetal velocity"
         axes_r = self.axes_r
         prev_axes_r = prev_move.axes_r
@@ -596,6 +599,10 @@ class ToolHead:
             callback(self.get_last_move_time())
             return
         last_move.timing_callbacks.append(callback)
+    def limit_next_junction_speed(self, speed):
+        last_move = self.lookahead.get_last()
+        if last_move is not None:
+            last_move.limit_next_junction_speed(speed)
     def note_mcu_movequeue_activity(self, mq_time, set_step_gen_time=False):
         self.need_flush_time = max(self.need_flush_time, mq_time)
         if set_step_gen_time:

Such that calls to toolhead.move(pos, v, max_junction_v2=s*s) be replaced with toolhead.limit_next_junction_speed(s); toolhead.move(pos, v).

I may be missing something though. If so, let me know.

Thanks again. In general the current code here looks good to me.
-Kevin

[dmbutyugin]

@KevinOConnor

I was thinking something like the following (untested):

Fair enough, I didn't think of doing it this way. So I made a change that follows this proposal. I think the PR is ready to be merged in general, and I think it can be squash-merged as a single commit.

[KevinOConnor]

Thanks. Looks good to me. I'll give a couple of days to see if there are any further comments, and then look to commit.

Cheers,
-Kevin

[dmbutyugin]

Thanks, sounds good from my side.

[KevinOConnor]

Thanks.

-Kevin

[mxfi]

Just fyi, this sweeping method seems to break shake n tune integration and usage, keep getting an internal klipper error when running belts calibration and axis calibration.

(I know that shake n tune isn't klipper or maintained by klipper, just thought since a lot of people use it might be worth looking into)

[Sineos]

(I know that shake n tune isn't klipper or maintained by klipper, just thought since a lot of people use it might be worth looking into)

If anything, the developers of SnT would have to look into it, so this is the wrong place to report it. With the abundance of Klipper based projects, it is completely impossible to keep track of them all or even remotely take them into account.

[mxfi]

(I know that shake n tune isn't klipper or maintained by klipper, just thought since a lot of people use it might be worth looking into)

If anything, the developers of SnT would have to look into it, so this is the wrong place to report it. With the abundance of Klipper based projects, it is completely impossible to keep track of them all or even remotely take them into account.

Apologies if this is the wrong place to report it, I'm not at all code inclined (and new to GitHub too) so don't really know how to look into it/where to post it. Also don't know if it's a Klipper configuration thing vs shake tune integration side breaking so thought I'd bring it up, if this thread isn't for reporting issues or its solely shake tune side and I should delete my comments -let me know!

[Fisheye3D]

Is this the correct outcome using the default sweeping settings. This is on a delta printer. Default test and sweeping test.

[Guilouz]

@dmbutyugin I confirm, I have also bad result on delta printers.

There is something wrong, i have played with settings to try, when i use higher value for sweeping_accel (like 1000), hotend goes to the right and klipper return out of range. Isn't that supposed to be the acceleration of movements?

[dmbutyugin]

@Fisheye3D, @Guilouz interesting. I would need more data to understand the situation better. Can you run the resonance tests with all default values for test parameters in [resonance_tester] section (except, of course, the test point and the accelerometer) using the following commands:

TEST_RESONANCES AXIS=X OUTPUT=raw_data NAME=sweep_test
TEST_RESONANCES AXIS=X OUTPUT=raw_data NAME=vibration_test SWEEPING_PERIOD=0

and post the generated CSV files?

There is something wrong, i have played with settings to try, when i use higher value for sweeping_accel (like 1000), hotend goes to the right and klipper return out of range. Isn't that supposed to be the acceleration of movements?

This is an acceleration of the slow sweeping moves and it is tightly coupled with another parameter - sweeping_period. In general, I do not expect the users to need to change these values. However, if you need to, you should keep in mind that sweeping_accel * sweeping_period**2 / 32 is the amplitude of the sweeping moves (18 mm with default settings), so if you increase sweeping_accel, you must decrease sweeping_period accordingly to keep the amplitude reasonably small.