Support for gcode_macro - #30
Conversation
|
Hi @hesiod Are you still working on this? Best regards, |
|
@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 ( |
|
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, |
dalegaard
left a comment
dalegaard
left a comment
There was a problem hiding this comment.
Added a few preliminary comments :-)
|
|
||
| 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)?; |
There was a problem hiding this comment.
This is probably not good, as e.g. tool selections fall under "traditional gcode" as well. Why was this change needed?
| let (s, _) = space0(s)?; | ||
|
|
||
| let (s, _line_no) = opt(line_number)(s)?; | ||
| // let (s, _line_no) = opt(line_number)(s)?; |
There was a problem hiding this comment.
Was there an issue with this?
| } | ||
|
|
||
| #[derive(Debug, PartialEq, PartialOrd, Clone)] | ||
| #[derive(Debug, PartialEq, Eq, PartialOrd, Clone, Serialize)] |
There was a problem hiding this comment.
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.
| 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( |
There was a problem hiding this comment.
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.
|
|
||
| #[derive(Debug, Clone, Default, Serialize, Deserialize)] | ||
| #[serde(default)] | ||
| pub struct GCodeMacro<S = serde_json::Value> |
There was a problem hiding this comment.
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.
| "restore_gcode_state" => { | ||
| // todo!() | ||
| } | ||
| other => { |
There was a problem hiding this comment.
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.
2 participants
Add (incomplete) support for G-Code macros.
Some architecture notes:
{ expr }instead of the normal double braces{{ expr }}), necessitating an additional regex replace step.PrinterObjutilizing 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 usingPrinterObj.gcode_macro foobar.TODOs (should definitely be fixed):
lib_klippershouldn't depend onserde_jsonKnown limitations:
replace_existingis not supportedtoolhead.position,toolhead.axis_maximumandtoolhead.homed_axes- the latter is always set to "xyz")float,int,min, andmaxare implemented atmabsfilter 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