-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
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
Comprehensive BLTouch implementation #13841
Changes from 10 commits
08e2a47
8226fa9
1b5e111
9cedd30
5b0ac05
fac5e30
c75639c
aeb266d
29c3d40
305928b
08c37ce
09f6983
f99c7ee
a6692e2
e68abf4
2220afd
e377dcf
adf902c
8505f9f
7309e87
57d3b2f
eb1216f
0d4531c
2deb61d
c3a2cbc
8321995
1103a79
ed93c56
f9f5676
9f0573e
3cf4209
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,18 +35,8 @@ void stop(); | |
#define DEBUG_OUT ENABLED(DEBUG_LEVELING_FEATURE) | ||
#include "../core/debug_out.h" | ||
|
||
void BLTouch::command(const BLTCommand cmd) { | ||
//SERIAL_ECHOLNPAIR("BLTouch Command :", cmd); | ||
MOVE_SERVO(Z_PROBE_SERVO_NR, cmd); | ||
safe_delay(BLTOUCH_DELAY); | ||
} | ||
|
||
void BLTouch::init() { | ||
reset(); // Clear all BLTouch error conditions | ||
stow(); | ||
} | ||
|
||
bool BLTouch::triggered() { | ||
// Used in the main loop to check the pin state when probing and internally here | ||
return ( | ||
#if ENABLED(Z_MIN_PROBE_USES_Z_MIN_ENDSTOP_PIN) | ||
READ(Z_MIN_PIN) != Z_MIN_ENDSTOP_INVERTING | ||
|
@@ -56,41 +46,138 @@ bool BLTouch::triggered() { | |
); | ||
} | ||
|
||
bool BLTouch::set_deployed(const bool in_deploy) { | ||
if (in_deploy && triggered()) { // If BLTouch says it's triggered | ||
reset(); // try to reset it. | ||
_deploy(); _stow(); // Deploy and stow to clear the "triggered" condition. | ||
safe_delay(1500); // Wait for internal self-test to complete. | ||
// (Measured completion time was 0.65 seconds | ||
// after reset, deploy, and stow sequence) | ||
if (triggered()) { // If it still claims to be triggered... | ||
SERIAL_ERROR_MSG(MSG_STOP_BLTOUCH); | ||
stop(); // punt! | ||
return true; | ||
} | ||
void BLTouch::init() { | ||
// This is called by marlin.cpp on initialization | ||
// SET_5V_MODE (if enabled). OD_MODE is the default on power on. | ||
// This mode will stay active until manual SET_OD_MODE or power cycle | ||
#if ENABLED(BLTOUCH_FORCE_5V_MODE) | ||
_set_5V_mode(); // Set 5V mode if explicitely demanded (V3 upwards) | ||
#endif | ||
clear(); | ||
// There really should be no alarm outstanding now, and no triggered condition. But if there is, | ||
// there is no need to worry people here on init right at the start of the printer. | ||
} | ||
|
||
void BLTouch::clear() { | ||
_reset(); // RESET or RESET_SW will clear an alarm condition but... | ||
// ...it will not clear a triggered condition in SW mode when the pin is currently up | ||
// ANTClabs <-- CODE ERROR | ||
_stow(); // STOW will pull up the pin and clear any triggered condition unless it fails, don't care | ||
_deploy(); // DEPLOY to test the probe. Could fail, don't care | ||
_stow(); // STOW to be ready for meaningful work. Could fail, don't care | ||
} | ||
|
||
bool BLTouch::command(const BLTCommand cmd, millis_t ms) { | ||
#if ENABLED(BLTOUCH_DEBUG_MSGS) | ||
SERIAL_ECHOLNPAIR("BLTouch Command :", cmd); | ||
#endif | ||
MOVE_SERVO(Z_PROBE_SERVO_NR, cmd); | ||
// If BLTOUCH_DELAY is higher than the desired ms delay, it will override | ||
if (BLTOUCH_DELAY > ms) { | ||
ms = BLTOUCH_DELAY; | ||
} | ||
// Don't wait longer than necessary if an ALARM pops up. Check trigger | ||
// in increments of 50ms to catch the ALARM right away. | ||
// Instead of using safe_delay in the following loop, we could use delay | ||
// and do the calling of the temperature-manager ourself. See safe_delay() for | ||
// details. TODO | ||
while (ms > 50) { | ||
ms -= 50; | ||
safe_delay(50); | ||
if (cmd == BLTOUCH_RESET) continue; | ||
if (triggered()) return true; | ||
} | ||
safe_delay(ms); | ||
return triggered(); | ||
} | ||
|
||
#if ENABLED(BLTOUCH_V3) | ||
#if EITHER(BLTOUCH_FORCE_5V_MODE, ENDSTOPPULLUPS) \ | ||
|| ALL(Z_MIN_PROBE_USES_Z_MIN_ENDSTOP_PIN, ENDSTOPPULLUP_ZMIN) \ | ||
|| (USES_Z_MIN_PROBE_ENDSTOP && ENABLED(ENDSTOPPULLUP_ZMIN_PROBE)) | ||
set_5V_mode(); // Assume 5V DC logic level if endstop pullup resistors are enabled | ||
#elif true || ENABLED(BLTOUCH_FORCE_OPEN_DRAIN_MODE) | ||
set_OD_mode(); | ||
bool BLTouch::deploy_wrapper() { | ||
// Do a DEPLOY | ||
#if ENABLED(BLTOUCH_DEBUG_MSGS) | ||
SERIAL_ECHOLN("BLTouch DEPLOY requested"); | ||
#endif | ||
|
||
// Attempt to DEPLOY, wait for DEPLOY_DELAY or ALARM | ||
if (_deploy_query_alarm()) { | ||
// The deploy might have failed or the probe is already triggered (nozzle too low?) | ||
#if ENABLED(BLTOUCH_DEBUG_MSGS) | ||
SERIAL_ECHOLN("BLTouch ALARM or TRIGGER after DEPLOY, recovering"); | ||
#endif | ||
|
||
clear(); // Get the probe into start condition | ||
|
||
// Last attempt to DEPLOY | ||
if (_deploy_query_alarm()) { | ||
// The deploy might have failed or the probe is actually triggered (nozzle too low?) again | ||
#if ENABLED(BLTOUCH_DEBUG_MSGS) | ||
SERIAL_ECHOLN("BLTouch ALARM or TRIGGER after DEPLOY, recovery failed"); | ||
#endif | ||
|
||
SERIAL_ERROR_MSG(MSG_STOP_BLTOUCH); // Tell the user something is wrong, needs action | ||
stop(); // but i'ts not too bad, no need to kill, allow restart | ||
|
||
return true; // Tell our caller we goofed in case he cares to know | ||
} | ||
} | ||
|
||
// The BLTouch V3 (and V2) and some clones can be used in "SWITCH" mode. When triggered, | ||
// instead of a 10ms pulse, there is an indefinetly long trigger condition, until | ||
// reset by the STOW as soon as this "very very long" pulse is registered. Might help in cases | ||
// where the trigger pulse is difficult to register (noise, caps on the signal) | ||
// Entering this mode manually also enables M119 display of the probe pin state. | ||
#if ENABLED(BLTOUCH_FORCE_SW_MODE) | ||
_set_SW_mode(); // If explicitely demanded, RESET into SW mode | ||
#endif | ||
|
||
if (in_deploy) { | ||
_deploy(); | ||
#if ENABLED(BLTOUCH_V3) | ||
set_SW_mode(); | ||
// Now the probe is ready to issue a 10ms pulse (or longer, if in SW mode) when the pin goes up. | ||
// The trigger STOW (see motion.cpp for example) will pull up the probes pin as soon as the pulse | ||
// is registered. | ||
|
||
if (DEBUGGING(LEVELING)) DEBUG_ECHOLNPAIR("bltouch.deploy_wrapper()"); | ||
|
||
return false; // report success to caller | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be reversed. True for command successful, and false for command failed. Thats the typical convention. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be my preference as well. It would be easy to inverse the logic at all the very few occurences where these wrappers are called. Note that I have seen a few other places where "false" is "success" in Marlin, so I believe that the original coder of bltouch probably thought that he should adhere to it. I just stuck to that logic. Look at the original bltouch.cpp, that's where I took it from. The callers typically look like this: if (triggered() & wrapper(...)) -> error exit So it needs a true to error out. Saves quite some machine instructions (8-bit code size) was probably the reasoning. Oherwise it would have been: if (triggered() & !wrapper(...)) -> error exit One instruction more at least. Other constructs using if then else are also possible but use even more instructions in otherwise straight line code. Sooner or later one should identify these places and gradually turn the logic around. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In fact, a very common convention is zero for no error, or non-zero for an error code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't write the original, @thinkyhead There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
bool BLTouch::stow_wrapper() { | ||
// Do a STOW | ||
#if ENABLED(BLTOUCH_DEBUG_MSGS) | ||
SERIAL_ECHOLN("BLTouch STOW requested"); | ||
#endif | ||
|
||
// A STOW will clear a triggered condition in the probe (10ms pulse or the "very very long" pulse | ||
// in SW mode). | ||
// At the moment that we come in here, we might (pulse) or will (SW mode) see the trigger on the pin. | ||
// So even though we know a STOW will be ignored if an ALARM condition is active, we will STOW. | ||
// Note: If the probe is deployed AND in an ALARM condition, this STOW will not pull up the pin | ||
// and the ALARM condition will still be there. --> ANTClabs should change this behaviour maybe | ||
|
||
// Attempt to STOW, wait for STOW_DELAY or ALARM | ||
if (_stow_query_alarm()) { | ||
// The stow might have failed | ||
#if ENABLED(BLTOUCH_DEBUG_MSGS) | ||
SERIAL_ECHOLN("BLTouch ALARM or TRIGGER after STOW, recovering"); | ||
#endif | ||
|
||
_reset(); // This RESET will then also pull up the pin. If it doesn't | ||
// work and the pin is still down, there will no longer be | ||
// an ALARM condition though. | ||
// But one more STOW will catch that | ||
// Last attempt to STOW | ||
if (_stow_query_alarm()) { // so if there is now STILL an ALARM condition: | ||
#if ENABLED(BLTOUCH_DEBUG_MSGS) | ||
SERIAL_ECHOLN("BLTouch ALARM or TRIGGER after STOW, recovery failed"); | ||
#endif | ||
|
||
SERIAL_ERROR_MSG(MSG_STOP_BLTOUCH); // Tell the user something is wrong, needs action | ||
stop(); // but it's not too bad, no need to kill, allow restart | ||
|
||
return true; // Tell our caller we goofed in case he cares to know | ||
} | ||
} | ||
else _stow(); | ||
|
||
if (DEBUGGING(LEVELING)) DEBUG_ECHOLNPAIR("bltouch.set_deployed(", in_deploy, ")"); | ||
if (DEBUGGING(LEVELING)) DEBUG_ECHOLNPAIR("bltouch.stow_wrapper()"); | ||
|
||
return false; | ||
return false; // report success to caller | ||
} | ||
|
||
#endif // BLTOUCH |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -112,7 +112,6 @@ void GcodeSuite::G34() { | |
#endif | ||
|
||
#if ENABLED(BLTOUCH) | ||
bltouch.reset(); | ||
bltouch.stow(); | ||
#endif | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pulse width of a trigger is 10ms. There is a good chance this will be picked up by this at some point. Should have a flag for triggered seen and only fault out of true twice in a row. Also, this wont truly wait safe delay, It may be off by up to 49ms. Could do while(ms > 0) then safe_delay MIN(ms, 50));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
safe_delay()
:Why might this be off by 49ms? It does a possible rest value outside the loop at the end?
Apart from the timing overhead of thermalManger.manage_heater() it looks reasonably precise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I though that unless something is very wrong, there should never be a trigger while in this loop. If this is a wait after a DEPLOY, movement only begins after the wait is over. Similar logic applies to the other commands.
But that is a really good idea to not get out of the loop when there is noise.
And you are right: In no case do we want to get into a situation where we might issue the next command before the wait for the previous command is over.
I think it would be best to scrap this exiting of the loop altogether. I kind of though it would speed up things - but we are talking about reducing (for example) a 750ms STOW wait to 350ms, who cares actually. How often do we probe, anyway.
So I will remove the two lines and let's let the wait do its thing without any pre-exit.
What do think of putting
thermalManager.manage_heater();
in there and not callingsafe_delay
but justdelay
? It would mean an include fortemperature.h
at the top though. In the end I was against it, but didn't evaluate code-bytes against the two versions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes very nice idea for the original
safe_delay
by the way, but how does the code bytes overhead ofMIN
compare to just doing the rest after the end of the loop?Original:
New idea:
Looks shorter but how expensive is MIN (its a a couple of branches and an immediate compare, I think) vs. two call instructions. Could be smaller, if you want I will compile and compare the OBJ code (8 bit and 32 bit, of course).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'new idea' will break things... If safe_delay() is called with a small amount of time... say 25ms, no delay will be performed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! That code is super easy to KNOW the right thing is going to happen. I much prefer that form. And incidentally... That is more 'optimum' too. That will generate less machine language code than what is there right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked:
The MIN is expensive.
The "new idea" looks better in the c code. As I feared: It is longer on AVR machine code. It is slightly longer on 32 bit code. The original code knew what he was doing.
Do we care about 10-20 bytes of machine code in this specific case? In general, we do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10 or 20 bytes isn't a big deal to make sure the problem is fixed... With regard to MIN() being expensive... In this case we know the data types going into the macro. If we side step the MIN() macro and just use a ternary operator, I would think this ends up as small as possible. The big problem is the type millis is a 4 byte long. And those comparisons and arithmetic are not 'natural' on the 8-bit AVR's.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. The original coder knew what he was doing.
ms<50? ms : 50
in machine code is loads for the ms var, multiple compares and 2 branches. The original one is shorter in machine code, believe me. But less shorter on 32 bit machines. AVR - it's hell.If I were a CPU I would go berserk anyway, first the guy is asking
while (ms > 50)
god knows how many times, and then he wants to knowms < 50
that many times. Bonkers, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there is actually no problem