-
-
Notifications
You must be signed in to change notification settings - Fork 19.3k
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
Parse safety commands even with e parser #26944
base: bugfix-2.1.x
Are you sure you want to change the base?
Parse safety commands even with e parser #26944
Conversation
void GcodeSuite::M876() { | ||
|
||
if (parser.seenval('S')) hostui.handle_response((uint8_t)parser.value_int()); | ||
if(TERN1(EMERGENCY_PARSER, emergency_parser.isEnabled())) | ||
if (parser.seenval('S')) hostui.handle_response((uint8_t)parser.value_int()); | ||
|
||
} |
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.
So I assume this is the command that you said was possible to mess up state if processed from both side-band and in the emergency parser:
The last one may leave a stale value so its internal execution is blocked if the emergency parser is currently active.
The logic I see here looks backward.
Breaking this down into more verbose form, I'm interpreting it as follows:
#if ENABLED(EMERGENCY_PARSER)
if (emergency_parser.isEnabled())
if (parser.seenval('S')) hostui.handle_response((uint8_t)parser.value_int());
#else
if (parser.seenval('S')) hostui.handle_response((uint8_t)parser.value_int());
#endif
Did you mean for this to be (added a !
)?
if(TERN1(EMERGENCY_PARSER, !emergency_parser.isEnabled()))
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.
c792921
to
37fb26b
Compare
37d77d6
to
aa44542
Compare
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.