virtual_sdcard: various additions

[Arksine]

This request proposes the following changes to the virtual_sdcard module:

• Add a RESET_SD gcode. This allows users to clear the virtual_sdcard's state in the event that they want to cancel a print.
• Walk through subdirectories when populating the file list, allowing Klipper to load these files via M23 subdir/myfile.gcode or M23 /subdir/myfile.gcode
• Report print progress while the virtual_sdcard is paused. The current behavior resets progress to 0. In addition, report "is_active" and "file_position" via get_status()
• Add a PrintStats class that tracks the total time elapsed since the print has started, the total time spent printing (not paused), and the total filament used based based on the gcode position. These stats, along with the currently loaded file's name, are reported via get_status().

This functionality is currently used by Moonraker/Mainsail, however it may also be useful for displays or gcode_macros.

Signed-off-by: Eric Callahan arksine.code@gmail.com

[KevinOConnor]

Thanks. I have some comments:

Add a RESET_SD gcode. This allows users to clear the virtual_sdcard's state in the event that they want to cancel a print.

Looks fine, but I'd suggest a more descriptive name - maybe SDCARD_RESET_FILE or SDCARD_CANCEL_PRINT or PRINT_FILE_CANCEL. Also, might be nice to add an internal _reset_file() helper function to reduce some code duplication.

Walk through subdirectories when populating the file list, allowing Klipper to load these files via M23 subdir/myfile.gcode or M23 /subdir/myfile.gcode

As near as I can tell, the M20 and M23 commands do not "normally" support directories (eg, https://www.reprap.org/wiki/G-code#M20:_List_SD_card ). Also, I'm not sure it's valid to limit the file extensions of the M20/M23 commands. I'm leery of "enhancing" the old g-code commands in "novel" ways. I'm fine with adding support for directories, but FWIW I'd prefer new commands like PRINT_FILE_START FILENAME=a/b/c.gcode.

Report print progress while the virtual_sdcard is paused. The current behavior resets progress to 0. In addition, report "is_active" and "file_position" via get_status()

Looks fine to me. It may be worthwhile to add a note to Config_Changes.md .

Add a PrintStats class that tracks the total time elapsed since the print has started, the total time spent printing (not paused), and the total filament used based based on the gcode position. These stats, along with the currently loaded file's name, are reported via get_status().

If I understand this correctly, it adds some useful statistics that are inferred from the print. I think that's fine, but I'd suggest adding it to its own top level "extra" module. (Or, perhaps add to the existing display_status.py module.) The virtual_sdcard.py module can auto-load the new module, so it can continue to unconditionally call its note_xxx() methods. I think a separate "extras" module may keep the low-level file code better separated from the high-level "stats interpretation" code.

Thanks again,
-Kevin

[Arksine]

Sounds good. I'll get to work on the suggested changes.

[Arksine]

Ok, I have made the following changes:

• The PrintStats class is now in its own module, print_stats.py
• Renamed RESET_SD to SDCARD_RESET_FILE
• Added PRINT_FILE_START and GET_FILE_LIST gcodes.
• Added _reset_file() and _load_file() helper functions
• Updated gcodes.md and config_changes.md

I thought that GET_FILE_LIST would be useful for users that want to view all files that can printed via PRINT_FILE_START. Below is a sample of the output:

// Begin File List
// ****
// Name: BME_280_Back_0.2mm_PLA_Ender2_7m.gcode
// Size: 194631
// ****
// Name: Cura/Cura_Cube.gcode
// Size: 466257
// ****
// Name: IdeaMaker/ideamaker_cube.gcode
// Size: 202133
// ****
// Name: KissSlicer/KissSlicer_Cube.gcode
// Size: 657666
// ****
// End File List

Also, if you prefer I can separate the get_file_list() method into two different methods (perhaps get_file_list() and get_extended_file_list().

[KevinOConnor]

Thanks. I have a couple of minor comments below.

-Kevin

[KevinOConnor]

I think it would be preferable if we could use a common prefix for all the commands of the module. For example, "SDCARD_LIST_FILES,SDCARD_PRINT_FILE,SDCARD_RESET_FILE" or "PRINT_FILE_LIST,PRINT_FILE_START,PRINT_FILE_RESET", or something similar.

[Arksine]

Ok, I will change to SDCARD_LIST_FILES and SDCARD_PRINT_FILE. I think that verbiage is most clear on what it does.

[KevinOConnor]

I don't understand this change. How could self.currentfile == None in this context?

EDIT: Oops - it can be assigned to None in the function itself. Shouldn't this call self._reset_file() though?

[Arksine]

Yes, I should change that to use reset_file.

[Arksine]

Suggested changes made. The virtual_sdcard's extended gcodes are now:
SDCARD_PRINT_FILE FILENAME=<filename>
SDCARD_GET_FILES
SDCARD_RESET_FILE

Thanks!

[KevinOConnor]

Thanks. Looks good to me. If there are no further comments, I'll commit in a couple of days.

-Kevin

[Arksine]

Hi Kevin, it recently occurred to me that if the print exits the timer due to an error there is currently to method of notifying clients than an error occured. What I thought I could is add a "state" value to the print_stats get_status() method. The state can be one of the following values: "printing", "paused", "error", "complete". When there is a gcode error I can call print_stats.note_error(message), and when the print is complete I can call print_stats.note_complete rather than _reset_file(). This allows clients to continue displaying print statistics after the print is complete. When a user wants to clear the stats they can either start a new print or call SDCARD_RESET_FILE. Does this sound like a reasonable approach to you? Thanks.

[KevinOConnor]

It sounds fine to me. Does that mean I should hold off on committing this series?

BTW, what's the value in the new SDCARD_GET_FILES command? Wouldn't it be simpler for a user to just list the directory using ls or some file management gui?

-Kevin

[Arksine]

Yes, please hold off. I'll push a commit, let you review it, then squash it back into the original series.

My thought was that SDCARD_GET_FILES would give Octoprint users (and the like) an easy way to list what files are available via the terminal. Now that you mention it, I suppose those users wouldn't use the extended command anyway. Would you like me to remove it?

[KevinOConnor]

Sounds good.

Now that you mention it, I suppose those users wouldn't use the extended command anyway. Would you like me to remove it?

The reason I asked was because it occurred to me that if the command iterates through sub-directories it may create a lot of data, which may overload the pseudo-terminal between Klipper and OctoPrint. So, I guess, if there isn't a defined use case, it may be better to not "open that can of worms".

Thanks,
-Kevin

[Arksine]

I think that makes sense. I just pushed a commit with the changes to print_stats. I'll remove SDCARD_GET_FILES shortly.

[KevinOConnor]

Thanks. I'm fine with merging. After the discussion in #3149 a question occurred to me - is SDCARD_RESET_FILE and SDCARD_PRINT_FILE primarily intended to be used by moonraker/mainsail? If so, would a webhook make more sense than a gcode command? The reason I ask is that the M24 and M25 commands are kinda goofy - it's unclear what they should do if they are contained in a gcode file, it's unclear what M24 should do if called when a print is already in progress, and error handling is difficult with them. I wonder if we'll look back and think SDCARD_RESET_FILE and SDCARD_PRINT_FILE are kinda goofy for similar reasons.

I'm fine with merging though. Just figured I'd ask.

Thanks again,
-Kevin

[Arksine]

I think SDCARD_RESET_FILE has to be a gcode, as I expect users to include it in their "CANCEL_PRINT" macro override. In fact, I should probably have cmd_CANCEL_PRINT() execute SDCARD_RESET_FILE by default if pause_resume detects a print from the virtual_sdcard.

SDCARD_PRINT_FILE could be a webhook. I added it as a gcode to give all users the option of launching from a subdirectory, but I'm unsure of how many users would find it useful. Perhaps the display "menu" could add support for subdirectories, which might make navigating a bit easier for users with a large number of gcode files in the the vsd path.

[KevinOConnor]

Sounds good. Thanks!

-Kevin