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

Adding an option "Post Connect Delay" #234

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

Dunstkreis
Copy link

@Dunstkreis Dunstkreis commented Mar 19, 2022

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:

  • Your PR targets the devel branch.
  • Your PR was opened from a custom branch on your repository (no PRs from your version of master or devel please)
  • Your PR only contains relevant changes: no unrelated files, no dead code, ideally only one commit - rebase your PR if necessary!
  • Your changes follow the existing coding style.
  • You have tested your changes (please state how!)

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

copied from postOnDelay and reused as postConnectDelay
copied the div from Post On Delay and reused it as Post Connect Delay.
copied the if statement from psucontrol.autoOn() an reused it for hiding the option if auto connect is not used.
@Rowbotronics
Copy link

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
Copy link

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

@Dunstkreis
Copy link
Author

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
Copy link

Gifford47 commented Dec 22, 2022

@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
Copy link

knewg commented Jan 23, 2023

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
Copy link

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

@Dunstkreis
Copy link
Author

@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
Copy link

knewg commented Aug 14, 2023

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

@Gifford47
Copy link

@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
Copy link

@kantlivelong could you check my last pr please?

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

Successfully merging this pull request may close these issues.

4 participants