-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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 callback for gcode responses. #1290
Conversation
… beside pseudotty. Signed-off-by: Stephan <[email protected]>
Oh hey, looks familiar :) |
EDIT: |
Thanks. At a high-level, my feedback is that an infrastructure commit like this should be accompanied by the commits that make use of the infrastructure change. So, for example, if you plan to submit a series of commits that add dwc support to Klipper, then this would be a good commit to include in that series. Otherwise, it's very difficult to review the particulars of an infrastructure change without a user of that infrastructure. -Kevin |
@KevinOConnor thanks for the feedback. Theese 4 lines of code wont hurt and it gives current dwc/paneldue users the ability to use klippers master branch. Maybe you take a few minutes and give the webif a try. On both sides, klipper and dwc, there are no more changes needed to keep this one maintainable.(beside this one here) |
Any update on DWC progress? -Kevin |
dwc2 is running great so far and i have some userfeedback. Personally i use it daytoday. there are 2 points left on my list, i will take care of. for now i will close that one here. |
@Stephan3 I have been testing dwc2-for-klipper module for several months. I think it is good enough to add to Klipper. |
++ for merging this change. It could help others too who want a frontend for Klipper. So I'd consider it more as a part of Klipper API than a change needed for a specific library |
@KevinOConnor could you please have a look here, and instruct what is needed to merge DWC2 to klipper? And to use it as an alternative UI. |
As with all Klipper features, a developer would need to submit the feature and address any questions or issues found during a review. -Kevin |
So far as I understand it, the PR here was simply adding the ability to hook in other tools. However, @KevinOConnor demands that DWC control integration is added? A unit test is all that’s needed to ensure the callback API works, with perhaps some exception handling wrapping the call. It seems a little mean spirited to me. |
@marcosscriven I don't think Kevin ever tries to be mean spirited. Kevin's actually been actively working with developers like @Arksine on implementing a proper Web API / interface for Klipper that's done in a way that's robust and maintainable. There's a great video here on the progress: https://www.youtube.com/watch?v=fQIutqzxOzE It's written from the ground up for klipper. |
To be clear, I think it’s absolutely fine to tell people that using a third party tool with Klipper means you don’t get support with that tool. You use DWC, or anything else, with Klipper, and you’re on your own. I don’t think it fair to not merge in a simple hook, for over a year, or demand that someone else’s code that uses it must become part of the Klipper repository. |
Instead of waiting for a merge for a simple callback system, dwc2 4 klipper could simply subclass the class it wants to use and extend it with the needed methods. This is unlikely to break in the future unless Kevin does some massive refactoring. The changes look simple enough. My big complaint about dwc2 for klipper is it is one giant python file that should be broken down further. |
The title of the PR is "Adding callback for gcode responses" - It's about third-party code having a mechanism to hook in. I've neither tried DWC, nor seen its code - but came here when I saw the installation method required patching Klipper. @KevinOConnor's first comment was:
Why should DWC support need to be added to Klipper? The principle of allowing tools to be hooked in can be covered by a unit test, and external calls can be bulletproofed if necessary with exception handling. |
we have a new extra module for DWC2 that is not taping into the gcode.py file. |
On reflection - it was mean spirited of me to say that about @KevinOConnor. Please accept my apologies. |
A basic callback for gcode executuion. It is usefull for lcds, paneldue, webapi and or webinterfaces.
Signed-off-by: Stephan [email protected]