Support for gcode_macro #30

hesiod · 2022-09-05T11:54:39Z

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.

dalegaard · 2023-01-26T14:38:32Z

Hi @hesiod

Are you still working on this?

Best regards,
Lasse

hesiod · 2023-02-20T14:52:50Z

@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.

dalegaard · 2023-02-20T15:03:38Z

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

Added a few preliminary comments :-)

dalegaard · 2023-02-20T17:50:24Z

lib/src/gcode.rs

@@ -314,7 +315,7 @@ mod parser {
    }

    fn traditional_gcode(s: &str) -> IResult<&str, (GCodeOperation, Option<&str>)> {
-        let (s, letter) = satisfy(|c| c.is_alphabetic())(s)?;
+        let (s, letter) = satisfy(|c| matches!(c.to_ascii_lowercase(), 'g' | 'm'))(s)?;


This is probably not good, as e.g. tool selections fall under "traditional gcode" as well. Why was this change needed?

dalegaard · 2023-02-20T17:50:33Z

lib/src/gcode.rs

@@ -279,7 +280,7 @@ mod parser {
    fn parse(s: &str) -> IResult<&str, GCodeCommand> {
        let (s, _) = space0(s)?;

-        let (s, _line_no) = opt(line_number)(s)?;
+        // let (s, _line_no) = opt(line_number)(s)?;


Was there an issue with this?

dalegaard · 2023-02-20T17:52:01Z

lib/src/gcode.rs

@@ -121,7 +122,7 @@ impl Display for GCodeTraditionalParams {
    }
 }

-#[derive(Debug, PartialEq, PartialOrd, Clone)]
+#[derive(Debug, PartialEq, Eq, PartialOrd, Clone, Serialize)]


I'd prefer to add a to_map or similar function that the caller can then serialize after. Doesn't feel right to need this to be Serialize within gcode.rs. A better option still is likely to add a wrapper in macros.rs. This wrapper can implement minijinja::value::Object and call directly to getters without going through a serialization step.

dalegaard · 2023-02-20T17:57:09Z

lib/src/macros.rs

+impl minijinja::value::Object for PrinterObj {
+    fn get_attr(&self, name: &str) -> Option<minijinja::value::Value> {
+        match name {
+            "toolhead" => Some(minijinja::value::Value::from_serializable(


Instead of serializing the toolhead, I think it's probably better to have a wrapper that implements Object. I fear serialization could have performance implications.

dalegaard · 2023-02-20T17:59:12Z

lib/src/macros.rs

+
+#[derive(Debug, Clone, Default, Serialize, Deserialize)]
+#[serde(default)]
+pub struct GCodeMacro<S = serde_json::Value>


Do you foresee a case that could need this S parameter to be configurable? I would probably remove it and statically use serde_json::Value.

dalegaard · 2023-02-20T18:05:21Z

lib/src/planner.rs

+                "restore_gcode_state" => {
+                    // todo!()
+                }
+                other => {


In Klipper macros can override any gcode line. Really we should perform macro rendering at the top of process_cmd, potentially even creating a process_non_macro that process_cmd can call after performing macro expansion. Some care needs to be taken as we need to track the correct number of commands that an input line expands in to, even if that happens in nested macros. This might take a bigger rework of the process_cmd input system entirely.

Initial support for macros

bccbe1a

hesiod force-pushed the gcode_macro branch from 2a4c0de to bccbe1a Compare September 5, 2022 12:03

hesiod added 2 commits September 5, 2022 15:25

Cleanup

9cdf4ee

Simplify macro module

45db570

dalegaard mentioned this pull request Jan 26, 2023

Add support for FastGyroidInfill #33

Closed

dalegaard reviewed Feb 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for gcode_macro #30

Support for gcode_macro #30

hesiod commented Sep 5, 2022

dalegaard commented Jan 26, 2023

hesiod commented Feb 20, 2023

dalegaard commented Feb 20, 2023 •

edited

Loading

dalegaard left a comment

dalegaard Feb 20, 2023

dalegaard Feb 20, 2023

dalegaard Feb 20, 2023

dalegaard Feb 20, 2023

dalegaard Feb 20, 2023

dalegaard Feb 20, 2023

Support for gcode_macro #30

Are you sure you want to change the base?

Support for gcode_macro #30

Conversation

hesiod commented Sep 5, 2022

dalegaard commented Jan 26, 2023

hesiod commented Feb 20, 2023

dalegaard commented Feb 20, 2023 • edited Loading

dalegaard left a comment

Choose a reason for hiding this comment

dalegaard Feb 20, 2023

Choose a reason for hiding this comment

dalegaard Feb 20, 2023

Choose a reason for hiding this comment

dalegaard Feb 20, 2023

Choose a reason for hiding this comment

dalegaard Feb 20, 2023

Choose a reason for hiding this comment

dalegaard Feb 20, 2023

Choose a reason for hiding this comment

dalegaard Feb 20, 2023

Choose a reason for hiding this comment

dalegaard commented Feb 20, 2023 •

edited

Loading