Incorrect parsing of M117, M118 commands when message starts with a digit

[blalor]

M117 ohai and M118 ohai are properly handled. M117 0hai and M118 0hai are not, with the 0 (I think actually 0h) being parsed as a number.

The following is what appears in the Fluidd console:

06:17:57 $ M118 0hai
06:17:57 // Unknown command:"M118 0"
06:18:07 $ M117 0hai
06:18:07 // Unknown command:"M117 0"
06:18:18 $ M118 ohai
06:18:18 echo: ohai

klippy.log just says

Unknown command:"M118 0"
Unknown command:"M117 0"

[github-actions]

Hello,

It looks like there hasn't been any recent updates on this
Klipper github issue. If you created this issue and no
longer consider it open, then please login to github and
close the issue. Otherwise, if there is no further activity
on this thread then it will be automatically closed in a few
days.

Best regards,

~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[blalor]

Still present.

07:04:12 $ m115
07:04:12 // mcu build version:
         // mcu: v0.10.0-223-g4c8d24ae
07:04:12 // FIRMWARE_VERSION:v0.10.0-223-g4c8d24ae FIRMWARE_NAME:Klipper
07:04:23 $ M118 0hai
07:04:23 // Unknown command:"M118 0"

[fleinze]

I experience the same issue. Seems it was introduced in PR #4727

The problem with the M117-Command is that any message starting with characters other than [A-Z_*/] will be ommitted.
M117 5% complete will display "complete" without "5%"

[Sineos]

@pedrolamas maybe you can cross-check

[pedrolamas]

Sure, will take a look ASAP!

[pedrolamas]

Ok, found the issue and the good news (for me!) is that it is not related to that PR!

The problem is with this regular expression that is used for command parsing.

This is ok:

>>> args_r.split("M118 TEST")
['', 'M', '118 ', 'TEST', '']

From the above, the code will use index 1 and 2 and build "M118" as the command.

But if the first parameter starts with a number:

>>> args_r.split("M118 0TEST")
['', 'M', '118 0', 'TEST', '']

The result of index 1 and 2 will be "M118 0" and that is obviously incorrect!

[KevinOConnor]

Unfortunately, #4727 did introduce a regression. (There is special handling of M117 in gcode.py to ensure the correct handler is called, but it doesn't fixup gcode._command.)

It should be fixed now (commit f97fd7c).

-Kevin

[pedrolamas]

@KevinOConnor seems it still fails for macro overrides:

[gcode_macro M117]
rename_existing: M117.1
gcode:
  M117.1 {rawparams}
  {action_respond_info(rawparams)}

If I have the above macro and then run M117 0hai:

// 0hai
// Unknown command:"M117.1 0"

[KevinOConnor]

There is special handling for M117, but there is not special handling for M117.1. So, this is expected behavior.

Technically, spaces should be ignored in gcode - so M117 55 should actually be handled as a M11755 command (if anyone actually cared about what gcode "standards" say). Klipper doesn't do that, but at the same time, trying to handle every weird corner-case doesn't seem useful either.

Cheers,
-Kevin

[fleinze]

Fixed my issue. Thanks!

[pedrolamas]

I understand that, I just wanted to point out that this latest change will probably break anyone overriding the M117 command (part of the reason why raw_params was added in the first place)

[KevinOConnor]

I understand that, I just wanted to point out that this latest change will probably break anyone overriding the M117 command (part of the reason why raw_params was added in the first place)

As I understand it, commit 7ef7bf6 introduced a regression (as M117 used to work with special characters, but did not after that commit). I don't think commit f97fd7c introduces a regression (as M117.1 did not work with special characters before nor after that commit).

If I've missed something, let me know.
-Kevin

[pedrolamas]

Sorry @KevinOConnor, you are correct: I should have tested the overrided M117 with a "regular" use case without special characters - those are working just as before this change and no regression has been introduced.

[blalor]

Yeah, the original intent of #4727 was to allow for overriding M117, which no longer works with @KevinOConnor's fix. What's the path forward? @pedrolamas can you take a crack at that?

[github-actions]

This ticket is being closed because the underlying issue is now thought to be resolved.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.