kantlivelong / OctoPrint-PSUControl

Smart control of your power supply via GPIO, GCODE Command, System Command, or variety of sub-plugins.
GNU Affero General Public License v3.0
202 stars 112 forks source link

Adding an option "Post Connect Delay" #234

Open Dunstkreis opened 2 years ago

Dunstkreis commented 2 years ago

Thank you for your interest into contributing to OctoPrint-PSUControl, it's highly appreciated!

Before submitting please make sure you have ticked all points on this checklist:

Feel free to delete all this help text, then describe your PR further. You may use the template provided below to do that. The more details the better!


What does this PR do and why is it necessary?

I'd summarize as: Adding an option "Post Connect Delay" same as and kinda copied from "Post On Delay" but after connecting to the printer to give Octoprint time to get fully connected before continuing its work (like trying to start a print)

This solves the issue with not starting a print after powering on the printer via PSU control due to connecting not being fast enough. https://community.octoprint.org/t/automatically-start-printing-after-turning-on-via-psu-controll-after-api-upload/43030/11

How was it tested? How can it be tested by the reviewer?

I just copied the two files from this repo to my local setup and run it. Worked as supposed.

Any background context you want to provide?

What are the relevant tickets if any?

Further notes

Take a look at: https://community.octoprint.org/t/automatically-start-printing-after-turning-on-via-psu-controll-after-api-upload/43030/11

Screenshots (if appropriate)

n/a

Rowbotronics commented 1 year ago

I've manually downloaded and installed the changes from this pull request and it fixed the issue I had with starting prints on my Prusa MK3S+. The added post connect delay was what it needed to actually get the print started after uploading and powering on. The changes in this pull request are minimal and look to conform well with what is already in the codebase, so hopefully this change can get merged into the version that is easily available through the OctoPrint plugin manager.

Gifford47 commented 1 year ago

i have also tested this PR and everything works! @kantlivelong please approve this PR

Dunstkreis commented 1 year ago

we discussed it here: https://community.octoprint.org/t/automatically-start-printing-after-turning-on-via-psu-controll-after-api-upload/43030/14

And in the linked post kantlivelong stated that relying on delays sucks. In general yes. But in this case, as its a simple copy&paste from another place in the same file I am still thinking my solution is kinda valid. Yes, its just another additional delay / timeout. But its not like its against the overall scheme or architecture of this plugin.

And I didn't find the time - or the need, cos its working for me - to check the other solution from oerkel47 he is pointing at.

To me its like I was trying to fix my issue with the least amount of work to put in - and to share my results. So I am happy you found this and it helps you.

My intention was not to "rebuild" any parts or to check if there is a better way of doing it. Which would be a good thing for sure. And I can completely understand why he asked me to do it. Just cos of lack of time and skill I wont be able to double check any other solutions in the near future.

And even if its a better solution to the problem. This PR is like the simplest solution and besides "there might be a better one" I have not yet seen a good reason against this PR to not include it.

So I gotta say I am with @Rowbotronics and @Gifford47 - its a valid solution. Its simple. Its nothing new. Its just like some other places of the plugin. And the both of them validated its solving the issue. So I'd be happy to see this PR included.

Gifford47 commented 1 year ago

@kantlivelong @Dunstkreis ok, thanks for all the infos! i have rewritten the code and now it works like a charm. Please commit the following change in your PR: on line 506 in __init__.py add following:

            if self.config['sensingMethod'] not in ('GPIO', 'SYSTEM', 'PLUGIN'):
                self._noSensing_isPSUOn = True

            time.sleep(0.1 + self.config['postOnDelay'])

            self.check_psu_state()

            threading.Timer(self.config['postConnectDelay'], self.connect_printer).start()

    def connect_printer(self):
        if self.config['connectOnPowerOn'] and self._printer.is_closed_or_error():
            self._printer.connect()
            time.sleep(0.1)

        if not self._printer.is_closed_or_error():
            self._printer.script("psucontrol_post_on", must_be_set=False)
knewg commented 1 year ago

It would be really helpful if this PR could be merged, not a perfect solution with a delay, but it's a simple fix for now... I would assume that this is an issue for all Prusa owners that try to print directly from the slicer.

Gifford47 commented 1 year ago

Look at my post above. My change is without a delay and works great in production! Please change the code like suggested.

Dunstkreis commented 1 year ago

@Gifford47 I wont put something else than my code in my PR. Simply cos I aint got no time right now to double check what your approach does and what not and to be sure it does what its supposed to do ...

If your approach is better than mine that's great. Just place a pull request yourself. Its not a big deal. Just a few clicks and you are good to go.

You might leave a reference here for others to cross check. Not to say I'd be happy to see a reference here!

Not sure as I am not a developer but I guess that's how it works and not to change a PR. (If thats possible at all!? dont know though ...)

knewg commented 1 year ago

@Gifford47 please can you make your own pull request so that any solution can be merged?

Gifford47 commented 1 year ago

@Gifford47 please can you make your own pull request so that any solution can be merged?

I can do it next week. But this repository seems not to be actively maintained.

Gifford47 commented 1 year ago

@kantlivelong could you check my last pr please?