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

axis_twist_compensation: Implement Y-axis support 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]>

axis_twist_compensation: new command implementation AXIS_TWIST_COMPENSATION_AUTOCALIBRATE 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 · 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

Signed-off-by: Jorge Apaza Merma <[email protected]>

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?

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)

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.

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.

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.

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.

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?

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.

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.

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 from 1fd8443 to 29fc0ac Compare June 23, 2024 15:22

yochiwarez force-pushed the y-axis-twist-support branch 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

docs: Deprecate z_compensations parameter

ac34a01

Signed-off-by: Jorge Apaza Merma <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add_y_axis_support and automatic_compensation to axis_twist_compensation #6624

add_y_axis_support and automatic_compensation to axis_twist_compensation #6624

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

blastrock Jul 16, 2024

blastrock Jul 16, 2024

blastrock Jul 16, 2024

blastrock Jul 16, 2024

blastrock Jul 16, 2024

blastrock left a comment

blastrock Jul 17, 2024

blastrock Jul 17, 2024

blastrock Jul 17, 2024

blastrock Jul 17, 2024

blastrock Jul 17, 2024

blastrock Jul 17, 2024

blastrock Jul 17, 2024

github-actions bot commented Aug 1, 2024

add_y_axis_support and automatic_compensation to axis_twist_compensation #6624

Are you sure you want to change the base?

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

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

github-actions bot commented Aug 1, 2024

yochiwarez commented Jun 21, 2024 •

edited

Loading

yochiwarez commented Jun 29, 2024 •

edited

Loading