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

Erronious model size when G92 misinterpreted by gcodeInterpreter #1906

Closed
CapnBry opened this issue May 8, 2017 · 7 comments

Comments

Projects
None yet
2 participants
@CapnBry
Copy link
Contributor

commented May 8, 2017

What were you doing?

Uploading a file and letting OctoPrint analyze the file

What did you expect to happen and what happened instead?

When printing, I was warned that my model could not be printed because it exceeded the print area of my printer. The model was determined to be 238.44mm × 190.00mm × 3687.78mm when it actually is 238.44mm x 190.00mm x 18.72mm. This is size listed in the file list details. The metadata.yaml shows that my printingArea.minZ is -3500.

I tracked this down to a G92 I have in my start GCode: G92 E0 Z1.52. The gcodeInterpreter code for G92 seems to take X Y and Z values as offsets when G92 is actually 'Set Absolute Position'. For example, if I was at X0 Y0 Z0 (homed) and said G92 X100 Y100 and then G1 X100 Y100, the toolhead would not move because it is already at X100 Y100.

The source code appears to be doing some sort of G92.1 or G92.2 which has to do with setting offsets (but only zeroing them?). I've never seen a G92.1 or .2 in practice (Slic3r, Cura, Simplify3D) so I can't say if this is used for anything. The code does the right thing for G92 Exxx. Here is how I believe it should be:

diff --git a/src/octoprint/util/gcodeInterpreter.py b/src/octoprint/util/gcodeInterpreter.py
index ac10954..db75ac8 100644
--- a/src/octoprint/util/gcodeInterpreter.py
+++ b/src/octoprint/util/gcodeInterpreter.py
@@ -400,11 +400,11 @@ class gcode(object):
                                        if e is not None:
                                                currentE[currentExtruder] = e
                                        if x is not None:
-                                               posOffset.x = pos.x - x
+                                               pos.x = x
                                        if y is not None:
-                                               posOffset.y = pos.y - y
+                                               pos.y = y
                                        if z is not None:
-                                               posOffset.z = pos.z - z
+                                               pos.z = z

                        elif M is not None:
                                if M == 82:   #Absolute E

Why does this cause a problem? On every absolute "move" the position is incremented by posOffset. There is no Z parameter in the X/Y G1 moves, so pos accumulates 1x posOffset.z per move. In my case that means -every X/Y move- thought it was also offsetting the Z axis by -1.52mm. On each layer change, OctoPrint also thinks it has to travel back from Z=-3000 all the way to the actual Z value (although the travel time is fairly quick because most slicers do not include a Z feedrate). This spills over into overestimating the print time.

The values could be way off in the X / Y direction as well, but this is mitigated by the fact that almost all moves include X and Y coordinates so the accumulated position does not grow over time as it does with the thousands of GCode G1 lines without a Z parameter. The only way a G92 with an X or Y would cause a notable increase in estimated print time is if the toolhead just moved back and forth along a single axis, in which case the other axis would show a ever-growing position. The "Model Size" would roughly report the model size plus (or minus) the G92 X or Y offset then.

Branch & Commit or Version of OctoPrint

Master 1.3.2 release

Operating System running OctoPrint

Raspbian Jessie on Pi 2 B

Printer model & used firmware incl. version

C-Bot Core XY with Smoothieware

Browser and Version of Browser, Operating System running Browser

N/A

Link to octoprint.log

Logs indicate model is queued for analysis and then completed in N seconds

Link to contents of terminal tab or serial.log

N/A

Link to contents of Javascript console in the browser

N/A

Screenshot(s) or video(s) showing the problem:

I have read the FAQ. And I still love cookies even if I am not required to.

@foosel

This comment has been minimized.

Copy link
Owner

commented May 9, 2017

You have perfect timing. I just had someone on IRC send me a demo file because they were experiencing the exact same problem. I was going to look into this today, but now your ticket + the analysis you already did ❤️ reminded me of #1863 and the fixing commit fe585e7 I did for that.

In a nutshell: Should already be fixed on maintenance and go out with 1.3.3 (which I hope to release a first RC for this week), but if you could take a look at maintenance and verify that it indeed works for you there too, that would be awesome :)

@foosel

This comment has been minimized.

Copy link
Owner

commented May 9, 2017

As a side note, I just took another look at the code because I was wondering why I did solve it the way I did and not your way, and then I remembered that the internal offset is used for two things, tracking offsets set through G92 and tool head offsets for multi extruder setups. Which now got me wondering why the heck I did do THAT because for the actual movement calculations and extrusion length it's either irrelevant or plain wrong. So while 10min ago I still thought, "hah, already solved", you've now plunged me into a deep crisis of questioning the decisions I made for the gcode interpreter around three years ago ;)

@CapnBry

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2017

Well, dammit, why didn't I find that issue when I searched? Oh man it was because I just started typing in the search bar which already had is:open in it and you'd already fixed it. I can confirm that the change in fe585e7 / jumping to the maintenance branch does work around the issue with the G92.

But see now you've done the same thing to me, I've spent the last 30 minutes trying to decide if I think that a G92 should be stored as an offset though. For one, there's no way to 'pop' the offset back off (via G92.1 or G92.2) in the interpreter. Looking at firmware sourcecode for Marlin it does not support G92 as an offset, but Smoothieware does store it as a world coordinate transform and supports resetting it. I do think that I agree with you that mixing the purpose between the toolhead offset and a G92 offset is ill-advised, especially when the E distance interprets it as an absolute position but the other axes interpret it as an offset with no way to remove the offset (apart from applying a negative G92 which would definitely break printers!).

We programmers love to reduce complexity though, right? So if we're not going to change it to use its own offset so that we can support G92.1/2 I would remove the code in the interpreter's G92 and just interpret it as setting the absolute position like the E code does.

foosel added a commit that referenced this issue May 9, 2017

@foosel

This comment has been minimized.

Copy link
Owner

commented May 9, 2017

why didn't I find that issue when I searched? Oh man it was because I just started typing in the search bar which already had is:open in it and you'd already fixed it

Nope, still open actually because 1.3.3 isn't released yet, only marked as solved. But neither I nor the reporter mentioned G92 in it, so you had basically no chance ;)

I just pushed a commit that does what you suggested. I thought it through a bit more throughout the day, also took a look at how the gcode viewer handles it, and yes, setting the position instead of mixing the values into the offset does make more sense considering the definition of G92 on the reprap wiki (not a spec, but the closest thing there is to a spec):

Allows programming of absolute zero point, by reseting the current position to the values specified.

A G92 without coordinates will reset all axes to zero.

I also noticed that the latter part was still missing, so I also added that.

Now I just hope I didn't miss anything critical - if you could also throw another look at it, that would be great!

@CapnBry

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2017

Oh! Then it must have been because I saw the word Modelgröße and said, this isn't my problem, it is a problem for people on the moon because this is crazy moon language!

I'll pull and re-test once this print is finished but I switched back to rc/maintenance because leaving my browser window open but not focused on maintenance causes a long hang when returning after 30+ minutes. Takes over 4 seconds on a desktop and over 90 seconds on a tablet. There's a knockout binding for an array change that's running the whole time I think. We're mixing PRs here but I'll chat you up when I trace it back?

@foosel

This comment has been minimized.

Copy link
Owner

commented May 10, 2017

Unrelated to this ticket, but just for the record: Thanks for the heads-up regarding the delay, the offending commits have now been reverted.

@foosel

This comment has been minimized.

Copy link
Owner

commented May 31, 2017

1.3.3 was just released.

@foosel foosel closed this May 31, 2017

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.