Skip to content

BLTOUCH promote to "normal" probe#14371

Closed
ghost wants to merge 2 commits intobugfix-2.0.xfrom
unknown repository
Closed

BLTOUCH promote to "normal" probe#14371
ghost wants to merge 2 commits intobugfix-2.0.xfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Jun 22, 2019

** DONT MERGE - DOCUMENTATION PR **

No more need for using bltouch.stow() and bltouch.deploy(). It is better to use the STOW_PROBE and DEPLOY_PROBE macros as this leads to correct endstop handling.

Also, motion.cpp then no longer needs a reference to bltouch.h

But there are still many more things to bring in line, still working on it.

Current status:

PART 1: #14380 PR merged
PART 2: #14381 PR merged.
PART 3: #14412 PR open

First bug reported and fixed as CLOSED PR #14393

PART 4: consolidation phase in progress - let's wait for more bugs before closing this

No more need for using bltouch.stow() and bltouch.deploy(). It is better to use the STOW_PROBE and DEPLOY_PROBE macros as this leads to correct endstop handling.
@thinkyhead
Copy link
Member

You may want to revisit the discussion long ago when it was decided that BLTouch needed certain events to occur at different points than other probes. Maybe those considerations are now obsolete. But I want to make sure there's not something we are forgetting.

@ghost
Copy link
Author

ghost commented Jun 23, 2019

You may want to revisit the discussion long ago when it was decided that BLTouch needed certain events to occur at different points than other probes. Maybe those considerations are now obsolete. But I want to make sure there's not something we are forgetting.

Yes, these places where there is a difference are exactly the problem. And it looks like not all of them can be eliminated.

@ghost
Copy link
Author

ghost commented Jun 23, 2019

I request NEEDS WORK and DONT MERGE flags on this one

@Roxy-3D Roxy-3D added Needs: Work More work is needed S: Don't Merge Work in progress or under discussion. labels Jun 23, 2019
@Roxy-3D
Copy link
Member

Roxy-3D commented Jun 23, 2019

I request NEEDS WORK and DONT MERGE flags on this one

Done.

@ghost
Copy link
Author

ghost commented Jun 24, 2019

Maybe I should go for a title change. #14393 shows that trying to consolidate BLTOUCH probe-specific stuff to

  • probe.cpp / probe.h namely STOW_PROBE() and DEPLOY_PROBE() macros

  • an absolute minimum of other places

will of course also touch other probe types and they will in the end also profit from this consolidation.

Maybe the title should be: ?

@thinkyhead
Copy link
Member

thinkyhead commented Jun 26, 2019

It's okay. As it is, it just allows BLTOUCH behavior to be handled by set_probe_deployed, and makes all cases of BLTOUCH deploy/stow go through set_probe_deployed so that it always gets the extra behaviors in that function.

@ghost
Copy link
Author

ghost commented Jun 26, 2019

There are now no more bltouch.this() and bltouch.that() calls out there that are not absolutely justified. So this is really looking ok now.

The "old classic" BLTOUCH operation mode - i.e. when BLTOUCH_HS_MODE is not enabled - is the reason for the few bltouch.deploy() and bltouch.stow() calls that still remain (in probe.cpp and motion.cpp).

If no more bugs turn up in the next week or so that are related to this PR, I will close it as successful.

@thinkyhead
Copy link
Member

Close as successful… or would we merge it?

@ghost
Copy link
Author

ghost commented Jun 26, 2019

This PR would have better been a "project". I cannot open a project.

It doesn't contain any code changes yet although I could think of one or two token changes to put in here instead of spinning them off into child-PR's. That way the documentation will live on.

Doing the small PR's step by step allows finding bugs/problems and reverting in a granular fashion.

@thinkyhead
Copy link
Member

You can always add more commits to this PR or replace its contents altogether without closing it.

@thinkyhead thinkyhead closed this Jun 26, 2019
@ghost ghost deleted the BLTOUCH_PROMOTE branch June 26, 2019 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

F: BLTouch Needs: Work More work is needed PR: General Cleanup S: Don't Merge Work in progress or under discussion.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants