-
-
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
Simplify: Z_AFTER_DEACTIVATE, UNKNOWN_Z_NO_RAISE, do_z_clearance #20444
Simplify: Z_AFTER_DEACTIVATE, UNKNOWN_Z_NO_RAISE, do_z_clearance #20444
Conversation
- Expect axis is allways trusted (Pos expected by Z_IDLE_POS) - raise allowed (unless Z_IDLE_POS = Z_MAX_POS) - Allways do a absolute Move from Z0 - No more multiple raises - Allow lower by request
See earlier commit to understand
Marlin/src/module/motion.cpp
Outdated
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 |
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.
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) };
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 believe TERN only works with constants that act as boolean options. They can be undefined, 0, or 1.
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.
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.
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.
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
.
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 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.
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. |
Mentioning the following PR's:
If we want Marlin to do a Z rise everytime after steppers are deactivated we must have a reason for that. If we dont want Marlin to rise before homing, it's because our bed drops. For this we had In this PR they now come together! @qwewer0 I realy would like to hear @thinkyhead 's opinion on this topic 😅 |
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
I agree. Others opinion would be useful too. |
@swissnorp what does your code currently do if @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. |
(With or without this PR), First ⬇️ call after the printer was powered on: G28/G28 Z
G28 XY
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
G28 XY
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
G28 Z
G28 XY
The operations above are how I think it should work. (Without this PR) ⬇️ Call after a G28 XY, but without stepper deactivation: G28
G28 Z
G28 XY
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. (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. (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. (Using this PR) ⬇️ Call after a G28, but without stepper deactivation: G28
G28 Z
G28 XY
The operations above are how I think it should work. (Using this PR) ⬇️ Call after a G28 XY, but without stepper deactivation: G28
G28 Z
G28 XY
The operations above are how I think it should work. 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") |
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.
@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 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
Could you test this again and make sure
There is an option for that: Uncomment 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 |
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 |
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
Description
Z_AFTER_DEACTIVATE
andUNKNOWN_Z_NO_RAISE
intoZ_IDLE_POS
do_z_raise
down to two variables:zclear
andlower_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
Related Issues
#19428 / I will comment on that tomorrow.
#19913