j-be / AutoBim

A bed tramming utility for OctoPrint using ABL
Other
29 stars 3 forks source link

Add basic support for Klipper #22

Closed scottg88 closed 2 years ago

scottg88 commented 2 years ago

I recently moved to Klipper away from Marlin and much prefer this plugin's style of tramming. This adds basic support for Klipper and associated documentation.

It doesn't support Klipper directly by sending different G-Codes, instead it adds the right parsing for the responses. Klipper uses can add appropriate macros for G29/G30 to avoid this plugin needing to support the alternate G-Codes.

It does:

Let me know if you need me to do anything license-wise to accept my changes. Also happy to make any changes based on feedback.

I've tested this on my own printer - a Creality CR10s Pro V2 running Klipper and it works great.

scottg88 commented 2 years ago

Squashed out some old changes so it won't include them in the merge.

j-be commented 2 years ago

Dude, this is awesome!

Let me know if you need me to do anything license-wise

No, as long as you are fine with it being AGPL (i.e. same license as OctoPrint uses) it is fine for me. If you don't mind I'd add you to the "Credits" section in README.md.

About the switch in settings: have you considered an auto-detection based on self._printer.firmware_info? Similar to what OctoPrint does here. I guess on Klipper the whole has_ubl detection is useless, isn't it?

EDIT: Fixed link.

scottg88 commented 2 years ago

Dude, this is awesome!

Thanks!

No, as long as you are fine with it being AGPL (i.e. same license as OctoPrint uses) it is fine for me. If you don't mind I'd add you to the "Credits" section in README.md.

Yep sounds good.

About the switch in settings: have you considered an auto-detection based on self._printer.firmware_info? Similar to what OctoPrint does here.

Great idea, I'll add that in. I think if that works well I might make the option Firmware Type with some info about what it effects. I can make the g30 regex option a custom input, letting others theoretically support more firmware without code changes, or versions of Marlin/Klipper with differing outputs.

I guess on Klipper the whole has_ubl detection is useless, isn't it?

Yeah there's no UBL in Klipper however as it fails gracefully there's no harm in letting the detection run anyway. The flag being on or off doesn't matter as long as the G29 macro supports both flags - I think I'll add a note somewhere.

I think I'll also add some example macros that get people up and running into the README.

scottg88 commented 2 years ago

@j-be I'm happy with this now - could you please test this still works with Marlin? I'm seeing the correct behaviour in the logs so it should be good.

What do you think of the new UI options? I tried to make it straight forward.

j-be commented 2 years ago

@scottg88 I took the freedom to further reduce complexity in G30Handler to avoid keeping more state than necessary. I also moved the "Custom pattern setting" to the very bottom of the settings, and tried to further emphasize that it is really dangerous.

I also noticed you used a format string once (i.e. self._logger.info(f"Updated custom pattern.")). F-Strings are a Python3 feature, and hence would break Python2 compatibility.

If you could test the code on Klipper once again, just to make sure I didn't break anything, I'd merge and release it.

scottg88 commented 2 years ago

@scottg88 I took the freedom to further reduce complexity in G30Handler to avoid keeping more state than necessary. I also moved the "Custom pattern setting" to the very bottom of the settings, and tried to further emphasize that it is really dangerous.

Nice, I didn't know you could access the underlying text of a pattern, that's much better!

I also noticed you used a format string once (i.e. self._logger.info(f"Updated custom pattern.")). F-Strings are a Python3 feature, and hence would break Python2 compatibility.

Thanks, yeah I realised yesterday this also needs to support Python 2 but I missed that one.

If you could test the code on Klipper once again, just to make sure I didn't break anything, I'd merge and release it.

Works!

I pushed a change to the README to remove the option you removed and also to remove the custom pattern feature from the planned features. I think the other one (Allow for a different amount of probe points) might also be implemented - is that referring to the +/- buttons for choosing the number of probe points?

j-be commented 2 years ago

@scottg88 merged and released. Thanks again for the effort, and happy printing :smiley:

scottg88 commented 2 years ago

Thanks @j-be. Also thanks to you for the plugin in general, makes the printing experience better 😄