Marlin
Marlin copied to clipboard
MarlinFirmware
Parse safety commands even with e parser
After seeing #26510 where a safety command was expected to operate and did not, it became apparent that we needed to revisit the current gcode / emergency parser structure.
Currently, it is expected all Gcode is passed through the serial queue to a degree, and the emergency parser blocked compiling certain gcodes, likely in order to save progmem and limit potential duplicate execution if the emergency parser did not dump the command before passing to the standard parser.
As it has become common for "sideloaded" commands to be pushed from menu buttons, UI interfaces, macro execution, and more, we can no longer rely on just the emergency parser to catch these commands.
This PR brings the excluded commands back in for standard execution regardless of the presence of the emergency parser in order to ensure that there is no path for a safety related command to skip execution. On the potential concern for a duplicate execution, 3/4 commands impacted here we have no concern if they are executed a second time. The last one may leave a stale value so its internal execution is blocked if the emergency parser is currently active. As this is just for host prompt support, it is highly unlikely to be fed through a sideload channel, and should it occur we can protect for it by disabling and reenabling the e parser around injection similarly to when we do SD read operations currently.
The basic concept of getting commands where they need to go is sound, but this PR needs more work to overcome some tricky issues.
First, M108 is specifically made to deal with a blocked queue. It makes no sense to run it as a G-code command in the queue. But, it appears to be harmless.
Probably M112 is the only command that ought to get into the queue, in spite of this causing a later execution. The others are meant to be handled immediately.
The most tricky issue of all… It's not entirely clear how to fix this PR to prevent EP commands being interpreted twice when the EP is present. I think that is an important consideration.
At the most simple level, the EP should swallow the commands it executes. But EP doesn't affect the serial buffer, so we can't easily do that, especially as we are reading serial bytes on the input end of the FIFO.
So, we might have each command that EP handles have a flag that it should ignore its next invocation if EP is doing the invoking. But this breaks if the command is sent more than once close together (within the G-code buffer size).
If we kept some kind of running count of lines received and a short circular buffer, we could store the line counts of commands that were handled by the EP, and then the command dispatcher could just not dispatch a command that has a line number in that buffer. But this would tend to slow down the command dispatcher more.
Thoughts?
Been almost a year since I put this together.... I'll read back through this week and figure out what was intended and what's still working after a rebate.
Some of the seemingly out of place handling with M108 for example comes from ensuring it'll work when tied to a button trigger action, in a macro, from an ExtUI screen, etc.
Double execution of most of these is harmless and safely ignored, eg m112 we seriously want to guarantee it's executed and if it gets to the queue being processed... Do it again! M108, of the wait is cleared it should just return harmlessly anyway.
The specific case that brought this up if my memory is correct was a G-Code script tied to a button trigger and M112 was not executed, which highlighted the need for execution from both sides. If any commands have an actual detrimental effect we can likely add a flag as you mentioned to skip it's next execution (or an accum counter for large serial buffers...) but I'll need to review again and see if any actually have a downside.
After reading back through this, I dont see any concern with a double execution of the affected commands aside from M876. It may set a runout response thats no longer relevant. To mitigate this, we could either initialize the value on entry to a wait state, or add an EP processed flag to M876 to skip the next local entry.
By setting to the wait for state when entering the idle loop, we avoid a stale M876 command being processed. Removed the return on the M876 function that would have blocked the potential issue from occurring. This allows menu buttons or extui injected M876 calls to still be parsed.
I still need to physically test this, but at least now appears correct for all cases.