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

[1.1.x] sd abort: expose sd abort code path via new M524 #11386

Merged
merged 2 commits into from
Oct 19, 2018

Conversation

zachwelch
Copy link

Description

This PR implements the Feature Request presented in issue #11377. It adds the M524 command as "SD abort job", performing the same logical checks that enable the Stop SD Job option in the LCD menu.

Why M524?

  • It pairs nicely with the M24 Start SD job command.
  • It lands adjacent to another SD-related command (M540).
  • I have no idea if there is an "official" code requisition process or central registry.

This feature was tested with my custom host software.

Benefits

Host software can trigger a graceful abort of an SD print job.

Related Issues

No new output has been added yet. If the command successfully closes the file, the firmware will produce the same output that the display would trigger. If a SD job is not being printed, that might deserve to be reported to the host, requiring a new message. That can be added as a follow-up patch in this PR, if all else looks good.

If the PR is accepted, the website needs to be updated to list the new code. How is that manged?

@zachwelch zachwelch changed the title sd abort: expose sd abort code path via new M524 [1.1.x] sd abort: expose sd abort code path via new M524 Jul 28, 2018
@thinkyhead
Copy link
Member

I have no idea if there is an "official" code requisition process or central registry.

Basically it just needs to be added to the http://reprap.org/wiki/G-code page to reserve it. It's the de-facto central repository for RepRap G-Codes.

@thinkyhead
Copy link
Member

thinkyhead commented Jul 29, 2018

the website needs to be updated to list the new code. How is that managed?

Check out the README and make a fork of https://github.com/MarlinFirmware/MarlinDocumentation and contribute to it just as you would contribute to the main Marlin repo:

  • Copy the master branch,
  • Make changes and commit them,
  • Submit a PR to the master branch of MarlinDocumenatation.

@thinkyhead
Copy link
Member

thinkyhead commented Jul 29, 2018

After looking through the RepRap wiki G-codes page now I'm thinking that M33 would be a better option, but it's currently used in Marlin to get the long filename. We have all the capabilities to implement the functionality, with the "restart" component simply being optional. Check it out: https://reprap.org/wiki/G-code#M33:_Stop_and_Close_File_and_save_restart.gcode

It might also be fine to make it an extension to M25 — as "indefinite pause" (stop with resume) and "pause and abort" (stop with no resume) optional behaviors.

@zachwelch
Copy link
Author

Further testing, reveals that a subsequent M24 causes an endless loop of "Error:SD read error" lines, but that seems like a latent issue that has simply been exposed (and could be addressed through separate patches). Right now, it is a drastic improvement to be able to abort the job cleanly.

@zachwelch
Copy link
Author

The reprap wiki lists M33 twice.... Marlin already implements the "get long file name" version of the code.

@zachwelch
Copy link
Author

After further reflection on this issue, I would rather see Marlin add a new gcode than overload (and thus change) the semantics of an existing code.

If hosts already support M25 for pause, adding a flag requires modifying existing support. That creates risk of breaking existing functionality, which could be avoided entirely with a new code. How do you detect such extra flag support? Such overloading also moves that m-code toward a "candy machine" interface, where the semantics drastically differ depending on the flags. The reprap firmware implemented this functionality separately as M33, so I bet their reasoning went similarly (KISS).

So, given a separate command, the existence of the M33 variant suggests that we might try to produce a unified vocabulary:

  1. We can rename/drop our long file name command (M33) and use that code to implement their functional spec, or
  2. We convince the reprap firmware to adopt M524 as an alias for M33.

Personally, I would not put money on changing either of our M33 implementations any time soon, because that would create complications with host software not knowing what semantics to expect (or even realize that they changed). Thus, conflicting vocabularies seem to be unavoidable in order to prevent either firmware projects from causing problems with their existing deployments.

I spent a little time to clone the MarlinDocumentation repository and add a blurb about M524 to my tree. Likewise, I joined the reprap wiki and started to add a section for it there (with a reference to it from M33). However, I have not pushed/committed any changes, as it is not clear how to balance needs of Marlin versus the needs of the broader 3-D printing community.

From this perspective, adding M524 (or another new code) seems like the best option; it moves the Marlin project forward and helps draw broader attention to the issue and drive any resolution process forward. At the very least, it becomes available for further testing/development purposes, even if we add a note explaining that we may need to rename it later to resolve this conflict. After all, the conflict is only hypothetical until Marlin actually implements this feature. In my experience, there is less pressure (and thus less resources) to resolve conflicts caused by hypothetical problems, as compared to real and deployed ones.

FWIW, Marlin could drop its M33 by simply incorporating long name support transparently into all other SD commands (if enabled by the configuration). Indeed, this exact solution seems implied by the "not required" support note for reprapfirmware in the Marlin's long path name variant of M33. As it happens, this solution relates directly to issue #11403, which I filed (after this PR) to address a couple of fairly well-characterized shortcomings in long name support.

Still, this solution runs into the problem of shifting semantics and host software assuming old behavior, where old hosts ask for long names and the new firmware tries to abort a SD print job. With that in mind, we find ourselves with a new code, don't we? If we have usable and meaningful protocol versioning, we can later bump the version when all of this is done and the time comes to rename it to M33, along with other backward-incompatible-but-globally-unifying changes?

Certainly, that last option seems like a good candidate for the 2.0.0 release/branch, yah? It seems like the perfect place breaking things in the name of forging a more-unified standard. However, I have not even looked at that branch yet, as I am still trying to fully utilize my printer with a "stable" firmware.

@thinkyhead thinkyhead force-pushed the bugfix-1.1.x branch 4 times, most recently from 72f7cd2 to 3ef6cf0 Compare August 17, 2018 02:17
@thinkyhead
Copy link
Member

With Marlin 1.1.x now end-of-life, all new code will have to be directed to the bugfix-2.0.x branch. Do you feel confident that you can re-do this PR for bugfix-2.0.x? If so, please start with bugfix-2.0.x and re-submit this feature targeted at that branch.

@thinkyhead thinkyhead merged commit 75298e6 into MarlinFirmware:bugfix-1.1.x Oct 19, 2018
thborges pushed a commit to thborges/Marlin that referenced this pull request Feb 6, 2019
thinkyhead pushed a commit that referenced this pull request Sep 2, 2019
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.

2 participants