Rewrote some parts of bed_mesh.py

[BillyNate]

Backstory can be found on klipper.discourse

Here's what I did:

• Removed the error message on L489
• Changed the check on L336 to accept negative distances
• Made within() on L25 more generic/robust: Now accepts "reversed" coordinates, needed to keep faulty regions working
• Rewrote creation of 2D list on L722: It is now based on the min/max positions & x_cnt/y_cnt, accepting points in any order, instead of relying on an expected zig-zag order only.
• Adjusted y-axis length check on L739 to handle changed 2D array
• Rewrote probed values extrapolation for round beds on L746:
  • Now handles the new 2D array setup
  • Instead of extrapolating by copying the previous x-axis z_offset it now works its way from the inside out and taking the average of two neighboring z_offsets
• Replaced x-axis length check on L764 for a final check if all 2D array mesh positions are set

This code has been tested with settings for

• a rectangular bed with low mesh_min / high mesh_max
• a rectangular bed with high mesh_min / low mesh_max
• a rectangular bed with a faulty region
• a round bed

This still definitely needs testing/reviewing, so if anyone wants give it a try, I'd be pleased. Any feedback would be welcome!

[JamesH1978]

Thank you for submitting a PR, please could you refer to point 3 in the "what to expect" section in https://github.com/Klipper3d/klipper/blob/master/docs/CONTRIBUTING.md and add a signed-off-by line.

Thanks James

[BillyNate]

My apologies for the missing signed-off-by line 🙈 I added it just now.

[github-actions[bot]]

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:

1. 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.
2. 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.
3. 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.

[Sineos]

Ping @Arksine

[Arksine]

Thanks. I do have some high level observations and comments about these proposed changes.

• Removed the error message on L489
• Changed the check on L336 to accept negative distances
• Made within() on L25 more generic/robust: Now accepts "reversed" coordinates, needed to keep faulty regions working

The above changes seem to be centered around accepting invalid configuration. I don't agree with this, I think its safer and easier to document when we are explicit about the coordinates accepted. The mesh_min, mesh_max, and options related to faulty regions have documentation specifying how they should be configured relative to the origin. That said, it may be useful to add a validation step to the faulty regions similar to that of the mesh, and be more explicit in the documentation about how they should be configured.

• Rewrote creation of 2D list on L722: It is now based on the min/max positions & x_cnt/y_cnt, accepting points in any order, instead of relying on an expected zig-zag order only.
• Adjusted y-axis length check on L739 to handle changed 2D array

Is there an immediate functional benefit to this change? The code seems fine, however any change risks introducing an unforeseen regression. The stated reasoning to to accept points in any order, however currently they will only be received according to the zigzag pattern.

• Rewrote probed values extrapolation for round beds on L746:
  • Now handles the new 2D array setup
  • Instead of extrapolating by copying the previous x-axis z_offset it now works its way from the inside out and taking the average of two neighboring z_offsets
• Replaced x-axis length check on L764 for a final check if all 2D array mesh positions are set

Part of the above changes seem to be related to the proposal to change the way the probed matrix is generated, which was addressed in the previous comment. The primary new addition appears to be a change in how round beds extrapolate points outside of the probed area. Is there any data to suggest that averaging the two nearest points provides a real world benefit over the current extrapolation? I had considered this during the original implementation and decided that it likely wouldn't make a difference, however I am open to data showing otherwise.

[BillyNate]

Hi @Arksine,

Thank you for your time. I understand your opinion about the 2D array changes and the extrapolation.

Theoretically a stepper in Klipper could be configured to have position_min: -300 and position_max: 0, right? A mesh_min: -300, -300 isn't "nearest to the origin" and mesh_min: 0, 0 would trigger the error. Isn't this a discrepancy between stepper configuration and bed_mesh configuration? Obviously the solution to this can be approached in different ways. Since I personally would like the bed mesh calibration to start at a different corner I came up with this. To be honest I have no idea if there's a noteworthy target audience for this, I'm just assuming I'm not the only one who wants to keep toolhead movement (and thus time) at a minumum.

I might add other patterns next to the zigzag pattern later on (primarily to influence the last point probed). Is this of interest to the community?

Several Klipper interfaces show a visualization of the bed mesh. This of course includes the extrapolated points, causing confusion with users on round beds. How do you feel about not extrapolating at all, but only saving the actual probed points?

Again, thank you for your time, and looking forward to your response.

[Arksine]

Theoretically a stepper in Klipper could be configured to have position_min: -300 and position_max: 0, right? A mesh_min: -300, -300 isn't "nearest to the origin" and mesh_min: 0, 0 would trigger the error.

Yes, this could be done in theory, but I can think of no valid reason to deviate from recommended configurations, a user would be asking for nothing but trouble. The origin can be placed at any corner, it depends on your stepper configuration and which way moves toolhead the positive direction, so it isn't really necessary to operate in negative coordinates when dealing with cartesian-like kinematics. That said, the documentation could be reworded to state "the lowest value on both the X and Y axes" or something similar. The option name of mesh_min is somewhat self explanatory as well.

I might add other patterns next to the zigzag pattern later on (primarily to influence the last point probed). Is this of interest to the community?

I'm willing consider additional patterns, but there would need to be a substantial audience and a valid functional reason for doing this. While I empathize with the reason of "because I'd like to", the maintenance burden of introducing changes to bed_mesh requires a higher standard.

Several Klipper interfaces show a visualization of the bed mesh. This of course includes the extrapolated points, causing confusion with users on round beds. How do you feel about not extrapolating at all, but only saving the actual probed points?

Making such a change would likely break the current visualizations and other functionality that relies on the points reported by bed_mesh. It should possible for front-ends to better accommodate round beds without significant changes to bed_mesh. We may need to add some additional reporting, but I don't think its necessary to refactor the extrapolation step.

[BillyNate]

Thank you for elaborating on this. I guess the pull request can be closed now? If there's anything else I can do concerning the bed mesh implementation, please let me know.

[github-actions[bot]]

Unfortunately a reviewer has not assigned themselves to this GitHub Pull Request and it is therefore being closed. 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.