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

Custom Button sends everything but M112 (emergency stop) #1031

Closed
Lord2Vader opened this issue Aug 20, 2015 · 23 comments

Comments

Projects
None yet
5 participants
@Lord2Vader
Copy link

commented Aug 20, 2015

Hi,

dont know why there isnt a big emergency button yet.
Thou I made a custom one which does not work in two ways:

  • name: Emergency Stop
    type: command
    command: M112

or

  • name: Emergency Stop
    type: commands
    commands:
    • M112

OR

  • name: Emergency Stop
    type: commands
    commands:
    • M107
    • M112

On the last one it only sends M107...

M106 S255 does work and M107 does work so what is the f***** problem here? ^^
Seems like it is intended...

Hope youll fix that.

@GitIssueBot

This comment has been minimized.

Copy link
Collaborator

commented Aug 20, 2015

Hi @Lord2Vader,

It looks like there is some information missing from your ticket that will be needed in order to process it properly. Please take a look at the Contribution Guidelines and the page How to file a bug report on the project wiki, which will tell you exactly what your ticket has to contain in order to be processable.

If you did not intend to report a bug, please take special note of the title format to use as described in the Contribution Guidelines.

I'm marking this one now as needing some more information. Please understand that if you do not provide that information within the next two weeks (until 2015-09-03 22:10) I'll close this ticket so it doesn't clutter the bug tracker. This is nothing personal, so please just be considerate and help the maintainers solve this problem quickly by following the guidelines linked above. Thank you!

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being, so don't expect any replies from me :) Your ticket is read by humans too, I'm just not one of them.

@markwal

This comment has been minimized.

Copy link
Collaborator

commented Aug 20, 2015

Does it cancel the print though? Looking at the sources, it looks like when M112 is enqueued, OctoPrint cancels the print. Probably it is the self.cancelPrint that clears it out of the queue before it gets sent to the printer.

Go ahead and fill out that form in the contribution guidelines. It'll be helpful to see the serial.log for this.

@Lord2Vader

This comment has been minimized.

Copy link
Author

commented Aug 21, 2015

Didnt even start the print. Its suppost to stop a motion like G28 Z0 for example...

Edit: I can tell you right away whats in the serial.log...nothing.
Because i can see everything in the terminal on octoprint and M112 does not show up in any cases, M107, G4 P100, M106 S255 and so on do.

@ntoff

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2015

I tried sending one manually and it does send M112 and it's visible in the console, but only after the G28 procedure has completed.

I wonder if it has something to do with G28 being a "long operation" command in the settings for octoprint. Maybe when issued a G28 command, octoprint waits until it gets an ok or something before sending another command.

@markwal

This comment has been minimized.

Copy link
Collaborator

commented Aug 22, 2015

Nah, it's simpler than that. Serial communication is metered by "ok"s from the firmware. OctoPrint will wait for the ok before it sends anything even an M112. I don't have a Marlin printer so I can test, but does your firmware accept and process an M112 during an M28 using some other host software?

For my firmware (Sailfish), it accepts cancel's immediately, so my OctoPrint plugin listens for the cancel event and sends the x3g cancel/abort before the M112 makes it out of the queue. I could have also done it via the gcode enqueuing hook.

I could propose a change that allows the M112 to be forced immediately, but I wouldn't have a way to test it. If I made a test branch would you be willing/able to pull it and give it a try?

@ntoff

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2015

Yep, it's a HUGE bug in octoprint. It's using the same code (I think at least) as the cancel button. It doesn't actually just send M112 if octoprint see's an M112. It sends a whole bunch of "regular" commands to stop the print and THEN sends M112.

comm.py line 1740 (well, 1741 to be specific) needs major reworking. It shouldn't "self.cancelPrint()", it should immediately send the M112 command straight to the printer.

does your firmware accept and process an M112 during an M28 using some other host software?

Yes. If I send M112 from printrun it immediately halts the printer even during a G28 command.

@markwal

This comment has been minimized.

Copy link
Collaborator

commented Aug 22, 2015

Will it make any difference? Does your firmware (and what firmware is it?) accept an M112 when it is busy? I'd assumed it was a HUGE bug in firmware design (at least the whole serial comm stuff in firmwares seem like a big design flaw to me: the responses aren't spec'ed, they don't have anyway to indicate what response goes with what request, there's no checksum on responses, it's huge mess).

@ntoff

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2015

I edited my post so you might not have seen it but yes, even when busy with a homing command Marlin (sorry I didn't mention it) will stop immediately if I send M112 through printrun (pronterface). It stops mid home and shuts down.

Whether or not it happens with other commands is up to the devs of Marlin to fix, however this issue does seem to be because octoprint is simply cancelling the print in a regular non panic kind of fashion in the same way it would if you hit the cancel button. I see a bunch of other commands echoed to the terminal before the M112 gets sent which is wrong. An emergency stop command should be prioritized over any and all others at the very least.

@markwal

This comment has been minimized.

Copy link
Collaborator

commented Aug 22, 2015

Like I say, I can change that, but I can't test it. Are you willing/able to test a branch I make?

@ntoff

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2015

Once I finish a print, yes I could test it. I have a spare raspberry pi I could throw it on and a second printer if it comes to that.

@foosel

This comment has been minimized.

Copy link
Owner

commented Aug 22, 2015

@markwal Cleanest approach:

  • self._doSendWithoutChecksum in self._gcode_M112_enqueuing - that will skip the queue completely and send it immediately. If the firmware then actually can/does process it depends on the firmware but at least it's out. Maybe even better: send first without, then with checksum, to make sure it doesn't get lost and also no odd stuff happens with firmware configured to demand checksums - would suck if that gets it ignored
  • then still self._cancelPrint but with error flag that prevents cancel gcode script, sets error state but also actually does cancel further print job progressing (contrary to what is written above that should in fact be done, after sending the M112, in order to reset everything on the host side properly too)

Gotta sleep over here, so can't do it myself right now, thought that hint might help though.

@markwal

This comment has been minimized.

Copy link
Collaborator

commented Aug 22, 2015

@foosel, nice, I had been thinking along the same lines as you. My first implementation was _doSendWithoutChecksum followed by the cancelPrint. I'll do the error state as you say. I had also gone down the road of clearing the queues as well which might be overkill considering cancelPrint with error state.

For current Marlin (M112 is new in the code base as of about a year ago), it looks safest to send without checksum, but I like your approach of sending it both ways.

https://github.com/MarlinFirmware/Marlin/blob/f643f4d67487e0d4ae190e579e9051e90985516f/Marlin/Marlin_main.cpp#L663

@markwal

This comment has been minimized.

Copy link
Collaborator

commented Aug 23, 2015

I think you meant self.close(is_error=True)? I don't see a cancelPrint with error state. close makes sense for Marlin since the firmware goes into a hard hang (on purpose) and has to be reset. But I don't know about other implementations. close probably works there too though since the host could choose to disconnect and reconnect anyway.

Sailfish abort is a little different, it clears the queue and turns everything off and stops printing from the SD card if any, but returns to the loop to listen for the next command. close probably isn't a big deal since reconnect is easy enough.

@markwal

This comment has been minimized.

Copy link
Collaborator

commented Aug 23, 2015

Seems like the second doSendWithChecksum is problematic because it could be that _currentLine isn't what Marlin expects depending on the race with the send loop. And even if it is correct, if Marlin saw the first one, the second will likely timeout since Marlin isn't listening any more and our main thread needs to get to the business of close the connection.

markwal added a commit that referenced this issue Aug 23, 2015

Send M112 to printer immediately
Address #1031 with an immediate send of M112 followed by closing the
serial connection to reset the host state
@markwal

This comment has been minimized.

Copy link
Collaborator

commented Aug 23, 2015

OK I've created a test branch with the changes for an immediate M112. It is branch test/immediateM112.

To switch to that branch (these commands assume an octopi image):

cd ~/OctoPrint
git pull && git checkout test/immediateM112
~/oprint/bin/python setup.py clean
~/oprint/bin/python setup.py install
sudo service octoprint restart

To switch back to master do the above, but with "master" instead of "test/immediateM112"

@foosel

This comment has been minimized.

Copy link
Owner

commented Aug 23, 2015

I think you meant self.close(is_error=True)? I don't see a cancelPrint with error state.

In a way that too, but I also meant introducing an additional parameter to cancel that would allow preventing the sending of the cancel script. Then again I just looked at the code (which last night I didn't), and apart from the state switch (which a switch to "Error" would override immediately anyhow) and the firing of the PRINT_CANCELLED event there's nothing in there that would be of importance. In the long run it might make sense to move the handling to a public method emergencyStop() that takes care of everything, but for now it's good.

Seems like the second doSendWithChecksum is problematic because it could be that _currentLine isn't what Marlin expects depending on the race with the send loop.

I thought of introducing a priority into the sending queue (emergency > resends > regular, I'd used that approach in the comm refactoring branch, although just with resends > regular) and that would get rid of that problem plus remove the possibility of a race condition from the equation. Something that would boil down to having the M112 handling go somewhat like that:

# enqueue emergency prio M112
self._enqueue_for_sending("M112", priority=SendingPriority.EMERGENCY)

# make sure that's the only thing in the queue
self._send_queue.clear(filter=lambda x: x.priority < SendingPriority.EMERGENCY)

# send and wait until it's sent
self._clear_to_send.set()
self._send_queue.join()

# set error state and close
self._errorValue = "Closing serial port due to emergency stop M112."
self._log(self._errorValue)
self.close(is_error=True)

# ...

return None,

Also having the send queue loop not do phase processing for anything with emergency prio, that way it really should just rush through and out on the line.

@foosel

This comment has been minimized.

Copy link
Owner

commented Aug 23, 2015

Also just reset the ticket status, even though @Lord2Vader carefully ignored ;) the bot's demand for more data (btw, those linked guidelines also would have contained the solution to the "the serial.log is empty" mystery), which @ntoff then kindly provided.

@ntoff

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2015

Ok I'll get into some tests and see what's what. I need to see if my bed will stay level so it'll be a double test (for some reason the carriage always end up higher on one side after a few prints).

I'm going to assume that because I installed my octoprint using the venv option that I'll just substitute your commands with the ones that contain the venv options.

edit: Ok I got it installed and running but for some reason I can't log in

edit2: Got the server to let me back in, button seems to work thus far.

One quirk I've found with octoprint is that every time I change something, like the webcam server IP, I have to ctrl + F5 because it'll continue running on an old presumably cached version of the page.

At first sending it through the terminal worked while sending through the button didn't but after a few ctrl + F5's it now works through the button too.

Is there some weird cache control shenanigans going on? Or is it just my browser? (google chrome on windows 7)

@foosel

This comment has been minimized.

Copy link
Owner

commented Aug 27, 2015

@ntoff could you please provide information on the firmware version you are testing this against?

I just tried with a 1.0.0 Marlin on a Printrboard Rev D and

If I send M112 from printrun it immediately halts the printer even during a G28 command.

this did not happen for me. And looking at the source it really can't either, since the get_command call in the loop will only happen after the G28 returns. So my guess is, there's a difference in behaviour due to a difference in Marlin versions at work here, I'd feel better having that acknowledged however.

edit Ok, never mind, my bad, the version I have here is apparently too old and doesn't have M112 yet.

@ntoff

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2015

Marlin v1.0.2 - Ramps 1.4

From configuration.h

#define STRING_VERSION "1.0.2"

I've had it for a while so I don't know which build number it is or whether it's had any updates since I last downloaded it.

Thrown it up on github: https://github.com/ntoff/marlin-frankenprinter/ if you want to take a look at it for any reason.

markwal added a commit that referenced this issue Aug 28, 2015

Send M112 to printer immediately
Address #1031 with an immediate send of M112 followed by closing the
serial connection to reset the host state
(cherry picked from commit 3d1d745)
@ntoff

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2015

@foosel It looks like this stuff has been added to the maintenance branch with some tweaks and fine tuning, would you like me to pull that branch and do any further testing?

@foosel

This comment has been minimized.

Copy link
Owner

commented Aug 30, 2015

@ntoff if you could give this branch a whirl in general it would be great. That's basically what's going to become the 1.2.5 release.

Also thanks for pinging me - I thought I'd posted here that it was now merged and also that I'd closed the ticket, but apparently I didn't O_o

@foosel

This comment has been minimized.

Copy link
Owner

commented Aug 31, 2015

Fixed on devel and in new release 1.2.5

@foosel foosel closed this Aug 31, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.