prusa3d / Prusa-Firmware

Firmware for Original Prusa i3 3D printer by PrusaResearch
GNU General Public License v3.0
2.01k stars 1.05k forks source link

PFW-1206: Implement Marlin's print job timer and add M75-M78 #4493

Closed gudnimg closed 9 months ago

gudnimg commented 10 months ago

This PR implements Marlin 2.1 stopwatch timer. See: https://github.com/MarlinFirmware/Marlin/blob/bugfix-2.1.x/Marlin/src/libs/stopwatch.cpp

The Marlin file has a license and as required I added a note at the top of the file to state the code is modified to work on Prusa's firmware. It's my understanding nothing else is required to use the code.

This will hopefully simplify our code a bit. The only downside is with this change the paused duration is no longer included in the print time statistics. The plan is to add print time statistics for remote host prints later (currently its only for SD prints).

This PR also implements new G-codes: M75, M76, M77, and M78

github-actions[bot] commented 10 months ago

All values in bytes. Δ Delta to base

Target ΔFlash ΔSRAM Used Flash Used SRAM Free Flash Free SRAM
MK3S_MULTILANG 142 0 246454 5655 7498 2537
MK3_MULTILANG 120 0 245734 5662 8218 2530
gudnimg commented 10 months ago

@3d-gussner Please review again :)

sarusani commented 8 months ago

This breaks print resume when the file is printed via OctoPrint and the gcode includes a M601. See https://github.com/prusa3d/Prusa-Firmware/issues/4554

OctoPrint is still sending lines (in this example: "N693 G1 Z.9*73") after the M601, but the printer no longer sends an acknowledgement ("ok") that the line was received. Which ends up in a communication timeout.

Before:

...
Send: N692 M601*41
Recv: ok
Send: N693 G1 Z.9*73
Recv: echo:enqueing "G1 E-1.000 F2700"
Recv: //action:paused
Printer signalled that it paused, switching state...
Recv: ok
Changing monitoring state from "Pausing" to "Paused"
Recv: T:210.4 /215.0 B:60.3 /60.0 T0:210.4 /215.0 @:86 B@:0 P:0.0 A:38.5
...

Notice the "Recv: ok" before "Changing monitoring state from "Pausing" to "Paused""

Now:

...
Send: N692 M601*41
Recv: ok
Send: N693 G1 Z.9*73
Recv: echo:enqueing "G1 E-1.000 F2700"
Recv: //action:paused
Printer signalled that it paused, switching state...
Changing monitoring state from "Printing" to "Pausing"
Recv: T:211.1 /0.0 B:60.1 /60.0 T0:211.1 /0.0 @:0 B@:33 P:0.0 A:38.7
...
sarusani commented 8 months ago

The ptoblem is that print_job_timer.pause() in ultralcd.cpp->lcd_pause_print() {} doesn't set the state to PAUSED. (print_job_timer state is STOPPED at the time of execution, so print_job_timer.pause() doesn't do anything)

Therefore the loop cmdqueue.cpp-> "while (((MYSERIAL.available() > 0 && !saved_printing) || (MYSERIAL.available() > 0 && print_job_timer.isPaused())) && !cmdqueue_serial_disabled)" stops as soon as Marlin_main.cpp->stop_and_save_print_to_ram() {} sets saved_printing = true.

In the old code the isPrintPaused continued the loop execution.

It looks to me that the print_job_timer is never started for USB prints. So everything that relies on print_job_timer states doesn't work correctly for USB prints.

gudnimg commented 8 months ago

For USB prints the new gcodes introduced in this PR need to be used to start/stop the print job timer.

However if the user does not use these gcodes, the firmware should behave like before. IMO this regression is a blocker for FW 3.14.0 release.

Thanks for sharing your findings @sarusani

sarusani commented 8 months ago

I assumed that the other states beside PAUSED would also be used in the general code, but after looking at it in detail, it's really only the PAUSED state that is relevant.

So we could force the PAUSED stated in lcd_pause_print() and reset it back to STOPPED in lcd_resume_usb_print(). By not setting the timestamps it should not interfere with the new gcodes.

I added a PR.