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

Allow same GPIO for switching and sensing #223

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

Conversation

xaver-k
Copy link

@xaver-k xaver-k commented Sep 6, 2021

What does this PR do and why is it necessary?

The changes allow to re-use the switching GPIO for sensing the current on/off state.
This addresses #182.

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

Tested on my personal setup, which uses a Raspberry 4 and pulls GPIO 2 to GND for switching the printer on:
circuit drawio

Further testing on other setups is needed, but I only have the one printer.

What are the relevant tickets if any?

#182

Further notes

  • The changes also clean up some inconsistencies in the code between the sensing and switching pin (both use CdevGPIO now).
  • The changes use the inverted-parameter of CdevGPIO to remove a lot of manual handling of inverted pin logic.
  • See this PR as a starting point. I would highly suggest to do some refactoring and splitting up the logic for each switching and sensing method. However, I did not want to put a lot of energy into that without knowing whether you would actually be interested.

Copy link
Owner

@kantlivelong kantlivelong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey thanks! I took a quick look and seems pretty good. Will carve out some time in the next week or so to test. Only one thing stands at the moment.

@@ -64,7 +64,7 @@ def __init__(self):
self._idleTimer = None
self._waitForHeaters = False
self._skipIdleTimer = False
self._configuredGPIOPins = {}
self._configuredGPIOPins: Dict[str, periphery.CdevGPIO] = {}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This plugin still supports older Python 2.7 installations which doesn't support type annotations.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that, I did not realize that octoprint still has support for python 2.7.

@kantlivelong
Copy link
Owner

RE: Refactoring, could you provide more detail in a separate ticket? Code definitely needs some general refactoring(format, make more use of constants, simplification, etc..) but not sure where your going with splitting things.

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.

2 participants