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

Simplify: Z_AFTER_DEACTIVATE, UNKNOWN_Z_NO_RAISE, do_z_clearance #20444

Conversation

swissnorp
Copy link
Contributor

Description

  • Unifies Z_AFTER_DEACTIVATE and UNKNOWN_Z_NO_RAISE into Z_IDLE_POS
  • Simplifies do_z_raise down to two variables: zclear and lower_allowed

Tested on cartesian for:
Z_IDLE_POS = Z_MIN_POS
Z_IDLE_POS = Z_MAX_POS

It may needs some testing on other machines and proper review!

Benefits

  • Code gets more compact and simple.
  • Users may understand better what its all about do_z_clearance (absolut move and not relative).
  • Z-raise only if needed and permitted. No more multiple z raises.
  • Lowering permitted if needed.

Related Issues

#19428 / I will comment on that tomorrow.
#19913

xyze_pos_t current_position = { X_HOME_POS, Y_HOME_POS, Z_IDLE_POS };
#else
xyze_pos_t current_position = { X_HOME_POS, Y_HOME_POS, Z_HOME_POS };
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this work?
xyze_pos_t current_position = { X_HOME_POS, Y_HOME_POS, TERN(IF_ENABLED(Z_IDLE_POS), Z_IDLE_POS , Z_HOME_POS) };

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe TERN only works with constants that act as boolean options. They can be undefined, 0, or 1.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, my bad, IF_ENABLED would give null if disabled.
Is there a macro that gives 1 if enabled and 0 if disabled? I still think that code should be shrunken down with macro.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the introduction of the TERN macros you will see patterns like this in various places:

#ifdef Z_IDLE_POS
   #define HAS_Z_IDLE_POS 1
#endif
...
TERN(HAS_Z_IDLE_POS, Z_IDLE_POS, Z_HOME_POS);

That first portion might be in the file that needs it, or it might be off somewhere like Conditionals_post.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to solve it with TERN first and failed 😄 .

#ifdef Z_IDLE_POS
   #define HAS_Z_IDLE_POS 1
#endif
...
TERN(HAS_Z_IDLE_POS, Z_IDLE_POS, Z_HOME_POS);

@sjasonsmith Thanks for that. I will implement it later on.

@qwewer0
Copy link
Contributor

qwewer0 commented Dec 12, 2020

This PR not only did not help on #19428 but it made the behavior back as it was prior to #20323, which is a step backwards in to the wrong direction my opinion.

So I don't think this PR should be merged just yet.

@sjasonsmith
Copy link
Contributor

Thanks @qwewer0. It seems like every time something with this behavior is fixed, something else breaks. I have certainly done that myself.

I haven't actually reviewed the content of the PR yet, so don't fully understand the intended improvements. I'll look at it further tomorrow. Your testing and feedback of it is certainly helpful.

@swissnorp swissnorp changed the title Simplify: Z_AFTER_DEACTIVATE, UNKNOWN_Z_NO_RAISE, do_z_raise Simplify: Z_AFTER_DEACTIVATE, UNKNOWN_Z_NO_RAISE, do_z_clearance Dec 12, 2020
@swissnorp
Copy link
Contributor Author

This PR not only did not help on #19428 but it made the behavior back as it was prior to #20323, which is a step backwards in to the wrong direction my opinion.

So I don't think this PR should be merged just yet.

Mentioning the following PR's:
#18576
#18906

do_z_clearance is not intended to raise everytime you call it!
It's only intended to raise Z, if clearance is not enough and never lower (for the most cases).

#19428 (comment)

If we want Marlin to do a Z rise everytime after steppers are deactivated we must have a reason for that.
In the most cases (or even allways) this reason is, that the nozzle moves down by a bit or even till it's touching the bed after steppers are disabled.
And for this we had Z_AFTER_DEACTIVATE.

If we dont want Marlin to rise before homing, it's because our bed drops. For this we had UNKNOWN_Z_NO_RAISE.

In this PR they now come together!


@qwewer0
If we wanted to have a Z raise for first homing after Z steppers were released. And at the same want to prevent it to rise over and over again (for doing G28XY without G28Z) ther must be a flag set.
At the beginning #18906 this was my idea. But we decided to go with Z_AFTER_DEACTIVATE.


I realy would like to hear @thinkyhead 's opinion on this topic 😅

@qwewer0
Copy link
Contributor

qwewer0 commented Dec 12, 2020

If we want Marlin to do a Z rise everytime after steppers are deactivated we must have a reason for that.

As I wrote it here #19428 (comment), a manufacturer base line is that their products Z axis won't fall, but over time and maybe with some grease, that might change, and as Z_AFTER_DEACTIVATE wasn't enabled then if a user cannot or not willing to thinker with marlin, then the printer might become unusable.
For bed drop, a z raise on the next homing could cause stepper grind, but that isn't as destructive as a nozzle grinding a bed.

I realy would like to hear @thinkyhead 's opinion on this topic 😅

I agree. Others opinion would be useful too.

@sjasonsmith
Copy link
Contributor

If we wanted to have a Z raise for first homing after Z steppers were released. And at the same want to prevent it to rise over and over again (for doing G28XY without G28Z) ther must be a flag set.

@swissnorp what does your code currently do if G28XY is called twice in a row? Will it raise twice?
I ask because I have "fixed" z-raise in the past and introduced behavior that would cause a multiple-raise, and multiple people noticed and reported bugs.

@qwewer0 you mentioned this does not work correctly, or even makes some things worse. Can you elaborate on which behaviors you are concerned with?

There is a lot of information in this PR and the associated bug reports, so it is quite difficult to understand the full impact of the change.

We might need something like a testing matrix that compares the old and new behavior in various scenarios.

@qwewer0
Copy link
Contributor

qwewer0 commented Dec 21, 2020

@qwewer0 you mentioned this does not work correctly, or even makes some things worse. Can you elaborate on which behaviors you are concerned with?

(With or without this PR), First ⬇️ call after the printer was powered on:

G28/G28 Z
  1. Z axis rises by Z_CLEARANCE_BETWEEN_PROBES (I think)
  2. X and Y homed
  3. X and Y moves to Z_SAFE_HOMING
  4. Z axis rises by (Z_CLEARANCE_DEPLOY_PROBE - Z_CLEARANCE_BETWEEN_PROBES - Z probe offset) (I think)
  5. Z probed/homed
  6. Z rises to Z_CLEARANCE_DEPLOY_PROBE (I think)
G28 XY
  1. Z axis rises by Z_CLEARANCE_BETWEEN_PROBES (I think)
  2. X and Y homed

The above operations are how I think it should work.


(Without this PR) ⬇️ Call after a G28/G28 XY and stepper deactivation:

G28/G28 Z
  1. Z axis rises by Z_CLEARANCE_BETWEEN_PROBES (I think)
  2. X and Y homed
  3. X and Y moves to Z_SAFE_HOMING
  4. (No Z raise)
  5. Z probed/homed
  6. Z rises to Z_CLEARANCE_DEPLOY_PROBE (I think)
G28 XY
  1. Z axis rises by Z_CLEARANCE_BETWEEN_PROBES (I think)
  2. X and Y homed

The above operations are how I think it should work, but if the 4th point on "First call after the printer was powered on" does a raise, then I think after a stepper deactivation this should raise too, but it would depend on what is the original intention of that raise, or if it is even needed at the fist occasion too.

(Without this PR) ⬇️ Call after a G28, but without stepper deactivation:

G28
  1. (No Z raise)
  2. X and Y homed
  3. X and Y moves to Z_SAFE_HOMING
  4. (No Z raise)
  5. Z probed/homed
  6. Z rises to Z_CLEARANCE_DEPLOY_PROBE (I think)
G28 Z
  1. (No Z raise)
  2. Z probed/homed
  3. Z rises to Z_CLEARANCE_DEPLOY_PROBE (I think)
G28 XY
  1. (No Z raise)
  2. X and Y homed

The operations above are how I think it should work.
(Not sure if there should be a raise at the 4th point or not after a stepper deactivation. It would depend on what is the original intention of that raise)

(Without this PR) ⬇️ Call after a G28 XY, but without stepper deactivation:

G28
  1. Z axis rises by Z_CLEARANCE_BETWEEN_PROBES (I think)
  2. X and Y homed
  3. X and Y moves to Z_SAFE_HOMING
  4. (No Z raise)
  5. Z probed/homed
  6. Z rises to Z_CLEARANCE_DEPLOY_PROBE (I think)
G28 Z
  1. Z axis rises by Z_CLEARANCE_BETWEEN_PROBES (I think)
  2. X and Y moves to Z_SAFE_HOMING
  3. (No Z raise)
  4. Z probed/homed
  5. Z rises to Z_CLEARANCE_DEPLOY_PROBE (I think)
G28 XY
  1. Z axis rises by Z_CLEARANCE_BETWEEN_PROBES (I think)
  2. X and Y homed

The operations above should not have a raise at the 1st point. Without a stepper deactivation, there should not be a raise on the next G28/G28 Z/G28 XY call.
(Not sure if there should be a raise after "X and Y moves to Z_SAFE_HOMING" or not after a stepper deactivation. It would depend on what is the original intention of that raise)


(Using this PR) ⬇️ Call after a G28 and stepper deactivation:

G28/G28 Z
  1. (No Z raise)
  2. X and Y homed
  3. X and Y moves to Z_SAFE_HOMING
  4. (No Z raise)
  5. Z probed/homed
  6. Z rises to Z_CLEARANCE_DEPLOY_PROBE (I think)
G28 XY
  1. (No Z raise)
  2. X and Y homed

The operations above should have a raise at the 1st point. After a stepper deactivation, there should always be a raise on the next G28/G28 Z/G28 XY call.
(Not sure if there should be a raise after the 4th point or not after a stepper deactivation. It would depend on what is the original intention of that raise)

(Using this PR) ⬇️ Call after a G28 XY and stepper deactivation:

G28/G28 Z
  1. (No Z raise)
  2. X and Y homed
  3. X and Y moves to Z_SAFE_HOMING
  4. Z axis rises by (Z_CLEARANCE_DEPLOY_PROBE - Z_CLEARANCE_BETWEEN_PROBES - Z probe offset) (I think)
  5. Z probed/homed
  6. Z rises to Z_CLEARANCE_DEPLOY_PROBE (I think)
G28 XY
  1. (No Z raise)
  2. X and Y homed

The operations above should have a raise at the 1st point. After a stepper deactivation, there should always be a raise on the next G28/G28 Z/G28 XY call.
(Not sure if there should be a raise after "X and Y moves to Z_SAFE_HOMING" or not after a stepper deactivation. It would depend on what is the original intention of that raise)

(Using this PR) ⬇️ Call after a G28, but without stepper deactivation:

G28
  1. (No Z raise)
  2. X and Y homed
  3. X and Y moves to Z_SAFE_HOMING
  4. (No Z raise)
  5. Z probed/homed
  6. Z rises to Z_CLEARANCE_DEPLOY_PROBE (I think)
G28 Z
  1. (No Z raise)
  2. Z probed/homed
  3. Z rises to Z_CLEARANCE_DEPLOY_PROBE (I think)
G28 XY
  1. (No Z raise)
  2. X and Y homed

The operations above are how I think it should work.
(Not sure if there should be a raise after "X and Y moves to Z_SAFE_HOMING" or not after a stepper deactivation. It would depend on what is the original intention of that raise)

(Using this PR) ⬇️ Call after a G28 XY, but without stepper deactivation:

G28
  1. (No Z raise)
  2. X and Y homed
  3. X and Y moves to Z_SAFE_HOMING
  4. Z axis rises by (Z_CLEARANCE_DEPLOY_PROBE - Z_CLEARANCE_BETWEEN_PROBES - Z probe offset) (I think)
  5. Z probed/homed
  6. Z rises to Z_CLEARANCE_DEPLOY_PROBE (I think)
G28 Z
  1. (No Z raise)
  2. X and Y moves to Z_SAFE_HOMING
  3. Z axis rises by (Z_CLEARANCE_DEPLOY_PROBE - Z_CLEARANCE_BETWEEN_PROBES - Z probe offset) (I think)
  4. Z probed/homed
  5. Z rises to Z_CLEARANCE_DEPLOY_PROBE (I think)
G28 XY
  1. (No Z raise)
  2. X and Y homed

The operations above are how I think it should work.
(Not sure if there should be a raise after "X and Y moves to Z_SAFE_HOMING" or not after a stepper deactivation. It would depend on what is the original intention of that raise)


If bed moves for Z-axis instead of the nozzle, then I don't think a Z-axis raise is needed after a stepper deactivation, because there is no way for the bed to move closer to the nozzle without external influence. So maybe there should be an option for that.

(Tested on an Ender 3, that is why I used "Z-axis rises" and not "bed lowers")

thinkyhead and others added 4 commits January 5, 2021 06:30
This is obsolete. Because the intend of this pull request is simplification: `UNKNOWN_Z_NO_RAISE` will be replaced by `Z_IDLE_POS Z_MAX_POS`. If z stepper was released or never homed, marlin will pretend it is at Z_MAX_POS and there will be no rise.
@swissnorp
Copy link
Contributor Author

@qwewer0 Thank you for testing this and sorry for my very late reply.

My results are different from yours and i tested all possible cases!

Have you had Z_IDLE_POS uncommented after you merged this PR? I guess not! And to be honest i dont know why you had not.

If you had, the results would be as you may wish, with a rise at the first point and a proper rise at the fourth point (if needed for deploy clearance).

We may could discuss about to have Z_IDLE_POS uncommented as default and forcing it to be enabled.

(Using this PR) ⬇️ Call after a G28 and stepper deactivation:
G28/G28 Z
G28 XY

The operations above should have a raise at the 1st point. After a stepper deactivation, there should always be a raise on the next G28/G28 Z/G28 XY call.
(Not sure if there should be a raise after the 4th point or not after a stepper deactivation. It would depend on what is the original intention of that raise)

(Using this PR) ⬇️ Call after a G28 XY and stepper deactivation:
G28/G28 Z
G28 XY

The operations above should have a raise at the 1st point. After a stepper deactivation, there should always be a raise on the next G28/G28 Z/G28 XY call.
(Not sure if there should be a raise after "X and Y moves to Z_SAFE_HOMING" or not after a stepper deactivation. It would depend on what is the original intention of that raise)

Could you test this again and make sure Z_IDLE_POS is uncommented and set to either Z_HOME_POS or Z_MIN_POS (which is the same for an ender).
I think you would like the result 😅 .

If bed moves for Z-axis instead of the nozzle, then I don't think a Z-axis raise is needed after a stepper deactivation, because there is no way for the bed to move closer to the nozzle without external influence. So maybe there should be an option for that.

(Tested on an Ender 3, that is why I used "Z-axis rises" and not "bed lowers")

There is an option for that: Uncomment Z_IDLE_POS and define it as Z_MAX_POS.

To be honest, it is all written in the description of this PR and you should read it and change your configuration. Thanks.

Kind regards, norp

@qwewer0
Copy link
Contributor

qwewer0 commented Jan 15, 2021

We may could discuss about to have Z_IDLE_POS uncommented as default and forcing it to be enabled.

My tests was aimed for printers that have default configurations and don't have a way to change it. So if for the affected printers the Z_IDLE_POS would be defined, then that would solve the problem. 👍

thinkyhead added a commit to MarlinFirmware/Configurations that referenced this pull request Feb 25, 2021
@thinkyhead thinkyhead merged commit bcda46e into MarlinFirmware:bugfix-2.0.x Feb 25, 2021
vyacheslav-shubin pushed a commit to vyacheslav-shubin/Marlin that referenced this pull request Mar 10, 2021
)

Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
vyacheslav-shubin pushed a commit to vyacheslav-shubin/Marlin that referenced this pull request Mar 10, 2021
)

Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
W4tel-BiDi pushed a commit to W4tel-BiDi/Marlin that referenced this pull request Apr 5, 2021
)

Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
thinkyhead added a commit that referenced this pull request Apr 30, 2021
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
@swissnorp swissnorp deleted the PullRequest-Z_AFTER_DEACTIVATE+UNKNOWN_Z_NO_RAISE--to-Z_IDLE_POS branch May 1, 2021 18:17
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