gcode_macro: Add "rawparams" pseudo-variable

[pedrolamas]

This adds a new rawparams pseudo-variable to the gcode_macro template context.

The main usage for this is to allow easier override of an existing G-Code like M117, where we don't have any parameters (so params is useless) but instead take the whole message after the gcode.

Here's an example of it:

[gcode_macro M117]
rename_existing: M117.1
gcode:
  M117.1 { rawparams }
  M118 { rawparams }

In the above example, any message sent via M117 would also be mirrored with M118.

Signed-off-by: Pedro Lamas pedrolamas@gmail.com

[KevinOConnor]

Thanks. In general, it looks fine. However, one issue is that the example is subtly broken if the command includes g-code line numbers (eg, N101 M117 Hello world*123). See the code in klippy/extras/display_status.py for a more complete example.

We may need to abstract that logic so that it can be used here and in M118 as well.

-Kevin

[pedrolamas]

I did see that line with the msg.rfind('*') but couldn't actually understand what it was for... till now!

For that specific example, I would just write the jinja code like this:

[gcode_macro M117]
rename_existing: M117.1
gcode:
  {% set message = commandline[5:] %}
  {% set index = message.rfind('*') %}
  {% if index >= 0 %}
    {% set message = message[:index] %}
  {% endif%}
  M117.1 { message }
  M118 { message }

My question right now is if we should just make the commandline not include the gcode line number/checksum, or keep it as it currently is and force the users to do like the fixed sample above... and I'm tempted in saying that we should truncate that bit and keep it off (so no need to this extra fix).

[pedrolamas]

Took me a while to get the whole "line number" thing, but I think I understand now!

Something like N1 M117 test*123 would be the case, correct?

Given that, this is how one could code it in a template:

[gcode_macro M117]
rename_existing: M117.1
gcode:
  {% set message = commandline %}
  {% if not message.startswith("M117") %}
    {% set start = message.find("M117") %}
    {% set end = message.rfind('*') %}
    {% if end >= 0 %}
      {% set message = message[:end] %}
    {% endif%}
    {% set message = message[start:] %}
  {% endif %}
  M117.1 { message }
  M118 { message }

But that does seem way to much verbose to me, so I think something better should be done on Klipper side... I will follow up on your comments and see how I can improve this!

[pedrolamas]

@KevinOConnor I've added a new get_sanitizedcommandline() method that takes the command and removes the line number and checksum value (plus trims the spaces) so that can be used directly for the M117, M118 and to easier command override.

I considered changing the existing _commandline and get_commandline() method directly, but I was a bit afraid that it could break something so I opted for adding a separate one instead.

[KevinOConnor]

Thanks. I agree with adding a new method and not changing get_commandline(). However, I'm not sure that regular expression is sufficient to catch all the weird cases with g-code. I suspect the existing M117/M118 logic is more robust. Also, we don't want to add any additional cpu overhead to the "fast path" g-code processing code - any overhead should be in the (rarely called) new method.

-Kevin

[pedrolamas]

Given that, I changed approach: instead of having the full command line as a variable, I'm just adding a rawparams that has the unparsed parameters (using the M117/M118 approach with a few minor touches)

[pedrolamas]

I've updated the PR title and description to match these changes.

[KevinOConnor]

Thanks. I'm fine with adding a get_raw_command_parameters() method, but I don't think we should make any changes to the existing command dispatch logic as that is a "fast path" - any cpu overhead for determining the parameters should only be on those callers that actually invoke the new method.

-Kevin

[pedrolamas]

I've moved all the logic to the get_raw_command_parameters() method, so it will only calculate "on demand", that should solve the "fast path" problem for existing commands.

Having said that, I think as it stands it will still invoke the logic for any macro... should I convert the current rawparams variable inside the macro to a Jinja function/method instead?

[pedrolamas]

@KevinOConnor just wanted to check if you think this needs more work or even a different approach to the current way I've implemented it?

[whi-tw]

Hmm, turns out I didn’t see this when making #4883. I went for the least intrusive way by not adding any extra functions and just passing the whole command line in, leaving it to the macro to sanitise the params

[pedrolamas]

@whi-tw that was actually my initial approach but after some discussion, @KevinOConnor pointed out some problems with that as the commandline (gcode) can include line numbers and checksums, hence why I ended with the proposed implementation in this PR.

[whi-tw]

@pedrolamas for sure, makes sense, although it depends where the responsibility for parsing the variable lies. I went for the responsibility being on the macro author, vs Klipper itself, as it’s feasible the line number could be helpful to someone?

[KevinOConnor]

Thanks. Looks fine to me.

One question - is there a reason that gcmd.get_raw_command_parameters() returns None when there are no parameters? I would think that would be awkward for macros to handle. The special M117 case could just do if not raw_commands: raw_commands = None?

-Kevin

[pedrolamas]

is there a reason that gcmd.get_raw_command_parameters() returns None when there are no parameters?

None can easily be handled inside the macro with { rawparams | default }, but I am ok to change it to something else if you think there's a better way here!

[KevinOConnor]

I'd say always returning a string would be preferable (and add the additional None logic to the M117 code). Otherwise, we'd need to document that {rawparams} can return None.

Thanks,
-Kevin

[pedrolamas]

No worries, I just pushed the necessary changes to make get_raw_command_parameters() return '' as discussed!

[KevinOConnor]

Thanks.

-Kevin