Parse safety commands even with e parser#26944
Parse safety commands even with e parser#26944thinkyhead merged 20 commits intoMarlinFirmware:bugfix-2.1.xfrom
Conversation
c792921 to
37fb26b
Compare
37d77d6 to
aa44542
Compare
|
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, Probably 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? |
0096bc7 to
a4fbbf9
Compare
|
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. |
4354891 to
efa1758
Compare
|
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. |
3791e7d to
6ea4a16
Compare
| case '8': if (command[2] == '0' && command[1] == '1') { wait_for_heatup = false; TERN_(HAS_MARLINUI_MENU, wait_for_user = false); } break; | ||
| case '2': if (command[2] == '1' && command[1] == '1') kill(FPSTR(M112_KILL_STR), nullptr, true); break; | ||
| case '0': if (command[1] == '4' && command[2] == '1') quickstop_stepper(); break; | ||
| } |
There was a problem hiding this comment.
Thusly, always keep M108, M112, M410 out of the queue when sent by serial, even without EP.
When EP is enabled the commands will have already been handled, so this could just do nothing but still keep all EP commands sent by serial out of the queue.
This does not currently handle M876, but if that was also done here it would be consistent.
52532da to
06c6c47
Compare
* upstream/bugfix-2.1.x: (24 commits) [cron] Bump distribution date (2025-12-04) 🐛 Fix Controller Fan Soft PWM speed 🎨 PSTR() => F() 🧑💻 Editorconfig for contributed lib-uhs3 🧑💻 Editorconfig for contributed HAL/DUE/usb 🧑💻 duration_t::remainingEstimate [cron] Bump distribution date (2025-12-03) 🧑💻 FT Motion accessors, G-code style 🩹 Trajectory FTM_POLYS followup 🩹 Fix and clean up some stuff (MarlinFirmware#28201) ⚡️ FTMotion optimized timing (MarlinFirmware#28187) ✨ FT_MOTION > FTM_POLYS (2) 🎨 Consistent tests paths 🚸 Extra parsing of safety commands (MarlinFirmware#26944) [cron] Bump distribution date (2025-12-02) 🧑💻 Add a "Marlin" class 🧑💻 Reduction via TERF 🎨 "controllerfan" 🎨 Pretty up timers 🧑💻 Relocate G38 data ...
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.