add_y_axis_support and automatic_compensation to axis_twist_compensation #6624

yochiwarez · 2024-06-21T01:01:59Z

add support for Y-axis in axis_twist_compensation.

JamesH1978 · 2024-06-23T13:34:37Z

Thank you for submitting a PR, pleas refer to point 3 in "What to expect in a review" in https://github.com/Klipper3d/klipper/blob/master/docs/CONTRIBUTING.md and provide a signed off by line.

Thanks
James

yochiwarez · 2024-06-29T06:54:24Z

Thank you for your feedback. I have reviewed point 3 in "What to Expect in a Review" and have now included a signed-off-by line in my commits.

Thanks!

KevinOConnor · 2024-07-09T20:11:49Z

Thanks. @koonweee, @blastrock - do you have any comments?

On a quick glance through this PR, it seems some of the config options change (z_compensations to zx_compensations). At a minimum that would need to be documented in docs/Config_Changes.md .

-Kevin

yochiwarez · 2024-07-09T21:57:52Z

Hi Kevin,

Thanks for the feedback. I've updated docs/Config_Changes.md to document the change from z_compensations to zx_compensations.

blastrock

Thanks for the contribution! I have reviewed only the first commit for the moment, I'll try to read the rest asap.

blastrock · 2024-07-16T06:09:35Z

klippy/extras/axis_twist_compensation.py

                                               default=[], parser=float)
        self.compensation_start_x = config.getfloat('compensation_start_x',
                                                    default=None)
-        self.compensation_end_x = config.getfloat('compensation_start_y',
+        self.compensation_end_x = config.getfloat('compensation_end_x',


Oh that was my bad. The variable seemed unused so it shouldn't have caused issues. Thanks for the fix.

blastrock · 2024-07-16T09:45:10Z

klippy/extras/axis_twist_compensation.py

+            if not all([
+                self.y_start_point[0],
+                self.y_end_point[0],
+                self.y_start_point[1]


Why isn't there a similar check for the X axis?

i think cause they are already required for the basic cofig:

self.calibrate_start_x = config.getfloat('calibrate_start_x') self.calibrate_end_x = config.getfloat('calibrate_end_x') self.calibrate_y = config.getfloat('calibrate_y')

Oh, I see, they were mandatory when we added the module. I think we should make the x twist variables optional now, to allow users to use y twist compensation without x.

You're right, I made it optional, and it will check for them when the calibration command is called.

blastrock · 2024-07-16T09:46:06Z

klippy/extras/axis_twist_compensation.py

+            raise self.gcmd.error(
+                "AXIS_TWIST_COMPENSATION_CALIBRATE: "
+                "Invalid axis.")
+            return


No need to return after a raise

blastrock · 2024-07-16T09:50:06Z

klippy/extras/axis_twist_compensation.py

                          compensation.calibrate_y)
+        self.y_start_point = (compensation.calibrate_start_y,
+                            compensation.calibrate_x)


Why swap x and y for the y axis? I find it a bit confusing to have a point(y, x)

i will swap them

blastrock · 2024-07-16T10:04:03Z

klippy/extras/axis_twist_compensation.py

@@ -26,13 +26,24 @@ def __init__(self, config):
        self.calibrate_start_x = config.getfloat('calibrate_start_x')
        self.calibrate_end_x = config.getfloat('calibrate_end_x')
        self.calibrate_y = config.getfloat('calibrate_y')
-        self.z_compensations = config.getlists('z_compensations',
+        self.zx_compensations = config.getlists('zx_compensations',


Might be a matter of taste, but I'd factorize these 4 variables in a single type (a named tuple or a dataclass, I'm not sure what's preferred) since they represent the same thing between x and y, maybe AxisCalibration. Then you can pass it as a single argument to _get_interpolated_z_compensation instead of three.

Maybe it is a little bit overkill since I am using it twice; it might result in more lines

blastrock

Overall this looks good to me, just a few minor issues.

I think the auto calibration feature deserves to be better documented. Especially how it works, and what are the downsides. It's still hard for me to understand why this would work, and to what extend the bed should be "completely flat" as the doc says. My bed has irregularities in the range of 0.2mm, the manual X axis twist calibration that I did resulted in values in the range of 0.1mm, how well would the auto calibration behave?

blastrock · 2024-07-17T08:48:51Z

docs/G-Codes.md

 twist calibration wizard. `SAMPLE_COUNT` specifies the number of points along
 the X axis to calibrate at and defaults to 3.
+`axis` can either be X or Y
+
+`AXIS_TWIST_COMPENSATION_AUTOCALIBRATE` performs automatic calibration to calculate the twist of the X and Y axes without manual measurement.


Does it take the same arguments as AXIS_TWIST_COMPENSATION_CALIBRATE? I would put this command in its own section.

i will add a new section, and make it clear.

blastrock · 2024-07-17T08:52:05Z

klippy/extras/axis_twist_compensation.py

+                    )
+
+        # check for valid sample_count
+        if sample_count is None or sample_count < 2:


I wouldn't test for None since it can't be None, it has a default value.

i will remove that comparison

blastrock · 2024-07-17T08:59:59Z

klippy/extras/axis_twist_compensation.py

+        max_y = self.y_end_point[0]
+
+        # calculate x positions
+        spcx = (max_x - min_x) / (sample_count - 1)


Rename to interval_x, to be consistent with the manual implementation? Or maybe rename the variable in the other implementation to spc.

blastrock · 2024-07-17T09:03:15Z

klippy/extras/axis_twist_compensation.py

+            flip = not flip
+
+        # verify no other manual probe is in progress
+        manual_probe.verify_no_manual_probe(self.printer)


I would move this check earlier. More specifically, before the clear_compensations call.

i will move it.

It looks like you forgot to move this

blastrock · 2024-07-17T09:07:55Z

klippy/extras/axis_twist_compensation.py

+
+        # probe the point
+        pos = probe.run_single_probe(self.probe, self.gcmd)
+        #self.current_measured_z = pos[2]


blastrock · 2024-07-17T09:38:55Z

klippy/extras/axis_twist_compensation.py

+        y_coords = [coord[1] for coord in coordinates]
+        z_coords = [coord[2] for coord in coordinates]
+
+        # Calculate the desired point (average of all corner points in z)


I am wondering why you take only the corners. My intuition tells me that the average of all points would better cancel out bed irregularities. Is there a particular reason this works better?

I use the average of the corners to establish a reference plane for compensation. However, I calculate the corrections using all points to ensure the adjustments reflect the actual axis conditions. It might need more testing, but it seems to work well.

My understanding may be wrong, but what happens if one of the four points is completely off, because there's a hole in the bed just there for example? Wouldn't twist compensation be messed up over the whole bed because of that single corner point?

You're right. That's why you shouldn't calibrate the axis twist compensation on a surface that isn't as flat as possible—using a glass bed is a good option. Once it's calibrated, you're free to use any bed you want.

blastrock · 2024-07-17T13:30:49Z

docs/Config_Changes.md

@@ -8,6 +8,10 @@ All dates in this document are approximate.

 ## Changes

+20240709: The `z_compensations` parameter in the `[axis_twist_compensation]`
+config section has been renamed to `zx_compensations`. If you don't want
+to recalibrate your x_axis_twist_compensation, simply rename the parameter.


I would either maintain compatibility and print a warning about the setting being deprecated, or put a fatal error if that setting is present in the config file. At some point, I will upgrade my klipper and I will lose my calibration silently, this is not very user-friendly.

I'm going to remove this; to make it fully backward compatible, I will keep z_compensations instead of using zx_compensations

github-actions · 2024-08-01T00:09:13Z

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:

Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review
If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

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,
~ Your friendly GitIssueBot

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

PhilBaz · 2024-10-16T17:59:26Z

@yochiwarez Hey, I'm interested in this feature. I'm sure others in the Voron community will be too! Keep going Bro!

PhilBaz · 2024-10-17T14:19:31Z

Awesome!!! Thanks guys! This will be a great feature.

PhilBaz · 2024-10-17T15:49:08Z

Guys, is it to late to add the logic for dock-able probes like klicky etc?

yochiwarez · 2024-10-17T16:44:26Z

Guys, is it to late to add the logic for dock-able probes like klicky etc?

It should work the same way as it does with bed leveling. You would need to add macros to attach and detach the probe within a custom macro.

blastrock

Added a few comments, otherwise this looks good to me 👍

blastrock · 2024-10-18T06:40:19Z

klippy/extras/axis_twist_compensation.py

+            if not all([
+                self.y_start_point[0],
+                self.y_end_point[0],
+                self.y_start_point[1]


Oh, I see, they were mandatory when we added the module. I think we should make the x twist variables optional now, to allow users to use y twist compensation without x.

blastrock · 2024-10-18T06:41:52Z

klippy/extras/axis_twist_compensation.py

+        self.y_end_point = (compensation.calibrate_end_y,
+                            compensation.calibrate_x)


This too should be (x, y)

blastrock · 2024-10-18T06:43:23Z

klippy/extras/axis_twist_compensation.py

+                    """
+                    )
+
+            start_point = (self.y_start_point[1], self.y_start_point[0])


start_point should be (x, y) too, I find (y, x) very confusing

i have changed that part to make it clear.

blastrock · 2024-10-18T06:45:46Z

klippy/extras/axis_twist_compensation.py

+        xps = [min_x + interval_x * i for i in range(sample_count)]
+
+        # Calculate points array
+        spcy = (max_y - min_y) / (sample_count - 1)


Suggested change

spcy = (max_y - min_y) / (sample_count - 1)

interval_y = (max_y - min_y) / (sample_count - 1)

for consistency

blastrock · 2024-10-18T07:01:04Z

klippy/extras/axis_twist_compensation.py

+        y_coords = [coord[1] for coord in coordinates]
+        z_coords = [coord[2] for coord in coordinates]
+
+        # Calculate the desired point (average of all corner points in z)


My understanding may be wrong, but what happens if one of the four points is completely off, because there's a hole in the bed just there for example? Wouldn't twist compensation be messed up over the whole bed because of that single corner point?

blastrock

lgtm! Could you have a look too @KevinOConnor?

Maybe some of the people interested in this branch could help testing it :)

KevinOConnor · 2024-10-21T17:45:53Z

Could you have a look too @KevinOConnor?

I think it's really your call on this.

I noticed a few minor things:

We should get a bunch of people to test this before making the change, as it would be preferable to avoid follow up fixes as much as we can.
In general, I'd prefer to avoid adding new commands - so if we can do something like AXIS_TWIST_COMPENSATION_CALIBRATE AUTO=1 that might be nicer than adding a new command.
If an existing user is going to observe some change to continue with existing functionality (eg, the need to recalibrate, the need to change some config, the need to change some scripts) then lets make sure Config_Changes.md is updated to alert them to this. (I don't know if it is there are any changes like this.)
If the "autocalibrate" is separate functionality from y-axis support it would be nice to have that in a separate commit. (And to squash and merge the commits with appropriate commit message wording so that I can commit them with a commit-and-rebase action.) https://www.klipper3d.org/CONTRIBUTING.html#format-of-commit-messages
The additions to the docs should ideally be wrapped to 80 columns.

Thanks,
-Kevin

yochiwarez · 2024-10-23T01:58:18Z

Hi @KevinOConnor , @blastrock

Thanks for your feedback! I've made the changes as you suggested:

The command has been updated to AXIS_TWIST_COMPENSATION_CALIBRATE AUTO=1.
I’ve ensured that no changes are needed to the configuration file, so it is now fully backward compatible.
I've clearly separated the two commits as requested.
I made sure that the documentation files are wrapped to 80 columns.

Let me know if there's anything else you need!

blastrock

This looks good to me, just tiny doc fixes. I will test this PR myself, but my printer does not exhibit Y axis twist, so it'd be nice if at least someone else could test it. @PhilBaz you seemed interested?

blastrock · 2024-11-02T07:29:32Z

docs/Axis_Twist_Compensation.md

-[fine-tuning](Axis_Twist_Compensation.md#fine-tuning) as desired
+### Basic Usage: X-Axis Calibration
+1. After setting up the `[axis_twist_compensation]` module, run:
+``


All these should be ``` I think

blastrock · 2024-11-02T07:30:22Z

docs/Config_Reference.md

+#   Defines the minimum Y coordinate of the calibration
+#   This should be the Y coordinate that positions the nozzle at the starting
+#   calibration position for the Y axis. This parameter must be provided if
+#   compensating for Y axis twist.


Above, you should update the This parameter must be provided. sentence for the x axis

blastrock · 2024-11-02T10:31:52Z

klippy/extras/axis_twist_compensation.py

+        manual_probe.verify_no_manual_probe(self.printer)
+
+        # clear the current config
+        self.compensation.clear_compensations()


This call clears all compensations, but we should remove only compensations for the axis we are calibrating, otherwise we lose one axis each time we calibrate the other.

I have fixed this.

blastrock · 2024-11-02T16:48:12Z

I did a couple prints with this version and my x twist compensation settings, that part seems to still work at least :) I tried calibrating the y twist compensation, but as expected I got values very close to 0, so I can't really test it. I didn't test the auto calibration as I understood the bed must be super flat, and mine is far from that.

This commit implements support for the Y-axis in the axis_twist_compensation module. This update enables the module to handle corrections for printers with a twisted Y rail. Signed-off-by: Jorge Apaza Merma <[email protected]>

…or autocalibration This commit adds automatic calculation support for compensating X and Y axis twist in the axis_twist_compensation module. Signed-off-by: Jorge Apaza Merma <[email protected]>

blastrock · 2024-11-02T18:56:00Z

Thank you!

PhilBaz · 2024-11-04T14:51:39Z

@blastrock Do you still need a tester? Sorry for the delay in response. Ive just moved across contry and s@#t is hectic.

blastrock · 2024-11-04T15:51:56Z

Yes please. It would be nice to see if this PR improves the prints of at least another person :)

PhilBaz · 2024-11-04T15:58:06Z

@blastrock Okay! Im more of a 3d printing guy. No coding or super advanced klipper skills. That being said, Im tech literate.

Can you give me a hint of how to proceede? Just need a thread I can pull with googleing etc.

Thanks guys!

blastrock · 2024-11-04T19:41:08Z

I don't know about your specific installation, but I guess you have a clone of the klipper repo (probably in your home on your raspberry pi?). Go inside and checkout this PR

cd klipper
git fetch origin pull/6624/head:ytwist
git checkout ytwist

You're ready to go, just restart klipper

sudo systemctl restart klipper

Don't forget to put the right config variables in printer.cfg to enable the y twist compensation as written in the doc of this PR.

If you want to switch back to the latest version after testing:

git checkout v0.12.0

Hope this helps, tell us if you need more :)

yochiwarez · 2024-11-04T23:37:45Z

This is my test for the auto calibration
before:

After auto calibration:

KevinOConnor · 2024-11-07T00:54:54Z

Okay, thanks. I'll give another couple of days just to see if there are further comments, but otherwise look to commit.

-Kevin

PhilBaz · 2024-11-07T03:52:47Z

@blastrock Thanks! Ill try get this done in the next couple of days. Just moved into a new house. Ill try send some others from Voron too test too.

KevinOConnor · 2024-11-13T03:10:10Z

Thanks.

-Kevin

yochiwarez changed the title ~~add_y_axis_support~~ add_y_axis_support to axis_twist_compensation Jun 21, 2024

yochiwarez force-pushed the y-axis-twist-support branch 3 times, most recently from 2c39e55 to 29fc0ac Compare June 21, 2024 03:09

yochiwarez force-pushed the y-axis-twist-support branch from c2fe44d to 1fd8443 Compare June 23, 2024 15:15

yochiwarez marked this pull request as draft June 23, 2024 15:20

yochiwarez force-pushed the y-axis-twist-support branch 2 times, most recently from 29fc0ac to bdf6ea1 Compare June 23, 2024 15:23

yochiwarez marked this pull request as ready for review June 23, 2024 15:30

yochiwarez force-pushed the y-axis-twist-support branch from cabc5b4 to 4585e73 Compare June 29, 2024 06:50

yochiwarez changed the title ~~add_y_axis_support to axis_twist_compensation~~ add_y_axis_support and automatica compensation to axis_twist_compensation Jun 29, 2024

yochiwarez changed the title ~~add_y_axis_support and automatica compensation to axis_twist_compensation~~ add_y_axis_support and automatic_compensation to axis_twist_compensation Jun 29, 2024

blastrock reviewed Jul 16, 2024

View reviewed changes

blastrock reviewed Jul 17, 2024

View reviewed changes

github-actions bot added the reviewer needed label Aug 1, 2024

yochiwarez force-pushed the y-axis-twist-support branch from 6113ea7 to e93f077 Compare October 17, 2024 09:21

yochiwarez requested a review from blastrock October 17, 2024 09:24

PhilBaz mentioned this pull request Oct 17, 2024

Please add macros for X-axis_twist_compensation jlas1/Klicky-Probe#268

Open

blastrock reviewed Oct 18, 2024

View reviewed changes

yochiwarez force-pushed the y-axis-twist-support branch from ea06b2f to 24c7979 Compare October 18, 2024 14:30

yochiwarez requested a review from blastrock October 21, 2024 15:48

blastrock approved these changes Oct 21, 2024

View reviewed changes

yochiwarez force-pushed the y-axis-twist-support branch 3 times, most recently from f3653e7 to e55d301 Compare October 23, 2024 01:38

blastrock reviewed Nov 2, 2024

View reviewed changes

axis_twist_compensation: Implement Y-axis support

43467a1

This commit implements support for the Y-axis in the axis_twist_compensation module. This update enables the module to handle corrections for printers with a twisted Y rail. Signed-off-by: Jorge Apaza Merma <[email protected]>

yochiwarez force-pushed the y-axis-twist-support branch from e55d301 to 1663ee3 Compare November 2, 2024 17:53

axis_twist_compensation: AXIS_TWIST_COMPENSATION new parameter AUTO f…

88c2a27

…or autocalibration This commit adds automatic calculation support for compensating X and Y axis twist in the axis_twist_compensation module. Signed-off-by: Jorge Apaza Merma <[email protected]>

yochiwarez force-pushed the y-axis-twist-support branch from 1663ee3 to 88c2a27 Compare November 2, 2024 18:24

blastrock approved these changes Nov 2, 2024

View reviewed changes

yochiwarez force-pushed the y-axis-twist-support branch from 108eb86 to 88c2a27 Compare November 8, 2024 22:02

KevinOConnor merged commit 38bf6f2 into Klipper3d:master Nov 13, 2024
2 checks passed

		self.y_end_point = (compensation.calibrate_end_y,
		compensation.calibrate_x)

	spcy = (max_y - min_y) / (sample_count - 1)
	interval_y = (max_y - min_y) / (sample_count - 1)

add_y_axis_support and automatic_compensation to axis_twist_compensation #6624

add_y_axis_support and automatic_compensation to axis_twist_compensation #6624

Conversation

yochiwarez commented Jun 21, 2024 • edited Loading

JamesH1978 commented Jun 23, 2024

yochiwarez commented Jun 29, 2024 • edited Loading

KevinOConnor commented Jul 9, 2024

yochiwarez commented Jul 9, 2024

blastrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blastrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 1, 2024

PhilBaz commented Oct 16, 2024

PhilBaz commented Oct 17, 2024

PhilBaz commented Oct 17, 2024

yochiwarez commented Oct 17, 2024

blastrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blastrock left a comment

Choose a reason for hiding this comment

KevinOConnor commented Oct 21, 2024

yochiwarez commented Oct 23, 2024 • edited Loading

blastrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blastrock commented Nov 2, 2024

blastrock commented Nov 2, 2024

PhilBaz commented Nov 4, 2024

blastrock commented Nov 4, 2024

PhilBaz commented Nov 4, 2024

blastrock commented Nov 4, 2024

yochiwarez commented Nov 4, 2024 • edited Loading

KevinOConnor commented Nov 7, 2024

PhilBaz commented Nov 7, 2024

KevinOConnor commented Nov 13, 2024

yochiwarez commented Jun 21, 2024 •

edited

Loading

yochiwarez commented Jun 29, 2024 •

edited

Loading

yochiwarez commented Oct 23, 2024 •

edited

Loading

yochiwarez commented Nov 4, 2024 •

edited

Loading