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

BLTouch v3 / 3DTouch Interoperability & performance #13814

Merged

Conversation

InsanityAutomation
Copy link
Contributor

Dont Merge! Id like to get some users with the 3D Touch clone probes to test this and confirm it allows the removal of the version flag.

Moved switch mode to M119 only, as it doesnt retain through commands to be useful during probing.
Changed default safe delay to 500ms
Default to open drain on all but creality boards

@InsanityAutomation
Copy link
Contributor Author

@rmcbc @davelPhoton @salfter Pinging people involved with clones.

@Roxy-3D
Copy link
Member

Roxy-3D commented Apr 25, 2019

I think I would argue... If you are going to turn on Open Drain mode... I think it makes sense to turn on

#define ENDSTOPPULLUPS

@InsanityAutomation
Copy link
Contributor Author

@Roxy-3D good call, but I dont want to turn it on for everything just in case they dont want / need it for other endstops. I added a sanity check to cover the situation with a message to turn it on.

@salfter
Copy link
Contributor

salfter commented Apr 25, 2019

My printer is currently down, waiting for a new heated bed to arrive (the original doesn't want to heat up anymore). Also, it currently has a BLTouch v2.2 installed (fried a v2.1 when I fat-fingered the power wiring); the 3DTouch is a backup that I'd have to swap in to test.

@InsanityAutomation
Copy link
Contributor Author

At least we know the sanity checks worked as they tripped travis :)

@Roxy-3D
Copy link
Member

Roxy-3D commented Apr 25, 2019

good call, but I dont want to turn it on for everything just in case they dont want / need it for other endstops. I added a sanity check to cover the situation with a message to turn it on.

At the risk of 'Creeping Elegance'.... We could just turn on (force on) the pull up for the Z-Min Probe. And just leave every other end stop alone.

@InsanityAutomation
Copy link
Contributor Author

@Roxy-3D maybe when we have more confidence that always works and is correct. For the moment I'd like to keep a more cautious approach with know actions.

@ghost
Copy link

ghost commented Apr 28, 2019

@InsanityAutomation and @Roxy-3D You might want to look at my BLTouch recode that I have been working on in the past (many) days and that is working nicely on my printers.
BLTouch-Special-Release
It's ready for a pull request/review, but anyone still fighting with his BLTouch would benefit from its error handling to see what is going wrong. I invite you to have a look at it or try it out with BLTOUCH_DEBUG_MSGS defined. I have commented the code extensively and maybe you can pick some stuff out of it. The main changes are in bltouch.cpp and in bltouch.h.

Moved switch mode to M119 only, as it doesnt retain through commands to be useful during probing.

Good idea. Saves you a couple of clicks in the config menu. But I left it in in the config to be added after a deploy.

I think I would argue... If you are going to turn on Open Drain mode... I think it makes sense to turn on

That's optional because you might have your own pull-up somewhere.

Changed default safe delay to 500ms

In my code, there are actually a number of commands that need even higher delays. But not everyone requires that. For stress testing, I actually go down to 150ms and with the exception of RESET, the probe works fine (V3). If you want to catch STOW or DEPLOY failures, you need 750ms. Look at the new code - heavily tested.

I've got a clone here. It actually supports SW mode, for example, just like my V2.0. So therefore I have moved being able to set SW mode to the general part of the config.

Here`s my new BLTouch section of the config, which should give you an indication of how extensive the reconding turned to be, I wanted the operation of the BLTouch to be more foolproof:

/**
 * The BLTouch probe uses a Hall effect sensor and emulates a servo.
 */
//#define BLTOUCH
#if ENABLED(BLTOUCH)
  /**
   * For all BLTouch probes and clones:
   */
  //#define BLTOUCH_DEBUG_MSGS  // Produce some debugging messages when commands are issued to the probe
  //#define BLTOUCH_DELAY 375   // (ms) Allow the servo signal to settle for this long before the next command
                                // Note: This is the sane default. Some setups will tolerate lower values.
  
  /**
   * For BLTouch V2.0 and newer probes which support SW mode. Some clones do too.
   * Set the probe into SW mode, producing a longer pulse when triggered, useful when there
   * is noise or high capacitance on the signal pin.
   * If your probe works fine without this, don't turn it on, as it slows down probing
   * Note: Performing a STOW and then setting this mode in the LCD BLTouch configuration menu
   *       manually will alloy you to check the wiring with a M119 command. You don't need to enable
   *       this in order to be able to do that.
   */
  //#define BLTOUCH_FORCE_SW_MODE
  
  /**
   * For BLTouch V3.0 and newer probes:
   * If you want the new functions of the BLTouch V3 or you need 5V mode (see below), enable this.
   * This adds the special V3 commands (5V and OD modes) to the LCD configuration menu for BLTouch
   */
  //#define BLTOUCH_V3
  #if ENABLED(BLTOUCH_V3)  
    /**
     * These probes default to OPEN DRAIN mode. All other probes are in 5V mode per default.
     * If you have a BLTouch V3.0 or newer and explicitely want 5V mode, use this define -  this
     * makes it more compatible with previous probe versions.
     * Warning: Make sure your controller boards input pin is 5V tolerant.
     */
    //#define BLTOUCH_FORCE_5V_MODE
    /**
     * Advanced users who fervently believe in the reliability of their BLTouch probes can
     * activate the HIGH SPEED mode for their unit. This will reduce the timing so far that
     * DEPLOY and STOW failures are only captured on the NEXT such command to be issued.
     * The result is faster probing but much less surety of catching and identifying errors.
     * NOT RECOMMENDED for novice users and in unstable environments. As homing and probing are
     * not done so often, the speed gain is nice but not really life changing.
     */
    //#define BLTOUCH_HS_MODE
  #endif
#endif

@Roxy-3D
Copy link
Member

Roxy-3D commented Apr 28, 2019

It's ready for a pull request/review, but anyone still fighting with his BLTouch would benefit from its error handling to see what is going wrong.

Yes! A Pull Request when you get a chance!

@ghost
Copy link

ghost commented Apr 28, 2019

Och, look at it first, perhaps play with it, you might not like it or something. And the changes are extensive, so I wouldn't know how you handle roll-outs of that kind. Also, I am not in a race with anyone, just putting my 2 cents in.

Nobody wants another disaster like ANTClabs got with their own stuff.

I come from big-iron - you stage this sort of thing to a couple of people first and see how it goes.

@Roxy-3D
Copy link
Member

Roxy-3D commented Apr 28, 2019

Mostly... I'm interested in the extended diagnostics and checks. And I'm thinking they should be sensitive to the DEBUG_LEVELING state (and M111).

The thing is... It is much easier to discuss and collaborate if it is a Pull Request that is active in the Marlin Domain. It is harder to pick out the changes if it is just a repository in your domain. (At least for non-GitHub experts!)

And don't worry about stuff being looked at... It will be combed through before it is merged.

@ghost
Copy link

ghost commented Apr 28, 2019

Ok, after dinner then. Here's a video of M48 in Tesla's "insane mode": M48, but that #define option is only for "safe setups" with no error checking.

@ghost
Copy link

ghost commented May 2, 2019

Moved switch mode to M119 only, as it doesnt retain through commands to be useful during probing.

When you set SW mode right after a deploy, it retains long enough to work when it tirggers. The subsequent STOW will reset it, but it will be there again if you once again set it after a DEPLOY.

@ghost
Copy link

ghost commented May 2, 2019

Moved switch mode to M119

Brilliant idea.

In your code, when you turn off the M119, it is safest to do a STOW after the RESET, because:

If the pin was up, the RESET will clear the SW mode but the high signal (sadly) will stay high. That could confuse subsquent logic (looks like ALARM).

It will drop once you issue some STOW or DEPLOY command.

Actually, you could do ONLY a STOW. That would reset SW mode and also clear the signal.

But RESET STOW is fine.

I'll steal this and put it in my special branch as well.

@thinkyhead thinkyhead merged commit 6811e29 into MarlinFirmware:bugfix-2.0.x May 8, 2019
@thinkyhead thinkyhead changed the title BLTouch v3 and 3DTouch Interoperability and performance tweak BLTouch v3 / 3DTouch Interoperability & performance May 8, 2019
@GMagician
Copy link
Contributor

GMagician commented May 8, 2019

@InsanityAutomation I think that after this merge there is an impressive bltouch deploy/stow everytime it's required to use it. I think that this should be done only to be sure to reset fault state, but there is no possibility to detect "faulty" situation and execute deploy/stow only in such case?
AFAIK when fault is active bltouch report "triggered" so why not use it?

@ghost
Copy link

ghost commented May 8, 2019

LOL @InsanityAutomation I think what @GMagician is saying is what we discussed early on: I quote you from discord: "i think im also going to leave the full init on G28 instead of just stow". So here you have the first guy complaining, didn't take long. Remember in my original PR I just had a STOW in there because the ticky-tacky quickly got on my nerves and seemed unneccessary when "just" homing. It's done one time on power up and if ok, it's ok. But we decided: Safety first. So that's the reason, @GMagician.

Apart from that small part, I don't really understand what you mean, @GMagician?

Because:

AFAIK when fault is active bltouch report "triggered" so why not use it?

It is used.

And on power up, (and on G28, see above), a ticky-tacky is done to make sure the pin is not blocked (down) before moving somewhere. It is not only impressive, it is safe. But in all other cases, there is no such action - unless the pobe encounters an error after trying to perform a STOW or a DEPLOY. In that case, I would assume the user would like a retry and clearing action, successfully, or not.

If someone gets such a clearing action on every usage of the probe, then something is wrong and must be analysed and corrected.

an impressive bltouch deploy/stow everytime it's required to use it.

Could you be more specific about what you mean by "everytime"?

Or more specific about what your actual suggestion is?

@GMagician
Copy link
Contributor

GMagician commented May 8, 2019

I'll try to explain:
Turn on printer: 2 times (autotest)
Marlin starts: 1 time
Repetier connection: 1 time
G28: 1times
G34: 1 times + 1 (implicit g28)

I'm wondering. On start bl-touch does an autotest and if not in fault it means pin can move down and up and finally is up... so unless a fault state detection why to execute deploy and stow? is not enough a simple stow?

@ghost
Copy link

ghost commented May 8, 2019

On start bl-touch does an autotest and if not in fault it means pin can move down and up and finally is up? is not enough a simple stow?

Ok. I understand better now.

Let's give @InsanityAutomation a chance to comment, but I also would like to ask, to what degree is this a problem for you specifically, as others have expressed approval at having more opportunities to catch their BLTouch misbehaving?

But I agree: Until a fault happens, a STOW - DEPLOY - STOW is debatable. STOW followed by a check for success and perhaps retry should be enough on Marlin start, on G28 etc. But that would be no problem to put in a new PR after some discussion.

@InsanityAutomation
Copy link
Contributor Author

We can discuss a bit more in depth later on if you'd like but as I'm finishing prepping dinner here a quick version is that I intended to clear potential inconsistent states left from switch mode. A reset command does not clear the input from switch mode, only a stow. In the interest of not causing a change of state by issuing a move command from a report, which would potentially break the high speed mode shown in the Delta video antclabs published, I felt this was more appropriate. That high speed mode not issuing a stow before doing the next check may be enabled for Delta's as the limiting factor is z raise response in the near future. So in the interest of being certain was it a leftover signal from switch mode or an actual alarm, I decided to error on the side of caution given how many issues we've seen with these probes as of late. This gives us the greatest chance of recovering from an inconsistent or error state and continuing without user interruption.

@GMagician
Copy link
Contributor

@InsanityAutomation I don't know about "switch mode" but let resume:
Old bltouch:
it start with auto-test and if everything is ok pin is up and no fault. if pin, for some reason, falls...then it will signal a fault (if my memory is good). So to be sure not to break anything, if bltouch is triggered, before moving, is enough to stow it and check again fault state. This "mode of working" will prevent all these deploy-stow actions, at worst it will be done only when trying to move when "triggered"
New bltouch:
From my understanding is "switch mode" a mode that continue to give triggered instead of a 10ms signal?

its not clear to me what is the meaning of:

In the interest of not causing a change of state by issuing a move command from a report

damn to language, English is not my best, sorry

@FanDjango

Until a fault happens, a STOW - DEPLOY - STOW is debatable. STOW followed by a check for success and perhaps retry should be enough on Marlin start, on G28 etc.

Yes, and I'm wondering why this solution is not considered "safe enough" for everyone... stow-deploy-stow is done only when really needed...in other cases just a stow + check will guarantee that pin is up

But that would be no problem to put in a new PR after some discussion.

of course not, but since I have limited knowledge about "bltouch" world I asked here if this solution is really needed to be done this way

@ghost
Copy link

ghost commented May 9, 2019

@GMagician
Let me see if I can add some data:

I had a look at it myself on my printer just now:

  1. Power ON: Yes, BLTOUCH self test
  2. Marlin Initialization nearly over: No check if already in alarm. And then another self-test.

Hmmm. Then I tried the reset button of the controller board.

But if someone presses the "reset button", there is no self-test by BLTOUCH, only the one from Marlin initialization. So that's safer in case the BLTouch is freshly blocked by chewing gum.

In case this single (or multiple) self tests result in an alarm, this will not be acted upon. It will be caught later on the first attempt to do anything - because why hassle the user if he is not intending to use the probe. The alarm will stay on the probe for the time being.

So actually, since we would catch the problem only later when the probe is actually used, why even do the self-test? Well here there is room for improvement (later stage), as we are doing this step-by-step to avoid huge PR's: With the new "status_proc", we can find out if the probe is deployed and in an error state and refuse to move XY to avoid breaking something.

And yes, when the probe is to be used, it could therefore be triggered already. Yes, one could check this and do all the clearing attemps before the actual desired action. That way you are clearing an error that occurred due to the previous action, perhaps some time ago. This caused problems with debugging - people who were looking for the failure reason failed to understand that the alarm clearing and perhaps failure was at a later time than when the probe was failing.

By the way, the alarm clearing sometimes fails. BLTouch has some bugs itself (especially on timing).

Instead of clearing before the next action, it is now checked and cleared directly after the action, and this is reported - the user can see, for example: A deploy was requested, and it failed.

at worst it will be done only when trying to move when "triggered"

Same with this sequence. Only done if there is an alarm as a result of an action.

The deploy and stow actions that you are complaining about are actually not due to any alarm: They are purposely hard coded STOW-DEPLOY-STOW sequences that happen even when there is no alarm:

  1. At Marin inititalisation (see above)
  2. At G28 and G34 (+ implicit G28)

(In the case of the G34 (two times) I am unhappy also - in my opinion this is an oversight)

These have nothing to do with the changed sequence. They are due to some other "safety" concerns.

These concerns were due to allowing the use of the SWITCH mode on M119 and in the lcd menus - which can lead to a third possible reason for a "triggered" condition sitting there, which it would be nice to clear away (remove the SWITCH mode) before moving. A STOW would be enough for that, but due to a bug in the BLTOUCH it actually needs a RESET + STOW.

So why do an additional DEPLOY and a STOW as well? Like I said in a previous comment, that is up for debate and could be changed if so desired. But as stated above, this also fits in with the strategy of checking for the success of an action. If I clear the probe I would like to know if it successfully cleared.

The "internal" users of this clearing process will do this check. The "external" users (init, G28, G34) currently do not, but in a later stage, *they certainly will.

A further mode of operation will also be allowed in the future - "high speed mode": Cleaning up the order in which things are done to the probe and how/when errors are checked (or not) is needed for that. Did you have a look at the video of that? That is future mode for fast Z-raise printers. This is what we are working towards

From my understanding is "switch mode" a mode that continue to give triggered instead of a 10ms signal?

Not totally correct: The "trigger signal" now reflects the position of the pin. If the pin oscillates up/down, so will the signal.

@ghost
Copy link

ghost commented May 9, 2019

I have documented the gist of what has been discussed in the past 5-6 comments.

See #13959

@GMagician
Copy link
Contributor

GMagician commented May 9, 2019

I think I can follow you but

The deploy and stow actions that you are complaining about are actually not due to any alarm: They are purposely hard coded STOW-DEPLOY-STOW sequences that happen even when there is no alarm:

is not completely true. Ok selftest forced by marlin on startup (it will signal some failing autotest or test bltouch when reset by button) and I see it twice because of mega reset after repetier connection (mega2560).
But after first startup, with no failing probe, when I ask to home G28, I see a deploy-stow action, X and Y move, and deploy-Z probe-stow...Why I have the first? is not in relation with G28 itself

And also G34...it does a deploy-stow, X move and deploy-Z probe-stow..

These first deploy-stow with no alarm condition are not clear to me (I say no alarm because of no red blinking led).

Not sure if this this will change anything, mine is a Chinese clone!

I can't follow you when you talk about "internal" and "external" users...that's my English capabilities, but

If I clear the probe I would like to know if it successfully cleared

is not enough check triggered state after "reset" + "stow"? if not triggered pin is up and working....maybe next commands, G28 and G34, have to properly check, when they deploy it, if some fault condition is present, after deploy, and before doing probe.

Please forgive me because I may say weird things because of my bad English interpretation

@ghost
Copy link

ghost commented May 9, 2019

But after first startup, with no failing probe, when I ask to home G28, I see a deploy-stow action, X and Y move, and deploy-Z probe-stow...Why I have the first? is not in relation with G28 itself

So you say you do a G28 and see:

DEPLOY-STOW (ok, that first on is the unneeded one, fair enough criticsism, see my PR #13959 )
Then you see XY move (ok, as is to be expected), right?
Then you see DEPLOY followed by STOW and again DEPLOY, then probe moves down? Is that what you see?

If I have understood that correctly:

The (reset)-stow-deploy is done if the deploy failed (trigger set). The alarm condition is so short you don't see the blinking.

Please:
your current BLTOUCH_DELAY setting or default?
Which BLTOUCH probe version? Ah, clone,ok
Would you be able to enable the levelling debugging and let's study the output?

@ghost
Copy link

ghost commented May 9, 2019

"internal" and "external" users

Sorry. other parts of the program calling BLTOUCH::stuff = external. BLTOUCH:: own progam uses "internal" routines.

internal and external callers I meant.

@ghost
Copy link

ghost commented May 9, 2019

is not enough check triggered state after "reset" + "stow"?

After many tests, sadly, no. Triggered state (thanks to ANTClabs JUST means ALARM). Not reliable to deduce the position of the pin from that.

Example: Pin is currently DOWN (deployed). Issue STOW command, but chewing gum is keeping the pin DOWN. BLTouch will go into ALARM mode, meaning "trigger signal" is HIGH continuously.
So, this ALARM is not telling me the position of the pin.
I get the same ALARM if I do the opposite (DEPLOY but the pin is stuck in the UP position). I will still get a trigger signal high.
But if I now set SWITCH mode (directly) this will clear the ALARM signal, and the signal will now reflect the position of the pin (HIGH for UP and LOW for DOWN).
Works on a clone, too.

@GMagician
Copy link
Contributor

DEPLOY-STOW (ok, that first on is the unneeded one, fair enough criticsism, see my PR #13959 )

That's what I say it's wrong (missed reading 13959, I try to re-read it and better understand it)

Then you see DEPLOY followed by STOW and again DEPLOY, then probe moves down? Is that what you see?

No I see deploy - Z probe - stow and this is correct

your current BLTOUCH_DELAY setting or default?

I think I never touched it. I also think that default is now increased to 500ms (if I correctly merged my cnf)

I think I only changed servo_delay

@ghost
Copy link

ghost commented May 9, 2019

No I see deploy - Z probe - stow and this is correct

Good, it's as it is supposed to be then

@ghost
Copy link

ghost commented May 9, 2019

That's what I say it's wrong (missed reading 13959, I try to re-read it and better understand it)

I put in that PR to provoke and channel the discussion, restricting it to the actual issues brought up here up to now. So that all points that come up are handled separately

@GMagician
Copy link
Contributor

ops....sorry...when I read #13959 I thought, I don't know why, it was an old and already merged PR..

when I read in it

The DEPLOY-STOW added sequence is used after detecting the failure of a DEPLOY or STOW from G28, G29, G34. It isn't needed at the beginning of such a command.

I though that this was the sequence I see now...but in that phrase you are simply saying you have done the changes I talking here!

@Roxy-3D
Copy link
Member

Roxy-3D commented May 10, 2019

Oh Oh! I just got an email that BL-Touch v3.1 is out and on its way to me....

@Rene046
Copy link

Rene046 commented Sep 4, 2019

Do you guys also know something about the 3d touch ?

@InsanityAutomation
Copy link
Contributor Author

Do you guys also know something about the 3d touch ?

Only that it hangs on any v3 command so leave em all off in config adv

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

Successfully merging this pull request may close these issues.

7 participants