klipper_estimator icon indicating copy to clipboard operation
klipper_estimator copied to clipboard

Support for gcode_macro

Open hesiod opened this issue 3 years ago • 3 comments

Add (incomplete) support for G-Code macros.

Some architecture notes:

  • The template environment is provided by minijinja. Unfortunately Klipper decided to use non-standard notation for expressions (single braces { expr } instead of the normal double braces {{ expr }}), necessitating an additional regex replace step.
  • The printer object is exposed using the struct PrinterObj utilizing minijinja's dynamic object support. I previously simply serialized the printer object as local context for template evaluation, but since the configuration can be a little large this made macro evaluation relatively slow. Still perfectly adequate in terms of runtime, but why not improve performance a little. So I decided to expose the printer attributes using PrinterObj.
  • The Moonraker configuration support was moved to another file for clarity. The settings can't be deserialized directly because the macros have unpredictable keys like gcode_macro foobar.

TODOs (should definitely be fixed):

  • [ ] lib_klipper shouldn't depend on serde_json
  • [ ] Support for local JSON configuration is disabled for simplicity atm, but should be an easy fix
  • [ ] Re-enable line number support (was disabled since it interferes with certain macro names)
  • [ ] Some random clippy fixes made their way into the PR, I'll remove those later to keep the PR self-contained

Known limitations:

  • replace_existing is not supported
  • Dynamic attributes of the printer object (i.e. attributes not set in the configuration) need to be exposed/translated manually. Currently only a commonly used subset of attributes are exposed (toolhead.position, toolhead.axis_maximum and toolhead.homed_axes - the latter is always set to "xyz")
  • Certain filters are not available in minijinja by default, float, int, min, and max are implemented atm
  • The abs filter in minijinja does not work for unsigned integers which are automatically used in some cases, throwing unnecessary errors - this is an upstream bug (?), but perhaps it can be fixed locally
  • There is no limitation on stack depth - during macro evaluation, the generated G-Code is simply parsed and executed immediately. This could of course cause issues if macros call themselves recursively or perform other shenanigans.

hesiod avatar Sep 05 '22 11:09 hesiod

Hi @hesiod

Are you still working on this?

Best regards, Lasse

dalegaard avatar Jan 26 '23 14:01 dalegaard

@dalegaard Actually I was waiting for some initial feedback (on whether you're interested in this feature in principle) before I worked on it any further 🙂

FYI, there is one known issue with the PR currently: I misunderstood how Klipper's macro variables (gcode_variable etc.) are supposed to work. They are currently set in the wrong scope, but I can't remember the details atm. But it shouldn't be a big deal to fix this.

hesiod avatar Feb 20 '23 14:02 hesiod

Hi @hesiod

That's my bad, I hadn't considered you were looking for feedback at this stage. Sorry about that.

I'm very interested in supporting this feature if it can be demonstrated to work well in real world use cases. Supporting every possible macro under the sun isn't an objective, but e.g. the FastGyroidInfill of #33 seems like it could be supported pretty well.

I haven't done any code checks at this point, but I'll try to take a look over things tonight.

Note that I changed a good chunk of the configuration system, so that part will likely need to be changed significantly from when you submitted this PR.

Best regards, Lasse

dalegaard avatar Feb 20 '23 15:02 dalegaard