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

[BUG] printer does not send busy: processing after Tx gcode is issued with MMU2S. #3411

Closed
knobik opened this issue Feb 16, 2022 · 16 comments
Closed

Comments

@knobik
Copy link

knobik commented Feb 16, 2022

Printer type - MK3S+
Printer firmware version - 3.10.1

MMU upgrade - MMU2S
MMU upgrade firmware version - 1.0.6

SD card or USB/Octoprint
Octoprint

Describe the bug
Printer stops sending busy: processing status after Tx and user input gcode. Effect: timeout in octoprint and M112 issued to the printer. This is the 3rd thread im describing this error, both in prusa github and in octoprint repo, as end user and end reciver of this weird behavior i dont have any energy to fight your both teams so you guys fight it out on yourself.

Related urls:
#3180
OctoPrint/OctoPrint#4422

To Reproduce
octoprint serial log of the behavior here: serial.log

Expected behavior
busy: processing issued while heating extruder and bed.

G-code
10.cm.bag.clip_0.2mm_PETG_MK3SMMU2S_1h16m.zip

@knobik knobik added the bug label Feb 16, 2022
@leptun
Copy link
Collaborator

leptun commented Feb 16, 2022

There is no issue with the Tx command. Octoprint is the one that is not handling it correctly by not waiting for "ok" before continuing to the next command.

2022-02-16 12:53:49,850 - Send: Tx
2022-02-16 12:53:49,852 - Send: N16 M190 S85*98

The "wait for target temperature for hotend/bed" commands never had "echo:busy: processing" in them. This is because the printer does in fact respond by reporting the temperatures. This is correct and has been like this for at least 4 years.
Tx has the correct busy messages:

echo:busy: paused for user
echo:busy: paused for user
echo:busy: paused for user
echo:busy: paused for user
echo:busy: paused for user
echo:busy: paused for user
mmu_get_response - begin move: T-code
MMU <= 'T0'
Unloading finished 2
echo:busy: processing
mmu_get_response - begin move: load
echo:busy: processing
echo:busy: processing
echo:busy: processing
echo:busy: processing
echo:busy: processing
echo:busy: processing
echo:busy: processing
echo:busy: processing
echo:busy: processing
echo:busy: processing
echo:busy: processing
echo:busy: processing
echo:busy: processing
echo:busy: processing
echo:busy: processing
echo:busy: processing
echo:busy: processing
echo:busy: processing
MMU <= 'A'
echo:busy: processing
MMU => 'ok'
mmu_get_response() returning: 1
ok

@knobik
Copy link
Author

knobik commented Feb 16, 2022

@leptun you are correct. thank you for your time on this. Im closing this issue.

@knobik knobik closed this as completed Feb 16, 2022
@foosel
Copy link

foosel commented Feb 17, 2022

Copying this from the other thread...

For the record, yes, busy: processing should be send on any lengthy operations that block printer communication. That includes long moves, heatups, whatever. So this is a bug in the firmware, plain and simple.

And OctoPrint sending Tx without line number and checksum has always been the case, since that is not a valid GCODE (it neither starts with G or M and it's also not a T or F followed by an integer) and to not cause issues with some firmwares out there those are send without line numbers and checksums and without using up an ok. That's always been the case, that's not a change in behaviour, and that's also something that can be configured in OctoPrint.

To add on to that, if the latter behaviour so far has not caused issues with Prusa FW, and now is in the latest version, and also correlates with a wrong implementation of the busy protocol in the FW, then this doesn't sound like an OctoPrint issue and maybe you someone at Prusa should investigate that instead of pointing at OctoPrint. I'm happy to assist as needed.

@leptun
Copy link
Collaborator

leptun commented Feb 17, 2022

@foosel I've checked a much older Prusa-Firmware version and it never sent busy messages while waiting for a heater, just the periodic unprompted temperature reports.
The only thing that changed in the meantime on our side was introducing temperature auto-reporting, but that shouldn't have anything to do with the fact that Tx and Tc (which have existed for as long as the MMU2 has been around) got treated as non-gcode and did not wait for "ok" to be sent back. It's waaaay too late to change the gcode considering it will break compatibility with all already sliced gcodes and slicer presets. I'm surprised this issue only now pops up. I guess people were simply not using the single material MMU2 print preset with octoprint. Not sure if I remember correctly, but I think there is also a plugin that captures the Tx and prompts the user to select the filament in the octoprint UI. If that is the case, then that's how the bug stayed hidden for so long.
As for getting busy messages during the "wait for heater" commands, we could change it so that also busy messages are sent, but it has worked like this for many years, as long as there is no "ok" message desync because of some previous command (Tx in this case). I just checked and Marlin 2.0 does indeed send busy messages during heatup, so it could be useful to "modernize" those commands also in Prusa-Firmware. At least this doesn't completely break the communication on its own.

I pointed at octoprint because there wasn't any change in the past year to Tx and the heating commands. And to be perfectly honest, when I saw octoprint not wait for "ok" on Tx I thought it was a bug recently introduced because that was the first time I saw it and assumed it wasn't something on our side (and also wrongly assumed that the single material MMU2 gcodes worked with octoprint in the first place). I'm sorry I blamed octoprint so quickly.

@leptun leptun reopened this Feb 17, 2022
@knobik
Copy link
Author

knobik commented Feb 17, 2022

@leptun about this bug being undetected, this was exacly also my thought process. Obviously no problems when using multi MMU2 profile. Still, enabling the line number and checksum for unknown commands fixes the issue.

@foosel
Copy link

foosel commented Feb 17, 2022

Apology accepted. It's sadly all too common that OctoPrint gets blamed when it does everything according to spec (well, the closest to a spec that we have in this space) and the problem actually lies with a misinterpretation of the protocol on side of the FW in question.

The way I see it, we are probably looking at a corner case here then that is caused in single extruder MMU usage and I'm equally surprised it only now popped up. As I said, OctoPrint's behaviour re Tx, Tc and prior T? has always been the same, so my guess is your assumption is correct here and it's indeed a case of "not enough use to cause issues so far". I've added setting the two mentioned config options in OctoPrint to true in case of detected Prusa Firmware, will be part of 1.8.0 so at least that should no longer cause any surprises for unsuspecting users (or FW 😉) in the field. It would be good to see Prusa FW fix the busy protocol implementation to align with the spec 👍 (and it should also not cause any issues).

If I may suggest to stick more closely to established standards for future new commands instead of going outside of the GCODE value range, and in general to sync up from time to time with the spec and also mainline Marlin, that would reduce the likelihood of such situations from cropping up again. I'm also happy to get looped into vendor-additions to the protocol so I can react to and possibly advise against them before they cause issues after a roll-out with things like OctoPrint or other hosts. Mainline Marlin has taken to dropping a mention at the host dev team on every protocol extension or modification, and it has done wonders for cooperation and interoperability 😉

@leptun
Copy link
Collaborator

leptun commented Feb 17, 2022

@foosel The capabilities approach should work great for features like the extended T commands.
Would something like this be parsed correctly? Cap:PRUSA_MMU2:1. I'm worried about the capability string ending with a number. Also, I still need approval for this. I'll let you know by tagging you in the PR if this change is approved.
I think the only non-standard gcode structure we have are the M862.x commands for checking gcode compatibility. But I think we're not the only firmware to have such an attrocity of a syntax (dot separated values to M). Nobody has complained so far, so I guess that works correctly. Are "D" codes supported by octoprint? (I swear, that's the last different-letter code that we have 😉). At least Dcodes are only for debugging purposes, but it would still be good to know if they are handled correctly.

Adding busy handling to the heater commands should be straightforward, but it will take a while before it will reach the public as right now almost all the work I'm doing goes into release 3.12 (3.11 isn't even released yet, but it will be very soon).

@foosel
Copy link

foosel commented Feb 17, 2022

Cap:PRUSA_MMU2:1 will be parsed fine. OctoPrint sees Cap:, strips that, splits on :, bails if it gets more than two parts, treats the first as identifier, ensures the second is either 0 or 1 and if not bails, otherwise saves capability flag as enabled for 1 and disabled for 0. For the record, would this indicate the presence of an MMU2, and ONLY of an MMU2? Answer can wait until related PRs ;)

Sub codes (M862.x for x being an integer) are also recognized and supported just fine since two years ago or so, and yes, they are also used in other firmware.

D codes will not be recognized as valid GCODE and hence not get a checksum/wait for ok treatment by default in anything up to 1.7.x (unless the aforementioned flags are set), but if firmware autodetection is enabled they too would be covered by the new detection on the "Prusa-Firmware" identifier string and trigger flagging in sending checksums with unknown commands, and requiring ok for unknown commands so should be fine.

Here's the GCODE regex that determines whether something is recognized as a GCODE command or not:

https://github.com/OctoPrint/OctoPrint/blob/17816dff74875bf06709e91d3b49a0bd5a261d7d/src/octoprint/util/comm.py#L73-L76

@leptun
Copy link
Collaborator

leptun commented Feb 17, 2022

@foosel I was thinking of sending Cap:PRUSA_MMU2:1 if the firmware supports the extended MMU2 commands subset, not necessarily that a MMU2 is attached. So it's mostly for gcode compatibility and nothing more. The problem is the MMU2 connects to the printer quite late, so when octoprint connects the M115 report would get requested before the MMU2 even boots (8s timeout in bootloader), so you can't use the extended capabilities to report the presence of the MMU2 unit.
Btw, I got approval for this.

About the D codes, @DRracer suggested that a Dcodes capability line is added as well. Would you implement also that or not?

@leptun
Copy link
Collaborator

leptun commented Feb 17, 2022

@foosel Oh, and before I forget, can the Prusa-Firmware detection be added as well? I'm asking this because the older firmware versions will have issues with the Tx, Tc and T? commands and we can't really do anything about them

@foosel
Copy link

foosel commented Feb 17, 2022

Please understand that I will not add individual support for printer manufacturer specific firmware forks to OctoPrint core. I'll do my best to do autodetection to work around any issues with general configuration settings (as here, by detecting Prusa Firmware and then setting the aforementioned flags for the user and for the connection session, which will cover everything you so far mentioned here, so no need on my part to parse additional capability lines, however, they are certainly welcome), but I won't start the endless circle of adding more and more vendor specific exceptions, processing steps, parsing or anything like that, and slowing down maintenance, debugging and processing time for everyone for the sake of a select few. That's what almost killed browsers in the late 90s and early 00s and it simply doesn't scale with regards to development. Try to keep things in line to the spec please.

If push comes to shove, there's always the option of a community maintained compatibility plugin but in general your goal should be to run with the spec and not break out of it left and right and demand hosts to adapt, as you surely can understand. It simply doesn't scale.

@foosel
Copy link

foosel commented Feb 17, 2022

@foosel Oh, and before I forget, can the Prusa-Firmware detection be added as well?

That already has been added and will be in 1.8.0, sorry if that was unclear before. See OctoPrint/OctoPrint#4423

@leptun
Copy link
Collaborator

leptun commented Feb 17, 2022

Didn't see that OctoPrint/OctoPrint#4423 was already implemented. That is great.
If possible please implement also the MMU2 capability line. This might not make much of a difference for FW 3.12 of the MK3 firmware, but as far as I know Marlin 2.0 also has MMU2 support implemented and the custom commands. So I can see how the capability line could be the common thing between different firmwares, instead of just enabling this for Prusa-Firmware-8bit. I've submitted the PR for the capability line in this firmware (#3416).

@foosel
Copy link

foosel commented Feb 17, 2022

I'm not sure what else you expect OctoPrint to do based on this capability line however. It will in general just happily forward any commands to the printer that is in printed files or entered manually, and also do the needful regarding line numbers and checksums and such if it's GCODE commands we are talking about. It's not like OctoPrint limits the subset of commands that may be sent through it, or anything like this.

Copy link

github-actions bot commented Nov 7, 2023

This issue has been flagged as stale because it has been open for 60 days with no activity. The issue will be closed in 7 days unless someone removes the "stale" label or adds a comment.

Copy link

This issue has been closed due to lack of recent activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants