prusa3d / PrusaSlicer

G-code generator for 3D printers (RepRap, Makerbot, Ultimaker etc.)
https://www.prusa3d.com/prusaslicer/
GNU Affero General Public License v3.0
7.64k stars 1.92k forks source link

Label objects feature sometimes generates malformed EXCLUDE_OBJECT_DEFINE macro (for klipper FW) #11926

Closed Le0Michine closed 7 months ago

Le0Michine commented 9 months ago

Description of the bug

I noticed that sometimes when I enable labeling objects for klipper fw using EXCLUDE_OBJECT_DEFINE macro the output gcode is malformed which causes printer to halt mid print. It appears that for some reason the original 3mf file I got included non breaking whitespace chars (U+00A0) in some of the model names, those characters weren't replaced with underscores causing printer to stop.

Archive.zip

Project file & How to reproduce

Open Frame Front - left.3mf from attached archive. Slice and export as gcode into a local folder. Open gcode file and look for lines with EXCLUDE_OBJECT_DEFINE macro.

An example of it is below

EXCLUDE_OBJECT_DEFINE NAME=Frame_Front__ left_stl CENTER=....

The name includes non breaking whitespace character which causes klipper to fail parsing the input. Expected behaviour is to replace that character with underscore just like it is already done to regular whitespaces and some other chars.

Checklist of files included above

Version of PrusaSlicer

Version 2.7.1+MacOS-x64

Operating system

MacOS Sonoma 14.1.2 (23B92)

Printer model

N/A

lukasmatena commented 8 months ago

This is a problem similar to https://github.com/prusa3d/PrusaSlicer/issues/11802.

As far as I can tell, Klipper documentation does not go further than EXCLUDE_OBJECT NAME=<name>. There is no information about what is the encoding of <name>, what characters are (not) allowed, if some need escaping, whether single/double quotes would solve some of these issues and similar. @jschuh tried to obtain this information from Klipper source code, but the list is apparently not complete.

@KevinOConnor Could the Klipper documentation be updated to contain this kind of information, as is customary anywhere where text data are shared? Sorry for tagging you like this, but I noticed that you are active in Klipper development. I would have opened an issue on Klipper github, but I apparently can't, and the alternative is a Discourse or Discord, advertised as "communities where Klipper users can discuss Klipper with other users", which is not what I am interested in doing. Thanks.

jschuh commented 8 months ago

@lukasmatena, the case in this report is handled correctly by PR #11813, which filters aggressively enough that it's safe for Klipper and the popular frontends. Assuming Klipper is running on Python3 (for unicode handling) it shouldn't be possible to generate a malformed EXCLUDE_OBJECT entry with that PR applied.

It's just that PR #11813 didn't make it into PrusaSlicer 2.7.1. Maybe that was an oversight?

lukasmatena commented 8 months ago

@jschuh Thanks for the reply. The fact that #11813 is not in 2.7.1 was not an oversight, we processed it when 2.7.1 was already frozen, so it will go into the next release (2.7.2). The reason I rumbled is that I somehow imagined that the non-breaking space was supposed to be replaced by an underscore too, which the PR doesn't do.

Now when I think about it more, you are probably right that it is simply a non-ASCII character with no special meaning, that it needs not to be replaced by anything and that the reason why @Le0Michine has problems is because he is still on python2, and that he would have had the same problem with any (?) non-ASCII character. Or at least that is how I understand it now, hopefully it is correct.

@Le0Michine Can you please share which python version you are running Klipper on?

Le0Michine commented 8 months ago

@lukasmatena I understand that lack of documentation on klipper side is a problem and it would be great to have an exhaustive list of supported characters, however I believe it should be safe enough to stick to alphanumeric charset to make it safe. In this particular report I was focused on non breaking whitespace specifically because it technically is a whitespace and often treated as such, meaning it can separate command arguments just like normal whitespace would.

Answering your question about python version, I'm not sure how to check what version is actually used by klipper v2 or v3, so here is both I see in the OS:

Python 2.7.16 Python 3.7.3

Klipper version (if important) is v0.12.0-9-gbb4711c5

jschuh commented 8 months ago

@lukasmatena, yeah, I didn't exhaustively replace problematic characters because quoting the arguments and replacing the quote and line termination characters was sufficient in my testing for all possible characters (assuming python 3). And I didn't mean to be presumptuous in asking about an oversight, I had just misinterpreted the original question as assuming the patch had already been applied.

@Le0Michine, a stock Klipper install should have the path ~/klippy-env/bin with Klipper's python binaries. So, you should be able to ssh in and run a command like this:

ls -la ~/klippy-env/bin/python

If you get a line of output like this, it means Klipper is running under Python 3:

lrwxrwxrwx 1 pi pi 16 May 25  2023 /home/pi/klippy-env/bin/python -> /usr/bin/python3

Whereas a line like this means Klipper is running under Python 2:

lrwxrwxrwx 1 pi pi 7 Dec 18  2020 /home/pi/klippy-env/bin/python -> python2
Le0Michine commented 8 months ago

thanks for the pointer @jschuh, it points to python2 for me

lrwxrwxrwx 1 pi pi 7 Dec  2  2020 /home/pi/klippy-env/bin/python -> python2
jschuh commented 8 months ago

@Le0Michine, Okay, so you'd still have issues after the PR makes it into a future release.

Assuming you don't have any reason to keep Klipper on Python 2, you could just update the environment. I've seen people recommend KIAUH for automatically upgrading your Klipper environment to Python 3. That would probably be the safest and easiest approach, but I haven't used it so I can't personally make a recommendation.

I'm used to manually installing and configuring Klipper, but that's more of a "do at your own risk" sort of thing. Here's a commented command history from the last time I updated a machine to Python 3:

# Stop the service and switch to the home directory.
sudo service klipper stop
cd ~

# Delete the old Python 2 environment.
rm -rf klippy-env

# Create the Python 3 environment
virtualenv -p python3 klippy-env

# Add the Klipper dependencies to the new environment
cd ~/klippy-env
bin/pip install -r ../klipper/scripts/klippy-requirements.txt

# Start Klipper back up
sudo service klipper start
Le0Michine commented 8 months ago

Oh, thanks for recommendation, I’m gonna try it, what can go wrong 🙂 though I’m assuming still would need the pr to be released for it to help

jschuh commented 8 months ago

Yeah, it still needs the PR because that adds the argument quoting. But I tested that exact string on my locally patched fork and it worked fine; the label just included the non-breaking whitespace.

lukasmatena commented 7 months ago

This should be fixed (for Python3 at least) in 2.7.2-alpha2. Closing.

@jschuh Thanks again.