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

Fix G28 O #18727

Closed
wants to merge 1 commit into from
Closed

Fix G28 O #18727

wants to merge 1 commit into from

Conversation

GMagician
Copy link
Contributor

This should fix #18725

@thisiskeithb thisiskeithb added C: Motion Needs: Testing Testing is needed for this change labels Jul 20, 2020
@thisiskeithb
Copy link
Member

thisiskeithb commented Jul 20, 2020

That doesn't fix G28 O on my build when running it after sending M18 (disable steppers).

I enabled M111 S247 to debug and I get a "homing not needed, skip" message despite XYZ position being unknown (and flashing on the LCD).

@thisiskeithb
Copy link
Member

thisiskeithb commented Jul 20, 2020

Using all_axes_known() instead works as expected:

- if (!homing_needed() && parser.seen('O')) {
+ if (all_axes_known() && parser.seen('O')) {  

I had to change similar logic in #17681 to get Marlin to work as expected, which made me think it'd help here...and it did!

Maybe the homing_needed() TERN is incorrect?:

FORCE_INLINE bool homing_needed() {
return !TERN(HOME_AFTER_DEACTIVATE, all_axes_known, all_axes_homed)();
}

@thisiskeithb
Copy link
Member

thisiskeithb commented Jul 20, 2020

Is G28 O only supposed to work if you have HOME_AFTER_DEACTIVATE enabled?

Either the code or documentation is incorrect (or incomplete). From the G28 page with no mention of HOME_AFTER_DEACTIVATE being required for the O parameter:

image

@GMagician
Copy link
Contributor Author

I think this PR is not needed at all. It seems original code works the same way, but an answer to above question should be given

@thisiskeithb
Copy link
Member

I think removing HOME_AFTER_DEACTIVATE as a requirement for G28 O would be good since it opens the feature up to everyone, but I don't have the background as to why it was required in the first place.

@thinkyhead
Copy link
Member

If you have HOME_AFTER_DEACTIVATE enabled and one or more of the steppers has gone to sleep, then homing is considered to be required (at least, for the sleeping axes).

The TERN condition in homing_needed is correct. The HOME_AFTER_DEACTIVATE option makes Marlin more strict about when homing is needed. Without this option, Marlin only cares that you did a G28 once since booting the machine.

@thisiskeithb
Copy link
Member

So the G28 documentation should be updated to say HOME_AFTER_DEACTIVATE is required for G28 O?

@thinkyhead
Copy link
Member

For boolean arguments to a G-code command, boolval is preferred over seen except for certain parameters which have too much legacy. For example, a lot of old G-code examples include G28 X0 Y0 so these are stuck using seen.

The reasoning is that some default behaviors may be changed by configuration so that a command behaves as if it is always getting —for example— an "O1" parameter. To override this from G-code you would send O0. If seen is used to interpret the O parameter then O0 is just the same as O1.

@thinkyhead thinkyhead closed this Jul 20, 2020
@thinkyhead
Copy link
Member

So the G28 documentation should be updated to say HOME_AFTER_DEACTIVATE is required for G28 O?

No. That is not correct.

@thisiskeithb
Copy link
Member

thisiskeithb commented Jul 20, 2020

Should I put in a PR to update the G28 documentation or not?

Currently, it’s not clear that G28 O requires HOME_AFTER_DEACTIVATE and reads like it will work without it enabled.

@thinkyhead
Copy link
Member

It doesn't require HOME_AFTER_DEACTIVATE. All that HOME_AFTER_DEACTIVATE does is tell Marlin that if a stepper goes to sleep, it should consider the position untrustworthy. If the option is disabled, Marlin might still mark an axis as "unhomed" for some other unspecified reason.

@thisiskeithb
Copy link
Member

That’s the issue we’re trying to solve. If we disable steppers and send G28 O, Marlin reports “ homing not needed, skip”, when homing is needed due to the steppers timing out and XYZ position is now unknown.

@thinkyhead
Copy link
Member

Then someone needs to do a better analysis to determine whether boolval is returning the wrong thing, or whether the all_axes_known() function is returning the wrong thing before pushing through a patch.

@thinkyhead
Copy link
Member

And of course… make sure the tests are being done with the latest code.

@GMagician
Copy link
Contributor Author

@thinkyhead

Then someone needs to do a better analysis to determine whether boolval

The problem is not boolvar VS seen (latter works for sure but maybe former works as well).
Problem is that G28 O calls homing_needed to check whetever home is needed or not but such function depends on HOME_AFTER_DEACTIVATE to return all_axes_known or all_axes_homed.

Then the

So the G28 documentation should be updated to say HOME_AFTER_DEACTIVATE is required for G28 O?

is more than correct. Now G28 O is changes its behaviours because of such flag

hence

It doesn't require HOME_AFTER_DEACTIVATE

is right but such option changes behaviours.

In conclusion. Issue is because documentation reports "unknown position" but this is true only when HOME_AFTER_DEACTIVATE.

@GMagician GMagician deleted the fix-g28-with-O branch July 21, 2020 12:34
@GMagician
Copy link
Contributor Author

@thinkyhead

If seen is used to interpret the O parameter then O0 is just the same as O1.

Since O is used without any value I think yes, O1 and O0 should behave the same so it seems more "correct" seen

@thinkyhead
Copy link
Member

Now G28 O is changes its behaviours because of such flag

Its behavior should be understood as "Home only if 'needed'" and there is no setting that makes O do nothing at all. Try this test….

  • Boot the machine and send G28 O right away.
  • Send another G28 O to see what happens.
  • Send M18, then another G28 O to see what happens.

Try this procedure both with HOME_AFTER_DEACTIVATE enabled and then again with HOME_AFTER_DEACTIVATE disabled.

You will see that G28 O causes the machine to home the first time in both cases. On the second G28 O it causes the machine to not-home in both cases. (i.e., Not the same as G28 without O.) On the third G28 O with the steppers asleep, it will re-home if HOME_AFTER_DEACTIVATE is enabled.

Therefore, it makes a difference when you have G28 O at the start of your G-code files. It is not the same as just including G28 regardless of your HOME_AFTER_DEACTIVATE setting.

Since O is used without any value…

All "boolean" parameters are allowed to be used without any value as a substitute for "1".

@thisiskeithb
Copy link
Member

thisiskeithb commented Jul 22, 2020

With HOME_AFTER_DEACTIVATE disabled:

Boot the machine and send G28 O right away.

Printer homes.

Send another G28 O to see what happens.

Printer doesn't home.

Send M18, then another G28 O to see what happens.

Printer doesn't home.
(this is where I believe it should since the G28 documentation implies that it should)

With HOME_AFTER_DEACTIVATE enabled:

Boot the machine and send G28 O right away.

Printer homes.

Send another G28 O to see what happens.

Printer doesn't home.

Send M18, then another G28 O to see what happens.

Printer homes.


I would still expect the printer to home with HOME_AFTER_DEACTIVATE disabled and sending G28 O after steppers timeout either after X amount of time or after M18 as done in tests above, but it doesn't despite the fact that G28 documentation doesn't say that HOME_AFTER_DEACTIVATE is required for G28 O to work after steppers timeout.

@GMagician
Copy link
Contributor Author

@thinkyhead & @thisiskeithb

Its behavior should be understood as "Home only if 'needed'" and there is no setting that makes O do nothing at all. Try this test…

I already tested all situation and, as written before, I totaly agree with current program behaviour. G28 O does exactly what I "logically" think it should do.
But issue has been opened because what is written on ducumentation may lead someone (and it lead someone) to think that "unknown position" should be that way even after a M84. Now what I propose is just a different text on documentation that may remove this doubt/misunderstanding.

I would still expect the printer to home with HOME_AFTER_DEACTIVATE

As written above, I think that only misunderstaning here is: What does it means "unknown position"? Because if you consider the "home after deactivate" (forgive me if I'm wrong) it means after "de-energization" position is no more to be considered valid, then if this is disabled the "de-energization" is no more considered as a condition to be in a "unknown position" state.

I simply follow the idea to be more detailed in G28 O to let people understand this logic

@thinkyhead
Copy link
Member

I would still expect the printer to home with HOME_AFTER_DEACTIVATE disabled and sending G28 O after steppers timeout either after X amount of time or after M18 as done in tests above

Why? When HOME_AFTER_DEACTIVATE is disabled it means you don't care whether the steppers sleep or not, presumably because you trust your axes to never (or rarely) be out of position.

…but it doesn't despite the fact that G28 documentation doesn't say that HOME_AFTER_DEACTIVATE is required for G28 O to work after steppers timeout.

What it says is "If the position is known then exit without homing." The HOME_AFTER_DEACTIVATE option is your way of defining what "known" means. With the option disabled, the position is always "known" after homing. But with the option enabled the position is only "known" when it has homed and kept the steppers powered.

If you want to clarify this point with more verbiage in the configuration or website, please submit those changes. No changes to code logic are needed.

@thinkyhead
Copy link
Member

P.S. The HOME_AFTER_DEACTIVATE option also determines when your XYZ will blink on the screen (as a "?") and this is a visual indication that G28 O will do a homing move.

@thisiskeithb
Copy link
Member

thisiskeithb commented Jul 23, 2020

What it says is "If the position is known then exit without homing."

So the inverse of that must be true:"If the position is not known then do not exit without homing."

The position is unknown after steppers timeout, so it should also home with another G28 O based on the documentation.

If you say the code is fine, then yes, the documentation needs updating because it not clearly written to explain that you need to have HOME_AFTER_DEACTIVATE enabled for G28 O to make "If the position is known then exit without homing." a true statement when steppers timeout.

@thinkyhead
Copy link
Member

thinkyhead commented Jul 24, 2020

The position is unknown after steppers timeout, so it should also home with another G28 O based on the documentation.

Perhaps adding this diagram to the website will help…

HOME_AFTER_DEACTIVATE Was
Homed
Still
Powered
G28 O
🔴 OFF 🔴 No -any- Home
🔴 OFF 🟢 Yes -any-
🟢 ON -any- 🔴 No Home
🟢 ON 🟢 Yes 🟢 Yes

@thisiskeithb
Copy link
Member

Maybe it'd make sense to make it a feature request to have G28 O work in all events, whether HOME_AFTER_DEACTIVATE was enabled or not. This would make it universally applicable/truly check if axis are unknown and not just if it was homed one time after power up.

That'd keep documentation the same & prevent further confusion of this g-code parameter.

@CRCinAU
Copy link
Contributor

CRCinAU commented Jul 31, 2020

Keeping in mind also that most folks have no idea if HOME_AFTER_DEACTIVATE is enabled or not in the firmware they use.

The default firmware from the manufacturer will never be known, the same as the million random binaries floating around. This makes it almost impossible for the end user to know how their printer will behave without resorting to trial and error.

Resorting to trial and error also makes the documentation hard - as it then becomes:
O: In some scenarios will only home your printer if needed, but sometimes not at all. Test it and see!

@GMagician
Copy link
Contributor Author

@CRCinAU there so problem with this, just remember that G28 is what you want/like. I think that to fully use Marlin you have to understand the available options, but I understand some are really cryptic for a lot of people.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Motion Needs: Testing Testing is needed for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants