Skip to content
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

Open
wants to merge 4 commits into
base: bugfix-2.1.x
Choose a base branch
from

Conversation

InsanityAutomation
Copy link
Contributor

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.

Comment on lines 37 to 41
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());

}
Copy link
Contributor

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()))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants