search

MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.13k stars 19.2k forks source link

[FR] Allow M112 / Emergency Parser to work with Custom User Buttoms #26510

Open JWSmythe opened 9 months ago

JWSmythe commented 9 months ago

Did you test the latest bugfix-2.1.x code?

Yes, and the problem still exists.

Bug Description

I was trying to add a physical button emergency stop button to a laser machine. I didn't want to run power wires to the control panel, so I'm trying to use BUTTON1 for now.

The machine is a Creality 4.2.7 mainboard on a TwoTrees laser engraver, running Marlin 2.1.2.1. Lightburn is sending the gcode to the machine over USB, line by line, the same way Octoprint sends.

This is the relevant configuration, with some things I've tried.

  #define EMERGENCY_PARSER
  //<snip>
  #define BUTTON1_PIN PC4        // PC4 is the stock bed thermister pin.  Bottom row, second from the right.
  #if PIN_EXISTS(BUTTON1)
    #define BUTTON1_HIT_STATE     LOW       // State of the triggered button. NC=LOW. NO=HIGH.
    #define BUTTON1_WHEN_PRINTING true     // Button allowed to trigger during printing?
    #define BUTTON1_GCODE         "M112" // Keeps running. 
    //#define BUTTON1_GCODE         "KILL" // This does nothing.
    //#define BUTTON1_GCODE         "M112;M5;M107;G1 S0;M112" // ChatGPT suggestion - Keeps running
    //#define BUTTON1_GCODE         "M999"      // Keeps running
    //#define BUTTON1_GCODE         "M5\nM112"  // Keeps running
    #define BUTTON1_DESC          "EMERGENCY STOP"  // Optional string to set the LCD status
  #endif

I suspect that just the current line is aborted, but since Lightburn is still sending commands, it just resumes when the next line is sent.

I also tried using FREEZE_FEATURE. That does freeze the steppers immediately, but it leaves the laser on in the previous state. If you happen to hit the button when the laser is activated, it stays on. If FREEZE_FEATURE turned the laser off, that would be acceptable (and expected) too.

Bug Timeline

Unknown. Discovered on 2.1.2.1

Expected behavior

Send M112. Laser/spindle/"fan" turns off.
Machine halts.

Actual behavior

Send M112. Machine continues running.

Steps to Reproduce

Configure firmware with CUSTOM_USER_BUTTONS. Set BUTTON1_PIN to PC4. Set BUTTON1_GCODE to "M112" Start sending via Lightburn

Version of Marlin Firmware

2.1.2.1

Printer model

TwoTrees laser engraver

Electronics

Creality 4.2.7 mainboard

LCD/Controller

Ender3/CR10 LCD

Other add-ons

Emergency stop button

Bed Leveling

None

Your Slicer

Other (explain below)

Host Software

Other (explain below)

Don't forget to include

Additional information & file uploads

Video showing the problem: https://youtu.be/H-NhQr8gtnQ

Marlin_2.1.2.1_M112_bug_report_configs.zip

Documentation of machine setup. It isn't updated with the emergency stop button yet. I'm waiting to resolve this problem, or abandon the emergency stop entirely. https://github.com/JWSmythe/MarlinLaser

Machine - Twotrees Totem Laser Engraver Mainboard - Creality 4.2.7 Sender/CAM - Lightburn Firmware - Marlin 2.1.2.1

ellensp commented 9 months ago

You cannot use M112 like this

Buttons simply add the gcode to the gcode queue. bypassing the serial input.

With emergency passer enabled M112 is detected and processed while it is in the serial buffer, there is no code to process it from the gcode buffer.

If you disable emergency passer, M112 is processed from the gcode buffer, but it has to wait till it gets there

This is a design limitation

EvilGremlin commented 9 months ago

Laser/spindle off must be added to FREEZE_FEATURE Meanwhile you can use KILL_PIN instead

JWSmythe commented 9 months ago

With emergency passer enabled M112 is detected and processed while it is in the serial buffer, there is no code to process it from the gcode buffer.

If you disable emergency passer, M112 is processed from the gcode buffer, but it has to wait till it gets there

Emergency parser is enabled. It sees it and puts it up on the LCD immediately, so it's not a problem with waiting for it to be read from the queue.

EvilGremlin commented 9 months ago

BUTTON1_DESC is not gcode, it's dispayed independently form command, while ommand itself might be invalid or empty, doesn't matter

JWSmythe commented 9 months ago

Meanwhile you can use KILL_PIN instead

I'll try KILL_PIN. It appears to be undocumented, but I see it in some of the pins files. Just not on this board's pins files. Is there any configuration that goes with it? I don't see it at all on the Marlin site.

JWSmythe commented 9 months ago

BUTTON1_DESC is not gcode, it's dispayed independently form command

Right. I don't have gcode in the _DESC, I have it in the _GCODE

    #define BUTTON1_GCODE         "M112" // Keeps running. 
    #define BUTTON1_DESC          "EMERGENCY STOP"  // Optional string to set the LCD status
EvilGremlin commented 9 months ago

No, you just define pin and state

JWSmythe commented 9 months ago

No, you just define pin and state

Sweet. That worked exactly as I want it. I'll leave the bug up, so maybe they can enhance M112, or document KILL_PIN in the confs near the EMERGENCY_PARSER entry.

InsanityAutomation commented 4 months ago

I think this is a hole that needs to be closed to a degree. There are too many places to inject gcode and safety related commands should not be able to slip through the gaps. I did a quick review and I believe I can rather quickly close the gap here with some simple changes on how sideline gcode is injected.

InsanityAutomation commented 4 months ago

I was trying to add a physical button emergency stop button to a laser machine. I didn't want to run power wires to the control panel, so I'm trying to use BUTTON1 for now.

Side note - Please do not call a button tied to an input to trigger a function an emergency stop. It is a legally protected term in most regions and this does NOT fit the required function for the definition.

InsanityAutomation commented 4 months ago

I made some changes here that seem to work as intended in the sim as far as I can take it... Some elements require physical testing beyond the sim. I may be able to get to it tomorrow, but here are my changes in case anyone gets to it sooner.

https://github.com/InsanityAutomation/Marlin/tree/ParseSafetyCommandsEvenWithEParser

I should note internal processing added in line like this adds a potential for double execution. On M876 I added a catch for that, but may be a non issue as M290 already fell through the same way. I will likely make this more robust as needed, as there are several methods to do so.

We don't care if M112 or M410 double execute in the slightest. If M108 double executes, were just setting a var already false to false again so Im not concerned with efficiency loss during UI interaction.