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
parser.value_bool() is broken #7164
Comments
I don't understand. G26 C1 O2.5 should result in G26 C O2.5 should result in The console log indicates that it's working correctly. |
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. |
"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 😉 |
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. |
I don't see any spaces in unexpected places in the first post in this thread. I see:
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()"? |
Perhaps the answer to the question: https://github.com/MarlinFirmware/Marlin/pull/7165/files |
I think I've found a way to make the code match the comment. In gcode.h replace line 135: 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. |
The comment is how it should behave:
The caller needs to check this beforehand:
Shorthand to do it in the right order is:
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! |
have we fixed this one in the latest bugfix 2.0 ? |
@thinkyhead i think we can close this one |
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. |
G26 can no longer do a 'C' parameter because parser.value_bool() is broken.
The text was updated successfully, but these errors were encountered: