Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add probe offset XY edit limits #26267

Merged
merged 7 commits into from
Nov 5, 2023

Conversation

vlsi
Copy link
Contributor

@vlsi vlsi commented Sep 11, 2023

Description

Previously, XY offset was allowed to have as high as X_BED_SIZE, Y_BED_SIZE values, however it might be too big for a common case.

I suggest adding explicit Z_PROBE_OFFSET_XRANGE_MIN, Z_PROBE_OFFSET_XRANGE_MAX, Z_PROBE_OFFSET_YRANGE_MIN, and Z_PROBE_OFFSET_YRANGE_MAX so users have saner defaults.

I keep existing Z_PROBE_OFFSET_RANGE_MIN and Z_PROBE_OFFSET_RANGE_MAX for backward compatibility.

Requirements

The change relates to bed probes only.

Benefits

UI will be better as min-max would be more reasonable than "machine size", and M851 would reject invalid XY offsets.

Samples of the newly added sanity checks

I've added several new sanity checks.

"Z_PROBE_OFFSET_RANGE_MAX=1000, Z_MAX_POS=200"

Marlin/src/HAL/shared/../../core/../inc/SanityCheck.h:1422:54: error: static assertion failed: "Z_PROBE_OFFSET_RANGE_MAX" must be within -(Z_MAX_POS) and Z_MAX_POS
   #define static_assert_within(varname, x, min, max) static_assert(WITHIN(x, min, max), #varname " must be within " #min " and " #max)
                                                      ^
Marlin/src/HAL/shared/../../core/../inc/SanityCheck.h:1429:3: note: in expansion of macro 'static_assert_within'
   static_assert_within("Z_PROBE_OFFSET_RANGE_MAX", Z_PROBE_OFFSET_RANGE_MAX, -(Z_MAX_POS), Z_MAX_POS);
   ^~~~~~~~~~~~~~~~~~~~

"NOZZLE_TO_PROBE_OFFSET.y exceeds Z_PROBE_OFFSET_YRANGE_MIN and Z_PROBE_OFFSET_YRANGE_MAX":

Marlin/src/HAL/shared/../../inc/SanityCheck.h:1422:54: error: static assertion failed: "NOZZLE_TO_PROBE_OFFSET.y" must be within Z_PROBE_OFFSET_YRANGE_MIN and Z_PROBE_OFFSET_YRANGE_MAX
   #define static_assert_within(varname, x, min, max) static_assert(WITHIN(x, min, max), #varname " must be within " #min " and " #max)
                                                      ^
Marlin/src/HAL/shared/../../inc/SanityCheck.h:1425:3: note: in expansion of macro 'static_assert_within'
   static_assert_within("NOZZLE_TO_PROBE_OFFSET.y", sanity_nozzle_to_probe_offset.y, Z_PROBE_OFFSET_YRANGE_MIN, Z_PROBE_OFFSET_YRANGE_MAX);
   ^~~~~~~~~~~~~~~~~~~~

It would be nice to have the actual computed values, however, it seems to require format(...) function or moving to c++20.

Configurations

Sample diff to trigger a sanity check

--- a/Marlin/Configuration.h
+++ b/Marlin/Configuration.h
@@ -1366,7 +1366,7 @@
  * A Fix-Mounted Probe either doesn't deploy or needs manual deployment.
  *   (e.g., an inductive probe or a nozzle-based probe-switch.)
  */
-//#define FIX_MOUNTED_PROBE
+#define FIX_MOUNTED_PROBE

 /**
  * Use the nozzle as the probe, as with a conductive
@@ -1540,7 +1540,7 @@
  *     |    [-]    |
  *     O-- FRONT --+
  */
-#define NOZZLE_TO_PROBE_OFFSET { 10, 10, 0 }
+#define NOZZLE_TO_PROBE_OFFSET { 1000, -10000, 0 }

Related Issues

#26263

@vlsi vlsi force-pushed the zprobe_offsetbounds branch 2 times, most recently from 9acc105 to 4cf0660 Compare September 11, 2023 12:13
…SET_YRANGE_MIN, Z_PROBE_OFFSET_YRANGE_MAX to make UIs nicer, and limit M851 XY offsets

M851 would respect the limits, so the system would be safer.

Fixes MarlinFirmware#26263
Marlin/Configuration.h Outdated Show resolved Hide resolved
Marlin/src/inc/SanityCheck.h Outdated Show resolved Hide resolved
@vlsi
Copy link
Contributor Author

vlsi commented Sep 11, 2023

Test fails for chitu_f103 because the board uses "sample configuration" that provides no value for Z_PROBE_OFFSET_XRANGE_MIN and properties like that.

Apparently, if people "just modify Configuration.h", then Marlin can't add new properties and keep backward compatibility.

What do you think if there was Configuration_defaults.h file that would apply the default values in case user did not provide one?

Then all subsequent logic (e.g. Configuration_adv.h, SanityChecks.h) would be able to rely on the presence of all the properties, and changes like this PR could become compatible with the old configurations.

Unfortunately, it would cause duplication of the default values as:

  1. Z_PROBE_OFFSET_XRANGE_MIN would need to be declared in Configuration.h, so users can see the value exists in the first place
  2. Z_PROBE_OFFSET_XRANGE_MIN would have to be declared in Configuration_defaults.h so Marlin could have sensible defauls if user-provided Configuration.h misses a value

I think that duplication would be a reasonable price for maintaining backward compatibility, however, if there are better alternatives I would be glad to hear them.

I suggest that Configuration_defaults.h should not be user-editable.

@thisiskeithb
Copy link
Member

Test fails for chitu_f103 because the board uses "sample configuration" that provides no value for Z_PROBE_OFFSET_XRANGE_MIN and properties like that.

You'd normally submit a companion PR to Marlin's Configurations repo when creating PRs that touch these kinds of settings/features.

What do you think if there was Configuration_defaults.h file that would apply the default values in case user did not provide one?

Configuration.h & Configuration_adv.h are the defaults. You'll see in other parts of the code, some settings are defined with #ifndefs if a user doesn't have it in their config or we'll assert a value through SanityCheck.h.

@vlsi
Copy link
Contributor Author

vlsi commented Sep 11, 2023

You'd normally submit a companion PR to Marlin's Configurations repo when creating PRs that touch these kinds of settings/features.

Frankly speaking, I find that GitHub does not support multi-repository PR.
If the relevant sources were in the same repository, then it would be way easier to make a single PR that performs all the changes.

At the same time, I believe having explicit _defaults.h would make it easier for consumers to upgrade, as they won't necessarily have to change their existing configs.


Configuration.h & Configuration_adv.h are the defaults. You'll see in other parts of the code, some settings are defined with

I am afraid Conditionals_LCD.h, Conditionals_post.h do have "defaults".

For instance:

/**
* Provide a DEFAULT_VOLUMETRIC_EXTRUDER_LIMIT in case NO_VOLUMETRICS is enabled
*/
#ifndef DEFAULT_VOLUMETRIC_EXTRUDER_LIMIT
#define DEFAULT_VOLUMETRIC_EXTRUDER_LIMIT 0.00
#endif

Conditionals_post.h redeclares a default value for DEFAULT_VOLUMETRIC_EXTRUDER_LIMIT, however, it does so after Configuration_adv.h is included, so one can't rely on DEFAULT_VOLUMETRIC_EXTRUDER_LIMIT in Configuration_adv.h

I had a similar issue with HAS_BED_PROBE (in Configuration_adv.h) and PROBE_MANUALLY (in Configuration.h).
Conditionals_LCD.h for unknown reasons un-defines PROBE_MANUALLY, so I have to use #if ENABLED(PROBE_MANUALLY) in Configuration.h and #if HAS_BED_PROBE in Configuration_adv.h.


Imagine Configuration.h and Configuration_adv.h are user-facing configuration files.
What do you think if there were Marlin-internal

  • Configuration_defaults.h that would set mandatory properties with their respective default values for the values typically declared in Configuration.h
  • Configuration_derived.h that would set derived properties that depend on several Configuration.h properties
  • Configuration_adv_defaults.h that would set mandatory properties with their respective default values for the values typically declared in Configuration_adv.h
  • Configuration_derived.h that would set derived properties that depend on several Configuration.h and Configuration_adv.h properties

Then, HAS_BED_PROBE computation could be moved from Conditionals_LCD.h to Configuration_derived.h.
It might even be a slightly better idea to move declaration of HAS_BED_PROBE right into Configuration.h itself, so the users could use HAS_BED_PROBE consistently right after PROBE_... declarations.

Just in case, my use case for HAS_BED_PROBE in Configuration.h and Configuration_adv.h is to enable/disable "assisted bed tramming" depending on "probe presence in the configuration" (e.g. see https://github.com/vlsi/reborn2-marlin/blob/4a2d03c37993ae8664a6c3524532c6ab8e05ca99/Marlin/Configuration_adv.h#L1032-L1034 )

It looks like "HAS_BED_PROBE" becomes a part of the API anyway, however, currently, it is hard to hell "in which files one can use HAS_BED_PROBE" since it is declared in Conditionals_LCD.h only (LCD?)

WDYT?

@EvilGremlin
Copy link
Contributor

It looks like "HAS_BED_PROBE" becomes a part of the API anyway, however, currently, it is hard to hell "in which files one can use HAS_BED_PROBE" since it is declared in Conditionals_LCD.h only (LCD?)

In any files, you just include MarlinConfig.h/MarlinConfigPre.h

@vlsi
Copy link
Contributor Author

vlsi commented Sep 11, 2023

@EvilGremlin , would you please elaborate?

Apparently, I can't include MarlingConfig.h and MarlinConfigPre.h into Configuraiton.h and/or Configuration_adv.h.

@EvilGremlin
Copy link
Contributor

EvilGremlin commented Sep 11, 2023

Yep, here you can't, gotta use macro from configs only. Else it'll circular dependency, which is illegal.
But there is no need to put your defines under HAS_BED_PROBE anyway.

@vlsi
Copy link
Contributor Author

vlsi commented Sep 11, 2023

But there is no need to put your defines under HAS_BED_PROBE anyway

What is your suggestion then?

As I add #ifdef ASSISTED_TRAMMING #define ASSISTED_TRAMMING #endif it enables me to use a single set of Configuration_* files to produce several firmware variants.
I don't want to copy-paste Configuration_adv.h for "machine with z-sensor" and "machine without z-sensor".

Of course, in my case I have enough flash and RAM, so I might eventually fold it into a single firmware and make "z-probe type" configurable from the menu. Unfortunately, I'm not there yet, so I have to generate several firmwares, and I find conditional activation of ASSISTED_TRAMMING the cleanest and easiest approach.

@EvilGremlin
Copy link
Contributor

ah, you can use config.ini then
or use -DWHATEVER key in env and #ifdef WHATEVER` in config

@vlsi
Copy link
Contributor Author

vlsi commented Sep 11, 2023

ah, you can use config.ini then
or use -DWHATEVER key in env and #ifdef WHATEVER` in config

That is exactly what I do: I pass -DPROBE_MANUALLY, but I can’t use it in Configuration.h and Configuration_adv.h consistently because reasons clarified in #26267 (comment)

@EvilGremlin
Copy link
Contributor

#if !HAS_LEVELING || ANY(MESH_BED_LEVELING, AUTO_BED_LEVELING_UBL)
  #undef PROBE_MANUALLY

no problems here

@vlsi
Copy link
Contributor Author

vlsi commented Sep 11, 2023

@EvilGremlin , frankly speaking, I think PROBE_MANUALLY issue is tangential to the current PR.
Please see how #26269 reproduces the issue, and in fact, PROBE_MANUALLY is not available in Configuration_adv.h even though other settings are available.

The error message is

In file included from buildroot/share/PlatformIO/scripts/../../../../Marlin/src/inc/MarlinConfigPre.h:56,
                 from buildroot/share/PlatformIO/scripts/../../../../Marlin/src/inc/MarlinConfig.h:28,
                 from buildroot/share/PlatformIO/scripts/common-dependencies.h:29:
buildroot/share/PlatformIO/scripts/../../../../Marlin/src/inc/../../Configuration_adv.h:1079:4: error: #error PROBE_MANUALLY must be enabled as we configured it in Configuration.h
 1079 |   #error PROBE_MANUALLY must be enabled as we configured it in Configuration.h
      |    ^~~~~
Error: Failed to parse Marlin features. See previous error messages.

Just in case: I do not need workarounds for "probe_manually" as I have enough of them and I have configured GitHub Actions for building several variants of the firmware for my machine.
However, I believe the configuration approach could be improved so users could get fewer surprises.
For instance, I do not understand the reason PROBE_MANUALLY disappears.

@EvilGremlin
Copy link
Contributor

EvilGremlin commented Sep 11, 2023

It works correctly, there is no issue. You don't have any valid leveling type enabled, thus PROBE_MANUALLY gets undefined becasue nothing can use it.

@vlsi
Copy link
Contributor Author

vlsi commented Sep 11, 2023

You don't have any valid leveling type enabled

@EvilGremlin , Could you clarify what you mean? I truly do not understand your words.
Does UBL qualify as a leveling type?

If I un-comment #define AUTO_BED_LEVELING_UBL, then the issue with PROBE_MANUALLY still reproduces. I believe the condition for undef PROBE_MANUALLY is wrong, and a better one should be like #if !HAS_LEVELING #undef PROBE_MANUALLY, however, un-defining user-provided values should probably be accompanied with a warning that explains the reasons the value was undefined.

The same goes for MESH_BED_LEVELING + PROBE_MANUALLY. Somehow PROBE_MANUALLY disappears.

It might be I do not understand what PROBE_MANUALLY means, however, it would be way greater if incompatible configuration would fail with a relevant error in SanityCheck rather than silently undefining user-provided configs.

@EvilGremlin
Copy link
Contributor

EvilGremlin commented Sep 11, 2023

Nope, undef condition is perfectly right. UBL and MESH has it's own manual probing routine

@vlsi
Copy link
Contributor Author

vlsi commented Sep 11, 2023

How about adding a sanity error in case the user supplies both PROBE_MANUALLY and UBL/MESH at the same time?

@EvilGremlin
Copy link
Contributor

EvilGremlin commented Sep 11, 2023

It it was necessary it would be there for years. Read docs first https://marlinfw.org/docs/gcode/G029.html

@vlsi
Copy link
Contributor Author

vlsi commented Sep 11, 2023

Thanks for referencing the manual. I have read it previously, and I have refreshed it right now. I can't find where it says PROBE_MANUALLY will be undefined for UBL or PROBE_MANUALLY will be undefined for MBL. Would you please clarify were do the docs explain it?

@EvilGremlin
Copy link
Contributor

🤦 because MBL is already manual and in UBL it's always available, non-optionally.

@vlsi
Copy link
Contributor Author

vlsi commented Sep 11, 2023

it's always available, non-optionally.

If it is always available, then I would expect PROBE_MANUALLY (or a flag like HAS_MANUAL_PROBING) to be defined rather than undefined.

@EvilGremlin
Copy link
Contributor

As it's still not clear to you - PROBE_MANUALLY does not exist in UBL and MBL code, it's irrelevant to them!

@vlsi
Copy link
Contributor Author

vlsi commented Sep 11, 2023

Would you please review #26271 ?

Marlin/Configuration.h Outdated Show resolved Hide resolved
@thinkyhead thinkyhead changed the title ADD Z_PROBE_OFFSET_XRANGE_MIN, Z_PROBE_OFFSET_XRANGE_MAX, Z_PROBE_OFFSET_YRANGE_MIN, Z_PROBE_OFFSET_YRANGE_MAX to make UIs nicer, and limit M851 XY offsets Add probe offset XY edit limits Oct 28, 2023
thinkyhead added a commit to MarlinFirmware/Configurations that referenced this pull request Oct 28, 2023
thinkyhead added a commit to MarlinFirmware/Configurations that referenced this pull request Oct 28, 2023
@thinkyhead
Copy link
Member

thinkyhead commented Oct 29, 2023

This is about ready, methinks, with more consistency in the naming of the options, and so on. I would have combined them into a single option, such as…

#define PROBE_OFFSET_LIMIT { { -50, 50 }, { -50, 50 }, { -20, 20 } } // (mm) Offset limits for XYZ

…but it makes all the code more complicated – enough to pass on doing it that way.


Your various suggestions about configuration are not unfamiliar. We've covered a lot of that ground before in looking for more maintainable and user friendly configuration options, and after evaluating all the trade-offs involved in each approach we have arrived at where we are. The focus going forward is not to abandon the Configuration.h/Configuration_adv.h files but to make configuration and migration tools. There is a new facility called config.ini that aims to make certain kinds of configuration management easier, and configuration/migration tools are being developed as part of the Auto Build Marlin extension.

Join the discussion on the Marlin Discord if you would like to get involved with the effort to build these tools. I'm just getting started on the configuration migration tool, so feedback will be helpful as that develops.

@thinkyhead
Copy link
Member

If it is always available, then I would expect PROBE_MANUALLY (or a flag like HAS_MANUAL_PROBING) to be defined rather than undefined.

The PROBE_MANUALLY is framed as a probe type, not as a leveling method. It was originally added to allow for ABL to be enabled when there is no electronic probe. ABL needs "some kind of probe," so the PROBE_MANUALLY probe was invented to fill that gap. ABL does not provide manual and auto leveling at the same time. If there is a real probe it will always be automatic, never manual.

Meanwhile, MANUAL_BED_LEVELING doesn't require any probe option to be enabled. It is always manual. Likewise, UBL always includes manual probing, but makes auto probing available when there's an electronic probe. It's normal to undefine PROBE_MANUALLY in such a case, when it's redundant and its being enabled might confuse LCD code that associates PROBE_MANUALLY with ABL.

@thinkyhead thinkyhead merged commit cb044d9 into MarlinFirmware:bugfix-2.1.x Nov 5, 2023
59 checks passed
@thisiskeithb
Copy link
Member

A couple renames were missed, so I've submitted a PR: #26397

@ellensp
Copy link
Contributor

ellensp commented Nov 13, 2023

@thinkyhead Please clarify why you made the changes in "smaller Z range" 9186a73

static_assert_within("PROBE_OFFSET_ZMIN", PROBE_OFFSET_ZMIN, -(Z_MAX_POS), Z_MAX_POS);
static_assert_within("PROBE_OFFSET_ZMAX", PROBE_OFFSET_ZMAX, -(Z_MAX_POS), Z_MAX_POS);

to

static_assert_within("PROBE_OFFSET_ZMIN", PROBE_OFFSET_ZMIN, -20, 20);
static_assert_within("PROBE_OFFSET_ZMAX", PROBE_OFFSET_ZMAX, -20, 20);

Later this was changed to the following, but the -20 to 20 limits remains.

PROBE_OFFSET_ASSERT("PROBE_OFFSET_ZMIN", PROBE_OFFSET_ZMIN, -20, 20);
PROBE_OFFSET_ASSERT("PROBE_OFFSET_ZMAX", PROBE_OFFSET_ZMAX, -20, 20);

These seem to be arbitrary constraints to -20,20 when some marlin provided example configs exceed these values

Ie

config/examples/delta/FLSUN/QQS-Pro/Configuration.h:#define PROBE_OFFSET_ZMIN -25 // (mm)
config/examples/delta/FLSUN/Q5-nano_v2/Configuration.h:#define PROBE_OFFSET_ZMIN -40 // (mm)
config/examples/delta/FLSUN/Q5-nano_v1/Configuration.h:#define PROBE_OFFSET_ZMIN -40 // (mm)
config/examples/delta/Anycubic/Predator/Configuration.h:#define PROBE_OFFSET_ZMIN -30 // (mm)
config/examples/delta/Anycubic/Predator/Configuration.h:#define PROBE_OFFSET_ZMAX 30 // (mm)
config/examples/delta/Anycubic/Kossel/Configuration.h:#define PROBE_OFFSET_ZMIN -40 // (mm)
config/examples/delta/Anycubic/Kossel Linear Plus/Configuration.h:#define PROBE_OFFSET_ZMIN -40 // (mm)
config/examples/AnyCubic/Chiron/Configuration.h:#define PROBE_OFFSET_ZMIN -30 // (mm)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants