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

PSU sensing blocks OctoPrint #84

Closed
btk5h opened this issue Dec 25, 2017 · 12 comments
Closed

PSU sensing blocks OctoPrint #84

btk5h opened this issue Dec 25, 2017 · 12 comments

Comments

@btk5h
Copy link

btk5h commented Dec 25, 2017

I've been using PSUControl with ouimeaux to toggle a smart plug connected to my printer. PSUControl works perfectly fine with internal sensing, but system command sensing is completely unusable.

After setting the sensing command to ~/oprint/local/bin/wemo switch "Printer" status | grep -q 1, OctoPrint blocks while waiting for the command to complete, effectively pausing the printer after every few instructions.

@kantlivelong
Copy link
Owner

You may need to escape characters but I would suggest just putting it in a script file and calling that.

@btk5h
Copy link
Author

btk5h commented Dec 26, 2017

Sorry if my original issue was unclear. The system command works as-is; the issue is that PSUControl seems to be blocking OctoPrint while waiting for the system command to complete. The command takes a couple of seconds to query the status of the switch and during that time, OctoPrint can't send G-code to the printer. This causes the printer to stop frequently mid-print, creating some nasty print artifacts.

Not a Python guy, but I'm guessing that this line is responsible for the issue:

output = p.communicate()[0]

@kantlivelong
Copy link
Owner

kantlivelong commented Dec 26, 2017

DOH

@kantlivelong
Copy link
Owner

Scratch that. I read that as switching and not sensing. You are saying that using a script such as:

#!/bin/bash
sleep 50

would hang OctoPrint?

@btk5h
Copy link
Author

btk5h commented Dec 26, 2017

Yes. Ideally, check_psu_state should poll the process using Popen.poll before calling communicate.

Maybe include a configurable timeout option to the plugin as well?

@kantlivelong
Copy link
Owner

I've committed a fix to a temporary branch. Ended up using poll() and removing anything to do with stdout/stderr as it wasn't doing anything with it anyway.

You can install the test branch with the URL (Remove the existing version first):
https://github.com/kantlivelong/OctoPrint-PSUControl/archive/issue_84_fix.zip

@btk5h
Copy link
Author

btk5h commented Dec 26, 2017

p = subprocess.Popen(self.senseSystemCommand, shell=True)
p.poll()
r = p.returncode

This fix shouldn't work. Popen.poll doesn't block the thread waiting for the process to finish. Instead, it returns None if the process hasn't finished and Popen.returncode if the process has finished.

@kantlivelong
Copy link
Owner

Hmm yeah I mean't to use wait() but it worked in testing. 🤷‍♂️

Changed in 1df8998

@kantlivelong
Copy link
Owner

Err nope. My head is broken today. Give me a bit.

@kantlivelong
Copy link
Owner

kantlivelong commented Dec 26, 2017

Interesting. Even using poll() is blocking even though check_psu_state() is in its own thread.

See d3daa18

Edit:
I now see why it's occurring. I have other calls to check_psu_state() that are not in a thread. So the original problem is really caused by those calls.

kantlivelong added a commit that referenced this issue Dec 26, 2017
@kantlivelong
Copy link
Owner

Okay I am fairly confident that this latest change should resolve the issue as well as unify how all system commands are executed.

Give it a shot and let me know how you make out.
https://github.com/kantlivelong/OctoPrint-PSUControl/archive/issue_84_fix.zip

PS:
I may add a configurable timeout as well as polling interval at a later date.

@kantlivelong
Copy link
Owner

Confirmed working. Merged into devel with commit 36b1282

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants