Marlin: parser.value_bool() is broken

[Roxy-3D]

G26 can no longer do a 'C' parameter because parser.value_bool() is broken.

SENDING:G28
>>> G26 C O2.5
SENDING:G26 C O2.5
G26 command started.  Waiting for heater(s).
Checking parser.value_bool()
?parser.value_bool() is broken...

    g26_keep_heaters_on       = parser.seen('K') && parser.value_bool();
    SERIAL_PROTOCOLLNPGM("Checking parser.value_bool()\n");
    g26_continue_with_closest = parser.seen('C') && parser.value_bool();

    if (!g26_continue_with_closest)
        SERIAL_PROTOCOLLNPGM("?parser.value_bool() is broken...\n");

[Bob-the-Kuhn]

I don't understand.

G26 C1 O2.5 should result in g26_continue_with_closestset to TRUE.

G26 C O2.5 should result in g26_continue_with_closestset to FALSE.
G26 C0 O2.5 should result in g26_continue_with_closestset to FALSE.

The console log indicates that it's working correctly.

[Roxy-3D]

G26 C O2.5 should result in g26_continue_with_closestset to FALSE.

No. Absolutely not. It has never been that way. If the user specifies the C parameter, it means continue with closest. No numerical value is necessary for that flag to take effect. We have a lot of flags that specify behavior that historically do not need a value. E for extending the probe on the original G29's for example.

AND... G29 P in UBL has always meant a prime of filament was desired but because no length was specified, the user got to control exactly how much would be extruded. The G29 P functionality got broken also with parser changes.

[Bob-the-Kuhn]

"parser.value_bool()" is operating as intended.

If you don't want that "functionality" then you'll need to delete "parser.value_bool()" from those statements.

It looks like "parser.value_bool()" was added in mid May.

I'll take bets as to whether Scott added it. He has a thing about putting bool qualifiers on parameters. I think he gets paid by the dozen 😉

[Roxy-3D]

"parser.value_bool()" is operating as intended.

I am not sure that is (or was) true. If you look at the logic, it is even commented that no value should return 'true'.

  // Bool is true with no value or non-zero
  inline static bool value_bool()        { return !has_value() || value_byte(); }

I think the intent was to allow a transition to a more specific parameter style. But still allow old documentation and code to be 'correct'.

But as you can see in the example code up above... It fails when spaces are in unexpected places.

[Bob-the-Kuhn]

I don't see any spaces in unexpected places in the first post in this thread. I see:

1. G26
2. spaces
3. "continue with closest" parameter
4. spaces
5. Oooze parameter
6. value 2.5 (no spaces between Oooze & value

Seems odd to me to be checking for anything to enable/disable a parameter. Even stranger to have it return true if the enable/disable flag isn't present.

@thinkyhead - is the code or the comment correct on "parser.value_bool()"?

[Roxy-3D]

is the code or the comment correct on "parser.value_bool()"?

Perhaps the answer to the question: https://github.com/MarlinFirmware/Marlin/pull/7165/files

[Bob-the-Kuhn]

I think I've found a way to make the code match the comment.

In gcode.h replace line 135:
if (b) value_ptr = command_ptr + param[ind];
with:
if (b) value_ptr = param[ind] ? command_ptr + param[ind] : (char*)NULL;

This makes "G26 C O2.5" work properly when FASTER_GCODE_PARSER is enabled.

The problem didn't exist when FASTER_GCODE_PARSER was disabled.

What I don't know is if there are any unintended side effects. I'm going to create a PR. Scott'll tell us if there is a better solution.

[thinkyhead]

// Bool is true with no value or non-zero
is the code or the comment correct…

The comment is how it should behave:

• Bool parameter with no value: true
• Bool parameter with 0: false
• Bool parameter with non-zero: true

The caller needs to check this beforehand:

• Bool parameter left out (!seen): default

Shorthand to do it in the right order is: boolval('Q', default_val)

---

It fails when spaces are in unexpected places.

Not at all. A parameter followed by spaces and a number now reliably combines as one parameter.

---

>

if (b) value_ptr = param[ind] ? command_ptr + param[ind] : (char*)NULL;

Good catch. I feel like that's what used to be there, but it got reverted. Anyway… yes, that's it!

[boelle]

have we fixed this one in the latest bugfix 2.0 ?

[boelle]

@thinkyhead i think we can close this one

[github-actions[bot]]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.