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

fix(ExtruderPanel): restore mode after extruding/retracting #1965

Merged
merged 3 commits into from
Aug 27, 2024

Conversation

mryel00
Copy link
Member

@mryel00 mryel00 commented Aug 8, 2024

Description

This PR restores the gcode state to the state before the extrusion, triggered through the UI, happenend.

The old behavior changes the extrusion mode and can create problems with absolute extrusion setups.
The new behavior saves the state with SAVE_GCODE_STATE an unique name and restores this state with RESTORE_GCODE_STATE after the extrusion happened.

Related Tickets & Documents

This extends on the idea of #1964 but uses only Klipper built in commands to make this more generic.

Mobile & Desktop Screenshots/Recordings

none

[optional] Are there any post-deployment tasks we need to perform?

none

@kraftaksvk
Copy link

kraftaksvk commented Aug 8, 2024

Hi @mryel00, thanks for extending on my PR, I will test it soon. I just have a question; is the counter really needed? Tell me if I'm wrong, but doesn't SAVE_GCODE_STATE overwrite the save?

EDIT: dude I was just writing this and I noticed the commit 🤦‍♂️😆

@meteyou meteyou self-requested a review August 8, 2024 19:51
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Aug 8, 2024
@kraftaksvk
Copy link

kraftaksvk commented Aug 8, 2024

Hello, seems to work fine for me, however i suggest you replace the server messages:
this.$store.dispatch('server/addEvent', { message: gcode, type: 'command' })
this.$store.dispatch('server/addEvent', { message: gcode, type: 'command' })
with:
this.$store.dispatch('server/addEvent', { message: `G1 E-${this.feedamount} F${this.feedrate * 60}\n`, type: 'command' })
this.$store.dispatch('server/addEvent', { message: `G1 E${this.feedamount} F${this.feedrate * 60}\n`, type: 'command' })
...or similar to declutter the console (4 lines of messages for one extrusion feels like a bit too much).

Goes from this:
before
To this:
after

edit: ....idk for me when i refresh the interface the console is cleared, but yeah, it would be a lie...

@meteyou
Copy link
Member

meteyou commented Aug 8, 2024

@kraftaksvk this would be just useless. if you refresh the interface, Mainsail will reload the console history from Moonraker and you will see 4 lines.

furthermore, it would be a lie to display only 1 line...

edit: ....idk for me when i refresh the interface the console is cleared, but yeah, it would be a lie...

the console have to be correct. not "clear"...

@kraftaksvk
Copy link

Very true, but can't it be somehow improved?

@meteyou
Copy link
Member

meteyou commented Aug 8, 2024

it needs 4 lines of gcodes to fix your issue, so it have to be 4 lines of gcodes. this is the shortest way without external sources and i se absolutly no issue with 4 lines of gcodes.

@mryel00
Copy link
Member Author

mryel00 commented Aug 8, 2024

Altering the output on the console will be very confusing.

  1. If someone got the printer in absolute extrusion mode and then only see a series of G1 E1 F300, the person could wonder why it's even extruding, as the console only shows movement to the same e coordinate.
  2. If we would only show M83 and G1 E1 F300 the same person could think that the extrusion mode got changed, as the old behavior did.
  3. If we would show all the commands in the same line, without line breaks, the output would look like a non functional gcode and might be even worse than 4 separate lines.

Those 3 points should get you the idea of why altering the output, is a pretty bad idea.

but can't it be somehow improved?

Only if you find something with less commands without the need of adding new macros. The solution has to be generic that everyone can use it right out of the box.

@kraftaksvk
Copy link

thanks for the explanation, it was somewhat late for me but now I realize the possible consequences, I apologize. I also remembered if I'm not wrong, that there is an option for text size in the console..? Which could in turn look better.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Aug 17, 2024
@meteyou meteyou changed the title fix(extruderPanel): restore extrusion mode after extruding/retracting fix(ExtruderPanel): restore mode after extruding/retracting Aug 27, 2024
@meteyou meteyou merged commit cab5ea6 into mainsail-crew:develop Aug 27, 2024
14 checks passed
@pedrolamas
Copy link
Contributor

Probably an overkill, but I do wonder if the same "save & restore" approach should be done for printhead moves (X, Y, and Z) initiated from the UI... 🤔

@meteyou
Copy link
Member

meteyou commented Sep 1, 2024

Probably an overkill, but I do wonder if the same "save & restore" approach should be done for printhead moves (X, Y, and Z) initiated from the UI... 🤔

Thank you for your comment. I thought Klipper would return to the original position with the RESTORE_GCODE_STATE command, but it doesn't. I've also pinged you in my test PR. I'll ask Alexz what he thinks about this PR. I trust his opinion in terms of macros.

@mryel00 mryel00 deleted the fix/reset-extrusion-mode branch September 1, 2024 22:10
@zellneralex
Copy link
Member

@pedrolamas my answer to it can be found at #1988.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants