thelastWallE / OctoprintKlipperPlugin

A plugin for a better integration of Klipper into OctoPrint.
GNU Affero General Public License v3.0
65 stars 14 forks source link

Editing printer.cfg from within Octoklipper nulls the printer.cfg #9

Closed girrrrrrr2 closed 4 years ago

girrrrrrr2 commented 4 years ago

octopi, Octoprint version 1.4.2 Octopi Version 0.17.0

octoprint.log

When updating the klipper Printer.cfg from within octoklipper it nulls your printer CFG config. Im not sure exactly how that happens, the file chmod is 0777.

Ive confirmed it twice on accident by editing and having to recover/remake my file

tvercruysse commented 4 years ago

Can confirm, editing the config file from the UI clears the file, also in my case the config file in the UI itself is cleared.

image image

PuddingBaer91 commented 4 years ago

you may used special characters? or comma instead of point? For me this was the cause in the old version..

girrrrrrr2 commented 4 years ago

you may used special characters? or comma instead of point? For me this was the cause in the old version..

Not as far as I know. I was editing the file that I was using between prints to lower a stepper current. From 600 to 550 and then saved. After that it was just a blank file.

Actually just checked and didn't see any special characters or commas that weren't in commented out lines.

luco85 commented 4 years ago

Me too! 🙈🙈😤😤

DiaHack commented 4 years ago

same issue

mental405 commented 4 years ago

This is an old issue related to precision in floating point numbers such as in the extruder step distance.

https://github.com/mmone/OctoprintKlipperPlugin/issues/45

AliceGrey commented 4 years ago

@girrrrrrr2 and @tvercruysse Could you please provide a copy of your config file for testing?

AliceGrey commented 4 years ago

@luco85 and @DiaHack you are also welcome to upload a copy of your config file.

AliceGrey commented 4 years ago

@mental405 I think this is most likely the case. I will attempt to recreate the issue but so far my config has saved. It's possible my config is not replicating the issue properly.

PuddingBaer91 commented 4 years ago

@mental405 I think this is most likely the case. I will attempt to recreate the issue but so far my config has saved. It's possible my config is not replicating the issue properly.

Just add , or ö ä ü or ß to your config and Press save. Or add more numbers after a dot:. 0.00000000000000009 for extruder step distance

DiaHack commented 4 years ago

printer.cfg.zip

@luco85 and @DiaHack you are also welcome to upload a copy of your config file.

AliceGrey commented 4 years ago

Ok. I need testers who are using python2 and testers who are using python3. Please Uninstall the plugin then manually install the following branch in octoprint. https://github.com/AliceGrey/OctoprintKlipperPlugin/archive/configUnicodeFix.zip.

As always. Make sure you have backups and know what you're doing and can fix issues with your octoprint install if they arise.

Thank you! Alice

mental405 commented 4 years ago

I blame javascript because it is always javascript. It could easily be something buried in octoprint itself where it is getting the text from the textbox and putting into the python collection. I have no idea why it would be crapping out due to a bunch of characters (numbers) after a another character (decimal) unless it was trying to parse said characters and convert them into something.

Regardless of whether it can get fixed or not, I think it would be a good idea to write the current config out to a backup file and add a check around line 104 to check the length of the string or something before blindly overwriting the config file.

AliceGrey commented 4 years ago

@mental405 It looks to be the unicode conversion in python actually. I definitely agree the handling of saving the file is really rough and needs more sanity checking. There is a lot of sanity checking that I've been working on adding to the plugin. It's just not done yet.

mental405 commented 4 years ago

Another thing that would be useful and idk how to do it with the oprint events.. .but maybe there is an on_terminal_string event or something... basically the plugin should reload the config file if the config file is altered outside of the plugin.

Example Calibrating Z offset => SAVE_CONFIG => Open config in plugin => config is cached from before the calibrations => click ok on plugin window. => old config overwrites new calibrations

sandos commented 4 years ago

I can not trigger this, either with unicode or numbers in the regular branch? Ive triggered the unicode bug in mmone/OctoprintKlipperPlugin before though. I am running python 2 though.

AliceGrey commented 4 years ago

Another thing that would be useful and idk how to do it with the oprint events.. .but maybe there is an on_terminal_string event or something... basically the plugin should reload the config file if the config file is altered outside of the plugin.

Example Calibrating Z offset => SAVE_CONFIG => Open config in plugin => config is cached from before the calibrations => click ok on plugin window. => old config overwrites new calibrations

@mental405 So in my fork this shouldn't happen anymore. I changed it so the file is reloaded whenever the settings page is opened to look at the config file. It shouldn't be opening a cached copy. That could be a browser/JS issue though.

Edit: If this is still happening though open an issue and I'll take a look at it :+1:

mental405 commented 4 years ago

Another thing that would be useful and idk how to do it with the oprint events.. .but maybe there is an on_terminal_string event or something... basically the plugin should reload the config file if the config file is altered outside of the plugin. Example Calibrating Z offset => SAVE_CONFIG => Open config in plugin => config is cached from before the calibrations => click ok on plugin window. => old config overwrites new calibrations

@mental405 So in my fork this shouldn't happen anymore. I changed it so the file is reloaded whenever the settings page is opened to look at the config file. It shouldn't be opening a cached copy. That could be a browser/JS issue though.

Edit: If this is still happening though open an issue and I'll take a look at it

Ah cool I haven't had a chance to download and run yours yet. I have been on vacation and someone mentioned that OctoKlipper was under new management so I thought I would pop in and check the commit history and open issues to see if this exact thing had been fixed yet :P

I plan on loading it up as soon as I get back.

luco85 commented 4 years ago

hellooo! I'm Here This is my printer file

IN TEST.CFG.zip

definitely-not-a-t-rex commented 4 years ago

Happened to me too while changing the temperature inside [gcode_macro START_PRINT] from 200 to 210, I'm using latest edge on octoprint 1.4.2 with python 3 on raspberry pi os arm64, if I try to change anything from the plugin the printer.cfg becomes blank and octoprint's terminal returns

Recv: // No section: 'printer'
Recv: // Once the underlying issue is corrected, use the "RESTART"
Recv: // command to reload the config and restart the host software.
Recv: // Printer is halted
Recv: !! No section: 'printer'

upon the reboot of the printer (to apply the config)

goeland86 commented 4 years ago

@AliceGrey I have an automated image building process going on so I can quickly build images to test whichever branch of the plugin you want. Check https://github.com/goeland86/ReFactor.

I'm currently validating the build with the py3 plugin works, as soon as my pi4 spits out a valid image to test I'll be testing it with py2 and py3 on a "sandbox" Beaglebone + Replicape setup.

definitely-not-a-t-rex commented 4 years ago

https://github.com/AliceGrey/OctoprintKlipperPlugin/archive/configUnicodeFix.zip.

Hi, I tested it with python 3 and it seems to work, I can edit and save the cfg file and it doesn't get wiped anymore

AliceGrey commented 4 years ago

@definitely-not-a-t-rex Thank you. I just need someone to test and verify in python 2 then I'll merge the fix. It works in my local dev environment but I'd like confirmation.

definitely-not-a-t-rex commented 4 years ago

@definitely-not-a-t-rex Thank you. I just need someone to test and verify in python 2 then I'll merge the fix. It works in my local dev environment but I'd like confirmation.

If nobody does it by Friday I can clone the install and try reverting to python 2 and test it there too, I'm really busy until then but after I should have a few days of freedom from uni

sandos commented 4 years ago

I plan on testing it tonight with my py2 install. Although I could not trigger the original bug, it might be a good smoke test.

goeland86 commented 4 years ago

To recreate the original bug, wouldn't trying to use one of the attached configs at the top of the ticket do the trick?

I'm happy to do it on my sandbox - building a py2 image right now.

sandos commented 4 years ago

To recreate the original bug, wouldn't trying to use one of the attached configs at the top of the ticket do the trick?

I'm happy to do it on my sandbox - building a py2 image right now.

Tried with the attached config, tried adding lots of zeroes to numbers and some unicode characters. Still no bug.

sandos commented 4 years ago

https://github.com/AliceGrey/OctoprintKlipperPlugin/archive/configUnicodeFix.zip.

Uhm. I dont know what to say. ^ This build clears the config at once for me with py2! The original branch from this repo, not a single time. Is this a py2/py3 issue? Either that or I am doing something really stupid.

AliceGrey commented 4 years ago

@sandos Yes this is a python 2 vs python 3 issue. Python 3 strings are unicode by default. The original developer had a unicode conversion that doesn't work in python 3 but worked in python 2.

@goeland86 Could you please test the build in your py2 image.

goeland86 commented 4 years ago

@AliceGrey I tried it on Py2... But when I do a change in OctoKlipper, it isn't saved to the file on disk. No matter what I put in - a simple change from 130 to 120 on the heaterbed max_temp, or using a comma instead of a dot for a decimal point. I'm not sure if it's the plugin or the image itself.

goeland86 commented 4 years ago

Actually, found this in the Octoprint log file:

2020-09-24 23:07:19,796 - octoprint.server.api.settings - ERROR - Could not save settings for plugin OctoKlipper (0.3.1)
Traceback (most recent call last):
  File "/home/debian/OctoPrint/venv/lib/python2.7/site-packages/octoprint/server/api/settings.py", line 586, in _saveSettings
    plugin.on_settings_save(data["plugins"][plugin_id])
  File "/home/debian/OctoPrint/venv/lib/python2.7/site-packages/OctoKlipper-0.3.1-py2.7.egg/octoprint_klipper/__init__.py", line 119, in on_settings_save
    f = open(configpath, "w")
UnboundLocalError: local variable 'configpath' referenced before assignment
goeland86 commented 4 years ago

Huh. Uninstalling the plugin in OctoPrint, then reinstalling it allows me to save known good changes.

goeland86 commented 4 years ago

@AliceGrey ok - so I confirm what @sandos reported. If there's a UTF-8 character in the config (I used æøå in a comment), it clears out the entire file.

Plugin was installed from URL using https://github.com/AliceGrey/OctoprintKlipperPlugin/archive/configUnicodeFix.zip

AliceGrey commented 4 years ago

Ok I'll work on a python 2/3 fix that works for both.

AliceGrey commented 4 years ago

@goeland86 and @sandos please test the newest version of https://github.com/AliceGrey/OctoprintKlipperPlugin/archive/configUnicodeFix.zip. It now includes an if statement to keep the old code for py2. It is tested and working on my python2 and python3 dev environment.

goeland86 commented 4 years ago

@AliceGrey I confirm that with the newer version, the comment with æøå insertion does not erase the config using py2. Will build again using py3 and test the plugin with py3.

goeland86 commented 4 years ago

And I confirm it works on py3 build as well 👍

sandos commented 4 years ago

Works perfectly, thanks!