Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PFW-1376 MMU error during mid-print does not pause/resume print #48

Open
wants to merge 217 commits into
base: mk3-mmu
Choose a base branch
from

Conversation

gudnimg
Copy link
Collaborator

@gudnimg gudnimg commented Jul 10, 2022

JIRA ticket: https://dev.prusa3d.com/browse/PFW-1376

  • If the MK3S reports an error in the middle of a print move then we want to pause the print until the error is resolved. The pausing and parking needs to be done in two main loop() iterations which is why I have split all the actions into switch-case.
  • If the MMU recovers from a communication timeout without the user interacting with the LCD screen, the error screen will be dismissed and the print resumed automatically.

wavexx and others added 30 commits March 29, 2022 22:37
Modify lcd_show_multiscreen_message_two_choices_and_wait_P to also
handle single-screen or empty (no-clear) prompts, making other functions
redundant. Saves 76 bytes.

Change existing functions to simply call
lcd_show_multiscreen_message_two_choices_and_wait_P with the correct
arguments.

This changes the prompt of existing Yes/No messages: the previous prompt
would use the last two lines of the LCD, while the new prompt is using
just the last line of the LCD instead.

Translation do not require updates, since the Yes/No translation was
already the same in both implementations.
Add an additional parameter to control the position of second choice
prompt position (while defaulting to the old).

This allows Yes/No prompts to be equally spaced.
@gudnimg
Copy link
Collaborator Author

gudnimg commented Jul 10, 2022

Marking as draft again. Will try to find another solution to this. c68a02e

EDIT: fixed

Do it in multiple parts to take into account when an error appears nested inside plan_buffer_line

When mmu_loop is called inside plan_buffer_line
while its executing a G1 gcode it is problematic to park the extruder.
We only set the state to PAUSE_PRINT and then on the next iteration of mmu_loop in the top-level loop()
we've exited plan_buffer_line and we can safely park the extruder

Previously this was problematic due the G1 gcode not being done executing
@gudnimg gudnimg marked this pull request as ready for review July 10, 2022 23:49
{
case MMUErrorPrintStates::PAUSE_PRINT:
start_pause_print = _millis();
stop_and_save_print_to_ram(0.0, -default_retraction);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't follow the technical reason for needing/implementing a different and completely orthogonal pause state here. I don't think you ever want an error condition to behave like a normal "paused" print, that's going to open up a whole complicated mess of potential issues relating to the tune menu, as well as users potentially doing a remote-resume from Octoprint without attending to the actual problem. IMO all MMU errors should follow the same pausing pathway in order to keep the code simpler and reduce future bugs/corner cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m confused how to proceed. I feel like we should at minimum set isPrintPaused to true etc. So any logic in the loop() doesnt do anything unexpected.

IMO all MMU errors should follow the same pausing pathway

Do I understand correctly you mean the code shouldn’t care whether we are printing or not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took out the Resume and Pause states and did quick test. Didn't see any issues. 4cb9f78

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - in effect all errors are "while printing" or doing something to prepare to print such as a load.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to now we’ve only been parking while in manage_response() which is its own loop and blocks any gcodes from processing during parking. The errors which can happen at any time like the Communication timeout or Protocol error are different in that they happen in the top level loop() and we can’t block queued gcodes moves without emptying the planner/command buffer. I’m seeing usually around 6 to 12 commands queued up.

@gudnimg
Copy link
Collaborator Author

gudnimg commented Jul 12, 2022

Parking doesn't work when communication timeout happens. Will look into it. Also stopping a print will cause the printer to crash its axis (its trying to execute some gcode)
image

@gudnimg
Copy link
Collaborator Author

gudnimg commented Jul 12, 2022

I reverted the change since it does not work properly. I tried emptying the planner and command queue just before parking, it didn't help (printer just ignored the parking move completely). I think it requires a full main loop() to work as Yuri mentioned. And we probably need stuff in stop_and_save_print_to_ram to save the position in the SD card file (or USB line). Either way this isn't as easy as I thought :)

@gudnimg
Copy link
Collaborator Author

gudnimg commented Jul 12, 2022

TODOs

  • Add TODO in the code to remind us to block any gcode parsing while parked. We’ll look at it after thermal model branch is merged?
  • Reduce lcd_resume_print. We don’t need all that stuff, just restore the command queue.

@gudnimg
Copy link
Collaborator Author

gudnimg commented Jul 12, 2022

I implemented the TODOs and fixed one bug I found while testing.

gudnimg and others added 12 commits July 13, 2022 08:42
I'm assuming we need this?
This is a draft PR showing the potential 3x retry implementation on the printer's side.
It is much less code and looks more reliable than the same functionality in the MMU FW.

Still, more work needs to be done:
- [ ] Button is sent to the MMU even before returning from the parking position
- [ ] Then the button is sent again
- [ ] Then the printer runs out of retryAttempts

We need to find a better spot to check for "automatic" retry and issuing of the buttons
Fix the unload procedure when the user has paused a print
then stopped the print after the temperature has reached below 175°C
Now the E-motor will move as expected
PFW-1375 MMU error reported by MK3S does not appear on LCD
Communication timeout and Protocol Errors are now distinguished.
In case of a Protocol Error, the printer waits for heartBeatTimeout to allow filling up the input UART buffer (we expect the MMU still produces some bytes).
Once the timeout elapsed, the input UART buffer is cleared and a new Start Sequence is initiated.
@gudnimg
Copy link
Collaborator Author

gudnimg commented Jul 22, 2022

Fixed the merge conflict. The diff is a lot cleaner now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants