Fix doubling of MIN_PROBE_EDGE for default ABL G29.

[sjasonsmith]

Description

Fixes an issue that accidentally counted MIN_PROBE_EDGE twice when performing a default G29 using ABL.

The minimum probe edge is already considered in the functions such as probe_min_x, so there is no need to apply it again in this file.

I also noticed that the X/Y/F/R arguments were being coerced to integers, even though everything else related to probing now uses floats. I went ahead and adjusted this to allow floats as well.

Related Issues

#16348
#16338

[sjasonsmith]

I tested the following two scenarios:

G29 - Verified proper offset on all sides (10mm, or min allowable by probe offset)

G29 F50.1 B150.2 L50.3 R150.4 - Verified probe_at_point was called with the specified floating point positions.

[sjasonsmith]

This issue was reported by @Scope666, @drphil3d, @dnasir, @bigtreeteche3, and probably @alantoy.

Hopefully at least one of them can verify this solves their problem before it is merged.

[Lord-Quake]

@sjasonsmith I made the changes in the 2.0.1 code im using and can confirm that your changes fixed the issue.

[bigtreeteche3]

I tested the following two scenarios:

G29 - Verified proper offset on all sides (10mm, or min allowable by probe offset)

G29 F50.1 B150.2 L50.3 R150.4 - Verified probe_at_point was called with the specified floating point positions.

could you please describe for noobs where to find this lines in the code... i'm not able to find them... ? best regards

[Lord-Quake]

Here are his changes (red is old, replace with green is new): abfca1d

[rmcbc]

@sjasonsmith for me your change fixes the problem #16348 . Thanks.

[InsanityAutomation]

This removes what was needed for nozzle as probe to function. I'll pull it up and poke Sunday.

[sjasonsmith]

This removes what was needed for nozzle as probe to function. I'll pull it up and poke Sunday.

I don't know how to configure nozzle as probe for a Lulzbot, so I'm not sure how it would misbehave in that case. I'll let this sit for you to look at tomorrow. I'll leave it open so that people can access these changes as a workaround for the normal use-case.

Is there any reason all the logic for this cannot be inside probe_min_x and other similar functions? If there is concern that it may expose non-probable range to other probing techniques, there are already plenty of ways to crash a probe on a printer that can only probe specific points.

[sjasonsmith]

This removes what was needed for nozzle as probe to function. I'll pull it up and poke Sunday.

@InsanityAutomation and I chatted about this earlier today. I said I'd attempt to fix it, but that he is free to come up with something totally different tomorrow. My feelings will be unharmed if I discard this in favor of an alternate fix from him.

I found the Lulzbot config file for a Taz6 and used that as a reference to set up a NOZZLE_AS_PROBE configuration. I couldn't physically test it with a printer, but I added constexpr statements as needed to validate the calculated probe positions in VS Code.

I believe that prior to my change it was actually miscalculating probe_position_rb for NOZZLE_AS_PROBE, so this does change behavior, but I believe it now works correctly for both positive and negative values on all sides of the bed.

As long as NOZZLE_TO_PROBE_OFFSET is all zeroes, NOZZLE_AS_PROBE should be able to share the same math as any other printer. Enforcing this in Conditionals_post.h allowed me to delete the NOZZLE_AS_PROBE-specific logic. NOZZLE_AS_PROBE becomes basically just an alias for FIXED_MOUNTED_PROBE with a zero-offset.

I did not update all the configuration files, because I don't want to add all that noise to the PR until I know we like the changes.

[sjasonsmith]

Enforcing this in Conditionals_post.h allowed me to delete the NOZZLE_AS_PROBE-specific logic.

Looking back at the NOZZLE_AS_PROBE logic I deleted in probe.h, I don't think it was working at all. It was working because G29.cpp had extra logic so the results of those functions ended up unused. They were virtually always disregarding the minimum probe edge, because it was being compared without adding it to a min/max bed position.

[InsanityAutomation]

Aside from adding the Z offset back in this appears to work as intended on a Taz 6 as well. The redundant check for min position being removed did the trick. We may want to add a sanity check for the number allowed to be negative only with nozzle as probe, and ill likely follow up with a change to M851 to disallow changing XY with nozzle as probe.

[sjasonsmith]

We may want to add a sanity check for the number allowed to be negative only with nozzle as probe

Is there any particular reason that nozzle as probe is special in this case? I know that’s what LulzBot needs, but they could just as easily use the same technique with an inductive probe or some other sensor.

Maybe a separate config option should be added in Confoguration_adv to disable a sanity heck? Something like ALLOW_PROBING_OUTSIDE_BED, which would allow negative probe offsets?

[sjasonsmith]

Although I suppose we could disallow it and address that of the need comes up later. I’m a fan of being overly restrictive to avoid supporting things that were never intentionally created.

[sjasonsmith]

I’m surprised by the increased scope of this PR with all the changes pushed to it. I’m not going to be available to validate these changes, hopefully somebody else will be able to pull them down and test them to make sure the original issues are still fixed.

[thinkyhead]

My patches to the PR are following up on NOZZLE_AS_PROBE to apply it in a more general way. To that end, I made the following changes:

• Added HAS_PROBE_XY_OFFSET conditional and applied to serial output, mostly.
• Added probe_offset_xy as an alias for probe_offset or as {0,0} when appropriate.
• Replaced instances of probe_offset with probe_offset_xy where appropriate.

I've squashed the original changes down to one commit, and squashed my follow-up as a second commit. The majority of changes are pretty obvious, and are a good generalization of the effect of NOZZLE_AS_PROBE. The sanity check for XY probe offsets of 0,0 isn't needed since the XY offsets will not be used in the code, but it's good to retain so configurations correctly reflects the values in use.

This can be merged once the tests are passing.

[sjasonsmith]

I was previously looking at this on my phone, which isn't the best place to review code changes.
Now that I've looked over your changes properly on a computer I don't see anything alarming. There were a couple lines I was questioning, but I think you updated them prior to the squash.

[sjasonsmith]

I'm not able to physically test this on a printer right now. I added some constexpr constants and used those to verify that the calculated probe edges still looks correct for both normal (BLTOUCH and +Edge) and nozzle as probe (NOZZLE_AS_PROBE and -Edge).

[tpruvot]

Could be useful to check UBL with and without this change... Its the first time i try UBL here and that skip many points by default (during the cold mesh creation) except if you set :

 #define MESH_MIN_X _MAX(MIN_PROBE_EDGE, MESH_INSET)
 #define MESH_MIN_Y _MAX(MIN_PROBE_EDGE, MESH_INSET)
 #define MESH_MAX_X X_BED_SIZE - 35 // NOZZLE_TO_PROBE_OFFSET
 #define MESH_MAX_Y Y_BED_SIZE - _MAX(MESH_INSET, MIN_PROBE_EDGE)

with MIN_PROBE_EDGE = 10, MESH_INSET 5 and NOZZLE_TO_PROBE_OFFSET { -35, -6, -0.95 }

[sjasonsmith]

Could be useful to check UBL with and without this change... Its the first time i try UBL here and that skip many points by default (during the cold mesh creation) except if you set :

The bug was introduced into ABL along with the addition of NOZZLE_TO_PROBE. I just checked that commit, and it did not change anything in the UBL code. That feature was really targeting a specific set of machines that can only probe four points, so UBL wouldn't be very useful.

I believe the behavior you are describing is expected. The way I understand UBL is that it by defaults creates a mesh to cover the entire bed, but this includes unreachable points. You have to take additional action to fill in the missing points automatically (by extrapolating values), or to manually probe the missing points.

[tpruvot]

k, i only tried with your PR... so i had no reference :p UBL logic made me mad

[sjasonsmith]

@thinkyhead, is anything blocking this from being merged at this point?

[rmcbc]

@sjasonsmith and @thinkyhead I got the pull request #16367 and it seems to solve y original problem that G29 didn't probe the correct positions, now it works.

[thinkyhead]

Thanks for the feedback … and of course the patch! It's good to iron these things out. I think we can expect 2.0.2 to be a very good version.

[bill-orange]

Yup. That looks like the problem. I am somewhat gratified that it really was a bug and not some error on my part. I have it pretty close to working by entering oddball integer values for each limit. I might stick with that until it’s fixed on a release.