Open dewhisna opened 2 years ago
cp2004
commented
2 years ago Rather than it being the () comments, is it the fact it has *** in it?
Usually, the is followed by a checksum, to ensure proper communication with the printer. So when there is a ``, my suspicion is that this is confusing your printer firmware.
If you remove the stars (but leave the other style of comments) are they properly ignored by the printer?
dewhisna
commented
2 years ago You know, I didn't even think about the * symbols and it being a special marker for the checksum. I will have to try that. I had assumed that it was the paren-comment in general since it worked when I prefixed the line with ;, but that would have caused OctoPrint to strip the line completely.
Since the file worked as-is when manually copied to the printer's SD card, I suppose the printer could ignore the * that's in a comment unless it's seen in the serial stream itself, which it would be when OctoPrint is streaming this content to it.
I'll have to try that it this evening and see, because if that works, it could be arguably a Marlin bug (or gcode parser quirk) instead.
dewhisna
commented
2 years ago Nope it wasn't the * symbols in the paren-comment lines. It seems that ALL paren-comments are handled incorrectly by OctoPrint.
In my earlier testing, from which this issue originated, I had switched that first paren-comment-only line to a ; and that worked for that line, but it failed further down at what I thought was the next paren-comment-only line in the file. At the time, I needed to get on with the print, so I just manually copied the gcode file to the SD card and got on with the program, so to speak.
Tonight I looked at it closer -- first replacing the * symbols with - symbols to test your theory. It still failed on that first paren-comment-only line. So, I removed that line to see where it would fail next. And to my surprise, it wasn't the next paren-comment-only line, but the next gcode command line with any paren-comment.
I then did a search-and-replace for all ( to be ;( to turn them into comments that OctoPrint would strip out, and the upload worked just fine (except for long-filename collision issues with OctoPrint not properly supporting long-filenames on Marlin and using a short-name instead that ended up stepping on the file I already had on the SD card, by it not realizing it was the same file -- but that's a different issue).
I did some quick math with the gcode command lines containing paren-comments in the serial log from OctoPrint, and it seems that OctoPrint is including the comment text itself in the checksum. But, if I'm reading the C++ source in Marlin/src/gcode/queue.cpp correctly, it is stripping both ; comments and paren-comments off prior to any checksum calculation.
I didn't take time to manually open the serial port and bang-out a gcode command by hand with a paren-comment to confirm that you can transfer the comment as long as it's not part of the checksum -- that would be another interesting test to see if OctoPrint can transfer those comments if it doesn't checksum them.
In any case, it seems I need to rename this issue to be GCode upload of file with any paren-comment lines fails. Or perhaps, more simply, Doesn't properly support paren-comments in GCode.
jneilliii
commented
2 years ago Based on our typical reference site, relative to comments here, these parenthesis are recommended to be removed by host software. Without looking at the comm layer I'm wondering if the line that is paren only and not contained within the middle of a command are getting stripped and then returning an empty line and sending it to printer and therefore getting a checksum mismatch?
nope, definitely not that. the comm layer definitely only seems to strip ; comments.
jneilliii
commented
2 years ago would it make sense to convert the strip_comment function to utilize a regular expression replacement here now with the complexity related to embedded parenthesis comments? Would be happy to submit a PR to fix.
dewhisna
commented
2 years ago Yes, I do think that this function https://github.com/OctoPrint/OctoPrint/blob/53b9b6185781c07e8c4744a6e28462e96448f249/src/octoprint/util/comm.py#L6060 should behave like https://github.com/MarlinFirmware/Marlin/blob/fb76ce841b8e6108207b2d33eb217191fdc80427/Marlin/src/gcode/queue.cpp#L325 for stripping paren-comments too.
The difficulty with such a regexp, that I can see, would be the nesting dilemma, https://3dprinting.stackexchange.com/questions/6410/are-parentheses-allowed-within-a-g-code-comment though most sources I can find say that nesting is simply not allowed, even if some firmware happens to support it.
But yes, if you have a regexp change for OctoPrint to remove the paren-comments, that would be a welcomed PR. Though I'm thinking the best solution for me is to simply remove all paren-comments from my files. It was an artifact from about six years ago when I was using a machine that supported both CNC Milling and 3D printing (which, BTW, is not a good idea, should anyone reading this think that it might be --- any machine beefy enough to be a decent CNC Mill will be too slow to be a decent 3D printer and vice versa).
foosel
commented
2 years ago Parenthesis style comments were so far never generated by 3d printer related tooling. As already mentioned they come from the CNC world, and the fully NIST standardized GCODE encountered there, contrary to intuition, has almost nothing to do with 3d printing, which abused the GCODE file format as a protocol, slapped on stuff like one sided transfer checksums and went with a lot of incompatible decisions.
Long story short, OctoPrint currently only supports ; style comments, in 10 years that never caused issues, if 3d printing tooling now suddenly realized that there actually is a NIST standard for GCODE and that it might be a good idea to actual read it then yes, OctoPrint should support these comments as well and I'll be happy to get a PR.
I'm kinda worried though about people using them in M117 and such for display purposes with firmware that don't support them and this would not strip them, so there'll need to be a feature flag for this I fear. There was a similar issue using : in M117 (which some firmwares interpret as for command separator but can't deal with checksumming then) that caused havoc in the past as no one knew it and it wasn't documented anywhere, so this behaviour change needs to be handled carefully to avoid a deluge of support requests.
dewhisna
commented
2 years ago Agreed -- and the M117 and this comment stripping debate gets even further convoluted when things like GCODE_QUOTED_STRINGS (in Marlin) is enabled and you start having to consider precedence of operators and/or order of nesting and then what to do when that nesting is invalid.
After this discussion, I'm leaning toward just leaving it as-is and say paren-comments aren't used in the 3D printer world, with the only possible OctoPrint change being it detecting them in a GCode file and aborting and/or warning the user about illegal file content rather than going down in flames.
jneilliii
commented
2 years ago good point about the M117 command and all the other additional complexities of embedded comments. maybe a compromise that would prevent the issue at hand, non-embedded paren comments?
regex_comment = re.compile(r"(;.+$)|(^\(.+\)$)")
"""Regex used to remove comments including ; and ()"""
def strip_comment(line):
if not regex_comment.match(line):
# shortcut
return line
result = regex_comment.sub("", line)
return result...but the escaping comes into play too...ugh.
cp2004
commented
2 years ago After this discussion, I'm leaning toward just leaving it as-is and say paren-comments aren't used in the 3D printer world, with the only possible OctoPrint change being it detecting them in a GCode file and aborting and/or warning the user about illegal file content rather than going down in flames.
That could be a feature submitted to the (bundled, but developed separately) OctoPrint FileCheck plugin. Depending of course on if there is a better solution or not - just pointing out that it would be a possible solution that we already have the framework for.
foosel
commented
2 years ago I'm leaning towards this as well now after learning that the file in question was not a recently generated one but rather something targeting a years old printer/cnc combo.
The problem
I was attempting to do a SD Card Upload of a GCode file. It fails with a repeating:
In looking at the OctoPrint log file and terminal dump, the problem seems to be in https://github.com/OctoPrint/OctoPrint/blob/master/src/octoprint/util/comm.py and how it processes the gcode lines that it is sending.
The source code seems to strip comments that use the ';' symbol, but not CNC-style paren-comments, which can be enabled in the printer's firmware and is used in this gcode file. If the paren-comments are on lines with actual gcode comments, there is no issue, as it sends the comment to the printer right along with the rest of the gcode command line. But, if a paren-comment appears on a line by itself, it seems that OctoPrint doesn't properly send the line number and checksum as it should, which causes the printer to rerequest the line and the two sides get out of sync.
The first few lines of the gcode file are:
The first line gets tossed completely by OctoPrint as a comment. However, the second line gets sent as-is, but without the proper line-number and checksum, since it contains no actual gcode command. That is,
gcode_command_for_cmdreturns nothing for it.This is what is seen in the OctoPrint log file:
That continues to repeat for a bit until it realizes it's stuck and tries to abort:
Then, for whatever reason, the printer and OctoPrint can't seem to get resynchronized until I reboot the printer board and reconnect with OctoPrint...
I think the problem is right here:
Where OctoPrint sends the
(**** start.gcode for The OctoForge ****)text without a line number or checksum and without stripping it as a pure comment line.Now arguably, paren-comment-only lines are a bit outside of gcode standard, even for CNC format, but the printer itself, if you copy this file directly to the SD card, interprets it just fine. So, there's a definite bug in OctoPrint's reading and sending the file.
My workaround, of course, is to just change the start.gcode segment to use the ';' symbol for comment-only lines and let OctoPrint just strip them. But I wanted to open this issue so that it can eventually get fixed in OctoPrint.
Did the issue persist even in safe mode?
Yes, it did persist
If you could not test in safe mode, please state why
No response
Version of OctoPrint
1.8.1
Operating system running OctoPrint
octopi_version: 0.18.0, octopiuptodate_build: 0.18.0-1.7.3-20220323100241
Printer model & used firmware incl. version
Custom rebuild of FFCP using BTT Octopus (i.e. an "OctoForge"). Firmware: "Marlin bugfix-2.0.x (Jun 5 2022 20:21:01)"
Browser and version of browser, operating system running browser
Firefox 100.0.2 on Xubuntu (but not related to problem)
Checklist of files to include below
Additional information & file uploads
Systeminfo Bundle: octoprint-systeminfo-20220609124913.zip
GCode file in question: FilamentSensorPlug_octoforge.gcode.zip
Full Log file from OctoPrint: octoprint.log.2022-06-06.zip