klippain icon indicating copy to clipboard operation
klippain copied to clipboard

Add check to prevent install script from interrupting a print on localhost

Open ruok5 opened this issue 1 year ago • 5 comments

This uses the Moonraker API on localhost to see if a print is currently running.

The logic is as follows:

  • It's already confirmed that the Klipper service is installed (going on faith that Moonraker is too)
  • If the Moonraker API responds to a server/info request with a 200 HTTP status, we can assume it can answer a print_stats request too
  • Demand that the status be 'standby'

If the Klipper service isn't running, the API checks will fail, but I'm allowing that to happen silently, because a print can't be running anyway.

Yes, I lost a print because I was dumb.

ruok5 avatar Jul 25 '23 18:07 ruok5

in which use case is this relevant? If a print is running, moonraker updates are disabled. if you install it, you get asked what happens, and a user should not install something while printing (imho). Same for ERCF, which also restarts klipper service without "warning". Just want to understand :)

Surion79 avatar Jul 25 '23 23:07 Surion79

I had a bad experience installing klippain. After reading the documentation, I believed my configuration files would be moved to a different directory and new configuration files would replace them. I expected that I would initiate a 'firmware restart' to cause the new config to be read the first time, not that the klipper service would be manipulated by the script. I admit complete fault on my part here. I'm the idiot who piped curl to bash without reading it line-by-line first.

I wanted to save the next person from my mistake.

This could be addressed in documentation ("hey, this is going to restart klipper no matter what!"), but my proposed solution helps people who read the docs and people who don't.

ruok5 avatar Jul 26 '23 14:07 ruok5

Thanks for the PR.

This looks pretty good for me. I just need to think a little bit at what could happens if the moonraker instance is in another computer. Even if this case should be not standard at all, it can potentially block Klippain install, so I would prefer to detect it and if it's the case, continue the installation with a warning.

Frix-x avatar Jul 31 '23 07:07 Frix-x

My personal opinion on this: It is too complex for a single point of time check where it would be ONLY needed if someone does install of a software that does a config "reset" while printing. Adding an additional note to the dialog asking that this installs klippain should be sufficient, without impacting the complexity. Independant of the localhost topic Frix wrote about, it could also get changes within the codeblock that makes it no longer work. This needs to cover all possibilities of set up so it does not result in a permanent fail for all klippain installs, as an error in that case would break the whole klippain install for all users. Relation of usage to risk does not fit here.

Surion79 avatar Aug 01 '23 21:08 Surion79

Ok, so I added another warning for now in the install script: 2514b894b7f881bc60c6d5c3d777ac37cab57811 I'm going to put this PR on hold and see how we can merge this in a smart way later. This warning should be enough for now.

Frix-x avatar Aug 04 '23 11:08 Frix-x