-
Notifications
You must be signed in to change notification settings - Fork 88
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
Allow return without expression #602
base: master
Are you sure you want to change the base?
Conversation
This is a breaking change? Returning an explicit value with e.g. FYI I am going to add expresssion support to |
No, it just means you can use any of:
I don't think I understand. Will read again tomorrow and see if brain is braining :P |
… does not need to be in []
… previous returns
@terjeio Do you think this can get merged? I can't see that it is breaking anything from using it for a while 🤔 |
I am a bit hesitant to add this as it breaks compatibility with LinuxCNC and is not a neccessary change. I'll come back to it a bit later. |
I don't see how this breaks compatibility with anything? it just extends the current mode of operation so that you do not need to encapsulate your return value with [] |
@@ -689,11 +689,13 @@ status_code_t ngc_flowctrl (uint32_t o_label, char *line, uint_fast8_t *pos, boo | |||
else if((g65_return = !!grbl.on_macro_return)) | |||
ngc_flowctrl_unwind_stack(stack[stack_idx].file); | |||
|
|||
if(ngc_eval_expression(line, pos, &value) == Status_OK) { | |||
if(ngc_read_real_value(line, pos, &value) == Status_OK) { |
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.
ngc_read_real_value will call ngc_eval_expression if the first character encountered is [, so this should not break compatibility with current usage. It only adds some flexibility 😅
ngc_named_param_set("_value", value); | ||
ngc_named_param_set("_value_returned", 1.0f); | ||
} else | ||
} else { | ||
ngc_named_param_set("_value", 0.0f); |
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.
The reason for setting _value
to 0 here, is that just setting it when a macro/subroutine starts does not ensure it is 0 when returning if the macro/subroutine also calls other subroutines or macros. In that case you risk "leaking" the returned _value
of an internally called macro/subroutine.
If this is also how it works in other gcode dialects, and it is done on purpose as a feature, I will agree that setting it to zero is a breaking change, and I will remove it 😅
Small change to allow writing
return #5070
instead ofreturn [#5070]
and similar.Still allows the old way of returning only using expressions, but I find this syntax less error-prone.
Also explicitly setting
_value
to 0 if no value is returned, as I have had some confusion where I get a unexpected_value
but_value_returned
is 0 😅