virtual_sdcard: Allow spaces in file path for M23

[jschuh]

Signed-off-by: Justin Schuh [email protected]

[jschuh]

The virtual SD Card allows files with spaces, and both SDCARD_PRINT_FILE and Moonraker support them, so it seems that M23 should for consistency. Also, this will make the LCD menus work correctly with spaces.

[Arksine]

If the original command contains a line number then I suspect that this will produce an invalid filename. I suspect that what you desire could be accomplished by adding maxsplit=1 to the original line, ie:

filename = orig[orig.find("M23") + 4:].split(maxsplit=1)[0].strip()

[jschuh]

Thanks, I was unaware that gcode line numbers were a thing or that Klipper supported them.

That stated, one thing I forgot to mention was that the current code breaks if the M23 is renamed to anything other than M23\d (where \d is a single digit). I didn't realize that until after I posted the PR, but I assume it's a bug worth fixing as well, so I would have to tweak your suggestion a bit to just skip a line number (if present).

[jschuh]

@Arksine, I updated to skip line numbers and noted in the commit message that it makes M23 work normally with rename_existing.

[pedrolamas]

I just took a quick look at this, but wouldn't it be safer to update gcmd.get_raw_command_parameters() to also check for M23 and use it instead of gcmd.get_commandline()?

[jschuh]

Good point, I should have used get_raw_command_parameters() since that won't duplicate parsing logic. I suppose I incorrectly anchored on the fact that the existing code was already doing that parsing.

That stated, I don't see a reason for M23 specific logic in get_raw_command_parameters() since it seems to already do what's needed, and putting logic there would break with a renamed M23. Or maybe I misunderstood something?

Either way, I've updated the PR to use get_raw_command_parameters().

[pedrolamas]

That stated, I don't see a reason for M23 specific logic in get_raw_command_parameters() since it seems to already do what's needed, and putting logic there would break with a renamed M23. Or maybe I misunderstood something?

Well, that's because M23 already existed before I added the get_raw_command_parameters() a couple of weeks ago! 🙂

Just a thought, but you might want to double-check that you can override M23 and that is works as expected, something like this:

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

[jschuh]

Yup, did that before pushing the updated PR. I 've been using this macro to verify that everything works with a renamed M23:

[gcode_macro M23]
rename_existing: M23.100
gcode:
  M118 Wrapper params: {rawparams}
  M23.100 {rawparams}

[Arksine]

Overall it looks good to me. I would think that it is no longer necessary to remove the checksum since get_raw_command_parameters() does that for us, although I wonder if its possible to receive a checksum without a line number. It shouldnt be, but with gcode you never know. That said, if it is possible that might be something to change in get_raw_command_parameters().

[jschuh]

Yeah, I saw that get_raw_command_parameters() was removing it in the line number case, but since the old code removed it no matter what I just left it as is to be safe. Happy to just take that out if you think I should.

[pedrolamas]

I would think that it is no longer necessary to remove the checksum since get_raw_command_parameters() does that for us, although I wonder if its possible to receive a checksum without a line number.

Indeed, I think the checksum part on the filename can be safely removed.

As for the code in get_raw_command_parameters(), the thought of malformed gcode did pass my mind when I wrote that code, but I decided to maintain the existing behavior of the code I was refactoring when I wrote #4727

[KevinOConnor]

I agree that the checksum check should be removed. Otherwise, it looks fine to me.

It is my understanding that a checksum is only valid if a line number is present. (Otherwise, commands like M117 wouldn't be able to display an * character.)

Separately, it is my understanding is that a line number may be present without a checksum - ah the fun of g-code.

-Kevin

[jschuh]

Okay, removed the checksum bit and pushed the updated file. Should be good to go.

[pedrolamas]

LGTM

[Arksine]

Ah, good point about the checksum and M117. This looks good to me as well.

[KevinOConnor]

Thanks.

-Kevin

[pedrolamas]

I have a feeling that as a byproduct of this change, the problem with non-ascii filenames (reported in #4794 and others) has been fixed!!

Ignore me... I'm running Klipper with Python 3 and seems the issue is only visible in Python 2.