G29 remove redundant probe deploy - Fix #14401

[ghost]

Does anyone lay claim on the probe deploy that is to be found in G29.cpp (see change diff)?

After much scrutiny I have not seen any use case for it, and later on, in the probe_pt(...) function, the probe is deployed anyway.

It is the fix for #14401. I have observed this behaviour also and although it has never bothered me, it is not good.

In all other places we are trying hard not to move around with deployed probes unless specifically desired - and here we deploy and then go speeding across half the bed to the probe start point.

[thinkyhead]

You might give it a try. But some probes might want to do some single deployment action there before the probe operation commences. I would have to look into the history to see the discussion leading to the change.

[ghost]

I'll do the research into the history of abl/G29.cpp

[thinkyhead]

I went as far back into history as here… f6b09cf?w=1#diff-1cb08de130a6ece2d1b5b9c37bcfef48R3468

…And I was reminded that probe deploy is not the same thing as probe enable. Some probes we want to remain deployed from start to finish of G29. Others will want to stow between probes. And there is an option to stow between probes, if desired.

Be very cautious about conflating probe deploy / stow with probe enable / disable.

[thinkyhead]

With that in mind, I will want to revisit recent changes to make sure I didn't let anything slip through that would break probe handling, as we did arrive at the code we see in 1.1.9 and up to now through an immense amount of testing and feedback.

[ghost]

I understand - please advise where I can be of help

[thinkyhead]

I have reverted the recent changes that need to be revisited. Also please revisit 8f99d45 from #14352 as this might get reverted too.

One of the contributors left some vague comment on another PR that implied the stow in G28 was wrong, maybe…. Or maybe it was about something else; I couldn't decipher.

So, I suggest we undo any other questionable probe changes that remain, collect all the recent changes (except this one) into a pile as a single PR, and get wider testing on those changes for a couple of weeks to make sure they are good before having faith enough to merge them.

[ghost]

Sure. By all means also revert #14352. Meanwhile I will inform #14328 to reopen his issue.

So, I suggest we undo any other questionable probe changes that remain, collect all the recent changes (except this one) into a pile as a single PR, and get wider testing on those changes for a couple of weeks to make sure they are good before having faith enough to merge them

Yes, that's a very good idea.

One of the contributors left some vague comment on another PR that implied the stow in G28 was wrong, maybe….

That was @marcio-ao, instead of opening an issue he immediately PR'd the removal - and that was what led to the discussion with @AnHardt. That was actually progressing towards a potential consensus.

[thinkyhead]

Wish I had been more conscious for the discussion. I'm no good before 1pm.

[AnHardt]

A bit of history about that,
from the early days when @thinkyhead , @boelle and me joined the project (no others of the then regulars are left), a few years back, around 1.0.1, a short time before Erik gave away the repository to @boelle.
(To my best knollige - i may be wrong)

At the beginning we had Servo-, Solenoid- and AllenKey-probes. All with their own set of deploy/stow g-codes.
G29 deployed the probe at the beginning and stowed it at the end. We had conditional coded for every kind of probe in G29.
More kinds of probes appeared. Manual deploy/stow for them was integrated into M401/402, solenoid/sled probes remained in G31/32. G29 grow to a Moloch.
G28, G30 suffered similar problems.
@Roxy-3D appeared to introduce M48. Again with a lot of conditional code for the different kind of probes. I guess she used G30 later.
M48 let us discover that for some kinds of probes (servo when depowerd to avoid jitter) we got better results when deployed for every probe, not so for others. G29 E was integrated - deploying the probe for every single probe. G29 code became completely unmaintainable.
Then we (Scott and me) unified the probe code, developed the blocking moves and generally cleaned up G29, G28 and everything around probes. A big task, but finally we succeeded.
At about the same time Roxy developed UnifiedBL in her own branch. When his was put into Marlin i realized nothing was unified but we had again, and even more, separate code for every kind of BL.
BL-touch appeared and due to the trigger pulse and the deploy for every probe, it needed a lot of code changes. The principles of the recently unification where messed up. BL-Touch code was scattered here an there an everywhere.

I lost my interest in G29 and probe code at this point.

[Roxy-3D]

@AnHardt Your summation of the history coincides with my memory. But one small correction: Unified Bed Leveling was an attempt to consolidate all of the different bed leveling FEATURES into one system. It was not an attempt to consolidate (and clean up) the underlying Z-Probe system.

[marcio-ao]

That was @marcio-ao, instead of opening an issue he immediately PR'd the removal - and that was what led to the discussion with @AnHardt. That was actually progressing towards a potential consensus.

@marcio-ao eats popcorn while wearing red/blue 3D glasses

[AnHardt]

It's not your fault - at least not as a developer/coder. My expectations, what I thought what should be done, was just not met by your intentions and code. It was about the opposite. Without doubt UBL is a successes. But a feature and code monster making code reuse even more difficult - what was my target then.

It comes down to:
Marlin developers have no common Vision, no common concept and no leaders enforcing something (except code format). That's not bad for the community, it enables everyone, even the not very experienced) to get in some crap. But its frustrating for the long time contributors to see their work permanently bugged by coders putting up dominos at the front and toss down others with their buts just because they can't see a concept (if there is one), don't know about the history of the code, don't know why the code is as it is, don't know what to do where, don't know what was already tried or discussed and have no clue what and how to search.

We should talk more about our visions how Marlin should look a few years ahead, develop concepts.
But from my experience with that i haven't much hope.

But maybe it's just me. I'm growing old and sarcastic.

[marcio-ao]

Marlin developers have no common Vision, no common concept and no leaders enforcing something (except code format).

True. And a lot of developers (myself included) only understand the issues as it pertains to our specific printer style (in my case, cartesian, using fixed probes, linear leveling, etc). My general philosophy regarding PRs is that I will propose a change that works for our particular printers, but I need to rely on the review of other Marlin devs to call out things I may not have accounted for. It's certainly challenging when no one person knows the big picture.

[InsanityAutomation]

@marcio-ao sounds much more fun than tomato farming!

Thats part of why ive accumulated 25 machines, most different now.....

[marcio-ao]

FWIW, the original PR #14393 I put in was a simple change to fix a compile error. This is something Travis-CI should have caught but didn't. I mentioned on my PR that I did not know if it was a correct fix or an ideal fix, but simply a suggestion that got the code to compile again.

[ghost]

BL-Touch code was scattered here an there an everywhere.

On the more ironic side of things: Maybe it should be called BL-Notouch code.

Seriously though, I was hoping to move back towards the remaining rest of a concept that I could still see in that part of Marlin. And I was trying to do it carefully, communicating, asking and discussing, step by step but was also prepared to take some flak and take the responsibility if something was overlooked. What can I do if any kind of discussion is cut short all of a sudden?

If I had known what kind of a hornets nest I was stirring up I would have gotten out my popcorn (salted, not sugared) too, instead of actively trying to restore the concept still visible in probe.* and in some other places by removing more and more of the special bltouch stuff.

I know that Marlin is very much more than just a BL-Touch pusher - but we have cleared up the mysteries, we cleared up the code and we now need to "untangle" it from Marlin - it's more of a normal probe than some of the other more complicated ones, so it doesn't deserve so much special stuff.

Before I bring in any crap and topple a domino, I will take care not to risk that kind of thing anymore. I though you might need an honest worker who wasn't too scared to touch some sensitive stuff - but it seems that the inertia of the status quo will win.

[InsanityAutomation]

Before I bring in any crap and topple a domino, I will take care not to risk that kind of thing anymore. I though you might need an honest worker who wasn't too scared to touch some sensitive stuff - but it seems that the inertia of the status quo will win.

I think with the focus right now being on getting to RC1 the timing just leans that way. Once we have a stable 2.0 then there may be wider acceptance to touching more sensitive stuff.

[Roxy-3D]

Before I bring in any crap and topple a domino, I will take care not to risk that kind of thing anymore. I though you might need an honest worker who wasn't too scared to touch some sensitive stuff - but it seems that the inertia of the status quo will win.

InsanityAutomation is correct about the RC1 push being a detraction from changing sensitive stuff. But with that thinking in place, please be aware we got up to RC9 on the v1.1.x release before it was locked down. I would have to look up the timing but it was over 9 months. It doesn't make sense to freeze needed development for that long just because a release is in process.

I would encourage you to keep your head down and keep charging forward. Fix anything you know needs fixing! Your attention to detail is both noticed and appreciated.

[thinkyhead]

As I recall the need to have BLTouch deploy/stow distinct from others was that the BLTouch didn't like to be left in a deployed state for a long period of time. It would time out and give us an error. So we aimed to make sure it was deployed just before the probe move, and then stowed (and set to OFF mode for even more safety) immediately after.

---

My visionary view of the Marlin is a future where contributors and I keep chipping away at the various bits needing cleanup, refactoring, implementation, improvement, and optimization bit by bit, as our limited free time allows (whilst we sip tea and listen to bebop jazz out on the veranda)… and that one day, all our incremental improvements will add up to something… that they will be inherited, cherished, and carried forward by the younglings into the future world of tomorrow….

[Roxy-3D]

My visionary view of the Marlin is a future where contributors and I keep chipping away at the various bits needing cleanup, refactoring, implementation, improvement, and optimization bit by bit,

My prophetic view of the future is Marlin continues to support what ever new and novel hardware shows up only limited by developer's interest. Where ever the developers take it... that is what Marlin will support.

In the mean time we try to limit the amount of craziness that gets merged into the code base and keep things as clean as possible while running on as many platforms as possible.

But there is no 'Central Controlling view' of where Marlin is heading. If an important, new technology appears, probably there will be developers willing to add a few more warts to Marlin to embrace it. And after the fact, we will get things cleaned up and remove the warts.

(That is kind of what has happened with BL-Touch probes. In a perfect world, we know everything and write clean perfect code the first time around. But in reality, it takes time to iterate towards a good, working, clean solution.)