Meta gcode result variable inconsistent with docs

NineMile

Not sure if this is a bug with the result variable or an issue with the documentation but here - https://docs.duet3d.com/User_manual/Reference/Gcode_meta_commands#named-constants - the result variable states that clicking a "Close" button on an active, blocking M291 window should set result to -1:

0 if the last G-, M- or T-command on this input channel was successful, 1 if it returned a warning, 2 if it returned an error, or -1 if it was a blocking M291 message box command that had a Close button, and either the Close button was pressed or the message box timed out. Meta commands do not change 'result'.

The first problem with this is that the only message box that actually has a Close button is non-blocking (S1). The second problem with this is that you would assume that this would apply to Cancel buttons that can be added to other message box types (e.g. S4 J1) - however, clicking the Cancel button on a blocking message box type appears to execute abort, or something similar to M99 and does not end up evaluating any if statement checking for the value of result.

I suspect this is actually a bug with result or the handling of M291 message boxes as the documentation seems to express the obvious behaviour, but I thought I'd confirm first.

To reproduce, run as a gcode 'job' file:

M291 P{"Blah, unimportant."} R"Big Title" S4 K{"Yes","No"} T0 J1
echo { "Result: " ^ result }
echo { "Input: " ^ input }

Based on the documentation, I would expect this to return something like -1 and null if I clicked the Cancel button on the modal in DWC, or 0 and 0 if I selected "Yes", but what actually happens is the print file is cancelled and the subsequent echo lines are not run.

This makes it pretty difficult to account for users wanting to cancel out of a process (say for example during a probing routine where they selected to probe a Z surface instead of an X surface or whatever), and actually recover successfully.

When this code is run outside of a print file, it looks like the M291 just acts like M99 which leaves any calling macros unsure of the state of the macro they ran - did the macro finish cleanly or did it abort early and not do what you expected?

T3P3Tony

@NineMile In the short term we need to update the documentation to more correctly describe what "Cancel" does when its pressed. We need to discuss what the impact will be on existing users who are relying on cancel to abort the macro/job if we change it not to do that as a work around for your requirement you can use this

echo { "Input: " ^ input }M291 P{"Blah, unimportant."} R"Big Title" S4 K{"Yes","No","Cancel"} T0 
echo { "Result: " ^ result }
echo { "Input: " ^ input }

dc42

@NineMile I think the documentation meant to say Cancel instead of Close. As to it aborting instead of returning -1 I can think of a few possibilities:

Change it to return -1 instead of aborting the macro. As Tony says, this might break macros for some users.
Change the M291 J parameter so that J1 aborts if Cancel is pressed as now, but J2 sets result to -1 instead.
Leave the behaviour as now but add some sort of exception-catching mechanism to meta GCode.

T3P3Tony

@NineMile if you don't mind please raise a feature request for the J2 option as @dc42 suggests

NineMile

@T3P3Tony said in Meta gcode result variable inconsistent with docs:

@NineMile if you don't mind please raise a feature request for the J2 option as @dc42 suggests

Done - the J2 option looks like a nice, clean way to implement both behaviours without breaking existing code.

NineMile

@dc42 I've been looking at implementing this and I have the J2 functionality working as described - however this also means that a J2 message box that times out will also return -1 (as well as if the user clicks Cancel).

I think this is the right behaviour, as in both cases it essentially means 'user did not supply a valid input', but I'd like to confirm that this is what you'd expect to happen as well.

I've tested on my stm32 based board and it works well - I have a Mini 5+ here that I'll test on once I get the build repo working. Happy to submit a PR for this if you'd like to look at the approach I've taken and verify I'm on the right track

dc42

@NineMile I was about to start implementing this, but your implementation may save me some time. If your code changes are ready, please share them.

Did you test them in SBC mode, or standalone mode, or both?

NineMile

@dc42 said in Meta gcode result variable inconsistent with docs:

@NineMile I was about to start implementing this, but your implementation may save me some time. If your code changes are ready, please share them.

Did you test them in SBC mode, or standalone mode, or both?

@dc42 I submitted a PR with the changes here.

I've tested in standalone mode only, currently on an stm32h7 board but the PR cherry picks the changes onto the duet branch. I can test standalone on a Mini 5 plus if necessary but I don't have a viable setup to run SBC mode for testing right now.