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 USE_PROBE_FOR_Z_HOMING to insist #17359

Merged

Conversation

djessup
Copy link
Contributor

@djessup djessup commented Apr 1, 2020

Description

Boards such as BTT SKR 1.4 have a dedicated pin for BLTouch/servo probes' z-trigger. To use these dedicated ports for z-homing requires hacks either to Z_MIN_PIN or the logic of Conditionals_LCD.h.

Neither of these are good for long-term maintenance.

This patch allows users to opt-in to use a custom z-pin for probing and homing by enabling a single flag in Configuration.h. If this flag is left disabled (default) there is no change in behaviour.

Tested against a BTT SKR 1.4 Turbo + TMC2209 drivers and BLTouch 3.1.

Benefits

  • Allows configuration driven support for using native capabilities of boards with dedicated pins for z-probes
  • Does not change any assumptions about the meaning of certain pins, or cause any impact for users who do not explicitly enable this flag.
  • Makes such boards more supportable long-term, as users will not have to resort to hacking Marlin core files.

Related Issues

#17063
bigtreetech/BIGTREETECH-SKR-V1.3#207
bigtreetech/BIGTREETECH-SKR-V1.3#232
bigtreetech/BIGTREETECH-SKR-V1.3#244
#16696
#16494

Copy link

@compuw22c compuw22c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes my SKR Pro trying to keep my cabling neat using the single header. Thanks!

@thinkyhead
Copy link
Member

thinkyhead commented Apr 6, 2020

Some questions pertaining to this new setting….

  • Is the Z_MIN_PIN now unused?
  • Is your USE_ZMIN_PLUG setting disabled?
  • What prevents moving the probe connector over to the Z_MIN_PIN on the board?
    • Presumably it is easier to change in software than it is to open the box.

Some thoughts come to mind seeing how this works.

  • One could add #define HOMING_Z_WITH_PROBE 1 to the config to get the same effect.
  • The fact that USE_ZMIN_PLUG is not defined could also act as a proxy for this new setting.

@djessup
Copy link
Contributor Author

djessup commented Apr 7, 2020

Some questions pertaining to this new setting….

  • Is the Z_MIN_PIN now unused?

Nothing is connected on the board, the pin definition is unchanged from the main Marlin branch.

  • Is your USE_ZMIN_PLUG setting disabled?

No. I looked at the impact of doing this early on but couldn't see that it would impact the ability to use the dedicated probe Z pinout. Please correct me if I'm wrong, as obviously the ripples of Z-logic go pretty deep, but from what I could discern reviewing the code it was immaterial in this case.

  • What prevents moving the probe connector over to the Z_MIN_PIN on the board?

Nothing physically (or software-wise) except manufacturers like BTT offer a dedicated port for this purpose, so the natural asssumption is that the dedicated BLT Z-pin is the "most correct" one to use. The amount of video tutorials and Github comments suggest I'm not the only one who assumed the BLTouch Z-pin was the best place to connect the BLTouch z-probe. There are numerous videos/comments from people saying to get their setup to work they had to move the BLT Z-plug to the native z-endstop connector, but that it was counter-intuitive.

  • Presumably it is easier to change in software than it is to open the box.

For me, at least, it's more about the principal that if a dedicated pin is available it make sense to use it. While for me the BLT Z-sense is enough, maybe I'll want to have a back-up physical microswitch as well. Regardless, the pin definitions for the board remain unchanged, and both pins are correctly defined this way.

Some thoughts come to mind seeing how this works.

  • One could add #define HOMING_Z_WITH_PROBE 1 to the config to get the same effect.

I did try this, but since Conditionals_LCD.h is applied post-Configuration[_adv].h it has no effect, as HOMING_Z_WITH_PROBE gets redefined.

  • The fact that USE_ZMIN_PLUG is not defined could also act as a proxy for this new setting.

Potentially there is a route that could use specific configuration settings as a proxy and make this option redundant, however the impact analysis would be non-trivial, as we've seen with prior attempts to resolve this same issue. While I can understand hesitation to introduce more config variables, at least it carries the least risk of adverse impact.

@thinkyhead You've contributed a huge amount to the Marlin project, and both I and thousands of others are in your debt. If you think there is a better approach to pursue I'm happy to support that as best I can, but to me this seemed like the path of least resistance.

@thinkyhead
Copy link
Member

Simplicity is good. I've made the option super general. Just an override to always home with the Z probe regardless of its connection point.

@thinkyhead thinkyhead force-pushed the feature/custom-probe-pin-homing branch 2 times, most recently from d641bef to 931bf50 Compare April 17, 2020 13:48
djessup and others added 2 commits April 17, 2020 08:53
- to allow probe z-homing when using custom z-endstop pin for the probe
@thinkyhead thinkyhead force-pushed the feature/custom-probe-pin-homing branch from 931bf50 to f3dae9d Compare April 17, 2020 13:53
@thinkyhead thinkyhead changed the title Added flag CUSTOM_PROBE_PIN_HOMING to allow probe z-homing when using… Add USE_PROBE_FOR_Z_HOMING to insist Apr 17, 2020
@thinkyhead thinkyhead merged commit 360e07c into MarlinFirmware:bugfix-2.0.x Apr 17, 2020
@djessup
Copy link
Contributor Author

djessup commented Apr 18, 2020

Thanks @thinkyhead I'll give your edits a shot and let you know if any issues arise.

As a sidebar, I did try playing with USE_[X|Y|Z]_[MIN|MAX]_PIN and the sanity check fails if neither are enabled for a given axis. This makes sense from a traditional perspective, however now we have things like probe-based homing and TMC StallGuard, I personally have none of these connected yet must have Marlin "think" at least one of the endstops are present for each Cartesian axis or it won't compile. Just a thought moving forward.

@thinkyhead
Copy link
Member

Yep, every new change requires some afterthought and it's not unusual for a patch or two to be needed once a new feature is "out there" and its implications are seen. So keep patching as needed and we'll get it in shape RSN.

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.

3 participants