Fix inaccurate error message

[ChasonDeshotel]

This pull request improves the error message related to the position_endstop in stepper sections (e.g., stepper_y) to provide clearer guidance on valid input. The original message was misleading by implying that the position_endstop must strictly lie between position_min and position_max.

The updated error message clarifies that the position_endstop must be within or equal to the travel range defined by position_min and position_max. This change aims to reduce confusion and help users configure their endstop positions more easily.

[JamesH1978]

Thank you for submitting a PR, please be aware that you need to sign off to accept the developer certificate of origin, please see point 3 in https://github.com/Klipper3d/klipper/blob/master/docs/CONTRIBUTING.md#what-to-expect-in-a-review

Thanks James

[JamesH1978]

I would concur on that one, the proposed wording is more ambiguous.

Not sure why you think its misleading?

Thanks James

[ChasonDeshotel]

I'm completely open to further revision. I wasn't crazy about the wording either but it was the best accurate wording I could come up at the time

I believe this wording is an improvement from my original PR, and will expose the disconnect:

position_endstop in section '%s' must lie inclusive between position_min and position_max

edit: alternatively, and probably betterly, "position_endstop in section '%s' must be between position_min and position_max inclusive"

*and while I'm not sure every user will understand what inclusive means, at least ChatGPT would, instead of spewing out an incorrect thesis on the endstop/min/max logic

I spent a good 5 minutes going "whyy can't I just set the 0 to the point the endstop tells me is the edge of travel??" And I finally said "this makes no sense maybe the error is wrong" and just yolo'd the following:

position_endstop: 0
position_min: 0
position_max: 179

And it worked. Yep. Bad error message.

This isn't what I was hoping to do--I gave up and physically moved the limit switch--but it works. This configuration is valid.

I'm interested in revising the docs on these parameters as they're also technically incorrect when they should pretty much be no-brainers. I'm still not fully grasping what should be a dirt simple concept, and the docs are making it worse

#position_min: 0
#   Minimum valid distance (in mm) the user may command the stepper to
#   move to.  The default is 0mm.

Distance is a measurement between multiple points. If it's a distance, we're missing information.. there's no direction or destination. If it's a move to then we're moving to a point/coord and we don't need to know the distance

position_endstop:
#   Location of the endstop (in mm). This parameter must be provided
#   for the X, Y, and Z steppers on cartesian style printers.

"Location of the endstop". This is entirely unhelpful.

To be clear, I'm not faulting any devs--I've been doing this professionally for a couple decades, I know. I love love love Klipper and it's a brilliant showcase of FOSS. Major kudos to everyone who contributed and there is loads of good documentation, this just looks like fatigue.

position_max:
#   Maximum valid distance (in mm) the user may command the stepper to
#   move to. This parameter must be provided for the X, Y, and Z
#   steppers on cartesian style printers.

Same as position_max

I only had 20-30 minutes to work on this, thus the rushed PR and me moving the limit switch instead of digging to further understand these parameters. I'm happy to make adjustments to the docs here and add it to this PR or submit it as a new PR. If someone wouldn't mind enlightening me that would make this a quick and easy improvement

As an aside, I was thinking I could set the endstop to -33, then my new 0 would be the minimum--where the nozzle meets the edge of the print bed. I saw a couple posts where people tried to do the same but I didn't have time to go further I just said "eh, guess it doesn't work like that". I think that would be worth mentioning in the docs in some form or fashion, whether directly or indirectly by explaining the underlying behavior

[bevanweiss]

I do agree with your other items. In those three statements, the term position should be used (instead of location or distance) i.e.

position_endstop:
#   Position of the endstop (in mm). 

#position_min: 0
#   Minimum valid position (in mm) the user may command the stepper to
#   move to....

position_max:
#   Maximum valid position (in mm) the user may command the stepper to
#   move to....

I would argue for the min/max the term the user may command the stepper to move to is extraneous. Because the limitation doesn't apply to the user, it applies to the movement engine itself. i.e. if the GCode commands movement outside of this range it will be a problem. But the user didn't necessarily consider that they commanded this movement...

I don't think the addition of the term inclusive to the position_endstop error message would be an issue. But I also don't think it would be the clarification that would have resolved your issue, since you were trying to set it to -33 which is neither inclusively or exclusively between 0 and 195.

The min/max are stepper absolute positions. So it makes no sense for the endstop position to be outside the range between min/max... how would the stepper ever get to the position where the endstop is activated.

[ChasonDeshotel]

I would argue for the min/max the term the user may command the stepper to move to is extraneous. Because the limitation doesn't apply to the user, it applies to the movement engine itself. i.e. if the GCode commands movement outside of this range it will be a problem. But the user didn't necessarily consider that they commanded this movement...

Yep. Agree fully. I think I'm solid on this. I was solid with it 5-6 years ago when I first set it up 😅 I'll try to get changes pushed this weekend. Honestly now that I've been thinking about it, I remember this being a pain point on my initial install

I don't think the addition of the term inclusive to the position_endstop error message would be an issue.

I'll update this to just add the word inclusive. That's a better/cleaner solution and should still pull up existing threads when people search the error message

But I also don't think it would be the clarification that would have resolved your issue, since you were trying to set it to -33 which is neither inclusively or exclusively between 0 and 195.

Oh, absolutely not, I agree. I was changing config values to try to learn how it all works and this ambiguity cost me some time. That was my first attempt and I got that error message. For subsequent attempts, my mind was trying to reconcile "endstop must be between (exclusive) min and max..." with my set of mental notes of how the machine reacted w/ the new settings

As I got closer to solving the puzzle, I started to realize that it behaves in a sane manner and this extra condition was throwing me off. That's when I got the confidence to go "I don't care what it says, 0, 0. Works. -33, -33. Works."

Apparently it's just a difference in how our brains interpret things 🌈 I kept visualizing points on a line and if someone drew two points on 0 and a point at 10, and then pointed at one of the 0 points and asked "is this point between 0 and 10?", I imagine very few would instinctively say "yes". It's either going to be "no" or someone who's run into this problem before asking if the between is inclusive or exclusive 😅

It took me reading "pick a number between 1 and 10..." aloud for it to click. My brain was all in on "parking space between two cars" mode

So it makes no sense for the endstop position to be outside the range between min/max... how would the stepper ever get to the position where the endstop is activated.

Yes! That too! But my mind went to "what in the world are they doing??" thinking there was some voodoo behind the scenes

I'll update the title of the PR as well, since "inaccurate" is inaccurate lol

[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.