Autostart "auto#.g files" -> autofile_check() fix

[kisslorand]

Requirements

Printing from onboard SD card.

Description

This PR meant to be a fix for a bug when printing from onboard SD and no "auto#.g" files on onboard SD card. When print is finished, Marlin never sends "Done file printing", it remains in a paused state. The problem was that the autofile_check() returned true even if no "auto#.g" was on the SD card.

Benefits

Printing from onboard SD as it meant to be

Related Issues

#19296 #20064 #20071

[qwewer0]

This seems to fix one of the problem that #20071 introduced, but the automatically started auto#.g files still stays in printing mode.
@kisslorand Do you think that you can look into it?

[rhapsodyv]

Hum... looking at the code, it explicitly says: * Return 'true' if there was nothing to do... so, the code seems to do what it was projected to.

I think we need to spend a little more time to understand what is going on here. If the fix is just return false when there's no autofile (as this PR does), I think that function logic should be reviewed, or the place where is calling it is doing the wrong comparison.

[qwewer0]

Hum... looking at the code, it explicitly says: * Return 'true' if there was nothing to do... so, the code seems to do what it was projected to.

I think we need to spend a little more time to understand what is going on here. If the fix is just return false when there's no autofile (as this PR does), I think that function logic should be reviewed, or the place where is calling it is doing the wrong comparison.

You are right, a true to false shouldn't be that answer here, because as you wrote, the code states that it should be true if there was nothing to do. I overlooked that.

[kisslorand]

Hum... looking at the code, it explicitly says: * Return 'true' if there was nothing to do... so, the code seems to do what it was projected to.

I think we need to spend a little more time to understand what is going on here. If the fix is just return false when there's no autofile (as this PR does), I think that function logic should be reviewed, or the place where is calling it is doing the wrong comparison.

I beg to differ. If (autofile_index == 0) that means there is no "auto#.g" file.
In cardreader.h it says:
"static uint8_t autofile_index; // Next auto#.g index to run, plus one. Ignored by autofile_check when zero."

When there's nothing to do we can see the logic down in the cardreader.cpp where it says:
"// Don't run auto#.g when a PLR file exists"
At that "if" conditional the return is "false". So you have to return "false" to not run any "auto#.g" file.

If autofile_check() returns "true" than M1001 is not executed.
void GcodeSuite::M1001() {
__// If there's another auto#.g file to run...
__if (TERN(NO_SD_AUTOSTART, false, card.autofile_check())) return;

If (autofile_index == 0) than there is no "another auto#.g file to run..."
autofile_index == 0 means there is no "auto#.g" file so M1001 should be executed.
Even TERN(NO_SD_AUTOSTART, false, card.autofile_check()) says that M1001 should be executed if no autostart is wanted nor needed (because there's no "auto#.g" file)

[qwewer0]

If autofile_check() returns true, then if (TERN(NO_SD_AUTOSTART, false, card.autofile_check())) return; will return nothing (?) and M1001 will exit, am I reading it right?

[kisslorand]

Yes, you're right. If M1001 exits than you'll never have "actions for SD print completion" including the message "Done printing file" from Marlin. The missing of this message got me into this.

For me "autofile_check()" translates to "check if there's a need for an "auto#.g" to process".

[rhapsodyv]

Hum... looking at the code, it explicitly says: * Return 'true' if there was nothing to do... so, the code seems to do what it was projected to.
I think we need to spend a little more time to understand what is going on here. If the fix is just return false when there's no autofile (as this PR does), I think that function logic should be reviewed, or the place where is calling it is doing the wrong comparison.

I beg to differ.

I'm not meant to say what is right or wrong, just what looked weird.

If (autofile_index == 0) that means there is no "auto#.g" file.

Well, the comments say exactly that: if there's nothing to do, return true.

In cardreader.h it says:
"static uint8_t autofile_index; // Next auto#.g index to run, plus one. Ignored by autofile_check when zero."

When there's nothing to do we can see the logic down in the cardreader.cpp where it says:
"// Don't run auto#.g when a PLR file exists"
At that "if" conditional the return is "false". So you have to return "false" to not run any "auto#.g" file.

The comment in the top of the function says: * Return 'true' if there was nothing to do.

If autofile_check() returns "true" than M1001 is not executed.
void GcodeSuite::M1001() {
__// If there's another auto#.g file to run...
__if (TERN(NO_SD_AUTOSTART, false, card.autofile_check())) return;

The fix wasn't change

if (TERN(NO_SD_AUTOSTART, false, card.autofile_check())) return;

to:

if (TERN(NO_SD_AUTOSTART, false, !card.autofile_check())) return;

?

I really didn't tested it. But I will take a look.

[qwewer0]

if (TERN(NO_SD_AUTOSTART, false, !card.autofile_check())) return;

@rhapsodyv, @kisslorand Tested it with the change above and it fixed everything. Print jobs and auto#.g ends as it should. 👍
So I think only an exclamation mark was missing before card.autofile_check().

[kisslorand]

if (TERN(NO_SD_AUTOSTART, false, !card.autofile_check())) return;

@rhapsodyv, @kisslorand Tested it with the change above and it fixed everything. Print jobs and auto#.g ends as it should. 👍
So I think only an exclamation mark was missing before card.autofile_check().

Twisted logic, but if it works, than it works.
Fixed with new commit.

[rhapsodyv]

Probably @thinkyhead needs to take a look on it too, as he is the author of the recent auto start refactoring.

[kisslorand]

@rhapsodyv
Thanks for the explanation of the logic used. In my book if I you check something it's true if it's there and false it it's not. It wasn't the case here :)
It would have been clearer if "autofile_check()" would have been "noAutoFile_check()".
Anyway, if it works, it works!