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.1 updates based on final probe sample unit tests #14064

Closed
wants to merge 14 commits into from
Closed

BLTouch v3.1 updates based on final probe sample unit tests #14064

wants to merge 14 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 19, 2019

READY FOR FINAL REVIEW

Latest BLTOUCH V3.1 version - SET 5V MODE is back but needs command sequence

I know this is getting tedious - but on the other hand the communication with ANTClabs is allowing us to influence the features and behavior of the (hopefully) final version of the BLTOUCH V3.1 prior to its release.

Thanks to the cooperation of ANTClabs, the newest version of the V3.1 BLTOUCH brings back the 5V mode being configurable by software alone. The new "mode_conv_proc" method produces the needed sequence.

But maybe the single commands also work by themselves. <-- we will see.

This is highly preliminary and is supposed to show the changes that might be needed sometime this week after testing the newest BLTOUCH V3.1 incarnation.

If all goes well we are informed that the BLTOUCH V3.1 is scheduled to be released towards the end of the coming week.

Informative back references are to PRs #14015, #13959, #13814 and issue #13345.

CR10s PRO users might like to read this, as a solution has been found for the flaky BLTOUCH performance on these machines.

Trigger mention: @thinkyhead @Roxy-3D @Bob-the-Kuhn @InsanityAutomation

@ghost
Copy link
Author

ghost commented May 19, 2019

Newest info from ANTClabs:

"The last command for mode conversion (S140 or S150) will be activated immediately and the 10ms red LED flashes two times and push-pin will be stowed. Therefore, power on / off is not required for approval."

Good news. We can use our current logic, we just need the correct command sequence instead of the single command we used to use. And a V3.0 will understand that also.

@thinkyhead thinkyhead changed the title Latest BLTOUCH V3.1 version - SET 5V MODE is back but needs command sequence BLTouch v3.1 updates based on best info May 19, 2019
@ghost
Copy link
Author

ghost commented May 22, 2019

I have been testing and there is something that needs to be discussed.

This pertains to the final version of the V3.1 BLTOUCH, as of 2019-05-20.

First of all, the probe can be set to 5V mode or back to OD mode by software alone, and of course by performing the right steps in the LCD configuration menu.

In both cases, the result of the setting-sequence will be stored in the probe's eeprom

@InsanityAutomation vehemently wanted this software setting capability, so that the needed 5V (or OD mode) could be set using a #DEFINE in the example configurations for printers (such as the very finnicky CR models).

Now here are the caveats:

  1. Since there is no way to interrogate the probe as to its stored setting, a desired 5V mode or OD mode from the config would be set at every power on. That is one eeprom write at every power on. It should definitely not be set at every G28 and other places, and doing it every power on is debatable as well.

  2. If paranoid, you would also discuss setting OD mode if the config does not specify 5V mode (in case some had manually set the probe to 5V from the LCD).

pro

Marlin takes full control over the setting of the probe unless neither 5V or OD mode is specified in configuration.h

contra

Once a 5V or OD mode setting is stored in the probe, there is no need to ever set it again. 100.000 eeprom writes boils down to 100.00 / 365 / n-reboots-per-day (say 5) = 54 years (Oh so its actually that long?).

idea

Perhaps we should also add two LCD configuration commands which perform the 5V or OD mode sequence for better ease of use. The user of some printers would connect the probe and then they would go into the menu and set it up once, finished. If they specify no parms in the configuration, there would be no eeprom wear and no delay. Just needs to be explained to them. That way both options would be available.

@ghost
Copy link
Author

ghost commented May 22, 2019

Todo List:

  1. Better BLTOUCH section for configuration.h with explanations of the possible options
    sample proposed, see below
  2. Put the 5V/OD sequences into the LCD configuration as single commands for ease of use.
    edit Done, see below.

@ghost
Copy link
Author

ghost commented May 22, 2019

And a question:

All the needed BLTOUCH LCD commands are in menu configuration-bltouch. There is really no need for

  //
  // BLTouch Self-Test and Reset
  //
  #if ENABLED(BLTOUCH)
    MENU_ITEM(gcode, MSG_BLTOUCH_SELFTEST, PSTR("M280 P" STRINGIFY(Z_PROBE_SERVO_NR) " S" STRINGIFY(BLTOUCH_SELFTEST)));
    if (!endstops.z_probe_enabled && bltouch.triggered())
      MENU_ITEM(gcode, MSG_BLTOUCH_RESET, PSTR("M280 P" STRINGIFY(Z_PROBE_SERVO_NR) " S" STRINGIFY(BLTOUCH_RESET)));
  #endif

in menu_advanced.cpp, is there?. I am suggesting removing this.

And it also allows making triggered() private hint hint 👍

@Roxy-3D
Copy link
Member

Roxy-3D commented May 22, 2019

The fact that the user is blind to the actual mode is troublesome. Especially users that have machines that are not running with precise clock speeds. What I'm concerned about is the user not really sending a command to switch modes, but the probe thinks it saw the mode switch frequency and does the switch.

I'm kind of thinking an LCD command to switch modes upon user request... And not trying to do it at every start up makes more sense. But no matter which path we go, there are pitfalls.

@ghost
Copy link
Author

ghost commented May 22, 2019

@Roxy-3D

The fact that the user is blind to the actual mode is troublesome

Not totally blind. ANTClabs have added a blink-code for 5V at power up as per our request (thanks), it is documented and quite visible. Maybe I will post a video here later. Also, when performing the voltage setup sequence by hand or per our little proc, the step by step commands are signalled nicely now by a blink of the LED, the final setting is confirmed also by the same blink-code as at start up.

Probes that are not in 5V mode, i.e. in safe mode, behave in the normal fashion, no blink.

Especially users that have machines that are not running with precise clock speeds. What I'm concerned about is the user not really sending a command to switch modes, but the probe thinks it saw the mode switch frequency and does the switch.

That's why the switch is NOW a sequence ( DEPLOY-SET_XX_MODE-STORE_MODE-SET_XX_MODE) with certain minimum times. Need some really flaky hardware to send that by mistake. Please note that the V3.0 was in dire danger of what you mentioned as the 5V mode could certainly be inadvertently set.

I'm kind of thinking an LCD command to switch modes upon user request...

I have already put that into this PR so that anyone can pull and test.

And not trying to do it at every start up makes more sense

The way I have coded it right now is that if you don't specify anything in configuration.h then it is not done at startup.

But no matter which path we go, there are pitfalls.

What shall I say? I would like for the topic of BLTOUCH to finally come to rest at some point.

@ghost
Copy link
Author

ghost commented May 22, 2019

@ghost ghost changed the title BLTouch v3.1 updates based on best info BLTouch v3.1 updates based on final probe sample unit tests May 22, 2019
@thinkyhead
Copy link
Member

I like the idea of the one-time setup and also having the EEPROM remember what that last setup values were so that Marlin can report them in the LCD Info Menu and mark the settings as ON/OFF in the BLTouch sub-menu. Users just need to remember to set up again if they replace the probe.

@thinkyhead
Copy link
Member

thinkyhead commented May 22, 2019

A last_mode_set saved to EEPROM would be another way to preserve the life of the BLTouch EEPROM. When Marlin boots it would check if the mode set in Configuration.h isn't the last mode that was set. If the mode is different then it changes the BLTouch mode, updates the last_mode_set, and stores the flag to EEPROM.

Currently we don't have a neat way to write just a single item to EEPROM with its own reserved place. But doing a settings.save only writes changed settings to EEPROM, so it's economical for a single changed setting value at boot.

@ghost
Copy link
Author

ghost commented May 23, 2019

Here are the results of some testing with the above changes:

(connected to my MKS SBASE 1.3 board, which is clean on the input pin).

Tested G28, G29, M48, G34

Geetech Clone
Works

BLTouch V2.0
Works
Goes into alarm mode if command "STORE MODE" is issued. This is not a problem, as a user would not activate the voltage options in configuration.h anyway.

BLTOUCH V3.0
Works

BLTOUCH V3.1 (final version)
Works

FanDjango and others added 14 commits May 23, 2019 10:42
The newest version of the V3.1 BLTOUCH brings back the 5V mode being configurable by software alone. The new "mode_conv_proc" produces the needed sequence.
... to explain our woes with the BLTOUCH evolution and trying to keep up with it
Using a real V3.1, the following small changes and we are set to go.
As I expect to really get clobbered on this one, I am definitely not going to change all the example configuration files until a final version has come out of this.
You can read it over and over again, and always find something...
And the editing goes on...
@ghost
Copy link
Author

ghost commented May 23, 2019

Tentative To-do list now consists of:

  1. Implement Voltage Mode storage in Marlin EEPROM

I like the idea of the one-time setup and also having the EEPROM remember what that last setup values were so that Marlin can report them in the LCD Info Menu and mark the settings as ON/OFF in the BLTouch sub-menu. Users just need to remember to set up again if they replace the probe.

A last_mode_set saved to EEPROM would be another way to preserve the life of the BLTouch EEPROM. When Marlin boots it would check if the mode set in Configuration.h isn't the last mode that was set. If the mode is different then it changes the BLTouch mode, updates the last_mode_set, and stores the flag to EEPROM.

Currently we don't have a neat way to write just a single item to EEPROM with its own reserved place. But doing a settings.save only writes changed settings to EEPROM, so it's economical for a single changed setting value at boot.

CAVEAT

I've finished coding this, but suddenly realised that that code would break the V3.0 handling:

For the BLTOUCH V3.0, people need to set BLTOUCH_FORCE_5V_MODE for quite a number of printers.

Since the V3.0 BLTOUCH probe does not have an EEPROM store itself, it always defaults on power up to OD mode. For this nice scheme to work, you would then need an additional parameter in configuration.h to override this mechanism for the V3.0 (like "BLTOUCH_I_REALLY_WANT_5V_SETTING_EVERYTIME" or whatever ("BLTOUCH_V30")

Aren't we making this too complicated? At 10 Marlin restarts per day, we get 27 years if the user does not "remove" the parm from the config after using it (for the V3.1).

And there are a large number of V3.0 probes out there.

@InsanityAutomation
Copy link
Contributor

Most likely tomorrow night I can make a followup to this that would include the following :
Toggle in config to set "safe" mode setting, which would send selected mode all the time for 3.0 with note that 3.1 may see reduced eeprom life
Add bit to eeprom for last state as Scott mentioned.
Update all sample configs
Add note to HS mode that its recommended for Deltas but Cartesian with a fast enough z may be able to handle it

I think with that we may have this finally put to bed...

@ghost
Copy link
Author

ghost commented May 23, 2019

@InsanityAutomation

I've finished coding this, but suddenly realised that that code would break the V3.0 handling

Most likely tomorrow night I can make a followup to this ...

Before this degenerates into a race: I need to go on a business trip (yes, over the weekend) for the next days, so go for it. The concept looks ok.

But but but and there is always a but:

Just asking myself: Do we really need this? REALLY? Look at the original BLTouch code, it was small and, ok, not the fastest. And now look at the bloated lot of logic. I agree: V3.0 brought things to a cusp. If all early V3.0s were spaced into the void, I would be happy. The ideal machine was the After-Apr5 V3.0. It was "real good".

On the other hand, I am also very happy with the final V3.1 - but I don't need to change its settings at all because I have sensible controller boards. Maybe it would be sufficient to have the needed mode conversion commands in the LCD menus and to completely forget about special settings in the configuration.h or needing to explain these.

You know in the end, unless we document this somewhere for huge amounts of people to easily find, we and @Bob-the-Kuhn and @Roxy-3D and a few others might end up being the only ones on the planet understanding the whole reasoning behind all this stuff - all because of the evolving sequence of BLTouch versions.

There are some forums out there and they are listening in on this. They are calling our discussions here "chatter". Well, before we put some untested und undiscussed stuff out for thousands of users, we will chatter to our hearts desire and then produce a solution. Never should we forget the many V2.0's, V2.1's and V3.0's (with whom all this started) out there. The code still needs to keep their investment viable.

We've got a couple of users out there who will give us early feed-back on our modifications and others should take care to accept: If something doesn't work on a special probe/printer situation, we will find the problem.

@InsanityAutomation
Copy link
Contributor

I'm perfectly happy letting you win any race! I have more than enough other projects! I've done eeprom work before though so figured I'd handle that part faster. I did make some headway here today as we discussed on discord for anyone who cares to see it - https://github.com/InsanityAutomation/Marlin/tree/BLTouchV31Followups

@Roxy-3D
Copy link
Member

Roxy-3D commented May 23, 2019

There are some forums out there and they are listening in on this. They are calling our discussions here "chatter". Well, before we put some untested and undiscussed stuff out for thousands of users, we will chatter to our hearts desire and then produce a solution.

I don't think they meant 'chatter' in a derogatory way. Given the number of variables and different versions of the probe (and clones), a lot of discussion is necessary to iterate to a solution.

@InsanityAutomation
Copy link
Contributor

I spent the last bit here trying to find a way to make a new marlin repo fork that all of us working on this could have write access to. GitHub seems to have a problem with me forking marlin twice. It wont fork again off @FanDjango's branch either for me to make a PR to his setup. I cant make a separate fork from my normal work to give us a repo to develop the bltouch stuff on. It seems if I want to do it correctly and let everyone work together on it, id need to make a new account, fork it, add all of us as collaborators, then work from our usual accounts.... What fun! So plan for the moment, im going to test what I have in my repo linked above completing most of what we discussed tonight. If it all looks good, ill open that pr which includes the changes here plus my additions. Hopefully we can have all of this in place before V3.1 goes to end users!

@Roxy-3D
Copy link
Member

Roxy-3D commented May 23, 2019

Yeah... I don't like that either... But what they expect you to do is fork a project, and then have various branches off of your top level fork. So what Scott recommends is you leave the top level fork unaltered. That makes it easy (easier!!!) to synchronize to the main branch. And then you can selectively absorb the updates on your lower level branches.

(This is coming from a Git amateur. But I'm pretty sure what I'm saying is close to the truth.)

@ghost ghost closed this May 23, 2019
@ghost ghost deleted the bugfix-2.0.x-BLTOUCH-0519 branch May 23, 2019 20:06
@InsanityAutomation
Copy link
Contributor

@Roxy-3D normally how I work, but the special case of wanting to have a few more working together than normal I was trying to get a separate repo to invite everyone to work together on the same branch... If this covers all we need, great... If I hear a whisper of V3.2 changing things, ill make the new account, fork, and invite everyone in a heartbeat!

@Roxy-3D
Copy link
Member

Roxy-3D commented May 24, 2019

@FanDjango Please don't give up on us! This effort is going to get merged! And it is going to help a lot of people! But we are all 'Part Time' and not being paid. Some times it takes a week for things to get merged.

Let's re-open this. And let's put all the extra Bell's and Whistles that we are being discussed in a new Pull Request so this one isn't changing. That will help speed up the merge.

The one thing I'm aware of that may need tweaking is I think we should change some wording. But we can do that in a follow up Pull Request:

/**
   * Use "HIGH SPEED" mode for probing.
   * Danger: Disable if your probe sometimes fails. Only suitable for stable well-adjusted systems.
   * This feature was designed for Delta's with very fast Z moves however higher speed cartesians may function
   * If the machine cannot raise the probe fast enough after a trigger, it may enter a fault state.
   */
  //#define BLTOUCH_HS_MODE

UPDATE: Oh! This Pull Request has sort of been re-opened. Or maybe duplicated is a better word. Let's all continue in #14104

@talldonkey
Copy link

There are some forums out there and they are listening in on this. They are calling our discussions here "chatter". Well, before we put some untested and undiscussed stuff out for thousands of users, we will chatter to our hearts desire and then produce a solution.

I made the post you are referencing. Apologies if the usage of the word chatter had unintended derogatory perception. My intent was to say more of hey theres lots of active work going on it if you wanted to dive in and check out the detailed conversations about it

This pull request was closed.
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